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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test:53-56
+# RUN: llvm-objcopy --set-section-flags=.foo=contents,readonly,exclude \
+# RUN:   --set-section-flags=.baz=contents,readonly,exclude \
+# RUN:   --set-section-flags=.rela.baz=contents,readonly,exclude %t %t.contents_ro_exclude
+# RUN: llvm-readobj --sections %t.contents_ro_exclude | FileCheck %s --check-prefixes=CHECK,PROGBITS,EXCLUDE
----------------
sdmitriev wrote:
> Should I remove this part as well?
Yes please.


================
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;
----------------
MaskRay wrote:
> 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`.
I second the suggestion to raise this with the GNU guys. They may have a specific interface they think should be used, and in that case, we've got one to follow too, without having to make hard decisions :)


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

https://reviews.llvm.org/D72128