This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add --set-start, --change-start, --adjust-start
ClosedPublic

Authored by evgeny777 on Feb 13 2019, 5:23 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Feb 13 2019, 5:23 AM

Please add a check for this new option in COFF/COFFObjCopy.cpp for the "Option not supported by llvm-objcopy for COFF" error, to avoid giving the illusion that the option is handled with that backend.

(The MachO backend doesn't yet look at options at all and doesn't have a similar error case for unimplemented options.)

rupprecht added inline comments.Feb 14 2019, 2:30 PM
tools/llvm-objcopy/CopyConfig.cpp
558

uint64_t

564

int64_t

tools/llvm-objcopy/CopyConfig.h
106

Can you use a more descriptive name/docs here?

tools/llvm-objcopy/ObjcopyOpts.td
245

Does this allow -adjust-start?

evgeny777 marked an inline comment as done.Feb 14 2019, 10:43 PM
evgeny777 added inline comments.
tools/llvm-objcopy/CopyConfig.cpp
558

On 64 bit platforms uint64_t is unsigned long, not unsigned long long. You'll get the compilation error on x64 if you change type (x86 will be ok)

evgeny777 updated this revision to Diff 186981.Feb 15 2019, 2:20 AM

Addressed some of review comments

This comment was removed by asl.
asl added a subscriber: asl.Feb 16 2019, 10:33 AM
jhenderson added inline comments.Feb 18 2019, 7:10 AM
test/tools/llvm-objcopy/ELF/set-start.test
1 ↗(On Diff #186981)

Maybe worth renaming the test to something like "change-entry-point.test" since set-start implies you are just changing testing the --set-start switch. Alternatively, split the switches into different tests.

2 ↗(On Diff #186981)

Could you also add test cases that show --set-start takes decimal, please?

What happens if you try to specify a negative number here?

6 ↗(On Diff #186981)

Haven't checked GNU objcopy to see if this makes sense, but can you specify -0xabcd? If so, please add a test case for that.

Please also add these test-cases:

  1. --adjust-start causes an underflow. I think this should be an error.
  2. The above switches with numbers that are in the 64-bit range (i.e. too big to fit in a 32-bit number).
24 ↗(On Diff #186981)

Nit: Put a space between # and the check-prefix, for readability.

tools/llvm-objcopy/CopyConfig.cpp
558

This isn't actually quite true, since it's the standard library which defines the underlying type of uint64_t (i.e. it could be unsigned long, unsigned long long, or some internal compiler type). See my comments in D58234.

559

You aren't checking the return value for success here.

565

Unchecked return here.

  1. What's the use case for this?
  2. What's the interaction for program headers?

For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.

  1. What's the use case for this?
  2. What's the interaction for program headers?

For relocatable files I'd expect addresses to be ignored and for program headers I'd like to put more thought into this. In general, I'd like to take a stance that we not implement flags that we don't have a use case for.

Re. 2) This will have no interaction with program headers (though we might want to check that the address is within a loadable program header). All it does is change the e_entry field of the ELF header, i.e. the start address of the program. Personally, I don't have an issue with it changing that value in an ET_REL file (normally it would be 0), but we should follow what GNU objcopy does in that case. It doesn't hurt to have the value set in such a file; it's just meaningless.

I don't personally have a use case for this, but I could imagine a program with multiple possible start locations, and these switches allow switching between them. I'd like to here the concrete use case though.

I don't personally have a use case for this, but I could imagine a program with multiple possible start locations, and these switches allow switching between them. I'd like to here the concrete use case though.

This is used together with --update-section, because the latter can modify section and segment addresses. However --update-section flag is not yet supported. I had plans to support it
as well but it involves both section and segment modification. If segment modification is not desired in llvm-objcopy then this patch probably doesn't make much sense.

--adjust-start causes an underflow. I think this should be an error.

Why? GNU objcopy allows both underflow and overflow of start address

jhenderson added inline comments.Feb 20 2019, 1:52 AM
test/tools/llvm-objcopy/ELF/change-entry-point.test
2

I'm having trouble reading the below set of tests. Could you split them up with blank lines between the test cases, and also group together the switches so that all --set-start tests are first, then --change-start etc.

18

I don't think you've responded to this comment?

Haven't checked GNU objcopy to see if this makes sense, but can you specify -0xabcd? If so, please add a test case for that.

tools/llvm-objcopy/CopyConfig.cpp
307

This should return an Expected<T> and not report the error in this function.

evgeny777 updated this revision to Diff 187535.Feb 20 2019, 2:43 AM
  • added test case for negative hexadecimal number
  • changed way error handling is done
jakehehrlich accepted this revision.Feb 20 2019, 1:49 PM

This change looks good to me and the semantics seem sound. I'd still appreciate a use case however. This is small and compact enough and I can imagine you might want to do this in cases where a process doesn't fix itself up at all and consequently there are multiple valid entry points but by and large this seems quite odd to me.

Please wait for for someone else to accept this as well.

This revision is now accepted and ready to land.Feb 20 2019, 1:49 PM
jhenderson added inline comments.Feb 21 2019, 1:58 AM
test/tools/llvm-objcopy/ELF/change-entry-point.test
25

When testing aliases, we've tended to run the same run twice, and then compared the output to show it is identical. I think that you should do the same here, i.e. run --change-start -128 then compare that to --adjust-start -128.

tools/llvm-objcopy/CopyConfig.h
106

Couple of nits here:

  1. "in input ELF file" -> "in the input ELF file"
  2. "Entry address in output file" -> "The entry address in the output file"
rupprecht added inline comments.Feb 21 2019, 11:15 AM
tools/llvm-objcopy/CopyConfig.cpp
566

Is --set-start X --change-start Y valid? Should set start to X+Y, or EAddr+Y, or be an error?
It looks like this sets it to EAddr+Y, but I think it should probably be an error or X+Y.

Isn't it an overkill to cover all possible misuses? This is a developer tool so far ...

Isn't it an overkill to cover all possible misuses? This is a developer tool so far ...

Isn't clang just a developer tool? Why bother going through the trouble of implementing compiler warnings -- shouldn't good C++ devs know the spec by heart?

IMO, we should be setting the bar high for the quality of these tools. If this were just a random script that a handful of developers will share, I'd agree, but this is a tool that we should be encouraging broad adoption for. Also, in this case, I think it's just a couple lines to check, right? No fancy algorithms needed to check if two flags are being used.

if (InputArgs.hasArg(OBJCOPY_set_start) && InputArgs.hasArg(OBJCOPY_change_start))
  error();

I agree with @rupprecht on this. It gets even more involved when multiple build scripts start interacting with each other, as it may not even be clear to the developer that they are using both switches. I would be happy either with --change-start changing the value set by --set-start, or a warning or error.

evgeny777 updated this revision to Diff 187941.Feb 22 2019, 8:18 AM

Didn't realize that D58316 has been pushed. Rebased.

jhenderson added inline comments.Feb 25 2019, 1:59 AM
test/tools/llvm-objcopy/ELF/change-entry-point.test
23

Nit: trailing full stop, here and on the other comments in this test.

tools/llvm-objcopy/CopyConfig.cpp
572

Could you just make this the default? I.e. set Config.EntryExpr to return the input unless something overrides it?

tools/llvm-objcopy/CopyConfig.h
106

You still are missing the first "the" in the third sentence, as requested. Also, it should be "The input parameter is..." not just "Input parameter is..."

evgeny777 marked an inline comment as done.Feb 25 2019, 2:16 AM
evgeny777 added inline comments.
tools/llvm-objcopy/CopyConfig.cpp
572

How to deal with COFF variant? Setting this option should be an error in such case.

jhenderson added inline comments.Feb 25 2019, 2:38 AM
tools/llvm-objcopy/CopyConfig.cpp
572

Ah, I didn't think about COFF here, fair enough.

evgeny777 updated this revision to Diff 188159.Feb 25 2019, 6:29 AM

Addressed and rebased

jhenderson accepted this revision.Feb 25 2019, 7:48 AM

LGTM, with one remaining nit.

test/tools/llvm-objcopy/ELF/change-entry-point.test
32

You don't need the "The" here.

It would probably be wise to wait for @rupprecht to confirm he's okay before committing.

rupprecht accepted this revision.Feb 25 2019, 2:51 PM

Two differences we're making from GNU objcopy:

  1. --set-start and --change-start are now order dependent, e.g.

--set-start 0x100 --change-start 0x4 => both GNU objcopy and llvm-objcopy agree the start is 0x104
--change-start 0x4 --set-start 0x100 => GNU objcopy starts at 0x104, llvm-objcopy starts at 0x100 (i.e. --set-start overrides previous values)

  1. --change-start is cumulative, e.g.

--set-start 0x100 --change-start 0x4 --change-start 0x2 => GNU objcopy starts at 0x102 (last value of --change-start wins), llvm-objcopy starts at 0x106 (both values are added together)

I'm ok with both of these changes (if anyone is relying on this... that's scary), but it'd be nice to call this out in the help string (or man pages if we had those).

tools/llvm-objcopy/ObjcopyOpts.td
240

Add to the help string: Overrides any previous --change-start or --adjust-start values

242

Add to the help string: Can be specified multiple times, all values will be applied cumulatively.

(I'm not sure if that's the best wording)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2019, 1:25 AM