[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 ↗

(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]

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]

• [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.

llvm/trunk/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/PPCMCTargetDesc.h"
 #include "MCTargetDesc/PPCInstPrinter.h"
 #include "MCTargetDesc/PPCMCAsmInfo.h"
 #include "PPCTargetStreamer.h"
 #include "TargetInfo/PowerPCTargetInfo.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCELFStreamer.h"
 #include "llvm/MC/MCExpr.h"
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);
+
     // 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);
+    else
+      UpdateOther.erase(Symbol);
+  }
+
+  void finish() override {
+    for (auto *Sym : UpdateOther)
+      copyLocalEntry(Sym, Sym->getVariableValue());
+  }
+
+private:
+  SmallPtrSet<MCSymbolELF *, 32> UpdateOther;
+
+  bool copyLocalEntry(MCSymbolELF *D, const MCExpr *S) {
+    auto *Ref = dyn_cast<const MCSymbolRefExpr>(S);
+    if (!Ref)
+      return false;
+    const auto &RhsSym = cast<MCSymbolELF>(Ref->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 {

llvm/trunk/test/MC/PowerPC/ppc64-localentry-symbols.s

+# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-freebsd13.0 %s -o %t
+# RUN: llvm-objdump -t %t | FileCheck %s
+
+# CHECK: 0000000000000000 gw    F .text  00000000 0x60 __impl_foo
+# CHECK: 0000000000000000 g     F .text  00000000 0x60 foo
+# CHECK: 0000000000000000 gw    F .text  00000000 0x60 foo@FBSD_1.1
+# CHECK: 0000000000000008 g     F .text  00000000 0x60 func
+# CHECK: 0000000000000008 gw    F .text  00000000 0x60 weak_func
+
+.text
+.abiversion 2
+
+.globl foo
+.type foo,@function
+foo:
+  nop
+  nop
+  .localentry foo, 8
+
+.symver __impl_foo, foo@FBSD_1.1
+.weak   __impl_foo
+.set    __impl_foo, foo
+
+.globl  func
+# Mimick FreeBSD weak function/reference
+.weak   weak_func
+.equ    weak_func, func
+
+.p2align 2
+.type    func,@function
+func:
+  nop
+  nop
+  .localentry func, 8

llvm/trunk/test/MC/PowerPC/ppc64-localentry-symver.s

-# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-freebsd13.0 %s -o %t
-# RUN: llvm-objdump -t %t | FileCheck %s
-
-# CHECK: 0000000000000000 gw    F .text  00000000 0x60 __impl_foo
-# CHECK: 0000000000000000 g     F .text  00000000 0x60 foo
-# CHECK: 0000000000000000 gw    F .text  00000000 0x60 foo@FBSD_1.1
-
-.globl foo
-.type foo,@function
-foo:
-  nop
-  nop
-  .localentry foo, 8
-
-.symver __impl_foo, foo@FBSD_1.1
-.weak   __impl_foo
-.set    __impl_foo, foo