This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Don't set versionId on undefined weak lazy symbols
ClosedPublic

Authored by MaskRay on Apr 16 2021, 12:34 AM.

Details

Summary

An unfetched lazy symbol (undefined weak) should be considered to have its
original versionId which is VER_NDX_GLOBAL, instead of the lazy symbol's
versionId. (The original versionId cannot be non-VER_NDX_GLOBAL because a
undefined versioned symbol is an error.)

The regression was introduced in D77280 when making version scripts work
with lazy symbols fetched by LTO calls.

Fix PR49915

Diff Detail

Event Timeline

MaskRay created this revision.Apr 16 2021, 12:34 AM
MaskRay requested review of this revision.Apr 16 2021, 12:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 12:34 AM
MaskRay updated this revision to Diff 338711.Apr 19 2021, 9:58 PM

use --dyn-syms instead of -V

peter.smith accepted this revision.Apr 20 2021, 6:25 AM

LGTM.

If I've got this right:

  • We assign versionid before LTO code generation
  • We assign versionid to lazy symbols from script in case LTO fetches the symbol
  • If the lazy symbol is not referenced we want to give it VER_NDX_GLOBAL so we have to do this at write time.

I'm trying to think if there is any reason we can't delay scanVersionScript() till after LTO code generation. If we could do that then we'd know which symbols were lazy and we wouldn't need to assign them a versionid. I don't know off the top of my head how much work it would be to check that though.

This revision is now accepted and ready to land.Apr 20 2021, 6:25 AM
MaskRay added a comment.EditedApr 20 2021, 11:19 AM

LGTM.

Thanks for the review.

If I've got this right:

  • We assign versionid before LTO code generation
  • We assign versionid to lazy symbols from script in case LTO fetches the symbol
  • If the lazy symbol is not referenced we want to give it VER_NDX_GLOBAL so we have to do this at write time.

I'm trying to think if there is any reason we can't delay scanVersionScript() till after LTO code generation. If we could do that then we'd know which symbols were lazy and we wouldn't need to assign them a versionid. I don't know off the top of my head how much work it would be to check that though.

If scanVersionScript() is before LTO code generation, these tests will fail.

Failed Tests (5):
  lld :: ELF/lto/devirt_vcall_vis_export_dynamic.ll
  lld :: ELF/lto/devirt_vcall_vis_localize.ll
  lld :: ELF/lto/dynamic-list.ll
  lld :: ELF/lto/internalize-version-script.ll
  lld :: ELF/lto/version-libcall.ll

We handle dynamic lists in scanVersionScript, so the includeInDynsym and inDynamicList properties can be incorrect in LTO.cpp BitcodeCompiler::add.
A bit recently, I added sym->computeBinding() != STB_LOCAL in D98220 so the whole program devirtualization pass will lose information as well.

So we need a scanVersionScript before compileBitcodeFiles. If we simply call scanVersionScript another time after compileBitcodeFiles, we will get error: attempt to reassign symbol 'foo' of version 'VER2' to version 'VER1' from version-script-symver2.s

MaskRay updated this revision to Diff 338937.Apr 20 2021, 11:22 AM

Add an inline comment

This revision was landed with ongoing or failed builds.Apr 20 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.