[PATCH] D72128: [llvm-objcopy][ELF] Allow setting SHF_EXCLUDE flag for ELF sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm assuming that GNU objcopy doesn't have this flag? Also, what is your use-case for adding it?

I think this is a perfectly reasonable extension to llvm-objcopy if there's a valid use-case, but in some cases it might just be more appropriate to remove the section.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test:51-53
+# RUN: llvm-objcopy --rename-section=.foo=.bar,contents,readonly,exclude  \
+# RUN:   --rename-section=.baz=.blah,contents,readonly,exclude %t %t.contents_ro_exclude
+# RUN: llvm-readobj --sections %t.contents_ro_exclude | FileCheck %s --check-prefixes=CHECK,PROGBITS,EXCLUDE
----------------
What's the reasoning for adding this extra part? Do you anticipate some interaction between exclude and readonly or contents that's relevant?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:73
+  SecExclude = 1 << 12,
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ SecExclude)
 };
----------------
This is an existing issue, but you might as well fix it as you change this line. The canonical way of adding a comment to name the parameter of a function is:
`LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/SecExclude)`

Note the different spacing. Clang-format recognises this pattern and adjusts comments to match, but only if there is no space after the close of the comment.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:95-98
+  const uint64_t PreserveMask =
+      ELF::SHF_COMPRESSED | ELF::SHF_GROUP | ELF::SHF_LINK_ORDER |
+      ELF::SHF_MASKOS | ELF::SHF_MASKPROC | ELF::SHF_TLS | ELF::SHF_INFO_LINK;
+  return (OldFlags & PreserveMask) | NewFlags;
----------------
I'm slightly concerned that SHF_EXCLUDE will no longer get preserved by default, which could break people's existing usage. I don't know if that's a big deal, but it could cause unexpected behaviour later in the build. Perhaps we should consider having exclude/noexclude options that add/remove the flag, but where the flag is otherwise maintained in its current state? What do others think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72128