[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

Delete this file.

llvm/test/MC/AsmParser/PPC64/ppc64-localentry.s

1

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/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

 //===-- PPCMCTargetDesc.cpp - PowerPC Target Descriptions -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides PowerPC specific target descriptions.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/PPCMCTargetDesc.h"
 #include "InstPrinter/PPCInstPrinter.h"
 #include "MCTargetDesc/PPCMCAsmInfo.h"
 #include "PPCTargetStreamer.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/test/MC/AsmParser/PPC64/lit.local.cfg

+if 'PowerPC' in config.root.targets:
+    config.unsupported = False
+else:
+    config.unsupported = True

llvm/test/MC/AsmParser/PPC64/ppc64-localentry.s

+# RUN: llvm-mc -triple powerpc64-unknown-unknown -filetype=obj %s -o %t
+# RUN: llvm-objdump -s -j .symtab %t | FileCheck %s
+
+  .text
+  .abiversion 2
+
+  .globl  func
+  # Mimick FreeBSD weak function/reference
+  .weak weak_func
+  .equ  weak_func, func
+
+  .p2align  2
+  .type func,@function
+func:
+.Lfunc_begin0:
+.Lfunc_gep0:
+  addis 2, 12, .TOC.-.Lfunc_gep0@ha
+  addi 2, 2, .TOC.-.Lfunc_gep0@l
+.Lfunc_lep0:
+  .localentry func, .Lfunc_lep0-.Lfunc_gep0
+  # Simple access to global var to force TOC usage:
+  # return g_var++;
+  addis 3, 2, g_var@toc@ha
+  addi 3, 3, g_var@toc@l
+  lwz 4, 0(3)
+  addi 5, 4, 1
+  stw 5, 0(3)
+  extsw 3, 4
+  blr
+  .long 0
+  .quad 0
+.Lfunc_end0:
+  .size func, .Lfunc_end0-.Lfunc_begin0
+  .type g_var,@object
+  .lcomm  g_var,4,4
+
+  .addrsig
+  .addrsig_sym g_var
+
+# We expect the st_other byte of the weak_func symbol to be 0x60
+# (localentry = gep + 2 instructions)
+#
+# Currently, llvm-objdump does not support displaying
+# st_other's PPC64 specific flags, thus we check the
+# result of the hexdump of .symtab section instead.
+#
+# Elf64_Sym:
+# Elf64_Word  st_name  = 0x00000025 ("weak_func" offset in strtab)
+# unsigned char st_info  = 0x22 (bind = STB_WEAK, type = STT_FUNC)
+# unsigned char st_other = 0x60
+#   (localentry = 0b011 (gep + 2 instructions),
+#    reserved   = 0b000,
+#    visibility = 0b00 (STV_DEFAULT))
+# Elf64_Half  st_shndx = 0x0002
+# Elf64_Addr  st_value = 0x0000000000000000
+# Elf64_Xword st_size  = 0x0000000000000030
+
+# CHECK:  0070 00000000 00000030 00000025 22600002