Fix visibility for CDM API types and functions.

Fix visibility for CDM API types and functions.

This change does three different things:

1. Renames CDM_EXPORT macro to CDM_API.
   While not strictly necessary, _EXPORT macros in Chrome
   are used for controlling visibility in the components builds.
   See https://chromium.googlesource.com/chromium/src/+/b387864d3341fca140a7bb837e959834d35b0591
   for more details.

2. On POSIX, CDM_API will set visibility("default") for both
   the exporter and importer code. Otherwise, the linker is free to make
   optimizations which will break the code, see
   https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c36

3. Mark interface classes with CDM_API to avoid being hit by devirtualization,
   see https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c35
   and other comments in that bug.

This CL should fix UBSan Vptr buildbot, which has become recently more rigorous
(after a roll of the new Clang).

BUG=609564, 609175
R=pcc@chromium.org, xhwang@chromium.org

Committed: https://chromium.googlesource.com/chromium/cdm/+/0f9542b7d8e81eb8aa6255438b2c1907ecc3d782

[krasin, 2016-05-06 23:25:52]

krasin@google.com changed reviewers:
+ pcc@chromium.org, thakis@chromium.org

[krasin, 2016-05-06 23:25:53]

Peter, Nico,

please, review this CL before the cdm/api owners, as the complexity of the
change is not in the diff, but in the low-level compiler/linker issues it aims
to fix.

[krasin, 2016-05-06 23:26:51]

Also, please, ignore the trybot. Trybots don't work for the media/cdm/api repo.

Locally, I verified that media_unittests now pass under UBSan Vptr.

[krasin, 2016-05-09 22:54:36]

Peter,

friendly ping.

[pcc1, 2016-05-09 23:01:15]

lgtm if owners are ok with it.

https://codereview.chromium.org/1956123002/diff/1/content_decryption_module.h
File content_decryption_module.h (left):

https://codereview.chromium.org/1956123002/diff/1/content_decryption_module.h...
content_decryption_module.h:18: // can be exported to consumers.
You might want to explicitly state that this is expected to be a separate DSO
even in non-component builds.

[krasin, 2016-05-10 01:04:06]

krasin@google.com changed reviewers:
+ ddorwin@chromium.org

[krasin, 2016-05-10 01:04:07]

Hi David,

this CL fixes inconsistent visibility declarations on Linux, which could lead to
miscompilations, especially for optimized builds (and more so for LTO builds,
which we want to launch for official Chrome on Linux).

I believe that these macros were taken from _EXPORT macros for component build.
While the issue persists for them too, component builds are not something we
ship to the users, and the risk of a miscompile is lesser than additional
benefits for tracking symbol dependencies between components. These don't apply
for CDM, and we actually have these macros effective for the official build, so
fixing the visibility is important.

See more discussion on the bug: https://crbug.com/609564.

[krasin, 2016-05-10 01:09:20]

https://codereview.chromium.org/1956123002/diff/1/content_decryption_module.h
File content_decryption_module.h (left):

https://codereview.chromium.org/1956123002/diff/1/content_decryption_module.h...
content_decryption_module.h:18: // can be exported to consumers.
On 2016/05/09 23:01:15, pcc1 wrote:
> You might want to explicitly state that this is expected to be a separate DSO
> even in non-component builds.

Done.

[ddorwin, 2016-05-10 01:16:21]

ddorwin@chromium.org changed reviewers:
+ xhwang@chromium.org

[ddorwin, 2016-05-10 01:16:22]

xhwang might remember more of the background on this.

[xhwang, 2016-05-10 05:19:31]

On 2016/05/10 01:16:22, ddorwin wrote:
> xhwang might remember more of the background on this.

When this file was created we were pretty much copying the model of MEDIA_EXPORT
and didn't really know much details about what's being discussed in the bug. I
read through the discussions in the bug and they make sense to me. LGTM!

The CDM interface is similar to the PPAPI interfaces. PP_EXPORT is actually
always __attribute__ ((visibility("default"))) on POSIX for both importer and
exporter code:

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/ppp.h&l=29

PP_EXPORT is used on the C interface functions (e.g. PPP_InitializeModule). But
in the PPAPI case there's no need to worry about the consistency of the C++
types since all PPAPI interfaces are C interfaces.

[krasin, 2016-05-10 05:38:12]

Thank you, Xiaohan!

[krasin, 2016-05-10 05:38:57]

Description was changed from

==========
Fix visibility for CDM API types and functions.

This change does three different things:

1. Renames CDM_EXPORT macro to CDM_API.
   While not strictly necessary, _EXPORT macros in Chrome
   are used for controlling visibility in the components builds.
   See
https://chromium.googlesource.com/chromium/src/+/b387864d3341fca140a7bb837e95...
   for more details.

2. On POSIX, CDM_API will set visibility("default") for both
   the exporter and importer code. Otherwise, the linker is free to make
   optimizations which will break the code, see
   https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c36

