Page MenuHomePhabricator

[llvm-objcopy] Add --prefix-alloc-sections
ClosedPublic

Authored by seiya on Mar 31 2019, 12:57 AM.

Details

Summary

This patch adds support for --prefix-alloc-sections, which adds a prefix to every allocated section names.

It adds a prefix after renaming section names by --rename-section as GNU objcopy does.

Fixes PR41266: https://bugs.llvm.org/show_bug.cgi?id=41266

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Apr 4 2019, 4:45 AM
llvm/tools/llvm-objcopy/ObjcopyOpts.td
235 ↗(On Diff #193688)

Nit: names -> name

seiya updated this revision to Diff 193694.Apr 4 2019, 5:34 AM
seiya marked an inline comment as done and an inline comment as not done.
  • Added a comment which describes how it handles dynamic relocations sections and why.
  • Fixed a help message.
seiya marked 4 inline comments as done.Apr 4 2019, 5:34 AM
seiya updated this revision to Diff 193695.Apr 4 2019, 5:37 AM
  • Removed a redundant whitespace from a comment.
jhenderson added inline comments.Apr 4 2019, 5:38 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
633 ↗(On Diff #193694)

Nit: remove the "in".

Just a couple small comments, but also LGTM

llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
59 ↗(On Diff #193695)

Checking the } isn't necessary (unless you're checking a whole section w/ CHECK-NEXT)

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
179–181 ↗(On Diff #193695)

I think this block needs to be clang formatted

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
650–654 ↗(On Diff #193695)

Can you add a comment why this block is necessary?

It looks like all the test coverage is .rel sections coming after their target section. Is it possible to add a test case for the other half of this clause? e.g. a section w/ .rel.data coming before .data?

seiya updated this revision to Diff 193816.EditedApr 4 2019, 6:08 PM
  • Updated a test case to test that it renames properly when a relocation section comes before its target section.
  • Removed unnecessary } from a test.
  • Added a comment which describes the case when a relocation section comes before its target section.
  • Fixed a comment.
  • Applied clang-format.
seiya marked 4 inline comments as done.Apr 4 2019, 6:09 PM
jhenderson added inline comments.Apr 5 2019, 1:12 AM
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
3 ↗(On Diff #193432)

I'm still not sure what extra coverage --relocs gives us. Could somebody explain, please?

seiya marked an inline comment as done.Apr 6 2019, 6:59 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
3 ↗(On Diff #193432)

I think it would be good to make sure that this feature (--prefix-alloc-sections) does not destroy the contents of relocation sections other than their names. I should add a comment here.

How do you think @rupprecht ?

rupprecht accepted this revision.Apr 9 2019, 11:22 AM

I'm out the rest of the week, I'll leave it to James to do the commit on your behalf

llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
3 ↗(On Diff #193432)

After talking to James offline, the --relocs effectively isn't testing anything in addition to what's already covered, so it should be fine to remove. Sorry for suggesting that.

seiya updated this revision to Diff 194424.Apr 9 2019, 5:35 PM
  • Removed --relocs from a test.
seiya marked 4 inline comments as done.Apr 9 2019, 5:35 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
3 ↗(On Diff #193432)

No problem! I've updated the diff.

seiya marked an inline comment as done.Apr 9 2019, 5:37 PM

Having looked at this a bit more, I wonder if this should be part of the existing SectionsToRename loop. After all, there isn't much difference between the two behaviours, apart from the fact that this new switch doesn't change the flags. Would you mind folding the two loops together? As a side effect of this, I expect you might want to rename relocation sections when --rename-section alone is used, which I don't think is currently happening.

Also @jakehehrlich explicitly requested changes, I'd like him to give a thumbs up before I commit this. Thanks for the work @seiya!

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
583–584 ↗(On Diff #194424)

This is the loop I'm talking about in the out-of-line comment.

seiya added a comment.Apr 11 2019, 6:02 AM

Having looked at this a bit more, I wonder if this should be part of the existing SectionsToRename loop. After all, there isn't much difference between the two behaviours, apart from the fact that this new switch doesn't change the flags. Would you mind folding the two loops together? As a side effect of this, I expect you might want to rename relocation sections when --rename-section alone is used, which I don't think is currently happening.

That's a good idea. I'll fold them together.

seiya updated this revision to Diff 194668.EditedApr 11 2019, 6:06 AM
  • Merged the existing SectionsToRename loop and the AllocSectionsPrefix loop into a single loop.
seiya marked an inline comment as done.Apr 11 2019, 6:06 AM
jhenderson added inline comments.Apr 11 2019, 6:13 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
583 ↗(On Diff #194668)

You should probably keep this if, but expand it to be:
if (!Config.SectionsToRename.empty() || !Config.AllocSectionsPrefix.empty())

That will mean that this loop only happens if one or both of the options is specified.

593 ↗(On Diff #194668)

Nit config -> Config to match the variable name.

seiya updated this revision to Diff 194671.Apr 11 2019, 6:28 AM
  • Check if one or both of --rename-section/--prefix-alloc-sections options is specified.
  • Fixed a comment.
seiya marked 2 inline comments as done.Apr 11 2019, 6:29 AM
jhenderson accepted this revision.Apr 11 2019, 6:33 AM

LGTM. As I mentioned earlier, @jakehehrlich should review this before it gets committed. However, he may be away, so you might have to wait a few days.

rupprecht accepted this revision.Apr 15 2019, 1:31 AM

A few comments.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
607 ↗(On Diff #194671)

You can do if (auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec)) and then SectionBase *TargetSec = RelocSec->getSection()

607 ↗(On Diff #194671)

I forget. Does this apply to dynamic relocation sections as well? Does this flag rename the dynamic relocation section? My hunch is that it doesn't.

626 ↗(On Diff #194671)

This is not a good way to do this.

I think the right way is to have the "OriginalName" and do this in a non-conditional way. I tried thinking about a bunch of alternatives but they all have sticky issues. Your best bet is to just add OriginalName to SectionBase and unconditionally assign the section name here.

seiya marked 2 inline comments as done.Apr 17 2019, 9:42 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
607 ↗(On Diff #194671)

Yes. This flag renames dynamic relocation sections too. It seems that GNU objcopy simply checks whether ALLOC flag is set:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/objcopy.c;h=330b93c1ad3f87a0f5777ac88dc4af1398959bbe;hb=HEAD#l3772

626 ↗(On Diff #194671)

I think that's not sufficient. If both --rename-section and --prefix-alloc-sections are given, GNU objcopy renames sections by --rename-section first and then adds the prefix specified by --prefix-alloc-sections.

For example, --rename-section=.text=.text2 --prefix-alloc-sections=.prefix renames .rel.text to .rel.prefix.text2.

It looks that the current patch only renames correctly if the target section comes before its relocation section by the way.

I have two ideas:

  • Iterate sections twice: rename sections first and then add the prefix.
  • Add Renamed flag to SectionBase: check the flag here and rename the section if needed.

I would like to hear your thoughts on this @jakehehrlich .

seiya marked an inline comment as done.Apr 19 2019, 5:11 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
626 ↗(On Diff #194671)

Or add OriginalName to SectionBase and use Config.SectionsToRename here:

const auto Iter = Config.SectionsToRename.find(TargetSec->OriginalName);
if (Iter != Config.SectionsToRename.end()) {
  // Both `--rename-section` and `--prefix-alloc-sections` are given.
  const SectionRename &SR = Iter->second;
  Sec.Name = (prefix + Config.AllocSectionsPrefix + SR.NewName).str();
} else {
  Sec.Name = (prefix + Config.AllocSectionsPrefix + TargetSec->Name).str();
}
jakehehrlich added inline comments.Apr 22 2019, 12:20 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
607 ↗(On Diff #194671)

Can you add a test case for this. We have a binary checked in that you can use. I want to make sure it isn't double renamed and all that. It's just a slightly more tricky real world use case.

626 ↗(On Diff #194671)

Ah right, good catch. I think the proposal you just gave is better than what we currently have but the issue of the section being renamed should probably happen more smoothly I suppose. In principal we could add a new mechanism for section renaming in the future which would invalidate this and then require us to update it in the future.

What if you build a map of renamed sections here and then check if the section is renamed. That should avoid the fragility of checking for the prefix like this.

seiya updated this revision to Diff 196167.Apr 22 2019, 7:00 PM
  • Use dyn_cast
  • Add PrefixedSections to check whether the target section is already prefixed AllocSectionsPrefix.
  • Handle the case when --rename-section and --prefix-alloc-sections are given and the relocation section comes before its target section.
seiya marked 5 inline comments as done.Apr 22 2019, 7:06 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test
54 ↗(On Diff #196167)

This .rela.plt is a testcase what I mean.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
607 ↗(On Diff #194671)

We have a test case which renames dynamic relocation sections. Should I clarify that in a comment or add a test case against a binary file, say, test/tools/llvm-objcopy/ELF/Inputs/dynrel.elf?

jakehehrlich added inline comments.Apr 25 2019, 2:17 PM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
629 ↗(On Diff #196167)

I'm *really* against doing this kind of string searching when we can build a map. What if a section already contains the prefix being used. This won't be consistent in that case.

Also you should add a test case for that above.

607 ↗(On Diff #194671)

Those two cases are slightly different and reflects more realistic set up than the yaml file does. Should be a short 5 or so line test.

seiya marked an inline comment as done.Apr 28 2019, 8:01 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
629 ↗(On Diff #196167)

Let me clarify my understanding a bit. You mean a map which contains a pointer to a section like DenseSet<SectionBase *> PrefixedSections?

jakehehrlich accepted this revision.Apr 30 2019, 3:54 PM

LGTM

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
629 ↗(On Diff #196167)

Ah whoops. I saw "find" and thought it hadn't changed. Big mistake on my part. Sorry. This LGTM.

This revision is now accepted and ready to land.Apr 30 2019, 3:54 PM
seiya marked an inline comment as done.Apr 30 2019, 6:27 PM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
629 ↗(On Diff #196167)

Thank you for your comment. I'll update the patch.

seiya updated this revision to Diff 197516.May 1 2019, 2:36 AM
seiya marked 6 inline comments as done.
  • Add a test case: a section which already has the prefix specified by this option (.prefix.already_prefixed).
  • Use ELF/Inputs/dynrel.elf in a test instead of a binary constructed by yaml2obj.
  • Store a pointer to a SectionBase in PrefixedSections instead of its name.
seiya updated this revision to Diff 197518.May 1 2019, 2:49 AM
  • Rename a test file.
jhenderson added inline comments.May 1 2019, 2:51 AM
llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test
1 ↗(On Diff #197516)

In this test, you don't really want to test the cases that you already tested in the main --prefix-alloc-sections test. As a result, you can probably remove the bar section, at the very least, nor do you need to test a section that isn't renamed at all.

I'm also not sure you get anything by testing two different section renames.

seiya marked 2 inline comments as done.May 1 2019, 7:35 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/ELF/rename-section-and-prefix-alloc-sections.test
1 ↗(On Diff #197516)

Indeed. I'll remove the test.

I'm also not sure you get anything by testing two different section renames.

We should test the cases when the relocation section comes after/before its target section because this patch deals with them differently to perform both --rename-section and --prefix-alloc-sections in one pass.

I'll update the patch to mention it in this test file.

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
633 ↗(On Diff #197518)

I mean this if block.

seiya updated this revision to Diff 197694.May 1 2019, 7:40 PM
  • Add a comment in rename-section-and-prefix-alloc-sections.test.
  • Remove some test cases which already tested in another test.
jhenderson accepted this revision.May 2 2019, 1:26 AM

Great! LGTM. Would you like me to commit it?

MaskRay added inline comments.May 2 2019, 1:34 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
599 ↗(On Diff #197694)

It looks most other places don't put != 0

MaskRay added inline comments.May 2 2019, 1:37 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
629 ↗(On Diff #196167)

PrefixedSections.count(TargetSec)

jhenderson added inline comments.May 2 2019, 1:40 AM
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
599 ↗(On Diff #197694)

Hmm... I find != 0 easier to read personally, but I don't feel strongly about it.

seiya updated this revision to Diff 198127.May 3 2019, 10:52 PM
seiya marked 4 inline comments as done.May 3 2019, 10:59 PM
seiya added a comment.May 3 2019, 11:04 PM

Great! LGTM. Would you like me to commit it?

Yes please if it got accepted by others :)

Great! LGTM. Would you like me to commit it?

Yes please if it got accepted by others :)

Okay. As @MaskRay had some previous comments, I'll give him a chance to review further before committing.

MaskRay accepted this revision.May 7 2019, 4:22 AM
MaskRay added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
637 ↗(On Diff #198127)

How about Sec.Name = (prefix + Config.AllocSectionsPrefix + Iter->second.NewName).str();

seiya updated this revision to Diff 198596.May 8 2019, 2:02 AM
seiya marked an inline comment as done.
  • Cleaned up the code a bit (suggested by @MaskRay). NFC.
seiya marked an inline comment as done.May 8 2019, 2:02 AM
seiya added inline comments.
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
637 ↗(On Diff #198127)

Looks better. Thanks.

seiya marked an inline comment as done.May 8 2019, 2:03 AM

Great! I'll get this landed later today.

This revision was automatically updated to reflect the committed changes.