This patch adds support for setting SHF_EXCLUDE flag for ELF sections.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test | ||
---|---|---|
51–53 | What's the reasoning for adding this extra part? Do you anticipate some interaction between exclude and readonly or contents that's relevant? | |
llvm/tools/llvm-objcopy/CopyConfig.h | ||
73 | 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: 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. | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
100–101 | 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? |
Yes, as far as I know GNU objcopy does not support setting “exclude” (SHF_EXCLUDE) flag, though I believe having such ability would be useful. BTW, COFF has similar flag IMAGE_SCN_LNK_REMOVE, so this functionality can also be extended to COFF.
And you are right, I have a use case in mind which needs this functionality. 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. These sections ideally should be discarded by the linker when it links the host image since they do not participate in the host linking process at all, and the best way to achieve that would be adding SHF_EXCLUDE flag to those target object sections. That is the reason why I want to add this functionality to the llvm-objcopy tool.
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test | ||
---|---|---|
51–53 | 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. | |
llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test | ||
53–56 | Should I remove this part as well? | |
llvm/tools/llvm-objcopy/CopyConfig.h | ||
73 | Sure, I will fix this. |
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?
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test | ||
---|---|---|
51–53 | I also feel that this is redundant. | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
100–101 | 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. |
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.
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag.test | ||
---|---|---|
51–53 | Ok, I will remove it. | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
100–101 | 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. |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 | 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@sourceware.org asking whether they'd like add exclude, and whether SHF_EXCLUDE should be preserved. Ideally llvm-objcopy can avoid the flag noexclude. |
llvm/test/tools/llvm-objcopy/ELF/set-section-flags.test | ||
---|---|---|
53–56 | Yes please. | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
100–101 | 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 :) |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 | 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. |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 | I replied there. I feel that inventing new syntax like +exclude and -exclude is really unnecessary. Let me send a patch to binutils. |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 | Are you going to use exclude/noexclude in your binutils patch? If so, I will update this patch to that syntax as well. |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
100–101 | Ok. I will update this patch to make it consistent with your binutils change. |
Aside from the inline comment, this looks good to me. Might want to get @MaskRay to approve before committing though.
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag-preserved.test | ||
---|---|---|
72–73 | I can see this is unreferenced in this test, but removing it shouldn't be part of this commit. Please make a separate change just deleting this line and the blank line above it, without need for further review. |
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag-preserved.test | ||
---|---|---|
72–73 | I've just found that I accidentally missed this comment and because of that committed this change together with the other changes, sorry. |
llvm/test/tools/llvm-objcopy/ELF/rename-section-flag-preserved.test | ||
---|---|---|
72–73 | No worries. It happens. |
Heads-up: GNU objcopy>=2.35 will support --set-section-flags=exclude. https://sourceware.org/ml/binutils/2020-02/msg00195.html
The current version does not remove 0x80000000 (SHF_EXCLUDE) when exclude is not specified, but it seems that Nick may want to fix this for architectures other than ARM/MIPS/PARISC.
The value 0x80000000 may be unfortunate. .debug_*.dwo uses the flag. From the discussions I don't think the llvm-objcopy vs GNU objcopy difference will cause any trouble.
I can see this is unreferenced in this test, but removing it shouldn't be part of this commit. Please make a separate change just deleting this line and the blank line above it, without need for further review.