This is an archive of the discontinued LLVM Phabricator instance.

[WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI
ClosedPublic

Authored by modimo on Jul 18 2023, 4:01 PM.

Details

Summary

Discussion about this approach: https://discourse.llvm.org/t/rfc-safer-whole-program-class-hierarchy-analysis/65144/18

When enabling WPD in an environment where native binaries are present, types we want to optimize can be derived from inside these native files and devirtualizing them can lead to correctness issues. RTTI can be used as a way to determine all such types in native files and exclude them from WPD providing a safe checked way to enable WPD.

The approach is:

  1. In the linker, identify if RTTI is available for all native types. If not, under --lto-validate-all-vtables-have-type-infos --lto-whole-program-visibility is automatically disabled. This is done by examining all .symtab symbols in object files and .dynsym symbols in DSOs for vtable (_ZTV) and typeinfo (_ZTI) symbols and ensuring there's always a match for every vtable symbol.
  2. During thinlink, if --lto-validate-all-vtables-have-type-infos is set and RTTI is available for all native types, identify all typename (_ZTS) symbols via their corresponding typeinfo (_ZTI) symbols that are used natively or outside of our summary and exclude them from WPD.

Testing:
ninja check-all
large Meta service that uses boost, glog and libstdc++.so runs successfully with WPD via --lto-whole-program-visibility. Previously, native types in boost caused incorrect devirtualization that led to crashes.

Diff Detail

Event Timeline

modimo created this revision.Jul 18 2023, 4:01 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: hoy, ormris, wenlei and 5 others. · View Herald Transcript
modimo requested review of this revision.Jul 18 2023, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 4:01 PM
modimo edited the summary of this revision. (Show Details)Jul 18 2023, 4:16 PM
modimo added a reviewer: tejohnson.

Currently this is only implemented on thinLTO as there's not a monoLTO use-case for this at Meta currently. If design-wise we want to keep parity I'm happy to add the pieces to support this in monoLTO/hybrid as well.

Thanks for the patch. I have some initial comments below. From my reading, I guess any native object with a vtable and no RTTI will disable WPD globally, which is unfortunate. Although I do have a suggestion below for making this slightly less pessimistic. I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

llvm/lib/LTO/LTO.cpp
1733

Would be better to have a more specific name, since this is only queried with type names. I.e. local symbols are not visible outside the summary but don't have a GlobalResolution entry. But you aren't calling this lambda in that case (but that isn't clear, where the lambda is defined). Before I suggest a name, I have a question about the usage of this lambda down in the WPD code.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

Rather than doing this down here in LTO/WPD could the linker simply unset the HasWholeProgramVisibility config flag? That would also allow WPD to proceed on types with hidden LTO visibility. This early return would prevent any and all WPD which seems overly conservative in the case of hidden LTO visibility classes.

2521

Can we ever get here if RTTI is not enabled? My understanding of the change to line 2413 is that we return early in that case. Given that early return, aren't we guaranteed that the typename symbol has a GlobalResolution if it is non-local?

Oh - I guess we are only early returning if RTTI is off in native objects, so you could get here if RTTI is only disabled in bitcode objects? And we need to be conservative for any typenames for vtables defined in bitcode objects with RTTI off? I didn't see a test for this case, can you add one (or did I miss it)?

Thanks for taking a look!

Thanks for the patch. I have some initial comments below. From my reading, I guess any native object with a vtable and no RTTI will disable WPD globally, which is unfortunate.

It makes sense from a correctness point of view but yeah it is a stringent requirement. The linker warning does keep this from being silent although how much churn this causes remains to be seen.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

That makes sense although it does tie this flag's functionality to requiring --lto-whole-program-visibility.

Doing that though means we can instead pass the blocklist to updateVCallVisibilityInIndex/updateVCallVisibilityInModule similarly to how D91583 does it for dynamically exported symbols which would be cleaner. Thoughts on that approach?

2521

Oh - I guess we are only early returning if RTTI is off in native objects, so you could get here if RTTI is only disabled in bitcode objects?

Yep!

And we need to be conservative for any typenames for vtables defined in bitcode objects with RTTI off?

This is primarily an implementation detail with how resolutions are only provided for IR symbols. If we instead pass more information from the linker (like resolutions for these summary symbols or the whole list of typenames) we can support bitcode files with RTTI off. There's not a correctness issue at play here since the summary information is a superset of RTTI.

I didn't see a test for this case, can you add one (or did I miss it)?

Good catch, will add a test case.

tejohnson added inline comments.Jul 24 2023, 2:58 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

That makes sense although it does tie this flag's functionality to requiring --lto-whole-program-visibility.

What would be the use case of the proposed handling without --lto-whole-program-visibility? Are you saying that there are cases where the normal LTO visibility is incorrect?

Doing that though means we can instead pass the blocklist to updateVCallVisibilityInIndex/updateVCallVisibilityInModule similarly to how D91583 does it for dynamically exported symbols which would be cleaner. Thoughts on that approach?

Yep I think that would be cleaner.

modimo added inline comments.Jul 24 2023, 3:59 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

What would be the use case of the proposed handling without --lto-whole-program-visibility? Are you saying that there are cases where the normal LTO visibility is incorrect?

I don't have a known case so this is more theoretical. Currently there's an assertion that it's on the user to make sure LTO visibility is correct but in this case and in D91583 we can catch violations and prevent them from causing problems. How much this should also apply to normal LTO visibility is a question but thinking more about it is orthogonal to this change.

modimo added inline comments.Jul 24 2023, 7:04 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

Ah right, the issue with doing this in updateVCallVisibilityInIndex/updateVCallVisibilityInModule is that vcall visibility is keyed off the vtable symbol. However, TypeID and RTTI are both keyed off of the typename symbol. There's not always a translation from typename to vtable since abstract base classes wouldn't have vtables and by the time we get the association we're in the same place the logic is right now.

Given that, I think I'll keep the logic as-is.

modimo updated this revision to Diff 543786.Jul 24 2023, 7:15 PM

Add test case for no RTTI in LTO Unit and explicitly hidden LTO types. Change logic to disable --lto-whole-program-visibility on validation failure. Change isVisibleOutsideSummary to typeInfoVisibleOutsideSummary.

I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).

lld/ELF/Driver.cpp
1054

Does this get both defs and refs? I think the latter as I am seeing a case where we are linking a bitcode object that contains both the vtable and typename defs but are disabling --lto-whole-program-visibility, and the reason seems to be a reference to the vtable from a native object.

1114

Prefer message() over warn() because the latter causes builds using -Werror to fail.

llvm/lib/LTO/LTO.cpp
1728–1729

Can you do the same for updateVCallVisibilityInModule to get this fix to apply to regular LTO?

1733

nit: lambda name should be upper camel case.

Also, can you add a comment here that this will return true for either the case where name is a local or where it is not defined, and so the expectation is that it will not be queried for local symbols.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

There's not always a translation from typename to vtable since abstract base classes wouldn't have vtables and by the time we get the association we're in the same place the logic is right now.

Can you clarify the case you are concerned about and how this is handled in the lld code that expects a translation from vtable to typename? I tried an example with an abstract base class and do get a vtable and typename.

I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).

