[PATCH] D58173: [llvm-objcopy] Add --set-start, --change-start, --adjust-start

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/set-start.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --set-start 0x1000 %t %t2
----------------
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.


================
Comment at: test/tools/llvm-objcopy/ELF/set-start.test:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --set-start 0x1000 %t %t2
+# RUN: llvm-readobj --file-headers %t2 | FileCheck %s --check-prefix=SET
----------------
Could you also add test cases that show --set-start takes decimal, please?

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


================
Comment at: test/tools/llvm-objcopy/ELF/set-start.test:6
+# RUN: llvm-readobj --file-headers %t3 | FileCheck %s --check-prefix=ADD
+# RUN: llvm-objcopy --adjust-start -128 %t %t4
+# RUN: llvm-readobj --file-headers %t4 | FileCheck %s --check-prefix=SUB
----------------
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).


================
Comment at: test/tools/llvm-objcopy/ELF/set-start.test:24
+
+#SET: Entry: 0x1000
+#ADD: Entry: 0x1150
----------------
Nit: Put a space between # and the check-prefix, for readability.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:486
+  if (auto *Arg = InputArgs.getLastArg(OBJCOPY_set_start)) {
+    unsigned long long EAddr;
+    getAsUnsignedInteger(Arg->getValue(), 0, EAddr);
----------------
evgeny777 wrote:
> rupprecht wrote:
> > uint64_t
> 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)
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.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:487
+    unsigned long long EAddr;
+    getAsUnsignedInteger(Arg->getValue(), 0, EAddr);
+    Config.EntryExpr = [EAddr](uint64_t) { return EAddr; };
----------------
You aren't checking the return value for success here.


================
Comment at: tools/llvm-objcopy/CopyConfig.cpp:493
+    long long EIncr;
+    getAsSignedInteger(Arg->getValue(), 0, EIncr);
+    Config.EntryExpr = [EIncr](uint64_t EAddr) { return EAddr + EIncr; };
----------------
Unchecked return here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58173/new/

https://reviews.llvm.org/D58173