[lld][ELF][ARM] Mapping symbols for PLT entries

[denis-protivensky]

Summary

Make PLT entry atoms represent mapping symbols in the Release mode, while in the Debug mode they are still function-like symbols with regular names.
It's legal that mapping symbols denote unnamed parts of code, and PLT entries are not required to have function-like names (in fact, they didn't have these names before in the Release mode).
This trick is done to simplify the code and not keep mapping symbol atoms along with regular PLT atoms.

[ruiu]

The code generally looks good, but I'm wondering if it's a good thing to link an executable in a different way only when the linker itself was compiled with NDEBUG. Should the debug flag affects outputs?

I saw this pattern in many places in the ELF port of the LLD, so this comment is not actually specific to your patch, though.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp

71

Maybe replacing this line (and similar lines) with a return statement could make this code a bit concise? (I didn't try myself, so I don't know how it would look like -- it's up to you.)

return part.empty() ? "$a" : "$a." + part;

[denis-protivensky]

Concerning different output when compiling with NDEBUG: these extra symbols do really help debugging sometimes. That's a good way to check that some call refers PLT entry, or veneer is used for some operations. I treat these symbol names as extra debug output and nothing more.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp

71

I'll take it into account.

[ruiu]

LGTM. The thing about NDEBUG is not new, so we can discuss that later.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp

 };
 
 // Veneer for switching from Thumb to ARM code for PLT entries.
 static const uint8_t ARMPltVeneerAtomContent[4] = {
     0x78, 0x47,              // bx pc
     0x00, 0x00               // nop
 };
 
+// Determine proper names for mapping symbols.
+static std::string getMappingAtomName(DefinedAtom::CodeModel model,
+                                      const std::string &part) {
+  std::string name;
+  switch (model) {
+  case DefinedAtom::codeARM_a:
+    name = "$a";
+    break;
+  case DefinedAtom::codeARM_d:
+    name = "$d";
+    break;
+  case DefinedAtom::codeARM_t:
+    name = "$t";
+    break;
+  default:
+    llvm_unreachable("Wrong code model of mapping atom");
+  }
+  if (!part.empty()) {
+    name += '.';
+    name += part;
+  }
+  return name;
+}
+
 /// \brief Atoms that hold veneer code.
 class VeneerAtom : public SimpleELFDefinedAtom {
   StringRef _section;
 
 public:
   VeneerAtom(const File &f, StringRef secName)
       : SimpleELFDefinedAtom(f), _section(secName) {}
 
protected:
   ARMGOTAtom(const File &f, StringRef secName) : GOTAtom(f, secName) {}
 };
 
 class ARMGOTPLTAtom : public ARMGOTAtom {
 public:
   ARMGOTPLTAtom(const File &f) : ARMGOTAtom(f, ".got.plt") {}
 };
 
+/// \brief PLT entry atom.
+/// Serves as a mapping symbol in the release mode.
 class ARMPLTAtom : public PLTAtom {
 public:
-  ARMPLTAtom(const File &f) : PLTAtom(f, ".plt") {}
+  ARMPLTAtom(const File &f, const std::string &name)
+      : PLTAtom(f, ".plt") {
+#ifndef NDEBUG
+    _name = name;
+#else
+    // Don't move the code to any base classes since
+    // virtual codeModel method would return wrong value.
+    _name = getMappingAtomName(codeModel(), name);
+#endif
+  }
+
+  DefinedAtom::CodeModel codeModel() const override {
+#ifndef NDEBUG
+    return DefinedAtom::codeNA;
+#else
+    return DefinedAtom::codeARM_a;
+#endif
+  }
 
   ArrayRef<uint8_t> rawContent() const override {
     return llvm::makeArrayRef(ARMPltAtomContent);
   }
 
   Alignment alignment() const override { return 4; }
+
+  StringRef name() const override { return _name; }
+
+private:
+  std::string _name;
 };
 
+/// \brief Veneer atom for PLT entry.
+/// Serves as a mapping symbol in the release mode.
 class ARMPLTVeneerAtom : public PLTAtom {
 public:
-  ARMPLTVeneerAtom(const File &f) : PLTAtom(f, ".plt") {}
+  ARMPLTVeneerAtom(const File &f, const std::string &name)
+      : PLTAtom(f, ".plt") {
+#ifndef NDEBUG
+    _name = name;
+#else
+    // Don't move the code to any base classes since
+    // virtual codeModel method would return wrong value.
+    _name = getMappingAtomName(codeModel(), name);
+#endif
+  }
 
   DefinedAtom::CodeModel codeModel() const override {
+#ifndef NDEBUG
     return DefinedAtom::codeARMThumb;
+#else
+    return DefinedAtom::codeARM_t;
+#endif
   }
 
   ArrayRef<uint8_t> rawContent() const override {
     return llvm::makeArrayRef(ARMPltVeneerAtomContent);
   }
 
   Alignment alignment() const override { return 4; }
+
+  StringRef name() const override { return _name; }
+
+private:
+  std::string _name;
 };
 
 class ELFPassFile : public SimpleFile {
 public:
   ELFPassFile(const ELFLinkingContext &eti) : SimpleFile("ELFPassFile") {
     setOrdinal(eti.getNextOrdinalAndIncrement());
   }
 

 #endif
     _gotAtoms[da] = g;
     return g;
   }
 
   /// \brief get a veneer for a PLT entry.
   const PLTAtom *getPLTVeneer(const DefinedAtom *da, PLTAtom *pa,
                               StringRef source) {
-    auto va = new (_file._alloc) ARMPLTVeneerAtom(_file);
+    std::string name = "__plt_from_thumb";
+    name += source;
+    name += da->name();
+    // Create veneer for PLT entry.
+    auto va = new (_file._alloc) ARMPLTVeneerAtom(_file, name);
     // Fake reference to show connection between veneer and PLT entry.
     va->addReferenceELF_ARM(R_ARM_NONE, 0, pa, 0);
-#ifndef NDEBUG
-    va->_name = "__plt_from_thumb";
-    va->_name += source;
-    va->_name += da->name();
-#endif
+
     _pltAtoms[da] = PLTWithVeneer(pa, va);
     return va;
   }
 
   typedef const GOTAtom *(Derived::*GOTFactory)(const DefinedAtom *);
 
   /// \brief get a PLT entry referencing PLTGOT entry.
   ///