Nice! That mirrors my experience as well.

lld/ELF/Driver.cpp
1114

Is that the case for lld? Looking through the equivalent functionality is under --fatal-warnings and on a small example -Werror doesn't affect this flag/cause the build to fail.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

Inputs/devirt_validate_vtable_typeinfos.ll has A as an abstract base class and Native deriving from it. Building down to an object file we get a vtable/typeinfo/typeid symbol for Native but only the typeinfo/typeid symbol for A:

~/llvm-project/lld/test/ELF/lto# ~/llvm-project/build-rel/bin/llc -filetype=obj Inputs/devirt_validate_vtable_typeinfos.ll -o devirt_validate_vtable_typeinfos.o
~/llvm-project/lld/test/ELF/lto# readelf -Ws devirt_validate_vtable_typeinfos.o | grep ZT
     5: 0000000000000000    16 OBJECT  WEAK   DEFAULT    3 _ZTVN10__cxxabiv117__class_type_infoE
     6: 0000000000000010    16 OBJECT  WEAK   DEFAULT    3 _ZTVN10__cxxabiv120__si_class_type_infoE
     7: 0000000000000020    32 OBJECT  WEAK   DEFAULT    3 _ZTV6Native
     8: 0000000000000050    24 OBJECT  WEAK   DEFAULT    3 _ZTI6Native
     9: 0000000000000040     8 OBJECT  WEAK   DEFAULT    3 _ZTS6Native
    10: 0000000000000070    16 OBJECT  WEAK   DEFAULT    3 _ZTI1A
    11: 0000000000000068     3 OBJECT  WEAK   DEFAULT    3 _ZTS1A

