This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Apply version script patterns to non-default version symbols
ClosedPublic

Authored by MaskRay on Jul 31 2021, 6:50 PM.

Details

Summary

Currently version script patterns are ignored for .symver produced
non-default version (single @) symbols. This makes such symbols
not localizable by local:, e.g.

.symver foo3_v1,foo3@v1
.globl foo_v1
foo3_v1:

ld.lld --version-script=a.ver -shared a.o
# In a.out, foo3@v1 is incorrectly exported.

This patch adds the support:

  • Move config->versionDefinitions[VER_NDX_LOCAL].patterns to config->versionDefinitions[versionId].localPatterns
  • Rename config->versionDefinitions[versionId].patterns to config->versionDefinitions[versionId].nonLocalPatterns
  • Allow findAllByVersion to find non-default version symbols when includeNonDefault is true. (Note: symtab keys do not have @@)
  • Make each pattern check both the unversioned pat.name and the versioned ${pat.name}@${v.name}
  • localPatterns can localize ${pat.name}@${v.name}. nonLocalPatterns can prevent localization by assigning verdefIndex (before parseSymbolVersion).

If a user notices new undefined symbol errors with a version script containing
local: *;, the issue is likely due to a missing global: pattern.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 31 2021, 6:50 PM
MaskRay requested review of this revision.Jul 31 2021, 6:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2021, 6:50 PM

I struggled a bit to understand the implementation, probably not helped by the complexity of symbol version scripts. I can agree with what you are trying to achieve though. Do we need any more tests? For example the error message.

lld/ELF/Config.h
89–90

Is it worth renaming this to nonLocalPatterns (globalPatterns might work, but may not be strictly accurate).

lld/ELF/SymbolTable.cpp
189–191

Suggest adding
// return false if no symbol definition matches ver.

250

I'm struggling a bit to understand this part. IIUC if we have a script like:

v1 { global: foo; }

then we'll call

assignExactVersion({"foo@v1", /*pat.isExternCpp*/ false, /*hasWildcard*/ false}, v1 id, "v1");

However in assignExactVersion we have:

// Get a list of symbols which we need to assign the version to.
  std::vector<Symbol *> syms = findByVersion(ver);

Which I'd expect to return symbols matching "foo@v1";
but later on in assignExactVersion there is:

if (versionId != VER_NDX_LOCAL && sym->getName().contains('@'))
      continue;

which I'd assume foo@v1 would always end up in continue.

If I'm right, the only affect of calling assignExactVersion here is to update found

251

Would be useful to have /* hasWildcard */ false)

259

The body of the loop is almost identical, may be worth extracting into a lambda or a function? Just a mild suggestion, difficult to know if it will be an improvement or not.

260

Would be useful to have /* hasWildcard */ false)

270

/* checkAt */ false

272

/* hasWildcard */ true

273

/* checkAt */ true

lld/ELF/SymbolTable.h
68–69

Instead of "checkAt" I suggest "includeNonDefault" or "includeNonDefaultVer".

74–75

Same as above, I suggest "includeNonDefault"

MaskRay updated this revision to Diff 363784.Aug 3 2021, 9:49 AM
MaskRay marked 11 inline comments as done.

Address comments.

Added a diagnostic test to version-script-noundef.s
The coverage should be quite good now.
version-script-symver.s & version-script-symver2.s check various complex cases.

verneed-* and other version-script-* cover misc ad-hoc cases.
(I was struggling with reorganizing them.)

lld/ELF/SymbolTable.cpp
250

Updating found is one use.

The other use is to allow a global: to override local: *. A symbol's version is unassigned when verdexIndex == UINT32_C(-1).
If a global: pattern assigns verdexIndex, a subsequent local: pattern won't be able to localize the symbol.

These tests would fail if I remove the assignExactVersion({"foo@v1" function call:

Failed Tests (5):
  lld :: ELF/partition-synthetic-sections.s
  lld :: ELF/verdef-defaultver.s
  lld :: ELF/verneed.s
  lld :: ELF/version-script-symver2.s
  lld :: ELF/version-symbol-undef.s
273

I picked /*checkAt=*/true.

clang-format will delete space before true if it recognizes this form.

clang-tidy -checks='-*,bugprone-argument-comment' recognizes the = form and can suggest argument renames.

MaskRay edited the summary of this revision. (Show Details)Aug 3 2021, 9:54 AM
MaskRay updated this revision to Diff 363802.Aug 3 2021, 10:18 AM

Add extern "C++" tests

peter.smith accepted this revision.Aug 4 2021, 2:44 AM

Thanks for the update LGTM.

This revision is now accepted and ready to land.Aug 4 2021, 2:44 AM
lld/test/ELF/version-script-extern-exact.s