This is an archive of the discontinued LLVM Phabricator instance.

[PPC64] Update LocalEntry from assigned symbols
ClosedPublic

Authored by luporl on Jan 11 2019, 3:25 AM.

Details

Summary

On PowerPC64 ELFv2 ABI, functions may have 2 entry points: global and local.
The local entry point location of a function is stored in the st_other field of the symbol, as an offset relative to the global entry point.

In order to make symbol assignments (e.g. .equ/.set) work properly with this, PPCTargetELFStreamer already copies the local entry bits from the source symbol to the destination one, on emitAssignment(). The problem is that this copy is performed only at the assignment location, where the source symbol may not yet have processed the .localentry directive, that sets the local entry. This may cause the destination symbol to end up with wrong local entry information. Other symbol info is not affected by this because, in this case, the destination symbol value is actually a symbol reference.

This change keeps track of these assignments, and update all needed st_other fields when finish() is called.

Event Timeline

luporl created this revision.Jan 11 2019, 3:25 AM
luporl edited the summary of this revision. (Show Details)Jan 11 2019, 3:48 AM
luporl added reviewers: labath, clayborg.
luporl added subscribers: lbianc, leitao.
labath resigned from this revision.Jan 11 2019, 4:18 AM

I suggest having this reviewed by the PPC target owner in llvm.

luporl edited reviewers, added: hfinkel; removed: labath, clayborg.Jan 11 2019, 4:21 AM
luporl added a subscriber: labath.

I suggest having this reviewed by the PPC target owner in llvm.

Ok, thanks, sorry.

One particular place this is needed is to handle FreeBSD powerpc64 (ELFv2 experimental) libc's weak symbols / symbol aliasing, especially when using LLD (in my experience, binutils ld will partially compensate for this issue.)

Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 11:42 AM
MaskRay added inline comments.Apr 21 2019, 9:24 PM
lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
194 ↗(On Diff #181234)
if (copyLocalEntry(Symbol, Value))
  (void)UpdateOther.insert(Symbol);
else
  (void)UpdateOther.erase(Symbol);

?

204 ↗(On Diff #181234)

Consider SmallPtrSet

207 ↗(On Diff #181234)

S->getKind() != MCExpr::SymbolRef + static_cast<const MCSymbolRefExpr *>(S)->getSymbol() may be changed to

auto *Ref = dyn_cast<const MCSymbolRefExpr>(S);
if (!Ref)
  return false;
...
luporl marked 3 inline comments as done.Apr 23 2019, 11:59 AM
luporl added inline comments.
lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
194 ↗(On Diff #181234)

Just to confirm, the suggestion is to replace lines 188-195 by the code above, right?

204 ↗(On Diff #181234)

Ok

207 ↗(On Diff #181234)

Ok

luporl updated this revision to Diff 196865.Apr 26 2019, 9:24 AM
  • [PPC64] Update LocalEntry from assigned symbols
  • Addressed review's comments
  • Added a test case for the fix
luporl marked 3 inline comments as done.Apr 26 2019, 9:27 AM

Any update on this?

MaskRay accepted this revision.EditedMay 10 2019, 5:41 PM

I've checked the behavior is consistent with GNU as. It has a final pass over the symbol table to copy symbol attributes. The call stack:

  • gas/write.c write_object_file
  • gas/config/obj-elf.c elf_copy_symbol_attributes
  • gas/symbols.c resolve_symbol_value "Resolve the value of a symbol. This is called during the final pass over the symbol table to resolve any symbols with complex values."
  • gas/symbols.c copy_symbol_attributes

What copy_symbol_attributes does:

S_SET_OTHER (dest, (ELF_ST_VISIBILITY (S_GET_OTHER (dest))
        | (S_GET_OTHER (src) & ~ELF_ST_VISIBILITY (-1))));

While it copies all non-visibility bits (0-1), MCSymbolELF::setOther copies just the 3 most significant bits (5-7). Bits 2-4 are not copied but they are not used anyway.

Consider adding the test to test/MC/PowerPC/ppc64-localentry-symver.s instead. You may add another sets of bar bar@FBSD_.1 __impl_bar, and move .set __impl_bar, bar before .localentry bar, 8. llvm-objdump can print non-visibility st_other bits now.

llvm/test/MC/AsmParser/PPC64/lit.local.cfg
1 ↗(On Diff #196865)

Delete this file.

llvm/test/MC/AsmParser/PPC64/ppc64-localentry.s
1 ↗(On Diff #196865)

Delete this file.

This revision is now accepted and ready to land.May 10 2019, 5:41 PM
luporl updated this revision to Diff 199262.EditedMay 13 2019, 6:28 AM
  • Addressed MaskRay review comments
luporl marked 2 inline comments as done.May 13 2019, 6:34 AM

Thanks for the review @MaskRay.
I've moved the test case to ppc64-localentry-symver.s, but then I thought it was better to rename it to ppc64-localentry-symbols.s, as now the test scope is broader than .symver testing.

@MaskRay, this change should be good to go now, the last revision should address your last comments.

This revision was automatically updated to reflect the committed changes.