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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:107
+       SectionFlag::SecRom | SectionFlag::SecDebug))
+    Sec.Type = SHT_PROGBITS;
+}
----------------
MaskRay wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > rupprecht wrote:
> > > > 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.
> > > `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;
> > > ```
> > Sorry, excessive `!`. It should be:
> > 
> > ```
> > if (Sec.Type == SHT_NOTES && NewFlags & SectionFlags::SecAlloc &&
> >     NewFlags & SectionFlags::SecLoad)
> >   Sec.Type = SHT_PROGBITS;
> > ```
> Really sorry, not `SHT_NOTES` ...
> 
> ```
> if (Sec.Type == SHT_NOBITS && NewFlags & SectionFlags::SecAlloc &&
>     NewFlags & SectionFlags::SecLoad)
>   Sec.Type = SHT_PROGBITS;
> ```
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)?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:104
+  // section types (RELA, SYMTAB, etc.).
+  const SectionFlag NoBitsToProgBitsMask =
+      SectionFlag::SecContents | SectionFlag::SecLoad | SectionFlag::SecNoload |
----------------
grimar wrote:
> 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 = ...
> ```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59958