In LLD we're ensuring there's a map from every vtable symbol to its type information but we expect additional type information without vtables for these cases.

modimo added inline comments.Jul 25 2023, 5:32 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

That being said, the information to map typeid->[associated vtables] is typeIdCompatibleVtableMap for thinLTO and in full LTO we have the actual vtable global variables which contains !type metadata that maps back to typeid.

For thinLTO, we can save a scan of typeIdCompatibleVtableMap for updateVCallVisibilityInIndex by currently combining it with the scan in DevirtIndex::run however that's probably not a big deal.

For full LTO the information is better passed through updateVCallVisibilityInModule since we don't build the corresponding TypeIDMap until codegen and carrying this information around until then is too much.

I think I've come back around to doing it in updateVCallVisibilityInIndex/updateVCallVisibilityInModule. A little less efficient for thinLTO but keeps consistency with full LTO.

modimo added inline comments.Jul 25 2023, 11:17 PM
lld/ELF/Driver.cpp
1054

Does this get both defs and refs?

This goes through the entirety of .symtab so both defs and refs.

I think the latter as I am seeing a case where we are linking a bitcode object that contains both the vtable and typename defs but are disabling --lto-whole-program-visibility, and the reason seems to be a reference to the vtable from a native object.

Mocking up a forward class declaration in the native object where the definition is in bitcode I see --lto-whole-program-visibility get disabled. Only looking at vtable defs makes sense, I'll make the change and add a test.

tejohnson added inline comments.Jul 26 2023, 9:06 AM
lld/ELF/Driver.cpp
1114

Sorry, you are correct. We use --fatal-warnings for this on linker actions (and -Werror on compile actions). In general, I think this should be an informational message, not a warning, since it is being handled automatically.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

Ok, thanks. I think I have lost track of what the change will be - is it to replace passing down a global flag AllVtablesHaveTypeInfos, or is it to replace what is being down below in DevirtIndex::run()? For the former alone it doesn't seem worth it, but it would be nice to move the handling from DevirtIndex::run() into the vcall_visibility updates.

modimo added inline comments.Jul 26 2023, 10:58 AM
lld/ELF/Driver.cpp
1114

The case I want to catch is when an existing project changes its native dependencies and disables the optimization which would better fit a warning. In the general case agreed that this isn't a warning. I don't have much experience in linker protocol here, @MaskRay thoughts?

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
2455

Ah sorry yeah there's 2 different things. For passing down AllVtablesHaveTypeInfos I like the approach of unsetting --lto-whole-program-visibility instead. For moving the safety logic out of DevirtIndex::run() making this work for full LTO wants the logic to be in vcall_visibility and it makes sense to be consistent even if slightly *less* efficient for thinLTO.

modimo updated this revision to Diff 544923.Jul 27 2023, 2:07 PM

Move logic to updateVCallVisibility*, apply change to full and split LTO, check only defs in linker.

modimo marked 4 inline comments as done.Jul 27 2023, 2:09 PM
modimo updated this revision to Diff 544926.Jul 27 2023, 2:15 PM

Remove unintentional change to WholeProgramDevirt.cpp

tejohnson added inline comments.Jul 29 2023, 7:51 AM
lld/ELF/Driver.cpp
1114

