[llvm-objcopy] Add --prefix-sections option

[jakehehrlich]

LGTM

[jhenderson]

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

[jhenderson]

In D50463#1193520, @jhenderson wrote:

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

It turns out that this is a wider bug in llvm-objcopy that is related to, but not specific to this issue: if you do --rename-section in GNU objcopy, you also implicitly rename relocation sections. Meanwhile, if you do --rename-section on relocation sections or strtab or symtab sections, nothing happens (if less concerned about this, because you requested something specific for those sections, but still, it is a behaviour difference).

tools/llvm-objcopy/llvm-objcopy.cpp

274

This should take a pair of StringRefs, not a std::string and a StringRef. And then you can use StringRef::startswith.

Also inline has no purpose here. Compilers generally ignore it when it comes to function inlining. It does have a completely separate purpose in header files though.

It should also be named with lower-case (@jakehehrlich and I discussed this offline, and I think that somewhere we started following a non-LLVM style for functions, contrary to the style guide, and we'd like to get back to the style it should be).

568

I'm wondering if GNU objcopy's behaviour is section-name specific rather than type specific. Have you checked?

[paulsemel]

In D50463#1193520, @jhenderson wrote:

I've got some comments that I'll send through a bit later today, but as things stand, this doesn't match GNU objcopy's behaviour for relocation sections at the very least - the "prefix" is placed between the end of ".rela" and the start of the section name so that they have the name ".rela<prefixed relocated section name>"

Hmm ok.. I just tried on the "chromium" binary on my laptop and it appeard the the prefix is at the beggining of the section, not between '.rela' and the rest of the section name.. am I missing smth ?

[jakehehrlich]

I don't think I want this option to do anything more than literally add a prefix to all section names. It isn't clear that those precise semantics are being relied on anywhere and the behavior of making relocation names and not renaming symtab etc... is a result of how bfd reconstructs the output binary without consideration for what the user initially put in which is opposite the stated goal of llvm-objcopy.

tools/llvm-objcopy/llvm-objcopy.cpp

274

+1 on all of these points actually. The lowercase thing is my fault but we should begin changing it now.

[jhenderson]

In D50463#1194301, @jakehehrlich wrote:

I don't think I want this option to do anything more than literally add a prefix to all section names. It isn't clear that those precise semantics are being relied on anywhere and the behavior of making relocation names and not renaming symtab etc... is a result of how bfd reconstructs the output binary without consideration for what the user initially put in which is opposite the stated goal of llvm-objcopy.

I can't think of any semantic reason why names should matter for the cases discussed, with the exception of a tiny number of sections (usually to do with debug information or similar such things), names of sections shouldn't matter (and indeed, this has been discussed on the ELF gABI mailing list as a stated goal of ELF). This option WILL break DWARF sections though, which rely on the section naming. I'm not sure if this is something we should care about though?

Also, blanket renaming sections seems like a bad thing for linking, since it will affect semantics of where sections get placed. Is there actually a use-case for it?

test/tools/llvm-objcopy/prefix-sections.test

+# RUN: yaml2obj %s > %t
+# RUN: llvm-objcopy --prefix-sections .prefix %t %t2
+# RUN: llvm-readobj -sections %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name:            .bar
+    Type:            SHT_DYNAMIC
+    Flags:           [ SHF_WRITE ]
+  - Name:            .foobar
+    Type:            SHT_STRTAB
+
+# CHECK: Sections [
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 0
+# CHECK-NEXT:     Name:  (0)
+# CHECK-NEXT:     Type: SHT_NULL
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Link: 0
+# CHECK-NEXT:     Info: 0
+# CHECK-NEXT:     AddressAlignment: 0
+# CHECK-NEXT:     EntrySize: 0
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 1
+# CHECK-NEXT:     Name: .prefix.foo
+# CHECK-NEXT:     Type: SHT_PROGBITS
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_ALLOC
+# CHECK-NEXT:       SHF_EXECINSTR
+# CHECK-NEXT:     ]
+# CHECK:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 2
+# CHECK-NEXT:     Name: .prefix.bar
+# CHECK-NEXT:     Type: SHT_DYNAMIC
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:       SHF_WRITE
+# CHECK-NEXT:     ]
+# CHECK:   }
+# CHECK-NEXT:   Section {
+# CHECK-NEXT:     Index: 3
+# CHECK-NEXT:     Name: .foobar
+# CHECK-NEXT:     Type: SHT_STRTAB
+# CHECK-NEXT:     Flags [
+# CHECK-NEXT:     ]

tools/llvm-objcopy/ObjcopyOpts.td

 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;
 def only_keep_debug : Flag<["-", "--"], "only-keep-debug">,
                           HelpText<"Currently ignored. Only for compaitability with GNU objcopy.">;
 def strip_unneeded : Flag<["-", "--"], "strip-unneeded">,
                       HelpText<"Remove all symbols not needed by relocations">;
 def keep_file_symbols : Flag<["-", "--"], "keep-file-symbols">,
                       HelpText<"Do not remove file symbols">;
+defm prefix_sections : Eq<"prefix-sections">,
+                       MetaVarName<"prefix">,
+                       HelpText<"Add <prefix> to the start of every section name">;

tools/llvm-objcopy/Object.h

public:
   ~BinaryWriter() {}
   void finalize() override;
   void write() override;
   BinaryWriter(Object &Obj, Buffer &Buf) : Writer(Obj, Buf) {}
 };
 
 class SectionBase {
 public:
-  StringRef Name;
+  std::string Name;
   Segment *ParentSegment = nullptr;
   uint64_t HeaderOffset;
   uint64_t OriginalOffset = std::numeric_limits<uint64_t>::max();
   uint32_t Index;
   bool HasSymbol = false;
 
   uint64_t Addr = 0;
   uint64_t Align = 1;

tools/llvm-objcopy/llvm-objcopy.cpp

struct CopyConfig {
   StringRef OutputFilename;
   StringRef InputFilename;
   StringRef OutputFormat;
   StringRef InputFormat;
   StringRef BinaryArch;
 
   StringRef SplitDWO;
   StringRef AddGnuDebugLink;
+  StringRef SectionsPrefix;
   std::vector<StringRef> ToRemove;
   std::vector<StringRef> Keep;
   std::vector<StringRef> OnlyKeep;
   std::vector<StringRef> AddSection;
   std::vector<StringRef> SymbolsToLocalize;
   std::vector<StringRef> SymbolsToGlobalize;
   std::vector<StringRef> SymbolsToWeaken;
   std::vector<StringRef> SymbolsToRemove;
if (Flags & SectionFlag::SecMerge)
       *SR.NewFlags |= ELF::SHF_MERGE;
     if (Flags & SectionFlag::SecStrings)
       *SR.NewFlags |= ELF::SHF_STRINGS;
   }
 
   return SR;
 }
 
