ODR issues from clear_key_cdm |
|||||
Issue descriptionclear_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
,
Sep 19
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)
,
Sep 19
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
,
Sep 19
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.
,
Sep 20
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.
,
Sep 20
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
,
Sep 20
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?
,
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
,
Sep 20
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.
,
Sep 20
,
Oct 11
,
Oct 11
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 |
|||||
Comment 1 by xhw...@chromium.org
, Sep 19