This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Make --set-section-flags work with --add-section
ClosedPublic

Authored by MaskRay on Oct 29 2020, 4:33 PM.

Details

Summary

This matches behavior GNU objcopy and can simplify clang-offload-bundler
(which currently works around the issue by invoking llvm-objcopy twice).

Diff Detail

Event Timeline

MaskRay created this revision.Oct 29 2020, 4:33 PM
MaskRay requested review of this revision.Oct 29 2020, 4:33 PM

You may ask what objcopy --add-gnu-debuglink=a --set-section-flags=.gnu_debuglink=alloc does. It does not change the flags of .gnu_debuglink and thus our behavior matches GNU.
It seems like an a combo users won't use and I don't bother adding a test.

LGTM. I'll wait for a second person to stamp it.

You may ask what objcopy --add-gnu-debuglink=a --set-section-flags=.gnu_debuglink=alloc does. It does not change the flags of .gnu_debuglink and thus our behavior matches GNU.
It seems like an a combo users won't use and I don't bother adding a test.

I wasn't going to, actually :)

Maybe we should allow that, but print a warning that the user is changing flags for a special section, and behavior after that may be unexpected. And apply that warning for other kinds of special sections. That may be beyond the scope of this patch, though.

The change looks good to me.

llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-flags.test
4

GNU objcopy (GNU Binutils for Ubuntu) 2.31.1 doesn't recognize exclude, btw:

objcopy: unrecognized section flag `exclude'
objcopy: supported flags: alloc, load, noload, readonly, debug, code, data, rom, share, contents, merge, strings

MaskRay marked an inline comment as done.Oct 30 2020, 12:19 AM
MaskRay added inline comments.
llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-flags.test
4

Right. I added 'exclude' to GNU objcopy 2.35 :)

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-flags.test
15

It might be a good idea to change this to --- !ELF so that other YAML docs could be added in the future, if needed, without needing to change this line.

MaskRay updated this revision to Diff 302296.Nov 2 2020, 8:43 AM
MaskRay marked 2 inline comments as done.

Add --- to the test

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

I know what you are getting at: objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags=.symtab=exclude a a2 => the flags of .symtab are unchanged. This may be due to BFD's special handling of .symtab
It is probably indeed an unhelpful behavior.

In this patch I don't want to touch that area:/ Changing the flags of .symtab is still an unlikely operation. --add-sections .* set-section-flags is more common.

I think it's unlikely any user is ever going to want to change the flags of .gnu_debuglink. I think there's a slightly higher chance that they might want to for .symtab, which could potentially be added later on. However, I don't see any particular reason to prevent it - in this case, I'd consider GNU's behaviour unhelpful as if somebody has explicitly asked to change the flags of one of these sections, they presumably actually want to and ignoring it is unhelpful. Consequently, where we can easily support it, we should (in this case, I believe it's just move the new code even later in the function), but I don't think it's worth doing if it's hard to support.

I know what you are getting at: objcopy -R .symtab --add-symbol newsym=1234 --set-section-flags=.symtab=exclude a a2 => the flags of .symtab are unchanged. This may be due to BFD's special handling of .symtab
It is probably indeed an unhelpful behavior.

In this patch I don't want to touch that area:/ Changing the flags of .symtab is still an unlikely operation. --add-sections .* set-section-flags is more common.

My only change request was to just move the code you're moving in this patch to later in the same function. It's no more a complicated a change than the change you are doing already. This may not give all the support necessary, but it should make it easier to add support later on, if the need arises. See inline comment for the concrete location I'm suggesting. If it does start working at that point, you might want a test, but I don't mind too much either way (since as you say it would be a rare behaviour).

llvm/test/tools/llvm-objcopy/ELF/add-section-and-set-flags.test
16–17

Tends to be more common in the YAML I've seen elsewhere.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
745–754

I suggest moving this block to...

773

... here.

MaskRay updated this revision to Diff 302601.Nov 3 2020, 8:55 AM
MaskRay marked an inline comment as done.

Comments

MaskRay marked 2 inline comments as done.Nov 3 2020, 8:55 AM
This revision is now accepted and ready to land.Nov 4 2020, 12:12 AM
This revision was landed with ongoing or failed builds.Nov 4 2020, 9:39 AM
This revision was automatically updated to reflect the committed changes.