[llvm-objcopy][ELF] Allow --set-section-flags src=... and --rename-section src=tst

[MaskRay]

Summary

• GNU objcopy supports --set-section-flags src=... --rename-section src=tst and --set-section-flags runs first.
• GNU objcopy processes --update-section before --rename-section.

To match the two behaviors, postpone --rename-section and allow its use together
with --set-section-flags.

As a side effect, --rename-section=.foo1=.foo2 --add-section=.foo1=/dev/null
leads to .foo2 while GNU objcopy surprisingly produces .foo1 (so
--set-section-flags --add-section --rename-section do not form a total order).
I think the deviation is fine as a total order makes more sense.

Rename set-section-flags-and-rename.test to
set-section-attr-and-rename.test and additionally test --set-section-alignment

[jhenderson]

If I'm not mistaken, this change does more than just enable --set-section-flags (and --set-section-alignment) to be run before --rename-section. In particular, I believe it now means that --rename-section can rename added sections, and that updated sections apply to sections with their original name, rather than renamed sections. (NB: I've only looked at these via code inspection, so might be misreading the code). I suspect these changes could potentially invalidate some workflows (whilst admittedly making new ones possible). For example, imagine I wanted to rename a section (e.g. to create a backup of it within the data), and then add a new section with the original name. This is no longer possible, I believe.

llvm/test/tools/llvm-objcopy/ELF/set-section-attr-and-rename.test

15

Nit: This line's getting quite long. Perhaps time to split it over two?

Aside: do we need a similar conflict error for --section-section-alignment?

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

813

[MaskRay]

Add tests. Clarify behavior changes

[MaskRay]

address comments

llvm/test/tools/llvm-objcopy/ELF/set-section-attr-and-rename.test

15

Probably yes, but the test can be a separate change.

[jhenderson]

LGTM, but I think the behaviour change warrants a release note.

[MaskRay]

add a release note

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

static Error handleArgs(const CommonConfig &Config, const ELFConfig &ELFConfig,
   // us to avoid reporting the inappropriate errors about removing symbols
   // named in relocations.
   if (Error E = replaceAndRemoveSections(Config, ELFConfig, Obj))
     return E;
 
   if (Error E = updateAndRemoveSymbols(Config, ELFConfig, Obj))
     return E;
 
+  if (!Config.SetSectionAlignment.empty()) {
+    for (SectionBase &Sec : Obj.sections()) {
+      auto I = Config.SetSectionAlignment.find(Sec.Name);
+      if (I != Config.SetSectionAlignment.end())
+        Sec.Align = I->second;
+    }
+  }
+
+  if (Config.OnlyKeepDebug)
+    for (auto &Sec : Obj.sections())
+      if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
+        Sec.Type = SHT_NOBITS;
+
+  for (const NewSectionInfo &AddedSection : Config.AddSection) {
+    auto AddSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
+      OwnedDataSection &NewSection =
+          Obj.addSection<OwnedDataSection>(Name, Data);
+      if (Name.startswith(".note") && Name != ".note.GNU-stack")
+        NewSection.Type = SHT_NOTE;
+      return Error::success();
+    };
+    if (Error E = handleUserSection(AddedSection, AddSection))
+      return E;
+  }
+
+  for (const NewSectionInfo &NewSection : Config.UpdateSection) {
+    auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
+      return Obj.updateSection(Name, Data);
+    };
+    if (Error E = handleUserSection(NewSection, UpdateSection))
+      return E;
+  }
+
+  if (!Config.AddGnuDebugLink.empty())
+    Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
+                                        Config.GnuDebugLinkCRC32);
+
+  // If the symbol table was previously removed, we need to create a new one
+  // before adding new symbols.
+  if (!Obj.SymbolTable && !Config.SymbolsToAdd.empty())
+    if (Error E = Obj.addNewSymbolTable())
+      return E;
+
+  for (const NewSymbolInfo &SI : Config.SymbolsToAdd)
+    addSymbol(Obj, SI, ELFConfig.NewSymbolVisibility);
+
+  // --set-section-flags works with sections added by --add-section.
+  if (!Config.SetSectionFlags.empty()) {
+    for (auto &Sec : Obj.sections()) {
+      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
+      if (Iter != Config.SetSectionFlags.end()) {
+        const SectionFlagsUpdate &SFU = Iter->second;
+        setSectionFlagsAndType(Sec, SFU.NewFlags);
+      }
+    }
+  }
+
   if (!Config.SectionsToRename.empty()) {
     std::vector<RelocationSectionBase *> RelocSections;
     DenseSet<SectionBase *> RenamedSections;
     for (SectionBase &Sec : Obj.sections()) {
       auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec);
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
       if (Iter != Config.SectionsToRename.end()) {
         const SectionRename &SR = Iter->second;
for (SectionBase &Sec : Obj.sections()) {
             Sec.Name = (RelocSec->getNamePrefix() + Config.AllocSectionsPrefix +
                         TargetSec->Name)
                            .str();
         }
       }
     }
   }
 
