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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

In D59958#1447533 <https://reviews.llvm.org/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.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test:52
   - Name:            .foo
-    Type:            SHT_PROGBITS
+    Type:            SHT_NOBITS
     Flags:           [ ]
----------------
grimar wrote:
> grimar wrote:
> > Would it be better to introduce a new section `.foo` with `SHT_NOBITS`?
> 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).
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:107
+       SectionFlag::SecRom | SectionFlag::SecDebug))
+    Sec.Type = SHT_PROGBITS;
+}
----------------
jhenderson wrote:
> Won't this change the type of other non-NOBITS sections (e.g. SHT_RELA or SHT_DYNSYM)? Is that intended (I suspect not...)?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59958/new/

https://reviews.llvm.org/D59958