[llvm-objcopy] Change SHT_NOBITS to SHT_PROBITS for some --set-section-flags

[rupprecht]

Summary

Some flags accepted by --set-section-flags and --rename-section can change a SHT_NOBITS section to a SHT_PROGBITS section. Note that none of them can change a SHT_PROGBITS to SHT_NOBITS.

The full list (found via experimentation of individually setting each flag) that does this is: contents, load, noload, code, data, rom, and debug.

This was found by testing llvm-objcopy with the gnu binutils test suite, specifically this test case: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/testsuite/binutils-all/copy-1.d;h=f2b0d9e90df738c2891b4d5c7b62f62894b556ca;hb=HEAD

[rupprecht]

Note: I committed rL357199 as a much less interesting refactoring in preparation for this patch; if you have any comments on that patch, I can correct them here.

[jhenderson]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

Won't this change the type of other non-NOBITS sections (e.g. SHT_RELA or SHT_DYNSYM)? Is that intended (I suspect not...)?

[MaskRay]

The intention makes sense. I just found this piece of code

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cc00a5d100973549bf5e4840937529633f4de1fa;f=bfd/elf.c#l3136

int
bfd_elf_get_default_section_type (flagword flags)
{
  if ((flags & (SEC_ALLOC | SEC_IS_COMMON)) != 0
      && (flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
    return SHT_NOBITS;
  return SHT_PROGBITS;
}

Can the SectionFlag rules be simplified?

[grimar]

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

52

Would it be better to introduce a new section .foo with SHT_NOBITS?

[grimar]

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

52

I mean a new section '.bar' :) Sorry. I.e. To add a section instead of changing the existent one in a test.
(The same applies for the 'rename-section-flag.test', btw).

[rupprecht]

• Only change the type if the input is SHT_NOBITS
• Add test coverage

[rupprecht]

In D59958#1447533, @MaskRay wrote:

The intention makes sense. I just found this piece of code

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;hb=cc00a5d100973549bf5e4840937529633f4de1fa;f=bfd/elf.c#l3136

int
bfd_elf_get_default_section_type (flagword flags)
{
  if ((flags & (SEC_ALLOC | SEC_IS_COMMON)) != 0
      && (flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
    return SHT_NOBITS;
  return SHT_PROGBITS;
}

Can the SectionFlag rules be simplified?

I don't (yet) see a good way to simplify it after looking at that snippet. I think there may be handling of the section flags elsewhere that isn't captured by that method.
For example, that only ever returns SHT_NOBITS or SHT_PROGBITS, but experimentation shows that the end result can be SHT_RELA.

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

52

Sure, reverted .foo and added .baz (also .rela.foo to test something else) instead of just changing the test case. Since the rename test is renaming it to .bar, the new section is .baz.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

I did some experimentation with gnu objcopy.

• For some sections (rela, symtab, init_array), it never touched the section type, *or* SHF flags (so we're actually already different there)
• For some sections like .gnu.hash, it *always* changed it to progbits (even for flags like alloc that don't change nobits to progbits).

Fixed this to only change the type if the old type is NOBITS.

[grimar]

It looks fine to me, though I'd wait for somebodies else opinion too.
Few minor comments/suggestions are inline.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

88

btw (not relative to this patch though), this is a bit strange name for this method.
It does not set anything, so seems it should be started with get*.

99

Seems Flags would be a better name for argument here? Since in the current context, there are no old/new flags.
Also, this method changes not only flags but also a type, so perhaps it should be named setSectionFlagsAndType?

104

What about doing the early return here? I.e.:

// Certain flags also promote SHT_NOBITS to SHT_PROGBITS. Don't change other
// section types (RELA, SYMTAB, etc.).
if (Sec.Type != SHT_NOBITS)
  return;

const SectionFlag NoBitsToProgBitsMask = ...

[MaskRay]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

SHT_NULL may also change but it may not have much sense.

Other than that, SHT_NOBITS -> SHT_PROGBITS is the only possible change. I believe GNU objcopy's rule is similar to the following:

if (Sec.Type == SHT_NOTES && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & !(SectionFlags::SecLoad))
  Sec.Type = SHT_PROGBITS;

[MaskRay]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

Sorry, excessive !. It should be:

if (Sec.Type == SHT_NOTES && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & SectionFlags::SecLoad)
  Sec.Type = SHT_PROGBITS;

[MaskRay]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

Really sorry, not SHT_NOTES ...

if (Sec.Type == SHT_NOBITS && NewFlags & SectionFlags::SecAlloc &&
    NewFlags & SectionFlags::SecLoad)
  Sec.Type = SHT_PROGBITS;

[rupprecht]

• Fix some method/variable names

[rupprecht]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

104

It'd be a mid-function return, not an early return, since it happens after some other important modifications to Section (SHF_ flags) have been made. IMO mid-function returns are harder to understand and cause pain in the long run. Otherwise I think that'd be a good idea.

107

That's not the behavior I see. For example, when I run --set-section-flags=.baz=contents on the set-section-flags.test obj2yaml test file, GNU objcopy changes SHT_NOBITS to SHT_PROGBITS for that section, but the expression you give only changes it for alloc and load.

Perhaps you are getting SEC_ flags (which are bfd abstractions) mixed up with SHF_ flags (i.e. the actual ELF flags)?

[grimar]

I have no other comments. Thanks!

[jhenderson]

LGTM.

[MaskRay]

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

107

The rules are more complex than I thought, but I feel I understand its rules now. See https://reviews.llvm.org/D60189 for the simplification.

llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test

 # RUN: yaml2obj %s > %t
 
 # Single flags on a section with no flags:
 # RUN: llvm-objcopy --rename-section=.foo=.bar,alloc %t %t.alloc
-# RUN: llvm-readobj --sections %t.alloc | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE
+# RUN: llvm-readobj --sections %t.alloc | FileCheck %s --check-prefixes=CHECK,NOBITS,ALLOC,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,load %t %t.load
-# RUN: llvm-readobj --sections %t.load | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.load | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,noload %t %t.noload
-# RUN: llvm-readobj --sections %t.noload | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.noload | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,readonly %t %t.readonly
-# RUN: llvm-readobj --sections %t.readonly | FileCheck %s --check-prefixes=CHECK
+# RUN: llvm-readobj --sections %t.readonly | FileCheck %s --check-prefixes=CHECK,NOBITS
 # RUN: llvm-objcopy --rename-section=.foo=.bar,debug %t %t.debug
-# RUN: llvm-readobj --sections %t.debug | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.debug | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,code %t %t.code
-# RUN: llvm-readobj --sections %t.code | FileCheck %s --check-prefixes=CHECK,EXEC,WRITE
+# RUN: llvm-readobj --sections %t.code | FileCheck %s --check-prefixes=CHECK,PROGBITS,EXEC,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,data %t %t.data
-# RUN: llvm-readobj --sections %t.data | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.data | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,rom %t %t.rom
-# RUN: llvm-readobj --sections %t.rom | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.rom | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,contents %t %t.contents
-# RUN: llvm-readobj --sections %t.contents | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.contents | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,merge %t %t.merge
-# RUN: llvm-readobj --sections %t.merge | FileCheck %s --check-prefixes=CHECK,MERGE,WRITE
+# RUN: llvm-readobj --sections %t.merge | FileCheck %s --check-prefixes=CHECK,NOBITS,MERGE,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,strings %t %t.strings
-# RUN: llvm-readobj --sections %t.strings | FileCheck %s --check-prefixes=CHECK,STRINGS,WRITE
+# RUN: llvm-readobj --sections %t.strings | FileCheck %s --check-prefixes=CHECK,NOBITS,STRINGS,WRITE
 # RUN: llvm-objcopy --rename-section=.foo=.bar,share %t %t.share
-# RUN: llvm-readobj --sections %t.share | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.share | FileCheck %s --check-prefixes=CHECK,NOBITS,WRITE
 
 # Multiple flags:
 # RUN: llvm-objcopy --rename-section=.foo=.bar,alloc,readonly,strings %t %t.alloc_ro_strings
-# RUN: llvm-readobj --sections %t.alloc_ro_strings | FileCheck %s --check-prefixes=CHECK,ALLOC,STRINGS
+# RUN: llvm-readobj --sections %t.alloc_ro_strings | FileCheck %s --check-prefixes=CHECK,NOBITS,ALLOC,STRINGS
 # RUN: llvm-objcopy --rename-section=.foo=.bar,alloc,code %t %t.alloc_code
-# RUN: llvm-readobj --sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,ALLOC,EXEC,WRITE
+# RUN: llvm-readobj --sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,PROGBITS,ALLOC,EXEC,WRITE
 
 # Invalid flags:
 # RUN: not llvm-objcopy --rename-section=.foo=.bar,xyzzy %t %t.xyzzy 2>&1 | FileCheck %s --check-prefix=BAD-FLAG
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .foo
-    Type:            SHT_PROGBITS
+    Type:            SHT_NOBITS
     Flags:           [ ]
-    Content:        "c3c3c3c3"
 
-# CHECK:        Name: .bar
-# CHECK-NEXT:   Type: SHT_PROGBITS
-# CHECK-NEXT:   Flags [
-# ALLOC-NEXT:     SHF_ALLOC (0x2)
-# EXEC-NEXT:      SHF_EXECINSTR (0x4)
-# MERGE-NEXT:     SHF_MERGE (0x10)
-# STRINGS-NEXT:   SHF_STRINGS (0x20)
-# WRITE-NEXT:     SHF_WRITE (0x1)
-# CHECK-NEXT:   ]
+# CHECK:         Name: .bar
+# NOBITS-NEXT:   Type: SHT_NOBITS
+# PROGBITS-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT:    Flags [
+# ALLOC-NEXT:      SHF_ALLOC (0x2)
+# EXEC-NEXT:       SHF_EXECINSTR (0x4)
+# MERGE-NEXT:      SHF_MERGE (0x10)
+# STRINGS-NEXT:    SHF_STRINGS (0x20)
+# WRITE-NEXT:      SHF_WRITE (0x1)
+# CHECK-NEXT:    ]
 
 # BAD-FLAG: Unrecognized section flag 'xyzzy'. Flags supported for GNU compatibility: alloc, load, noload, readonly, debug, code, data, rom, share, contents, merge, strings

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

 # RUN: yaml2obj %s > %t
 
 # Single flags on a section with no flags:
 # RUN: llvm-objcopy --set-section-flags=.foo=alloc %t %t.alloc
-# RUN: llvm-readobj --sections %t.alloc | FileCheck %s --check-prefixes=CHECK,ALLOC,WRITE
+# RUN: llvm-readobj --sections %t.alloc | FileCheck %s --check-prefixes=CHECK,NOBITS,ALLOC,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=load %t %t.load
-# RUN: llvm-readobj --sections %t.load | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.load | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=noload %t %t.noload
-# RUN: llvm-readobj --sections %t.noload | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.noload | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=readonly %t %t.readonly
-# RUN: llvm-readobj --sections %t.readonly | FileCheck %s --check-prefixes=CHECK
+# RUN: llvm-readobj --sections %t.readonly | FileCheck %s --check-prefixes=CHECK,NOBITS
 # RUN: llvm-objcopy --set-section-flags=.foo=debug %t %t.debug
-# RUN: llvm-readobj --sections %t.debug | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.debug | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=code %t %t.code
-# RUN: llvm-readobj --sections %t.code | FileCheck %s --check-prefixes=CHECK,EXEC,WRITE
+# RUN: llvm-readobj --sections %t.code | FileCheck %s --check-prefixes=CHECK,PROGBITS,EXEC,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=data %t %t.data
-# RUN: llvm-readobj --sections %t.data | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.data | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=rom %t %t.rom
-# RUN: llvm-readobj --sections %t.rom | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.rom | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=contents %t %t.contents
-# RUN: llvm-readobj --sections %t.contents | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.contents | FileCheck %s --check-prefixes=CHECK,PROGBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=merge %t %t.merge
-# RUN: llvm-readobj --sections %t.merge | FileCheck %s --check-prefixes=CHECK,MERGE,WRITE
+# RUN: llvm-readobj --sections %t.merge | FileCheck %s --check-prefixes=CHECK,MERGE,NOBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=strings %t %t.strings
-# RUN: llvm-readobj --sections %t.strings | FileCheck %s --check-prefixes=CHECK,STRINGS,WRITE
+# RUN: llvm-readobj --sections %t.strings | FileCheck %s --check-prefixes=CHECK,STRINGS,NOBITS,WRITE
 # RUN: llvm-objcopy --set-section-flags=.foo=share %t %t.share
-# RUN: llvm-readobj --sections %t.share | FileCheck %s --check-prefixes=CHECK,WRITE
+# RUN: llvm-readobj --sections %t.share | FileCheck %s --check-prefixes=CHECK,NOBITS,WRITE
 
 # Multiple flags:
 # RUN: llvm-objcopy --set-section-flags=.foo=alloc,readonly,strings %t %t.alloc_ro_strings
-# RUN: llvm-readobj --sections %t.alloc_ro_strings | FileCheck %s --check-prefixes=CHECK,ALLOC,STRINGS
+# RUN: llvm-readobj --sections %t.alloc_ro_strings | FileCheck %s --check-prefixes=CHECK,NOBITS,ALLOC,STRINGS
 # RUN: llvm-objcopy --set-section-flags=.foo=alloc,code %t %t.alloc_code
-# RUN: llvm-readobj --sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,ALLOC,EXEC,WRITE
+# RUN: llvm-readobj --sections %t.alloc_code | FileCheck %s --check-prefixes=CHECK,PROGBITS,ALLOC,EXEC,WRITE
 
 # Invalid flags:
 # RUN: not llvm-objcopy --set-section-flags=.foo=xyzzy %t %t.xyzzy 2>&1 | FileCheck %s --check-prefix=BAD-FLAG
 
 # Bad flag format:
 # RUN: not llvm-objcopy --set-section-flags=.foo %t %t2 2>&1 | FileCheck %s --check-prefix=BAD-FORMAT
 
 # Setting flags for the same section multiple times:
 # RUN: not llvm-objcopy --set-section-flags=.foo=alloc --set-section-flags=.foo=load %t %t2 2>&1 | FileCheck %s --check-prefix=MULTIPLE-SETS
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .foo
-    Type:            SHT_PROGBITS
+    Type:            SHT_NOBITS
     Flags:           [ ]
 
-# CHECK:        Name: .foo
-# CHECK-NEXT:   Type: SHT_PROGBITS
-# CHECK-NEXT:   Flags [
-# ALLOC-NEXT:     SHF_ALLOC (0x2)
-# EXEC-NEXT:      SHF_EXECINSTR (0x4)
-# MERGE-NEXT:     SHF_MERGE (0x10)
-# STRINGS-NEXT:   SHF_STRINGS (0x20)
-# WRITE-NEXT:     SHF_WRITE (0x1)
-# CHECK-NEXT:   ]
+# CHECK:         Name: .foo
+# NOBITS-NEXT:   Type: SHT_NOBITS
+# PROGBITS-NEXT: Type: SHT_PROGBITS
+# CHECK-NEXT:    Flags [
+# ALLOC-NEXT:      SHF_ALLOC (0x2)
+# EXEC-NEXT:       SHF_EXECINSTR (0x4)
+# MERGE-NEXT:      SHF_MERGE (0x10)
+# STRINGS-NEXT:    SHF_STRINGS (0x20)
+# WRITE-NEXT:      SHF_WRITE (0x1)
+# CHECK-NEXT:    ]
 
 # BAD-FORMAT: Bad format for --set-section-flags: missing '='
 # MULTIPLE-SETS: --set-section-flags set multiple times for section .foo
 
 # BAD-FLAG: Unrecognized section flag 'xyzzy'. Flags supported for GNU compatibility: alloc, load, noload, readonly, debug, code, data, rom, share, contents, merge, strings

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp

if (AllFlags & SectionFlag::SecCode)
     NewFlags |= ELF::SHF_EXECINSTR;
   if (AllFlags & SectionFlag::SecMerge)
     NewFlags |= ELF::SHF_MERGE;
   if (AllFlags & SectionFlag::SecStrings)
     NewFlags |= ELF::SHF_STRINGS;
   return NewFlags;
 }
 
 static uint64_t setSectionFlagsPreserveMask(uint64_t OldFlags,
                                             uint64_t NewFlags) {
   // Preserve some flags which should not be dropped when setting flags.
   // Also, preserve anything OS/processor dependant.
   const uint64_t PreserveMask = ELF::SHF_COMPRESSED | ELF::SHF_EXCLUDE |
                                 ELF::SHF_GROUP | ELF::SHF_LINK_ORDER |
                                 ELF::SHF_MASKOS | ELF::SHF_MASKPROC |
                                 ELF::SHF_TLS | ELF::SHF_INFO_LINK;
   return (OldFlags & PreserveMask) | (NewFlags & ~PreserveMask);
 }
 
+static void setSectionFlags(SectionBase &Sec, SectionFlag NewFlags) {
+  Sec.Flags = setSectionFlagsPreserveMask(Sec.Flags, getNewShfFlags(NewFlags));
+
+  // Certain flags also promote SHT_NOBITS to SHT_PROGBITS
+  if (NewFlags &
+      (SectionFlag::SecContents | SectionFlag::SecLoad |
+       SectionFlag::SecNoload | SectionFlag::SecCode | SectionFlag::SecData |
+       SectionFlag::SecRom | SectionFlag::SecDebug))
+    Sec.Type = SHT_PROGBITS;
+}
+
 static ElfType getOutputElfType(const Binary &Bin) {
   // Infer output ELF type from the input ELF object
   if (isa<ELFObjectFile<ELF32LE>>(Bin))
     return ELFT_ELF32LE;
   if (isa<ELFObjectFile<ELF64LE>>(Bin))
     return ELFT_ELF64LE;
   if (isa<ELFObjectFile<ELF32BE>>(Bin))
     return ELFT_ELF32BE;
static Error handleArgs(const CopyConfig &Config, Object &Obj,
 
   if (!Config.SectionsToRename.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SectionsToRename.find(Sec.Name);
       if (Iter != Config.SectionsToRename.end()) {
         const SectionRename &SR = Iter->second;
         Sec.Name = SR.NewName;
         if (SR.NewFlags.hasValue())
-          Sec.Flags = setSectionFlagsPreserveMask(
-              Sec.Flags, getNewShfFlags(SR.NewFlags.getValue()));
+          setSectionFlags(Sec, SR.NewFlags.getValue());
       }
     }
   }
 
   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;
-        Sec.Flags = setSectionFlagsPreserveMask(Sec.Flags,
-                                                getNewShfFlags(SFU.NewFlags));
+        setSectionFlags(Sec, SFU.NewFlags);
       }
     }
   }
 
   for (const auto &Flag : Config.AddSection) {
     std::pair<StringRef, StringRef> SecPair = Flag.split("=");
     StringRef SecName = SecPair.first;
     StringRef File = SecPair.second;