const PLTAtom *getPLTEntry(const DefinedAtom *da, bool fromThumb,
 
     // Create specific GOT entry.
     const auto *ga = (static_cast<Derived *>(this)->*gotFactory)(da);
     assert(_gotAtoms.lookup(da) == ga &&
            "GOT entry should be added to the map");
     assert(ga->customSectionName() == ".got.plt" &&
            "GOT entry should be in a special section");
 
+    std::string name = "__plt";
+    name += source;
+    name += da->name();
     // Create PLT entry for the GOT entry.
-    auto pa = new (_file._alloc) ARMPLTAtom(_file);
+    auto pa = new (_file._alloc) ARMPLTAtom(_file, name);
     pa->addReferenceELF_ARM(R_ARM_ALU_PC_G0_NC, 0, ga, -8);
     pa->addReferenceELF_ARM(R_ARM_ALU_PC_G1_NC, 4, ga, -4);
     pa->addReferenceELF_ARM(R_ARM_LDR_PC_G2, 8, ga, 0);
-#ifndef NDEBUG
-    pa->_name = "__plt";
-    pa->_name += source;
-    pa->_name += da->name();
-#endif
+
     // Since all PLT entries are in ARM code, Thumb to ARM
     // switching should be added if the relocated place contais Thumb code.
     if (fromThumb)
       return getPLTVeneer(da, pa, source);
 
     // Otherwise just add PLT entry and return it to the caller.
     _pltAtoms[da] = PLTWithVeneer(pa);
     return pa;
#ifndef NDEBUG
     g->_name += da->name();
 #endif
     _gotAtoms[da] = g;
     return g;
   }
 
   /// \brief get the PLT entry for a given IFUNC Atom.
   const PLTAtom *getIFUNCPLTEntry(const DefinedAtom *da, bool fromThumb) {
-    StringRef source;
-#ifndef NDEBUG
-    source = "_ifunc_";
-#endif
-    return getPLTEntry(da, fromThumb, &Derived::createIFUNCGOTEntry, source);
+    return getPLTEntry(da, fromThumb, &Derived::createIFUNCGOTEntry, "_ifunc_");
   }
 
   /// \brief Redirect the call to the PLT stub for the target IFUNC.
   ///
   /// This create a PLT and GOT entry for the IFUNC if one does not exist. The
   /// GOT entry and a IRELATIVE relocation to the original target resolver.
   std::error_code handleIFUNC(const DefinedAtom &atom, const Reference &ref) {
     auto target = dyn_cast<const DefinedAtom>(ref.target());

test/elf/ARM/plt-mapping.test

+# Check that mapping symbols are properly generated for PLT entries.
+
+# RUN: yaml2obj -format=elf -docnum 1 %s > %t-arm.o
+# RUN: yaml2obj -format=elf -docnum 2 %s > %t-thm.o
+# RUN: lld -flavor gnu -target arm-linux-gnu \
+# RUN: -Bstatic --noinhibit-exec %t-arm.o %t-thm.o -o %t
+# RUN: llvm-readobj -symbols %t | FileCheck %s
+
+# CHECK: Name: {{[$]?[at]?[.]?}}__plt_ifunc_f1
+# CHECK: Name: {{[$]?[at]?[.]?}}__plt_from_thumb_ifunc_f2
+# CHECK: Name: {{[$]?[at]?[.]?}}__plt_ifunc_f2
+
+# arm.o
+---
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         04B02DE500B08DE2003000E3003040E30300A0E100D04BE204B09DE41EFF2FE100482DE904B08DE2FEFFFFEB0030A0E10300A0E10088BDE8
+  - Name:            .rel.text
+    Type:            SHT_REL
+    Link:            .symtab
+    AddressAlign:    0x0000000000000004
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000008
+        Symbol:          main
+        Type:            R_ARM_MOVW_ABS_NC
+      - Offset:          0x000000000000000C
+        Symbol:          main
+        Type:            R_ARM_MOVT_ABS
+      - Offset:          0x0000000000000028
+        Symbol:          f1
+        Type:            R_ARM_CALL
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+Symbols:
+  Global:
+    - Name:            f1
+      Type:            STT_GNU_IFUNC
+      Section:         .text
+    - Name:            main
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x0000000000000020
+
+# thm.o
+---
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         80B400AF40F20003C0F200031846BD465DF8047B704700BF80B500AFFFF7FEFF0346184680BD00BF
+  - Name:            .rel.text
+    Type:            SHT_REL
+    Link:            .symtab
+    AddressAlign:    0x0000000000000004
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000004
+        Symbol:          f
+        Type:            R_ARM_THM_MOVW_ABS_NC
+      - Offset:          0x0000000000000008
+        Symbol:          f
+        Type:            R_ARM_THM_MOVT_ABS
+      - Offset:          0x000000000000001C
+        Symbol:          f2
+        Type:            R_ARM_THM_CALL
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000001
+    Content:         ''
+Symbols:
+ Global:
+    - Name:            f2
+      Type:            STT_GNU_IFUNC
+      Section:         .text
+      Value:           0x0000000000000001
+    - Name:            f
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x0000000000000019
+...