[PPC64] Update LocalEntry from assigned symbols

[luporl]

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.

[labath]

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

[luporl]

In D56586#1354013, @labath wrote:

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

Ok, thanks, sorry.

[Bdragon28]

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.)

[emaste]

@dim

[MaskRay]

lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

194

if (copyLocalEntry(Symbol, Value))
  (void)UpdateOther.insert(Symbol);
else
  (void)UpdateOther.erase(Symbol);

?

204

Consider SmallPtrSet

207

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]

lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

194

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

204

Ok

207

Ok

[luporl]

• [PPC64] Update LocalEntry from assigned symbols
• Addressed review's comments
• Added a test case for the fix

[luporl]

Any update on this?

[MaskRay]

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.

[luporl]

• Addressed MaskRay review comments

[luporl]

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.

[luporl]

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

lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

 #include "llvm/MC/MCSymbolELF.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include <set>
+
 using namespace llvm;
 
 #define GET_INSTRINFO_MC_DESC
 #include "PPCGenInstrInfo.inc"
 
 #define GET_SUBTARGETINFO_MC_DESC
 #include "PPCGenSubtargetInfo.inc"
 
void emitLocalEntry(MCSymbolELF *S, const MCExpr *LocalOffset) override {
     // set e_flags to indicate ELFv2 ABI.
     unsigned Flags = MCA.getELFHeaderEFlags();
     if ((Flags & ELF::EF_PPC64_ABI) == 0)
       MCA.setELFHeaderEFlags(Flags | 2);
   }
 
   void emitAssignment(MCSymbol *S, const MCExpr *Value) override {
     auto *Symbol = cast<MCSymbolELF>(S);
+
+    auto I = UpdateOther.find(Symbol);
+    if (I != UpdateOther.end())
+      UpdateOther.erase(I);
+
     // When encoding an assignment to set symbol A to symbol B, also copy
     // the st_other bits encoding the local entry point offset.
-    if (Value->getKind() != MCExpr::SymbolRef)
-      return;
-    const auto &RhsSym = cast<MCSymbolELF>(
-        static_cast<const MCSymbolRefExpr *>(Value)->getSymbol());
-    unsigned Other = Symbol->getOther();
+    if (copyLocalEntry(Symbol, Value))
+      UpdateOther.insert(Symbol);
+  }
+
+  void finish() override {
+    for (auto *Sym : UpdateOther)
+      copyLocalEntry(Sym, Sym->getVariableValue());
+  }
+
+private:
+  std::set<MCSymbolELF *> UpdateOther;
+
+  bool copyLocalEntry(MCSymbolELF *D, const MCExpr *S) {
+    if (S->getKind() != MCExpr::SymbolRef)
+      return false;
+    const auto &RhsSym =
+        cast<MCSymbolELF>(static_cast<const MCSymbolRefExpr *>(S)->getSymbol());
+    unsigned Other = D->getOther();
     Other &= ~ELF::STO_PPC64_LOCAL_MASK;
     Other |= RhsSym.getOther() & ELF::STO_PPC64_LOCAL_MASK;
-    Symbol->setOther(Other);
+    D->setOther(Other);
+    return true;
   }
 };
 
 class PPCTargetMachOStreamer : public PPCTargetStreamer {
 public:
   PPCTargetMachOStreamer(MCStreamer &S) : PPCTargetStreamer(S) {}
 
   void emitTCEntry(const MCSymbol &S) override {