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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

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



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:88
 
 static uint64_t setSectionFlagsPreserveMask(uint64_t OldFlags,
                                             uint64_t NewFlags) {
----------------
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*`.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:99
 
+static void setSectionFlags(SectionBase &Sec, SectionFlag NewFlags) {
+  Sec.Flags = setSectionFlagsPreserveMask(Sec.Flags, getNewShfFlags(NewFlags));
----------------
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`?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59958