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

[Sergey Dmitriev via Phabricator via llvm-commits]

sdmitriev added a comment.

In D72128#1804073 <https://reviews.llvm.org/D72128#1804073>, @MaskRay wrote:

> > As you probably know llvm-objcopy tool is used by another tool clang-offload-wrapper for creating fat object files (when OpenMP offloading is enabled). Fat object is really just a normal host object file with few extra sections (one section per each offload target) which contain target object as data. Clang-offload-bundler uses llvm-objcopy for adding target sections to the fat output.
>
> Does clang-offload-wrapper add SHF_EXCLUDE sections with llvm-objcopy?


No, there is currently no way to add it using llvm-objcopy, but that is what I want to do. That is the reason why I want to add this extension to llvm-objcopy.



================
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
----------------
MaskRay wrote:
> sdmitriev wrote:
> > jhenderson wrote:
> > > What's the reasoning for adding this extra part? Do you anticipate some interaction between exclude and readonly or contents that's relevant?
> > No, nothing special, just wanted to add more test cases for the "exclude" flag:) I can remove this part if you believe it is redundant.
> I also feel that this is redundant.
Ok, I will remove it.


================
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:
> 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.


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

https://reviews.llvm.org/D72128