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

Sat Jan 4 23:40:21 PST 2020

MaskRay added inline comments.

================
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;
----------------
sdmitriev wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > 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?
> > `objcopy --set-section-flags=.text.foo=alloc c.o cc.o` GNU objcopy preserves `SHF_EXCLUDE`, while we won't after this patch.
> > 
> > The flags can be only be changed by --rename-section and --set-section-flags. Neither seems possible operations people may do to a `SHF_EXCLUDE` section. I am fine with the code.
> Breaking backward compatibility or compatibility with the GNU objcopy is probably not good, but having an explicit "noexclude" flag as @jhenderson suggested above would allow to preserve it, I guess. I will update the patch to add "noexclude" flag as well.
I still doubt whether a new flag in llvm-objcopy will be useful. Yes, it will diverge from GNU, but I suspect anyone other than clang-offload-wrapper has such as use case.

You can also send an email to binutils at sourceware.org asking whether they'd like add `exclude`, and whether `SHF_EXCLUDE` should be preserved. Ideally llvm-objcopy can avoid the flag `noexclude`.

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

https://reviews.llvm.org/D72128