This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][COFF] Implement --redefine-sym and --redefine-syms
ClosedPublic

Authored by MaskRay on Nov 8 2019, 5:21 PM.

Details

Summary

The bad usage tests in ELF/redefine-symbols.test are not specific to ELF.
Move them to redefine-symbols.test .

Also fix the documentation regarding --redefine-syms: the old and new
names are separated by whitespace, not an equals sign.

Event Timeline

MaskRay created this revision.Nov 8 2019, 5:21 PM
MaskRay marked an inline comment as done.Nov 8 2019, 5:24 PM
MaskRay added inline comments.
llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test
31

test/tools/llvm-objcopy/ELF/redefine-symbol.test contains tests for incorrect option usage (generic). I guess we probably don't need to duplicate that.

Looks okay to me, but I don't know enough about COFF to know whether there are any hidden gotchas with renaming symbols that might need accounting for.

llvm/test/tools/llvm-objcopy/COFF/redefine-symbol.test
6–7

Maybe add some leading whitespace to one of these two lines?

31

We don't need to duplicate it, but perhaps we should move that bit into test/tools/llvm-objcopy (i.e. out of the ELF layer)?

MaskRay updated this revision to Diff 228739.Nov 11 2019, 10:46 AM
MaskRay marked 2 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Improve tests

MaskRay updated this revision to Diff 228741.Nov 11 2019, 10:49 AM

%t3 -> /dev/null

Harbormaster completed remote builds in B40761: Diff 228741.
jhenderson added inline comments.Nov 12 2019, 1:24 AM
llvm/test/tools/llvm-objcopy/redefine-symbols.test
1 ↗(On Diff #228741)

Maybe worth putting a comment saying why this test is not in the ELF specific test folder (i.e. it contains just the common parsing stuff).

Looks good in general, with two minor comments

llvm/docs/CommandGuide/llvm-objcopy.rst
96

While this just moves the documentation, it seems to me that this documentation is wrong; in the text file, there's no equals sign between old and new names, they're just separated by whitespace.

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
138

One pretty relevant functional aspect of this, is the fact that a request to rename a symbol that doesn't exist passes through silently (which I presume matches GNU behaviour). It might be good to add a test of that aspect.

MaskRay updated this revision to Diff 228910.Nov 12 2019, 9:45 AM
MaskRay marked 4 inline comments as done.

Address review comments

llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp
138

A non-existent symbol does not error. Added a test.

mstorsjo accepted this revision.Nov 12 2019, 11:15 AM

LGTM, but please mention the documentation fix in the commit message, as it's not immediately obvious that the paragraph is edited while it's moved, and the edit is kind of unrelated to this change (other than the lines are moved).

This revision is now accepted and ready to land.Nov 12 2019, 11:15 AM
MaskRay updated this revision to Diff 228925.Nov 12 2019, 11:26 AM
MaskRay edited the summary of this revision. (Show Details)

Update description: fixed the documentation

This revision was automatically updated to reflect the committed changes.