[PATCH] D60270: [llvm-objcopy] Add support for Intel HEX input/output format

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Looked mostly at the test for now, going to take a pass over the code today.



================
Comment at: test/tools/llvm-objcopy/ELF/Inputs/ihex-elf-segments.yaml:20
+    Content:         "000102030405060708090A0B0C0D0E0F1011121314"
+  - Name:            .data
+    Type:            SHT_PROGBITS
----------------
I think this should be .data1?

```
Command Output (stderr):
--
error: Unknown section referenced: '.data1' by program header.
```

(I think this is a recent validation added by rL359663)


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-writer.test:1
+# RUN: yaml2obj %p/Inputs/ihex-elf-sections.yaml -o %t
+# RUN: llvm-objcopy -O ihex %t %t2.hex
----------------
All these hex outputs have diffs when compared to what GNU objcopy produces... is that expected? I haven't yet debugged exactly why.


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-writer.test:3
+# RUN: llvm-objcopy -O ihex %t %t2.hex
+# RUN: cat %t2.hex | FileCheck %s
+
----------------
`cat X | FileCheck` should be replaced with `FileCheck --input-file=X` everywhere


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-writer.test:20
+# RUN: yaml2obj %p/Inputs/ihex-elf-sections2.yaml -o %t-sec2
+# RUN: llvm-objcopy -O ihex --only-section=.text1 %t-sec2 %t-sec2.hex
+# RUN: cat %t-sec2.hex | FileCheck %s --check-prefix=SIGN_EXTENDED
----------------
When I run GNU objcopy on this test case, I get an error: `address 0xffffffff80001000 out of range for Intel Hex file`. Maybe we shouldn't be supporting it? Are we able to handle it correctly somehow even though GNU objcopy can't?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:155
+  // Sign extended 32 bit addresses (e.g 0xFFFFFFFF80000000) are ok
+  return Addr > UINT32_MAX && Addr + 0x80000000 > UINT32_MAX;
+}
----------------
Isn't relying on `Addr + 0x80000000` to loop around UB? Could this just directly check `Addr & 0xffffffff80000000 == 0xffffffff80000000` instead?


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

https://reviews.llvm.org/D60270