Page MenuHomePhabricator

[WPD][ELF] Allow whole program devirtualization for version script localized symbols
ClosedPublic

Authored by MaskRay on Mar 8 2021, 2:13 PM.

Details

Summary

A local: version node in a version script can change the effective symbol binding
to STB_LOCAL. The linker needs to communicate the fact to enable WPD
(otherwise LTO does not know that the !vcall_visibility metadata has
effectively changed from VCallVisibilityPublic to VCallVisibilityLinkageUnit).

Diff Detail

Event Timeline

MaskRay created this revision.Mar 8 2021, 2:13 PM
MaskRay requested review of this revision.Mar 8 2021, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 2:13 PM
tejohnson added inline comments.Mar 8 2021, 2:38 PM
lld/test/ELF/lto/devirt_vcall_vis_hidden.ll
25 ↗(On Diff #329140)

These hidden symbols should start out with Linkage Unit LTO visibility (CodeGenModule::GetVCallVisibilityLevel)

lanza added a comment.EditedMar 8 2021, 3:38 PM

Thanks Fangrui! A cherry-pick and test and local run makes it seem like it works.

lanza added inline comments.Mar 8 2021, 3:45 PM
lld/test/ELF/lto/devirt_vcall_vis_hidden.ll
25 ↗(On Diff #329140)

Yea, this shouldn't have !vcall_visibility. A version script could be used here to say that _ZTV1B is global but _ZTV1A is local.

{
    global:
    _ZTV1B;
    local:
    *;
};
tejohnson added inline comments.Mar 8 2021, 4:10 PM
lld/test/ELF/lto/devirt_vcall_vis_hidden.ll
25 ↗(On Diff #329140)

Well right now they are marked with public visibility, which is the same thing as not having any vcall_visibility. Since they are marked in the IR as having hidden visibility, clang presumably should have marked them with a more restricted vcall_visibility (linkage unit level).

MaskRay updated this revision to Diff 329160.Mar 8 2021, 4:25 PM
MaskRay retitled this revision from [WPD][ELF] Allow whole program devirtualization for hidden/internal symbols to [WPD][ELF] Allow whole program devirtualization for version script localized symbols.
MaskRay edited the summary of this revision. (Show Details)

Test --version-script

lanza added a comment.Mar 8 2021, 11:47 PM

Well right now they are marked with public visibility, which is the same thing as not having any vcall_visibility. Since they are marked in the IR as having hidden visibility, clang presumably should have marked them with a more restricted vcall_visibility (linkage unit level).

Hmm? You first said "they are marked with public visibility" and then said "they are marked in the IR as having hidden visibility." I don't follow. A __attribute__(visibility("hidden"))) symbol gets the hidden attr.

lanza added inline comments.Mar 9 2021, 12:10 AM
lld/test/ELF/lto/devirt_vcall_vis_localize.ll
3

So this test and description works fine, but the case I was originally inquiring about is an additional usage.

This version script that you have below marks _ZTV1A as local and thus changes the versionId to STB_LOCAL. (SymbolTable::assignWildcardVersion)

Your change in this diff causes r.ExportDynamic to be false thanks to the STB_LOCAL which is later propagated to llvm's LTO GlobalResolutions directly.

This stops the vtable from being put in the DynamicExportSymbols set. (LTO::run at the top).

This lets the vcall_visiblity metadata get applied to the vtable in updateVCallVisibilityInModule WholeProgramDevirt.cpp. And thus the devirtualization now happens even though the vtbl was not hidden to start.

13

You can remove the vcall_visibility here and the --lto-whole-program-visibility combined with the version script will allow the vcall_visibility to be applied.

Well right now they are marked with public visibility, which is the same thing as not having any vcall_visibility. Since they are marked in the IR as having hidden visibility, clang presumably should have marked them with a more restricted vcall_visibility (linkage unit level).

Hmm? You first said "they are marked with public visibility" and then said "they are marked in the IR as having hidden visibility." I don't follow. A __attribute__(visibility("hidden"))) symbol gets the hidden attr.

An older version of the patch had the vtables marked "hidden" in the LLVM assembly. My point was that clang would have given them a non-public vcall_visibility in that case. The patch and the test case have changed though, so that now the local binding comes from the linker version script and not the input IR, which is more consistent with the case I believe you are trying to handle?

@lanza I did not know that -flto -fvisibility=hidden -fwhole-program-vtables -fvirtual-function-elimination create s`!vcall_visibility VCallVisibilityLinkageUnit`.
With VCallVisibilityLinkageUnit, WPD will be performed, no need for this change (test/ThinLTO/X86/devirt_vcall_vis_hidden.ll).
This change specifically handles localization via a local: version node in a version script, where WPD was not performed previously.

lanza added a comment.Mar 9 2021, 11:38 AM

An older version of the patch had the vtables marked "hidden" in the LLVM assembly. My point was that clang would have given them a non-public vcall_visibility in that case.

Ahh, sorry, I missed that!

The patch and the test case have changed though, so that now the local binding comes from the linker version script and not the input IR, which is more consistent with the case I believe you are trying to handle?

Correct.

MaskRay marked an inline comment as done.Mar 9 2021, 11:47 AM
MaskRay added inline comments.
lld/test/ELF/lto/devirt_vcall_vis_localize.ll
13

!vcall_visibility is here to better reflect the clang codegen.

This revision is now accepted and ready to land.Mar 9 2021, 9:40 PM
MaskRay edited the summary of this revision. (Show Details)Mar 9 2021, 10:23 PM
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.