The problem is if it is a warning, then we have to go in and manually change options or builds will fail. Can you intercept message output, which should always be emitted (i.e. don't need verbose options).

lld/test/ELF/lto/devirt_validate_vtable_typeinfos_no_rtti.ll
9

The earlier version didn't have this second input file - why is it needed now for this test?

llvm/include/llvm/LTO/Config.h
85

This would read better like "If all native vtables have corresponding type infos, allow usage..."

llvm/include/llvm/LTO/LTO.h
367 ↗(On Diff #544926)

What's the downside in practice with using VisibleOutsideSummary?

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
799

Just early return true here, and return false below the loop.

898

Suggest naming this VisibleToRegularObjVTables.

915

Hmm, now that I think about it, shouldn't local symbols have gotten VCallVisibilityTranslationUnit from clang? Same question in the regularLTO handling.

modimo marked 2 inline comments as done.EditedJul 31 2023, 3:37 PM

I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).

For clarification, were the builds only to validate the linker check? If so, are there plans to try out the E2E solution?

lld/ELF/Driver.cpp
1114

Sure, changed to message.

lld/test/ELF/lto/devirt_validate_vtable_typeinfos_no_rtti.ll
9

Good catch, I re-used the index/hybrid/full commands from devirt_validate_vtable_typeinfos.ll and that came along for the ride, removed.

llvm/include/llvm/LTO/LTO.h
367 ↗(On Diff #544926)

It doesn't extend to Full LTO. However, the same functionality for full LTO is captured with GlobalResolutions[name].Partition == GlobalResolution::External so this doesn't need to be broken out separately.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
898

Another area that's unknown for thinLTO are the types used in full LTO where VisibleOutsideSummary is set to false and vice-versa with full LTO where types used in thinLTO get GlobalResolution::External partition. I'm thinking then to categorize vtables we want to not upgrade as something like RefOutsideWPD instead of VisibleToRegularObj. WDYT?

915

Good point. Originally excluding based on type name had to explicitly take into account local vcall_visibility. Now, it's a test setup where VCallVisibilityTranslationUnit was only used for one of the local types. I'll remove this check and the type that doesn't have the proper vcall_visibility in the tests.

modimo updated this revision to Diff 545835.Jul 31 2023, 3:37 PM

Review Feedback

modimo updated this revision to Diff 545868.Jul 31 2023, 4:59 PM

Exclude .virtual typeIDs

I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).

For clarification, were the builds only to validate the linker check? If so, are there plans to try out the E2E solution?

I was just looking for the linker check messages, I didn't enable WPD. With this solution in place we can hopefully try it out internally, but it might not be immediate. However, I'm keen to have this solution available so we can move forward on WPD!

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
898

RegularLTO summaries are added to the combined index used by ThinLTO, but it looks like the vtable summaries aren't currently created for them. I think you are right in that there is a potential hole here for ThinLTO WPD if linked with a regular LTO object containing an override. Can you test this case to confirm? If that is an issue, then I guess we do need another GlobalRes field. Maybe VisibleOutsideLTOUnit or something like that?

modimo added a comment.Aug 6 2023, 8:03 PM

I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them. I did find a couple that seem spurious (see comment inline below about this).

For clarification, were the builds only to validate the linker check? If so, are there plans to try out the E2E solution?

I was just looking for the linker check messages, I didn't enable WPD. With this solution in place we can hopefully try it out internally, but it might not be immediate. However, I'm keen to have this solution available so we can move forward on WPD!

Thanks for the clarification! Definitely very interested in your results internally when this finalizes.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
898

Added devirt_validate_vtable_typeinfos_mixed_lto.ll to test mixing LTO modes:

  1. RegularLTO without summary indeed does not export vtable summaries. With validation, because the type is present in a ThinLTO module the partition is set to GlobalResolution::External this type does not get its visibility upgraded in RegularLTO and with VisibleOutsideSummary also set to true this type does not get its visibility upgraded in ThinLTO either.
  2. RegularLTO with summary we do get all the vtable summaries in the combined index. With validation, GlobalResolution::External blocks RegularLTO visibility upgrade but since VisibleOutsideSummary is not set everything is optimized in the combined index.

For the purposes of validation I think this is the behavior we want. It does seem like we may want to fix this hole even without validation enabled although that seems more of a separate change since it alters baseline behavior.

modimo updated this revision to Diff 547631.Aug 6 2023, 8:04 PM
modimo added a comment.Aug 6 2023, 8:04 PM

Add devirt_validate_vtable_typeinfos_mixed_lto.ll, minor test changes

tejohnson added inline comments.Aug 10 2023, 1:08 PM
lld/test/ELF/lto/devirt_validate_vtable_typeinfos_mixed_lto.ll
118

Both this and the CHECK-SUMMARY-IR case below are incorrect devirtualizations, right? Is this another case that we are not doing correctly without the validation options in this patch?

124

I think we only get the vtable summary from the regular LTO object because it doesn't have the EnableSplitLTOUnit module flag set in the IR here. Normally, this is added by clang when building -flto. And this currently prevents vtable summaries being added to the LTO summary (https://github.com/llvm/llvm-project/blob/8a15bdb5e637f81041591d97bea0267b5f053f16/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L734-L736).

When I added that guard, it was because I didn't think we needed these summaries when splitting was enabled, as I was thinking of either the everything-is-regular LTO case or the -fsplit-lto-unit case that you get by default with -flto=thin -fwhole-program-vtables, where all the vtables are placed in the regular LTO split modules. It's possible that we could remove that guard, but with it I think this case would do the wrong thing if the regular IR was built from clang with -flto.

Headed out on PTO and will be back on the 24th, will pick this back up then!

lld/test/ELF/lto/devirt_validate_vtable_typeinfos_mixed_lto.ll
124

I see, so the BASE scenario being tested here is already guarded by EnableSplitLTOUnit since ThinLTO would have EnableSplitLTOUnit=0 and RegularLTO would have EnableSplitLTOUnit=1. Is this the scenario described in the previous comment?

RegularLTO summaries are added to the combined index used by ThinLTO, but it looks like the vtable summaries aren't currently created for them. I think you are right in that there is a potential hole here for ThinLTO WPD if linked with a regular LTO object containing an override. Can you test this case to confirm? If that is an issue, then I guess we do need another GlobalRes field. Maybe VisibleOutsideLTOUnit or something like that?

The mixed case then would be RegularLTO combined with ThinLTO + -split-lto-unit where neither generate typeidCompatibleVTable and all the analysis is done on the combined RegularLTO module.

Looking at https://github.com/llvm/llvm-project/blob/9b6b6bb/clang/lib/CodeGen/BackendUtil.cpp#L170-L173, RegularLTO will have a summary attached through Clang by default except for ld64 targets. The mixed case then reduces to RegularLTO+Summary and ThinLTO+Split since EnableSplitLTOUnit must be consistent so everything is done by DevirtModule::run and there's no mixture with DevirtIndex::run. The mechanism for IsVisibleToRegularObj for RegularLTO also ends up being identical to that of ThinLTO since all symbols will be present in the combined summary.

For ld64 targets there is a potential hole mixing RegularLTO+NoSummary with ThinLTO which would be caught with the current validation scheme. However, the validation functionality is only for the lld ELF target so the ld64 targets are unchanged.

modimo updated this revision to Diff 553602.Aug 25 2023, 1:39 PM

Change mixed scenario to RegularLTO+Summary and ThinLTO+Split. Modify other tests to have RegularLTO+Summary.

modimo added inline comments.Aug 25 2023, 1:53 PM
lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll
7

Appending module flags so RegularLTO correctly generates it's summary without typeidCompatibleVTable means the test can be re-used. However I think duplicating the tests is reasonable as well and could be cleaner, WDYT?

modimo updated this revision to Diff 553610.Aug 25 2023, 1:56 PM

Set EnableSplitLTOUnit=1 for RegularLTO tests as well

tejohnson accepted this revision.Aug 29 2023, 9:26 AM

lgtm with a couple comments/suggestions below. Thanks!

Looking at https://github.com/llvm/llvm-project/blob/9b6b6bb/clang/lib/CodeGen/BackendUtil.cpp#L170-L173, RegularLTO will have a summary attached through Clang by default except for ld64 targets. The mixed case then reduces to RegularLTO+Summary and ThinLTO+Split since EnableSplitLTOUnit must be consistent so everything is done by DevirtModule::run and there's no mixture with DevirtIndex::run. The mechanism for IsVisibleToRegularObj for RegularLTO also ends up being identical to that of ThinLTO since all symbols will be present in the combined summary.

Ok, yes - I think the scenario I was worried about is caught by the existing verification that EnableSplitLTOUnit is set consistently.

lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll
7

Do we need these module flags for correct operation of this test (ditto for the similar no_rtti one later)? If not, then probably don't bother adding in these tests (I think these may only be needed in practice for the hybrid testing). If they are now needed for correct operation of the regular LTO testing, then I am ok with the approach here as I think it is probably better to reduce duplication of nearly identical IR tests (and I see this approach used in other tests too).

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
893

nit: the braces can be removed from the for loop

This revision is now accepted and ready to land.Aug 29 2023, 9:26 AM

Thanks for the patch. Will try to read through this :)

