This is an archive of the discontinued LLVM Phabricator instance.

[WPD][lld] Test handling of vtable definition from shared libraries
ClosedPublic

Authored by tejohnson on Feb 15 2021, 9:51 AM.

Details

Summary

Adds a lld test for a case that the handling added for dynamically
exported symbols in 1487747e990ce9f8851f3d92c3006a74134d7518 already
fixes. Because isExportDynamic returns true when the symbol is
SharedKind with default visibility, it will treat as dynamically
exported and block devirtualization when the definition of a vtable
comes from a shared library. This is desireable as it is dangerous to
devirtualize in that case, since there could be hidden overrides in the
shared library. Typically that happens when the shared library header
contains available externally definitions, which applications can
override. An example is std::error_category, which is overridden in LLVM
and causing failures after a self build with WPD enabled, because
libstdc++ contains hidden overrides of the virtual base class methods.

The regular LTO case in the new test already worked, but there are
2 fixes in this patch needed for the index-only case and the hybrid
LTO case. For the index-only case, WPD should not simply ignore
available externally vtables. A follow on fix will be made to clang to
emit type metadata for those vtables, which the new test is modeling.
For the hybrid case, we need to ensure when the module is split that any
llvm.*used globals are cloned to the regular LTO split module so
available externally vtable definitions are not prematurely deleted.

Another follow on fix will add the equivalent gold test, which requires
a small fix to the plugin to treat symbols in dynamic libraries the same
way lld already is.

Diff Detail

Event Timeline

tejohnson created this revision.Feb 15 2021, 9:51 AM
tejohnson requested review of this revision.Feb 15 2021, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 9:51 AM

(I did not know much about WholeProgramDevirt but I did spend some time to get an idea what the pass does.)

Just adding the following code

if (!VS)
      return false;

without introducing CurVS can make the new test pass as well. Is the CurVS change needed?

Also, regarding if (!VS) return false;, can TargetsForSlot.push_back(VTP.FuncVI); add an element in a previous iteration before if (!VS) return false; triggers?

lld/test/ELF/lto/devirt_vcall_vis_shared_def.ll
17

Nit: not reusing the same output filename for different opt commands may make debugging easier.

While debugging this to understand what happens, I need to run a specific ld.lld command and have to confirm the %t1.o object file is produced by the desired opt command...

34

%t5.o is the same as %t2.o

40

FileCheck /dev/null ... --allow-empty can be replaced by count 0

90

Drop ;

ditto below

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

(I did not have WholeProgramDevirt but I did spend some time to get an idea what the pass does.)

Just adding the following code

if (!VS)
      return false;

without introducing CurVS can make the new test pass as well. Is the CurVS change needed?

tejohnson marked 4 inline comments as done.Feb 16 2021, 10:49 PM

I've addressed all the test suggestions, reply to the source code question below.

(I did not know much about WholeProgramDevirt but I did spend some time to get an idea what the pass does.)

Thanks!

Just adding the following code

if (!VS)
      return false;

without introducing CurVS can make the new test pass as well. Is the CurVS change needed?

Probably, but comment further below about why I think this change is still good to have.

Also, regarding if (!VS) return false;, can TargetsForSlot.push_back(VTP.FuncVI); add an element in a previous iteration before if (!VS) return false; triggers?

It could, for another vtable compatible with the current type id, but it will be ignored. The caller doesn't do anything with TargetsForSlot if this method returns false.

lld/test/ELF/lto/devirt_vcall_vis_shared_def.ll
17

Modified everything to be unique.

34

Yep, and with the unique names used now I can also avoid re-doing the rest of the opt invocations below.

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

For correctness reasons, I think you are right in that we could probably just ignore all available_externally vtables associated with the current type id - if we only have available_externally copies of a particular vtable then there's no way we should promote. But after thinking through this some more, it makes sense to use the new check I've added there for compile time reasons - if we have many copies of an available_externally vtable (possible since this may come from a header that is widely included), if the symbol is defined in a library and was thus marked ExternDynamic, with the new code we don't waste time walking through all copies but will return false early on the first copy (which will have public visibility).

tejohnson marked 2 inline comments as done.

Address comments in test

MaskRay accepted this revision.Feb 17 2021, 10:46 AM
This revision is now accepted and ready to land.Feb 17 2021, 10:46 AM