+static inline bool StartsWith(std::string S1, StringRef S2) {
+  return std::equal(S2.begin(), S2.end(), S1.begin());
+}
+
 static bool IsDebugSection(const SectionBase &Sec) {
-  return Sec.Name.startswith(".debug") || Sec.Name.startswith(".zdebug") ||
+  return StartsWith(Sec.Name, ".debug") || StartsWith(Sec.Name, ".zdebug") ||
          Sec.Name == ".gdb_index";
 }
 
 static bool IsDWOSection(const SectionBase &Sec) {
-  return Sec.Name.endswith(".dwo");
+  return StringRef(Sec.Name).endswith(".dwo");
 }
 
 static bool OnlyKeepDWOPred(const Object &Obj, const SectionBase &Sec) {
   // We can't remove the section header string table.
   if (&Sec == Obj.SectionNames)
     return false;
   // Short of keeping the string table we want to keep everything that is a DWO
   // section and remove everything else.
if (Config.StripNonAlloc)
     };
 
   if (Config.StripAll)
     RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
       if (RemovePred(Sec))
         return true;
       if (&Sec == Obj.SectionNames)
         return false;
-      if (Sec.Name.startswith(".gnu.warning"))
+      if (StartsWith(Sec.Name, ".gnu.warning"))
         return false;
       return (Sec.Flags & SHF_ALLOC) == 0;
     };
 
   // Explicit copies:
   if (!Config.OnlyKeep.empty()) {
     RemovePred = [&Config, RemovePred, &Obj](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
for (const auto &Flag : Config.AddSection) {
       auto BufSize = Buf->getBufferSize();
       Obj.addSection<OwnedDataSection>(SecName,
                                        ArrayRef<uint8_t>(BufPtr, BufSize));
     }
   }
 
   if (!Config.AddGnuDebugLink.empty())
     Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink);
+
+  if (!Config.SectionsPrefix.empty()) {
+    for (auto &Sec : Obj.sections()) {
+      if (Sec.Type != SHT_STRTAB && Sec.Type != SHT_SYMTAB)
+        Sec.Name = Config.SectionsPrefix.str() + Sec.Name;
+    }
+  }
 }
 
 static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
                                       Buffer &Out) {
   ELFReader Reader(&Binary);
   std::unique_ptr<Object> Obj = Reader.create();
 
   HandleArgs(Config, *Obj, Reader, Reader.getElfType());
static CopyConfig ParseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   Config.InputFilename = Positional[0];
   Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
   Config.InputFormat = InputArgs.getLastArgValue(OBJCOPY_input_target);
   Config.OutputFormat = InputArgs.getLastArgValue(OBJCOPY_output_target);
   Config.BinaryArch = InputArgs.getLastArgValue(OBJCOPY_binary_architecture);
 
   Config.SplitDWO = InputArgs.getLastArgValue(OBJCOPY_split_dwo);
   Config.AddGnuDebugLink = InputArgs.getLastArgValue(OBJCOPY_add_gnu_debuglink);
+  Config.SectionsPrefix = InputArgs.getLastArgValue(OBJCOPY_prefix_sections);
 
   for (auto Arg : InputArgs.filtered(OBJCOPY_redefine_symbol)) {
     if (!StringRef(Arg->getValue()).contains('='))
       error("Bad format for --redefine-sym");
     auto Old2New = StringRef(Arg->getValue()).split('=');
     if (!Config.SymbolsToRename.insert(Old2New).second)
       error("Multiple redefinition of symbol " + Old2New.first);
   }