modimo updated this revision to Diff 554490.Aug 29 2023, 2:03 PM
modimo marked an inline comment as done.

Fix nit: braces

Thanks for the review!

lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll
7

Do we need these module flags for correct operation of this test (ditto for the similar no_rtti one later)?

Yeah, to trigger summary generation but on the RegularLTO pipeline requires these module flags.

If they are now needed for correct operation of the regular LTO testing, then I am ok with the approach here as I think it is probably better to reduce duplication of nearly identical IR tests (and I see this approach used in other tests too).

Sounds good, I'll leave it as is.

Will study...

lld/ELF/Driver.cpp
1114

This long list here makes me nervous. Will try to learn it.

lld/ELF/Options.td
607

New options use EEq to disallow single-dash long options, to not conflict with -l.

lld/test/ELF/lto/Inputs/devirt_validate_vtable_typeinfos.ll
5

grtev4 => unknown

llvm/tools/opt/opt.cpp
573

The prevailing and recommended style liked by clang-format and clang-tidy is

/*WholeProgramVisibilityEnabledInLTO=*/false

modimo updated this revision to Diff 555114.Aug 31 2023, 11:30 AM

Review Feedback

modimo marked 3 inline comments as done.Aug 31 2023, 11:30 AM
modimo added inline comments.Aug 31 2023, 3:41 PM
lld/ELF/Driver.cpp
1114

