This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] Allow setting SHF_EXCLUDE flag for ELF sections
ClosedPublic

Authored by sdmitriev on Jan 3 2020, 12:50 AM.

Diff Detail

Event Timeline

sdmitriev created this revision.Jan 3 2020, 12:50 AM

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:
LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/SecExclude)

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?

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.

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.

sdmitriev updated this revision to Diff 236165.Jan 3 2020, 11:06 PM
sdmitriev marked 2 inline comments as done.

Addressed review comment.

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.

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.

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.

MaskRay added inline comments.Jan 4 2020, 11:36 PM
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.

jhenderson added inline comments.Jan 6 2020, 1:56 AM
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 :)

sdmitriev marked 6 inline comments as done.

Rebased and addressed review comments.

sdmitriev added inline comments.Jan 15 2020, 11:17 PM
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.

MaskRay added inline comments.Jan 16 2020, 12:01 AM
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.

sdmitriev marked an inline comment as done.Jan 16 2020, 5:26 AM
sdmitriev added inline comments.
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.

MaskRay added inline comments.Jan 17 2020, 12:36 AM
sdmitriev added inline comments.Jan 17 2020, 2:01 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
100–101

Ok. I will update this patch to make it consistent with your binutils change.

sdmitriev updated this revision to Diff 238742.Jan 17 2020, 4:32 AM

Addressed review comments.

sdmitriev marked an inline comment as done.Jan 17 2020, 10:05 AM
jhenderson accepted this revision.Jan 20 2020, 12:52 AM

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.

This revision is now accepted and ready to land.Jan 20 2020, 12:52 AM
MaskRay accepted this revision.Jan 20 2020, 10:25 AM
This revision was automatically updated to reflect the committed changes.
sdmitriev added inline comments.Jan 27 2020, 6:56 AM
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.

jhenderson added inline comments.Jan 27 2020, 7:08 AM
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.