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

[Sergey Dmitriev via Phabricator via llvm-commits]

sdmitriev 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;
----------------
jhenderson wrote:
> 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 :)
I have opened binutils bug report for this extension few days ago (please see https://sourceware.org/bugzilla/show_bug.cgi?id=25371), and GNU guys proposed to use `+exclude`/`-exclude` flags instead of `exclude`/`noexclude`. I will update this patch according to that proposal.


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

https://reviews.llvm.org/D72128