Taking a look again I don't think these names need special exclusion. They're defined in libstdc++/libc++abi and the release packages in my scenarios have RTTI enabled meaning their type info symbols are present during linking. Testing E2E linking on some of our large services with these removed succeeds.

If RTTI is disabled on these libraries considerably more symbols would not have matching type infos and --lto-whole-program-visibility should be disabled. Looking back I think these exclusions came about when I was only examining the .symtab/.dynsym of individual object/shared files which didn't take into account that these symbols would be resolved by libstdc++/libc++abi.

modimo updated this revision to Diff 555188.Aug 31 2023, 3:41 PM

Remove explicit knownSafeVtableNames entries

modimo edited the summary of this revision. (Show Details)Sep 5 2023, 3:42 PM

Gentle ping @MaskRay

Sorry for the delay. (There were Phabricator issues to handle beside work...)

I will need to re-read https://discourse.llvm.org/t/rfc-safer-whole-program-class-hierarchy-analysis/65144 and an internal discussion in May 2022 when I was thinking about _ZTI.

-frtti is discouraged by https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ , so I think it may not benefit us... but this feature is still useful. I need to read these discussions...

lld/ELF/Driver.cpp
1046

omit braces for single-line single-statement body

1076

drop the two nested braces

1080
1083

If not starting with _ZTV, consider reporting an error?

I wonder why the following example is still incorrect. I haven't carefully studied the LTO part of code. It seems that you do handle isUsedInRegularObj.

cat > a.h <<'eof'
struct A { virtual int foo(); };
int bar(A *a);
eof
cat > a.cc <<'eof'
#include "a.h"
int A::foo() { return 1; }
int bar(A *a) { return a->foo(); }
eof
cat > b.cc <<'eof'
#include "a.h"
struct B : A { int foo() { return 2; } };
int baz() { B b; return bar(&b); }
eof
cat > main.cc <<'eof'
#include "a.h"
#include <stdio.h>
extern int baz();
int main() {
  A a;
  printf("%d %d\n", bar(&a), baz());
}
eof