3. Mark interface classes with CDM_API to avoid being hit by devirtualization,
   see https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c35
   and other comments in that bug.

This CL should fix UBSan Vptr buildbot, which has become recently more rigorous
(after a roll of the new Clang).

BUG=609564,609175
==========

to

==========
Fix visibility for CDM API types and functions.

This change does three different things:

1. Renames CDM_EXPORT macro to CDM_API.
   While not strictly necessary, _EXPORT macros in Chrome
   are used for controlling visibility in the components builds.
   See
https://chromium.googlesource.com/chromium/src/+/b387864d3341fca140a7bb837e95...
   for more details.

2. On POSIX, CDM_API will set visibility("default") for both
   the exporter and importer code. Otherwise, the linker is free to make
   optimizations which will break the code, see
   https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c36

3. Mark interface classes with CDM_API to avoid being hit by devirtualization,
   see https://bugs.chromium.org/p/chromium/issues/detail?id=609564#c35
   and other comments in that bug.

This CL should fix UBSan Vptr buildbot, which has become recently more rigorous
(after a roll of the new Clang).

BUG=609564,609175
R=pcc@chromium.org, xhwang@chromium.org

Committed:
https://chromium.googlesource.com/chromium/cdm/+/0f9542b7d8e81eb8aa6255438b2c...
==========

[krasin, 2016-05-10 05:38:59]

Committed patchset #2 (id:20001) manually as
0f9542b7d8e81eb8aa6255438b2c1907ecc3d782 (presubmit successful).

[krasin, 2016-05-10 18:42:31]

FYI: while trying to roll the deps, I realized that this change is not correct
on Windows.
See https://codereview.chromium.org/1959093004/

I am fine to revert this CL, but given that we don't break anything in Chrome,
and this repo is not rolled often (last time it was rolled ~6 months ago), I
would prefer to send a follow-up CL with a fix and then roll them both.

David, Xiaohan,
is that fine with you?

[ddorwin, 2016-05-10 18:49:53]

On 2016/05/10 18:42:31, krasin wrote:
> FYI: while trying to roll the deps, I realized that this change is not correct
> on Windows.
> See https://codereview.chromium.org/1959093004/
> 
> I am fine to revert this CL, but given that we don't break anything in Chrome,
> and this repo is not rolled often (last time it was rolled ~6 months ago), I
> would prefer to send a follow-up CL with a fix and then roll them both.
> 
> David, Xiaohan,
> is that fine with you?

Yes, that is fine assuming you plan to fix it soon.

[krasin, 2016-05-10 18:57:29]

> Yes, that is fine assuming you plan to fix it soon.
Sure. That's a single most important thing for me today.

[Nico, 2016-05-10 21:06:52]

Previously, only C functions got exported. Now, we export C++ classes. Do we
really want to use C++ across a dll boundary?

[krasin, 2016-05-10 21:20:15]

On 2016/05/10 21:06:52, Nico wrote:
> Previously, only C functions got exported. Now, we export C++ classes. Do we
> really want to use C++ across a dll boundary?

The code already does that. See https://crbug.com/609564#c35

[Nico, 2016-05-10 21:23:06]

On 2016/05/10 21:20:15, krasin wrote:
> On 2016/05/10 21:06:52, Nico wrote:
> > Previously, only C functions got exported. Now, we export C++ classes. Do we
> > really want to use C++ across a dll boundary?
> 
> The code already does that. See https://crbug.com/609564#c35

Right, but maybe we want to fix that instead of exporting these classes (a
question for the CDM folks).

[xhwang, 2016-05-11 06:06:56]

On 2016/05/10 21:23:06, Nico wrote:
> On 2016/05/10 21:20:15, krasin wrote:
> > On 2016/05/10 21:06:52, Nico wrote:
> > > Previously, only C functions got exported. Now, we export C++ classes. Do
we
> > > really want to use C++ across a dll boundary?
> > 
> > The code already does that. See https://crbug.com/609564#c35
> 
> Right, but maybe we want to fix that instead of exporting these classes (a
> question for the CDM folks).

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 :(

[xhwang, 2016-05-11 06:07:25]

On 2016/05/11 06:06:56, xhwang wrote:
> On 2016/05/10 21:23:06, Nico wrote:
> > On 2016/05/10 21:20:15, krasin wrote:
> > > On 2016/05/10 21:06:52, Nico wrote:
> > > > Previously, only C functions got exported. Now, we export C++ classes.
Do
> we
> > > > really want to use C++ across a dll boundary?
> > > 
> > > The code already does that. See https://crbug.com/609564#c35
> > 
> > Right, but maybe we want to fix that instead of exporting these classes (a
> > question for the CDM folks).
> 
> 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 :(

Maybe we should move this discussion to the bug, where ddorwin has other
questions :)