I believe we should fix all or none of them. There's no real point going half-way.

Nico, who would be the right person to triage this issue? I am certainly fine to make an actual change, but I would like to have a discussion from the gate keepers first.

We used to have `#define foo_EXPORT __attribute__((visibility("default")))`. I had to change it what we currently have to make the component build work, long ago. I tried to write a good CL description back then: https://chromium.googlesource.com/chromium/src/+/b387864d3341fca140a7bb837e959834d35b0591 Years later, I'm not sure I succeeded.

Here's another breadcrumb: https://bugs.chromium.org/p/chromium/issues/detail?id=90078#c58

"With the
components build, the linker can't decide to drop these inline methods,
so every .so that uses this header file will have the same public symbol."

Isn't this the problem that -fvisibility-inlines-hidden solves? Although it looks like we're already using that (but weren't in 2012 perhaps?)

Nico,

thank you for the fast and up to the point reply (as usual). 

>Years later, I'm not sure I succeeded.
Sorry, what exactly you're not sure about? About making the component build work or about writing a good CL description?

In the former case, what are the current status with the component build, and what are the plans?

Not sure if the CL description is good, it doesn't list which problems exactly it solved. (But comment 58 linked to above hints at it.)

Pretty sure we've always been using -fvisibility-inlines-hidden -- I think if a class has an explicit visibility annotation, that wins over -fvisibility-inlines-hidden though, hence the patch.

(clang's visibility stuff was hugely in flux back when we brought up the component build on mac btw -- it's possible this was needed with clang-back-then and isn't needed any longer with clang-today)

Yeah, it looks like -fvisibility-inlines-hidden overrides the class attribute at least today:

pcc@pcc-linux:/tmp$ cat v.cc
struct __attribute__((visibility("default"))) A {
  inline void f() {}
};

typedef void (A::*mp)();

__attribute__((noinline)) mp foo() {
  return &A::f;
}

int main() {
  (A().*(foo()))();
}

pcc@pcc-linux:/tmp$ ~/src/llvm/ra/bin/clang++ -fvisibility-inlines-hidden -o v.o v.cc -c
pcc@pcc-linux:/tmp$ readelf -s v.o

Symbol table '.symtab' contains 7 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS v.cc
     2: 0000000000000000     0 SECTION LOCAL  DEFAULT    2 
     3: 0000000000000000     0 SECTION LOCAL  DEFAULT    5 
     4: 0000000000000000    15 FUNC    GLOBAL DEFAULT    2 _Z3foov
     5: 0000000000000000    10 FUNC    WEAK   HIDDEN     5 _ZN1A1fEv
     6: 0000000000000010   107 FUNC    GLOBAL DEFAULT    2 main

Taking a bit of a step back, do we want to support LTO builds for the component build? Why?

Probably not. We only care about LTO for the static build.

Am I reading your mind correctly that you want to extend the macros to only omit the visibility("default") attribute if it's a component build and it's the importer code, not the implementation.

I don't think we do, at least for now. However, this problem doesn't just affect component builds. CDM_EXPORT, for example, is used even in non-component builds.

Huh! The _EXPORT macros are only supposed to be used for component builds, in general. For the vast majority of them, I bet that's true. Maybe the ones that are actually used for api level exports should be called something else. I can believe that CDM needs visibility in prod builds, but that case should be really rare. (You rarely want an exported interface over the C++ ABI, for example.)

Okay, now the issue becomes more clear. It seems really be mostly isolated in the CDM / DRM land, where they have an actual DSO from Widevine (?) and which has an exported C++ API defined in https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/api/content_decryption_module.h

If true, we can safely change it from:

#if defined(CDM_IMPLEMENTATION)
#define CDM_EXPORT __attribute__((visibility("default")))
#else
#define CDM_EXPORT
#endif

to
#define CDM_EXPORT __attribute__((visibility("default")))

as the file does not have any inlineable functions other than a few empty constructors / destructors.

Using a differently named macro for "internal API" exports seems reasonable to me.

krasin@, would you be willing to take the lead on finding any more of these and renaming them to something like foo_API (and fixing the bug while you're at it)?

After second thought, my proposal does not address the issue described in the original issue description. We still have an ODR violation, don't we?

Are you talking about an ODR violation in CDM or elsewhere?

>krasin@, would you be willing to take the lead on finding any more of these and renaming them to something like foo_API (and fixing the bug while you're at it)

Yes, sure. But let's decide what to do with them and also with the regular _EXPORT macros.

>Are you talking about an ODR violation in CDM or elsewhere?
Elsewhere. From your description in #1:

"This is an ODR violation, although it isn't normally observable. One case where it could lead to problems is if the class declares a static data member:
..."

I think we should at least try to fix the problem elsewhere as well.

Elsewhere, the current setup helps detect missing link dependencies, which is kind of nice.

If you can get away with changing just files using exports in prod builds (order of 1-4 _export files, I'd guess), I'd probably recommend doing that as it's two orders of magnitude less work than changing all _export.h files.

I thought that "-z defs" is already detecting missing link dependencies? But I suppose that doesn't work on every platform, and I can easily see this becoming more work than we anticipate for a problem that we don't have right now, so maybe better to hold off for now.

Let's just change the exports affecting prod then.

According to the CFI blacklist, we have only three cases:

1. CDM_EXPORT discussed above;
2. MOJO_TEST_SUPPORT_EXPORT used only in tests, but which breaks CFI (and likely UBSan Vptr):
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/c/test_support/test_support_export.h&sq=package:chromium&type=cs
3. libosmesa, which has a macro PUBLIC correctly setting visibility("default")

Chances are that I have missed something, but we could fix the remainders, as we discover them.

SGTM.

Hm, MOJO_TEST_SUPPORT_EXPORT being used in non-component builds might be unintentional. (Not sure.)

I am also suspicious about this one, but don't have anyone familiar with mojo stuff around. Worth adding someone to cc?

There's a libmojo_public_test_support.so that we use in non-component builds. Not sure why that needs to be a DSO, but it certainly seems to be intentional:

https://code.google.com/p/chromium/codesearch#chromium/src/mojo/mojo_public.gyp&q=mojo_public_test_support&sq=package:chromium&type=cs&l=356

rockot, do you know why libmojo_public_test_support is a shared_library even in non-component builds?

I believe it was previously used by a test framework which had to fix up its exported symbols dynamically at runtime. This should no longer necessary. That library is something I want to delete, but we could probably just change it to a static library / source set for now and nothing should break.

Thank you, Ken. I will send a CL in a sec.

The CL is ready: https://codereview.chromium.org/1953503004/
I sent it to trybots, let's wait a bit to see if I missed something obvious.

The cleanup CL that removes MOJO_TEST_SUPPORT_EXPORT (see #31) is passing CQ. Looks good so far. Big thanks to Ken for the hints.

I've also taken a closer look at media/cdm/api stuff. It's terribly interesting.
See, for example, https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_specific.h&l=46&rcl=1462475911

// Allows exporting a class that inherits from a non-exported base class.
// This uses suppress instead of push/pop because the delimiter after the
// declaration (either "," or "{") has to be placed before the pop macro.
//
// Example usage:
// class EXPORT_API Foo : NON_EXPORTED_BASE(public Bar) {
//
// MSVC Compiler warning C4275:
// non dll-interface class 'Bar' used as base for dll-interface class 'Foo'.
// Note that this is intended to be used only when no access to the base class'
// static data is done through derived classes or inline methods. For more info,
// see http://msdn.microsoft.com/en-us/library/3tdb471s(VS.80).aspx
#define NON_EXPORTED_BASE(code) MSVC_SUPPRESS_WARNING(4275) \
                                code

#else  // Not MSVC
...

An example of such class would be cdm::Host_8:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/api/content_decryption_module.h&sq=package:chromium&type=cs&l=988&rcl=1462475911

And then cdm::CdmAdapter inherits from cdm::Host_8:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/cdm_adapter.h&q=NON_EXPORTED_BASE&sq=package:chromium&type=cs&l=50

Removing MOJO_TEST_SUPPORT_EXPORT triggered some mysterious failures on Windows 7. Everything is fine on Windows 8 and on Windows 8 x64.

Worse, while rel build fails while running tests, dbg build has a compile error:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/217278
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg_ng/builds/1304

The build-time error:

ninja explain: output obj/chrome/browser/media/router/mojo/media_router_test_support.media_router_mojo_test.obj older than most recent input gen/services/shell/public/interfaces/connector.mojom-internal.h (484213074 vs 484213104)
ninja explain: obj/chrome/browser/media/router/mojo/media_router_test_support.media_router_mojo_test.obj is dirty
ninja explain: obj/chrome/browser/media/router/media_router_test_support.lib is dirty
ninja explain: obj/chrome/browser/media/router/mojo/media_router_test_support.media_router_mojo_test.obj is dirty
ninja explain: browser_tests.exe is dirty
ninja explain: browser_tests.exe is dirty
ninja explain: browser_tests.exe is dirty
ninja explain: obj/chrome/browser/media/router/media_router_test_support.lib is dirty
ninja explain: obj/chrome/browser/media/router/mojo/media_router_test_support.media_router_mojo_test.obj is dirty
ninja explain: unit_tests.exe is dirty
ninja explain: obj/build/chromium_builder_tests.actions_depends.stamp is dirty
ninja explain: unit_tests.exe is dirty
<Thread(Thread-1, started 4432)> ProcessRead: proc.stdout finished.
<Thread(Thread-1, started 4432)> ProcessRead: cleaning up.
<Thread(Thread-2, started daemon 4640)> TimedFlush: Finished
<Thread(Thread-1, started 4432)> ProcessRead: finished.
Failing build because ninja reported work to do.
This means that after completing a compile, another was run and
it resulted in still having work to do (that is, a no-op build
wasn't a no-op). Consult the first "ninja explain:" line for a
likely culprit.

I am looking into this...

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

commit 7b2368d72cd3c3fe3236ce679ed5adda380760f2
Author: krasin <krasin@google.com>
Date: Fri May 06 22:20:22 2016

mojo: make libmojo_public_test_support a static library.

The plan is to eventually get rid of it completely, but
for now, we want to remove ODR violations related to
MOJO_TEST_SUPPORT_EXPORT macro in non-components build.

BUG= 609564 , 515347 

Review-Url: https://codereview.chromium.org/1953503004
Cr-Commit-Position: refs/heads/master@{#392180}

[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/common/BUILD.gn
[delete] https://crrev.com/210c20eff22bee39130d425fed027e71c1a35020/mojo/mojo.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_common_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_js_integration_tests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_js_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_public.gyp
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_public_bindings_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_public_system_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/mojo_system_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/public/c/test_support/BUILD.gn
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/public/c/test_support/test_support.h
[delete] https://crrev.com/210c20eff22bee39130d425fed027e71c1a35020/mojo/public/c/test_support/test_support_export.h
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/mojo/public/tests/test_support_private.h
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/services/shell/mojo_shell_unittests.isolate
[modify] https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2/tools/cfi/blacklist.txt

Okay, MOJO_TEST_SUPPORT_EXPORT is gone.

Also, CDM_EXPORT is only applied to the functions, not types. So, no ODR issue here.

What's wrong with media/cdm/api/content_decryption_module.h is the types which have inconsistent visibility. For instance, it defines cdm::ContentDecryptionModule_8 which gets private visibility:

https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/api/content_decryption_module.h&q=cdm/api&sq=package:chromium&type=cs&l=647

This class is abstract for everything but empty constructor and destructor.

Then we have libclearkeycdm.so that defines a subclass of cdm::ContentDecryptionModule_8:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/external_clear_key/clear_key_cdm.h&sq=package:chromium&type=cs&l=37&rcl=1462547914

class ClearKeyCdm : public cdm::ContentDecryptionModule_8 { ... }

Now, Chromium calls one of the functions declared in content_decryption_module.h and defined in libclearketcdm.so and gets void* for an instance of ClearKeyCdm which it casts to cdm::ContentDecryptionModule_8. Then UBSan Vptr reports the issue:

$ ./out/gn-vptr/media_unittests --gtest_filter=CdmAdapter/AesDecryptorTest.CloseSession/0
...
../../media/cdm/cdm_wrapper.h:142:5: runtime error: member call on address 0x3dcf1a051a00 which does not point to an object of type 'cdm::ContentDecryptionModule_8'
0x3dcf1a051a00: note: object is of type 'media::ClearKeyCdm'

None of these two types have visibility("default"), and in fact there're two hidden copies of cdm::ContentDecryptionModule_8. One is inside Chromium, another comes from libclearkeycdm.so. And here goes the issue: *the code relies that virtual tables for both types are the same*. This is obviously wrong in a general case, as a linker is fine to apply any kind of devirtualization technique for the hidden types. We need to mark these types with visibility("default") to guarantee that they are the same in both modules (Chromium and libclearkeycdm.so).

Peter, Eugenii - can you please verify that my explanation and conclusion is correct?

> Also, CDM_EXPORT is only applied to the functions, not types. So, no ODR issue here.

That's not technically true, but it's probably not important yet. It could affect function pointer equality, which is very unlikely to be a problem in practice. If we want to enable CFI for indirect function calls in the future, we might need to worry about it though.

> We need to mark these types with visibility("default") to guarantee that they are the same in both modules (Chromium and libclearkeycdm.so).

Yes, agree.

Ugh. Okay, then we need to mark these functions with visibility("default"), while we're on it. No reason to have a time bomb sitting in the code.

I will go ahead and create a CL.

Nico, do you by chance know how would be a good reviewer for the CDM changes? Your guess for the mojo CL was perfect.

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

commit 0cc1fb3aedf871cf08d3a218105d14e8ff41198b
Author: krasin <krasin@chromium.org>
Date: Sat May 07 00:28:41 2016

Revert of mojo: make libmojo_public_test_support a static library. (patchset #8 id:140001 of https://codereview.chromium.org/1953503004/ )

Reason for revert:
Broke official builds.

BUG=610020

Original issue's description:
> mojo: make libmojo_public_test_support a static library.
>
> The plan is to eventually get rid of it completely, but
> for now, we want to remove ODR violations related to
> MOJO_TEST_SUPPORT_EXPORT macro in non-components build.
>
> BUG= 609564 , 515347 
>
> Committed: https://crrev.com/7b2368d72cd3c3fe3236ce679ed5adda380760f2
> Cr-Commit-Position: refs/heads/master@{#392180}

TBR=rockot@chromium.org,krasin@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 609564 , 515347 

Review-Url: https://codereview.chromium.org/1960703004
Cr-Commit-Position: refs/heads/master@{#392216}

[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/common/BUILD.gn
[add] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_common_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_js_integration_tests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_js_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_public.gyp
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_public_bindings_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_public_system_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/mojo_system_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/public/c/test_support/BUILD.gn
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/public/c/test_support/test_support.h
[add] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/public/c/test_support/test_support_export.h
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/mojo/public/tests/test_support_private.h
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/services/shell/mojo_shell_unittests.isolate
[modify] https://crrev.com/0cc1fb3aedf871cf08d3a218105d14e8ff41198b/tools/cfi/blacklist.txt

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

commit da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae
Author: krasin <krasin@google.com>
Date: Mon May 09 23:50:26 2016

mojo: make libmojo_public_test_support a static library.

The plan is to eventually get rid of it completely, but
for now, we want to remove ODR violations related to
MOJO_TEST_SUPPORT_EXPORT macro in non-components build.

This is a second attempt to land the CL. The first one:
https://codereview.chromium.org/1953503004/

The problem was a typo in mojo_shell_unittests.isolate,
which is now fixed.

BUG= 609564 ,  515347 , 610020

Review-Url: https://codereview.chromium.org/1963883002
Cr-Commit-Position: refs/heads/master@{#392479}

[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/common/BUILD.gn
[delete] https://crrev.com/7bd26f185bdbf8762bf69cdc6b4d8834e5e2b968/mojo/mojo.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_common_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_js_integration_tests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_js_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_public.gyp
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_public_bindings_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_public_system_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/mojo_system_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/public/c/test_support/BUILD.gn
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/public/c/test_support/test_support.h
[delete] https://crrev.com/7bd26f185bdbf8762bf69cdc6b4d8834e5e2b968/mojo/public/c/test_support/test_support_export.h
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/mojo/public/tests/test_support_private.h
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/services/shell/mojo_shell_unittests.isolate
[modify] https://crrev.com/da412f2ccdbb6fdc5e1373bb7bedc8ace040aeae/tools/cfi/blacklist.txt

Ivan's change caused this build failure on Windows:

https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/145383/steps/compile%20%28with%20patch%29/logs/stdio

This is the result of introducing a dllimport dependency from an exe to a dll, which is not allowed on Windows.

This does seem a lot like the case described in the LTO visibility doc (http://clang.llvm.org/docs/LTOVisibility.html):

"Some ABIs provide the ability to define an abstract base class without visibility attributes in multiple linkage units and have virtual calls to derived classes in other linkage units work correctly. One example of this is COM on Windows platforms. If the ABI allows this, any base class used in this way must be defined with public LTO visibility."

My suggestion is to introduce a separate macro for exporting class definitions (CDM_CLASS_API?), and have it expand to:
- [[clang::lto_visibility_public]] on Windows/Clang
- nothing on Windows/MSVC (or the same as Windows/Clang if MSVC accepts that)
- __attribute__((visibility("default"))) on all other platforms.

> This is the result of introducing a dllimport dependency from an exe to a dll, which is not allowed on Windows.

No, that's not what's going on. The failing links are exes that want to link with the CDM dll.

If we can tolerate a hard runtime dependency on the CDM dll in Chrome, we can probably fix the problem just by introducing that dependency. Otherwise, we might need to do what I wrote in c#40.

I think we'll need to go with #40, as there're multiple CDM implementations, so a hard dependency is probably not an option.

Peter, is there a difference between __attribute__((visibility("default"))) and [[clang::lto_visibility_public] for our case? I don't see a reason to mark it one way on Windows and another way on Linux, as on Windows we're not using any special ABIs like COM. 

here is the proposed fix, pending the resolution regarding to #43:
https://codereview.chromium.org/1964083003/

The reason why [[clang::lto_visibility_public]] is necessary here on Windows is that on Windows, unlike on other platforms, the attributes that are normally used to share classes between linkage units (dllimport/dllexport) have ABI-visible consequences. One aspect of this is that some linkage unit must contain the "definition" of the class, and all others must be able to link to it.

On non-Windows we can just use default visibility, which will cause the dynamic loader to choose some definition of the vtable in some DSO (since it has weak linkage). There's no mechanism for DSO symbols to have weak linkage on Windows.

There's another commonly used mechanism on Windows to share class definitions between linkage units, and that's to define the class without dllexport/dllimport attributes, and to define the class as a pure abstract class. This is the mechanism that COM uses, and it's the same mechanism that's used here.

Okay, I somewhat understand why [[clang::lto_visibility_public]] is needed on Windows.

Would it also work on Linux, or we really want  __attribute__((visibility("default")))?

Sorry, it's still not entirely clear to me.

[[clang::lto_visibility_public]] wouldn't fix the UBSan issue, because the vtables would continue to have hidden visibility and the dynamic loader wouldn't be able to merge their identities.

Thank you, Peter. I don't have any more questions (you've help to draw a non-self-contradicting picture in my mind), but I honestly claim that I will not be able to draw similar conclusions in the future. There's a whole hidden world of visibilities that I am yet to learn.

The change to content_decryption_module.h will (once DEPS rolled) only affect the Chrome binaries until the CDM build also rolls its DEPS. Is it okay if the CDM is built with the version before this work started? On all platforms?

For the question in the CL about exposing C++ classes [1]. Posting the reply here so we have one central place for discussion:

I am really no expert in this area. But when we created this interface, my
understanding was that the exported functions have to be C functions, but the C
functions can create C++ objects, whose methods can be called by another
library. One pitfall is that the new and delete of the C++ objects must live in
the same library. That's we have Destroy() call on the CDM interface.

I think this is one of the reference I was reading:

http://www.yolinux.com/TUTORIALS/LibraryArchives-StaticAndDynamic.html

With that, is it actually necessary to export the C++ classes? Was my assumption
wrong? I don't really understand how devirtualization works :(

[1] https://codereview.chromium.org/1956123002/#msg20

Re #49:
>The change to content_decryption_module.h will (once DEPS rolled) only affect the Chrome binaries until the CDM build also rolls its DEPS. Is it okay if the CDM is built with the version before this work started? On all platforms?

I will start with obvious: Windows / MSVC will continue to work, as CDM_CLASS_API is empty for this platform.

I would delegate answers for the remaining platforms to Peter, as I am no expert here:

- Linux, Android
- Mac, iOS
- Windows / Clang

Just to clarify, this interface is not used on Android and iOS. So we are only talking about Linux/ChromeOS, Mac and Windows here. 

Is Windows/Clang used in production?

Not yet, to the best of my knowledge, but Nico might correct me.

> Is it okay if the CDM is built with the version before this work started? On all platforms?

I believe so. The effect of this change was to change symbol visibility in Chrome, not the CDM. So it wouldn't have had an effect on the CDM binaries.

Peter,

thank you for the explanation.

Can you please also comment on #50?

> With that, is it actually necessary to export the C++ classes?

The definition is shared between two linkage units, so yes, to do otherwise is technically an ODR violation. Up until now that hasn't mattered, but devirtualization relies on symbol visibility to decide which classes can be devirtualized. For example, if you have two linkage units, DSO1 and DSO2, and DSO1 contains a translation unit that looks like this:

class __attribute__((visibility("hidden"))) C1 {
  virtual void f() = 0;
};

class __attribute__((visibility("hidden"))) C2 : C1 {
  virtual void f() { ... }
}

and DSO2 contains one that looks like this:

class __attribute__((visibility("hidden"))) C1 {
  virtual void f() = 0;
};

class __attribute__((visibility("hidden"))) C3 : C1 {
  virtual void f() { ... }
}

then devirtualization would be able to conclude that because there is a single visible derived class of C1, calls to C1::f() can be devirtualized to C2::f(). If an instance of C3 somehow made it into the code in DSO1, we would miscompile any calls to C1::f().

On the other hand if we s/hidden/default/g in the above, devirtualization would not be able to make any conclusions about how many derived classes there are of C1, so it would inhibit devirtualization for C1.

pcc: Thanks for the explanation! It makes a lot of sense to me, though I am really not expert in this area to have my own judgement. I reviewed the CL and let's make sure the changes doesn't break existing CDMs.

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

commit c5197d2ab04247a33e6934b576cdbd071ba562c2
Author: krasin <krasin@google.com>
Date: Mon May 16 23:44:09 2016

Roll media/cdm/api deps.

This CL brings fixes for visibility of types used across Chromium->CDM bridge. See https://chromiumcodereview.appspot.com/1956123002/ and https://codereview.chromium.org/1964083003/

Also, removing now unnecessary lines in CFI blacklist.

BUG= 609564 ,  609175 ,  515347 

Review-Url: https://codereview.chromium.org/1959093004
Cr-Commit-Position: refs/heads/master@{#393981}

[modify] https://crrev.com/c5197d2ab04247a33e6934b576cdbd071ba562c2/DEPS
[modify] https://crrev.com/c5197d2ab04247a33e6934b576cdbd071ba562c2/tools/cfi/blacklist.txt