clang++ -c -flto=thin -fwhole-program-vtables -O main.cc a.cc b.cc
clang++ -c -O b.cc -o b0.o
% clang++ -flto=thin -Wl,--lto-whole-program-visibility -fuse-ld=lld main.o a.o b.o && ./a.out
1 2
% clang++ -Wl,--lto-validate-all-vtables-have-type-infos -flto=thin -Wl,--lto-whole-program-visibility -fuse-ld=lld main.o a.o b0.o && ./a.out
1 1
lld/ELF/Config.h
251

Move ltoAllVtablesHaveTypeInfos (not an option) to Ctx

lld/ELF/Driver.cpp
1055

getGlobalELFSyms to skip local symbols

1087

and delete the assignment below

2879
lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll
14

You can remove the relocation-model=static object file as there is no testable difference.

Then, consider renaming %t2_pic.o to %t2.o

85
; VALIDATE-NOT: single-impl:
; VALIDATE:     single-impl: devirtualized a call to _ZN1D1mEi
; VALIDATE-NOT: single-impl:
164

Consider pasting the source code as well for readability and upgradability?

I haven't carefully studied the tests yet...

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
795

typo: will will

Add a period.

888

typo: will will

append a period.

893

delete braces in this nested case when the only body has just one line.

MaskRay requested changes to this revision.Sep 6 2023, 9:24 PM
MaskRay added inline comments.
lld/ELF/Driver.cpp
1089

the order is not guaranteed to be deterministic. Consider a SmallSetVector with inline size=0.

auto & => StringRef

This revision now requires changes to proceed.Sep 6 2023, 9:24 PM

Gentle ping @MaskRay

Sorry for the delay. (There were Phabricator issues to handle beside work...)

I will need to re-read https://discourse.llvm.org/t/rfc-safer-whole-program-class-hierarchy-analysis/65144 and an internal discussion in May 2022 when I was thinking about _ZTI.

-frtti is discouraged by https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ , so I think it may not benefit us... but this feature is still useful. I need to read these discussions...

I looked at this more closely after this patch was mailed. While use of RTTI is discouraged, building with -fno-rtti isn't specifically encouraged afaict, and when I built a large number of important binaries internally with this patch in validation mode, it turned out that there were only a few objects/symbols affected, and we could allowlist them to enable WPD. See my earlier comment from July 25:

I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them.

I wonder why the following example is still incorrect.

Hmm, that seems like exactly the case that should be caught and handled automatically by this patch. Oh, I just compiled the same source code to a native object and nm shows a reference to the typeinfo for A, but no type name for A (_ZTS1A):

$ nm b.o
                 U _Z3barP1A
0000000000000000 T _Z3bazv
0000000000000000 W _ZN1B3fooEv
                 U _ZTI1A
0000000000000000 V _ZTI1B
0000000000000000 V _ZTS1B
0000000000000000 V _ZTV1B
                 U _ZTVN10__cxxabiv120__si_class_type_infoE

The _ZTS symbol is the one referenced by the type metadata, and what this patch is going to look for being referenced in native objects. Not familiar with the rules around when that symbol ends up being used, but if it isn't consistently referenced from native objects then this solution isn't going to work as well as hoped. Should it be looking for either the type name or the corresponding type info?

modimo added a comment.EditedSep 7 2023, 2:43 PM

