Page MenuHomePhabricator

[clang] Don't emit type test/assume for virtual classes that should never participate in WPD
ClosedPublic

Authored by aeubanks on Jun 15 2022, 9:47 AM.

Diff Detail

Event Timeline

aeubanks created this revision.Jun 15 2022, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 9:47 AM
aeubanks requested review of this revision.Jun 15 2022, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 9:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aeubanks updated this revision to Diff 437224.Jun 15 2022, 9:50 AM

update some comments

I realize that this is not a full fix for the general problem involving -lto-whole-program-visibility, but I believe if you ignore -lto-whole-program-visibility (which Chrome doesn't use) then this fix makes sense.

pcc requested changes to this revision.Jun 15 2022, 10:14 AM

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

This revision now requires changes to proceed.Jun 15 2022, 10:14 AM

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

pcc added a comment.Jun 15 2022, 10:21 AM

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

We documented the feature in D75655 and there it says "all classes" (and still does).

aeubanks updated this revision to Diff 437258.Jun 15 2022, 11:15 AM

update docs

We documented the feature in D75655 and there it says "all classes" (and still does).

I've updated the documentation. It was already slightly inaccurate in that we weren't emitting type test/assumes for std/stdext namespace classes when -flto-visibility-public-std, this now corrects that part and generalizes that part of the documentation.

I prefer the way proposed here, which was what I initially intended to do in fact. @pcc, I recall from our early internal conversations about my proposal that you felt the new mechanism should apply to all classes, so that's why the final design did that. I do tend to think that if the source code has specifically annotated the classes with public LTO visibility it is better to enforce that always. I think revisiting this decision is worthwhile.

pcc accepted this revision.Jun 15 2022, 4:46 PM

Okay, it seems reasonable enough to have the [[clang::lto_visibility_public]] attribute override the --lto-whole-program-visibility flag.

What I'm not sure about though is whether __declspec(uuid) and std/stdext in /MT//MTd should also override it.

Fortunately we don't need to solve that now because those are Windows-specific attributes/features and we don't support passing --lto-whole-program-visibility to Windows linkers.

So LGTM if you only mention [[clang::lto_visibility_public]] in your change to the documentation.

This revision is now accepted and ready to land.Jun 15 2022, 4:46 PM
aeubanks updated this revision to Diff 437420.Jun 15 2022, 6:57 PM

update docs to only mention [[clang::lto_visibility_public]]

This revision was landed with ongoing or failed builds.Jun 16 2022, 9:39 AM
This revision was automatically updated to reflect the committed changes.