This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add basic support for --rename-section
ClosedPublic

Authored by rupprecht on Jul 19 2018, 4:48 PM.

Details

Summary

Add basic support for --rename-section=old=new to llvm-objcopy.

A full replacement for GNU objcopy requires also modifying flags (i.e. --rename-section=old=new,flag1,flag2); I'd like to keep that in a separate change to keep this simple.

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Jul 19 2018, 4:48 PM
rupprecht updated this revision to Diff 156390.Jul 19 2018, 4:50 PM

Fixing no trailing \n in a test

alexander-shaposhnikov requested changes to this revision.Jul 19 2018, 4:58 PM
alexander-shaposhnikov added inline comments.
tools/llvm-objcopy/Object.cpp
1087 ↗(On Diff #156390)

nit: for iterators i think it's fine to use simply auto

tools/llvm-objcopy/llvm-objcopy.cpp
434 ↗(On Diff #156390)

nit: for one-line "if" braces are not necessary, for instance, in https://llvm.org/docs/CodingStandards.html examples they are omitted

597 ↗(On Diff #156390)

these error messages need to be tested (lines 597, 600)

This revision now requires changes to proceed.Jul 19 2018, 4:58 PM
rupprecht updated this revision to Diff 156400.Jul 19 2018, 5:07 PM

Addressed all comments

rupprecht marked 3 inline comments as done.Jul 19 2018, 5:10 PM
rupprecht added inline comments.
tools/llvm-objcopy/llvm-objcopy.cpp
597 ↗(On Diff #156390)

600 should already be covered by --check-prefix=MULTIPLE-RENAMES in test/tools/llvm-objcopy/rename-section.test, let me know if you meant something else.
Added a test case for bad format.

to me LG, but I'd wait for Jake as well

tools/llvm-objcopy/Object.cpp
1088 ↗(On Diff #156400)

nit (braces)

This revision is now accepted and ready to land.Jul 20 2018, 10:33 AM
rupprecht marked 3 inline comments as done.Jul 20 2018, 10:49 AM
rupprecht updated this revision to Diff 156543.EditedJul 20 2018, 10:49 AM

Fixed braces

jakehehrlich requested changes to this revision.Jul 20 2018, 11:05 AM

I want to keep the public interface of Object as small as possible but other than that LGTM

tools/llvm-objcopy/Object.cpp
1085 ↗(On Diff #156543)

This should be done where the option is handled, not by adding a method.

tools/llvm-objcopy/Object.h
705 ↗(On Diff #156543)

We don't need a rename section function because you can already loop over sections and update them.

This revision now requires changes to proceed.Jul 20 2018, 11:05 AM

Revert changes to Object; do the renaming in objcopy directly.

rupprecht marked 2 inline comments as done.Jul 20 2018, 11:23 AM

Thanks for the suggestion; removing the Object method will probably make the next change (updating flags) simpler.

jakehehrlich accepted this revision.Jul 20 2018, 11:25 AM
This revision is now accepted and ready to land.Jul 20 2018, 11:25 AM
This revision was automatically updated to reflect the committed changes.