Good catch with the example! Looks like this is an interaction with the class A having a key function (https://lld.llvm.org/missingkeyfunction.html) defined in a.cc so b.cc doesn't generate a vtable symbol for class A and RTTI only emits a reference to _ZTI1A. The Itanium C++ ABI mandates type name as a field for every type info (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#rtti) but because we only get a reference _ZTS1A doesn't come along for the ride. The vtable for class A being defined only inside the LTO Unit then means there's no native reference to key off of.

I think this means native symbol lookup should be keyed off the type info symbol corresponding to the type name we have in metadata. The layout of RTTI guarantees we'll have the base type info symbol(s) but as seen here not necessarily the type name symbol. WDYT?

modimo updated this revision to Diff 556213.Sep 7 2023, 4:35 PM
modimo marked 13 inline comments as done.

Review Feedback

Good catch with the example! Looks like this is an interaction with the class A not having a key function (https://lld.llvm.org/missingkeyfunction.html) so b.cc doesn't generate a vtable symbol for class A and RTTI only emits a reference to _ZTI1A. The Itanium C++ ABI mandates type name as a field for every type info (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#rtti) but because we only get a reference _ZTS1A doesn't come along for the ride. The vtable for class A being defined only inside the LTO Unit then means there's no native reference to key off of.

I think this means native symbol lookup should be keyed off the type info symbol corresponding to the type name we have in metadata. The layout of RTTI guarantees we'll have the base type info symbol(s) but as seen here not necessarily the type name symbol. WDYT?

This seems ok to me - there is already code in the lld part of this patch that maps _ZTV to _ZTI, so mapping _ZTS to _ZTI is not significantly different. Can you also add this as a test case (suggest including the original c++ code in a comment).

modimo marked an inline comment as not done.Sep 8 2023, 6:15 PM
modimo added inline comments.
lld/test/ELF/lto/devirt_validate_vtable_typeinfos.ll
164

I pulled the base IR from lld/test/ELF/lto/devirt_vcall_vis_export_dynamic.ll and modified it so no direct source.

The test case is effectively:

struct A {
  virtual void f(int) = 0;
  virtual void n(int) { return 0; }
};

struct B {
  virtual void f(int) { return 0; }
};

struct C {
  virtual void f(int) { return 0; }
};

namespace {
  struct D {
    virtual void int m(int) { return 0; }
  };
}

int _start(A *obj, D* obj2, int a) {
  call = obj->n(a); // single implementation in A, devirtualize unless a native type derives from A
  call2 = obj->f(call); // multiple implementation in B and C, never devirtualize
  call3 = obj2->m(call2); // local type, always devirtualize
  return call3;
}
modimo updated this revision to Diff 556329.Sep 8 2023, 6:28 PM

Key off of type info (_ZTI) symbols, add test case

modimo updated this revision to Diff 556723.Sep 13 2023, 2:30 PM

Move symbol check to common function

MaskRay accepted this revision.Sep 17 2023, 7:34 PM

Looks great with some nits! Checking _ZTS in TypeIDVisibleToRegularObj (switch to typeIDVisibleToRegularObj) looks good to me. Sorry for the delay.
There are a number of resolved comments you may want to mark as done.

lld/ELF/Driver.cpp
1042

The conventional style in lld/ omits llvm:: for DenseSet.

1051
1073

This still relies on the iteration order of DenseSet vtableSymbols. We need SetVector for vtableSymbols as well.

auto &s => StringRef s

2879

Not done.

lld/ELF/Options.td
607

When --lto-validate-all-vtables-have-type-infos is enabled, skip validation on these vtables (_ZTV symbols)

llvm/lib/LTO/LTO.cpp
1282

redundant hash table lookup here.

Better to use find(name) with slightly more code

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
785
796

Use consume_front here to avoid consume_front below.

805

to avoid constructing a possibly heap-allocated std::string twice.

809

skipUpdateDueToValidation

This revision is now accepted and ready to land.Sep 17 2023, 7:34 PM
MaskRay added inline comments.Sep 17 2023, 7:34 PM
lld/ELF/Config.h
481

We need ltoAllVtablesHaveTypeInfos = false in reset

modimo marked 27 inline comments as done.Sep 18 2023, 2:22 PM

Looks great with some nits! Checking _ZTS in TypeIDVisibleToRegularObj (switch to typeIDVisibleToRegularObj) looks good to me. Sorry for the delay.
There are a number of resolved comments you may want to mark as done.

Appreciate the thorough review! Good callout on the resolved comments, keeping those up to date keeps the context clearer for everyone involved--will keep them updated in the future.

llvm/lib/LTO/LTO.cpp
1282

Good call, other places here also use the find pattern.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
785

Sounds good, I'll follow up with the correct style on the rebase

modimo updated this revision to Diff 556975.Sep 18 2023, 2:35 PM
modimo marked 2 inline comments as done.

Review Feedback

modimo edited the summary of this revision. (Show Details)Sep 18 2023, 3:51 PM
This revision was landed with ongoing or failed builds.Sep 18 2023, 3:52 PM
This revision was automatically updated to reflect the committed changes.