New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 887110 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Oct 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

ODR issues from clear_key_cdm

Project Member Reported by spang@chromium.org, Sep 19

Issue description

clear_key_cdm causes ODR violations in tests due to pulling in a large amount of chrome code.

It seems we cannot turn on vulkan on Linux CrOS until this is resolved.

See  bug 887048 
 
clear_key_cdm is a loadable_module by itself. I am not sure what ODR violations it causes. Do you have any details?
Cc: reve...@chromium.org
 Bug 887048  shows the failure but you can repro locally using:

gn gen out/linux_cros_asan --args='dcheck_always_on = true
is_asan = true   
is_component_build = false
is_debug = false
is_lsan = true
strip_absolute_paths_from_debug_symbols = true
symbol_level = 1
target_os = "chromeos"
use_goma = true
enable_vulkan = true
'

ninja -C out/linux_cros_asan media_unittests

./out/linux_cros_asan/media_unittests

This breaks with an ODR violation on zwp_linux_dmabuf_v1_interface.

I suppose it wouldn't happen if that structure wasn't marked with default __attribute__ ((visibility("default"))) (+reveman)
Here's more info on the duplicated dependency:

gn path out/linux_cros_asan //media:media_unittests //third_party/wayland-protocols:linux_dmabuf_protocol     
//media:media_unittests --[private]-->
//media/gpu:unit_tests --[private]-->
//media/gpu:gpu --[private]-->
//gpu/ipc/service:service --[private]-->
//ui/ozone:ozone --[public]-->
//ui/ozone:platform --[private]-->
//ui/ozone/platform/wayland:wayland --[private]-->
//third_party/wayland-protocols:linux_dmabuf_protocol


gn path out/linux_cros_asan //media/cdm/library_cdm/clear_key_cdm //third_party/wayland-protocols:linux_dmabuf_protocol                                                  
//media/cdm/library_cdm/clear_key_cdm:clear_key_cdm --[private]-->
//media:media --[public]-->
//media/renderers:renderers --[private]-->
//components/viz/client:client --[public]-->
//components/viz/common:common --[private]-->
//gpu/vulkan/init:init --[private]-->
//ui/ozone:ozone --[public]-->
//ui/ozone:platform --[private]-->
//ui/ozone/platform/wayland:wayland --[private]-->
//third_party/wayland-protocols:linux_dmabuf_protocol


gn path --with-data out/linux_cros_asan //media:media_unittests //media/cdm/library_cdm/clear_key_cdm
//media:media_unittests --[private]-->
//media/cdm:unit_tests --[data]-->
//media/cdm/library_cdm/clear_key_cdm:clear_key_cdm



This is the new dependency edge that broke the bot:

//components/viz/common:common --[private]-->
//gpu/vulkan/init:init --[private]

This is not actually necessary; replacing it by //gpu/vulkan prevents pulling in ozone & its deps twice, and makes the ASAN ODR issue go away.

Uploaded CL @ https://chromium-review.googlesource.com/c/chromium/src/+/1235286

It's probably worth trying to narrow down the deps of this module though.
Thank you so much for the details. But I still don't see how ODR is caused by media_unittests + clear_key_cdm. As you can see, clear_key_cdm is a data_deps of media_unittests:

//media:media_unittests --[private]-->
//media/cdm:unit_tests --[data]-->
//media/cdm/library_cdm/clear_key_cdm:clear_key_cdm

IOW, media_unittests loads clear_key_cdm as a standalone shared library at run time. Then how could it cause an ODR issue? Also, I am not familiar with zwp_linux_dmabuf_v1_interface.
A dependency from an executable, shared_library, or loadable_module target to a source_set or static_library target puts a copy of all the code in the source_set or static_library into the executable, shared_library, or loadable_module.

In this case both media_unittests and clear_key_cdm are getting separate copies of //media and all of its dependencies. That clear_key_cdm is a loadable_module, and not a component, is actually the cause of the duplication. Duplication of code results in subtle issues (since it breaks the guarantees about object and function identity that would normally apply) as well as bloat.

The duplication of //media and its dependencies was already happening even before the change to enable vulkan.

The ASAN bot broke after the change to enable vulkan because with that flag enabled, //media grew a dependency on //third_party/wayland-protocols:linux_dmabuf_protocol, and that target has some data structures that are explicitly marked for export [1]. ASAN has code to detect when exported names are duplicated because that is a particularly dangerous case which is also easy to diagnose.

[1] https://cs.chromium.org/chromium/src/third_party/wayland/src/src/wayland-util.h?rcl=1361da9cd5a719b32d978485a29920429a31ed25&l=45

clear_key_cdm is used for testing and it needs to be a loadable_module to simulate the real world scenario. We are aware that it'll have a copy of media/, base/ etc but that's not a concern as long as those symbols are not exposed.

It seems to me the issue is //third_party/wayland-protocols:linux_dmabuf_protocol that exposes some data structures by default. Can we fix that?
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f40d46d761aa6bc5e6ec676f69dc609ed7844db

commit 2f40d46d761aa6bc5e6ec676f69dc609ed7844db
Author: Michael Spang <spang@chromium.org>
Date: Thu Sep 20 18:35:11 2018

ozone: Enable vulkan by default (reland)

Vulkan is enabled by default on Linux desktop (X11) builds. Enable it on
ozone as well to make sure we have build coverage.

Fix chromium.memory/Linux Chromium OS ASan LSan Tests issue by breaking
unnecessary dependency on //gpu/vulkan/init.

Bug:  887110 
Test: compile
Change-Id: I298704cf3b199c5c4ecf5173e137293650c5734a
Reviewed-on: https://chromium-review.googlesource.com/1235286
Commit-Queue: Michael Spang <spang@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592885}
[modify] https://crrev.com/2f40d46d761aa6bc5e6ec676f69dc609ed7844db/components/viz/common/BUILD.gn
[modify] https://crrev.com/2f40d46d761aa6bc5e6ec676f69dc609ed7844db/components/viz/common/gpu/vulkan_in_process_context_provider.cc
[modify] https://crrev.com/2f40d46d761aa6bc5e6ec676f69dc609ed7844db/gpu/vulkan/features.gni
[modify] https://crrev.com/2f40d46d761aa6bc5e6ec676f69dc609ed7844db/gpu/vulkan/init/vulkan_factory.cc

I agree that the exports here are unusual and are part of the cause of this issue. We can investigate that.

However it's also true that clear_key_cdm has a large dependency footprint. When building a library like this, I think it would be best to break out the code you need into source_sets instead of depending directly into the component structure of chromium. It makes me nervous how close this crypto library is to pulling in a complete copy of //ui/ozone and all of its dependencies.
Labels: -Pri-2 Pri-3
Status: Assigned (was: Untriaged)
Status: WontFix (was: Assigned)
Again clear_key_cdm is only for testing and we do not have any plan to refactor our targets only for this. Mark as WontFix for now since there's nothing we can do. Feel free to reopen!

Sign in to add a comment