-  if (!Config.SetSectionAlignment.empty()) {
-    for (SectionBase &Sec : Obj.sections()) {
-      auto I = Config.SetSectionAlignment.find(Sec.Name);
-      if (I != Config.SetSectionAlignment.end())
-        Sec.Align = I->second;
-    }
-  }
-
-  if (Config.OnlyKeepDebug)
-    for (auto &Sec : Obj.sections())
-      if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
-        Sec.Type = SHT_NOBITS;
-
-  for (const NewSectionInfo &AddedSection : Config.AddSection) {
-    auto AddSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
-      OwnedDataSection &NewSection =
-          Obj.addSection<OwnedDataSection>(Name, Data);
-      if (Name.startswith(".note") && Name != ".note.GNU-stack")
-        NewSection.Type = SHT_NOTE;
-      return Error::success();
-    };
-    if (Error E = handleUserSection(AddedSection, AddSection))
-      return E;
-  }
-
-  for (const NewSectionInfo &NewSection : Config.UpdateSection) {
-    auto UpdateSection = [&](StringRef Name, ArrayRef<uint8_t> Data) {
-      return Obj.updateSection(Name, Data);
-    };
-    if (Error E = handleUserSection(NewSection, UpdateSection))
-      return E;
-  }
-
-  if (!Config.AddGnuDebugLink.empty())
-    Obj.addSection<GnuDebugLinkSection>(Config.AddGnuDebugLink,
-                                        Config.GnuDebugLinkCRC32);
-
-  // If the symbol table was previously removed, we need to create a new one
-  // before adding new symbols.
-  if (!Obj.SymbolTable && !Config.SymbolsToAdd.empty())
-    if (Error E = Obj.addNewSymbolTable())
-      return E;
-
-  for (const NewSymbolInfo &SI : Config.SymbolsToAdd)
-    addSymbol(Obj, SI, ELFConfig.NewSymbolVisibility);
-
-  // --set-section-flags works with sections added by --add-section.
-  if (!Config.SetSectionFlags.empty()) {
-    for (auto &Sec : Obj.sections()) {
-      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
-      if (Iter != Config.SetSectionFlags.end()) {
-        const SectionFlagsUpdate &SFU = Iter->second;
-        setSectionFlagsAndType(Sec, SFU.NewFlags);
-      }
-    }
-  }
-
   if (ELFConfig.EntryExpr)
     Obj.Entry = ELFConfig.EntryExpr(Obj.Entry);
   return Error::success();
 }
 
 static Error writeOutput(const CommonConfig &Config, Object &Obj,
                          raw_ostream &Out, ElfType OutputElfType) {
   std::unique_ptr<Writer> Writer =

llvm/test/tools/llvm-objcopy/ELF/set-section-attr-and-rename.test

+# RUN: yaml2obj %s -o %t
+
+# RUN: llvm-objcopy --rename-section=.foo=.bar --set-section-alignment=.foo=16 --set-section-flags=.foo=alloc %t %t.1
+# RUN: llvm-readobj -S %t.1 | FileCheck %s
+
+# CHECK:      Name: .bar
+# CHECK-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT: Flags [
+# CHECK-NEXT:   SHF_ALLOC
+# CHECK-NEXT:   SHF_WRITE
+# CHECK-NEXT: ]
+# CHECK:      AddressAlignment:
+# CHECK-SAME:   {{^}} 16
+
+# RUN: not llvm-objcopy --rename-section=.foo=.bar --set-section-flags=.bar=alloc %t %t.2 2>&1 | FileCheck %s --check-prefix=SET-BAR
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:  .foo
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC ]
+
+# SET-BAR: --set-section-flags=.bar conflicts with --rename-section=.foo=.bar

llvm/test/tools/llvm-objcopy/ELF/set-section-flags-and-rename.test

-# RUN: yaml2obj %s -o %t
-
-# RUN: not llvm-objcopy --rename-section=.foo=.bar --set-section-flags=.foo=alloc %t %t.2 2>&1 | FileCheck %s --check-prefix=SET-FOO
-# RUN: not llvm-objcopy --rename-section=.foo=.bar --set-section-flags=.bar=alloc %t %t.2 2>&1 | FileCheck %s --check-prefix=SET-BAR
-
-!ELF
-FileHeader:
-  Class:           ELFCLASS64
-  Data:            ELFDATA2LSB
-  Type:            ET_REL
-  Machine:         EM_X86_64
-
-# SET-FOO: --set-section-flags=.foo conflicts with --rename-section=.foo=.bar
-# SET-BAR: --set-section-flags=.bar conflicts with --rename-section=.foo=.bar

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

if (!SFU)
       return SFU.takeError();
     if (!Config.SetSectionFlags.try_emplace(SFU->Name, *SFU).second)
       return createStringError(
           errc::invalid_argument,
           "--set-section-flags set multiple times for section '%s'",
           SFU->Name.str().c_str());
   }
   // Prohibit combinations of --set-section-flags when the section name is used
-  // by --rename-section, either as a source or a destination.
+  // by the destination of a --rename-section.
   for (const auto &E : Config.SectionsToRename) {
     const SectionRename &SR = E.second;
-    if (Config.SetSectionFlags.count(SR.OriginalName))
-      return createStringError(
-          errc::invalid_argument,
-          "--set-section-flags=%s conflicts with --rename-section=%s=%s",
-          SR.OriginalName.str().c_str(), SR.OriginalName.str().c_str(),
-          SR.NewName.str().c_str());
     if (Config.SetSectionFlags.count(SR.NewName))
       return createStringError(
           errc::invalid_argument,
           "--set-section-flags=%s conflicts with --rename-section=%s=%s",
           SR.NewName.str().c_str(), SR.OriginalName.str().c_str(),
           SR.NewName.str().c_str());
   }