[PATCH] D62583: [llvm-objcopy] Implement IHEX reader

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:4
+# RUN: llvm-objcopy -O ihex %t %t.hex
+# RUN: llvm-objcopy -I ihex %t.hex %t2
+# RUN: llvm-readobj -section-headers %t2 | FileCheck %s
----------------
The output format here is unspecified, so it's not clear if we should convert this to elf32-i386, elf64-x86-64, any of the (unimplemented) macho/coff outputs, etc. We should either assume the output is also ihex (what GNU objcopy appears to do) or return some kind of warning/error (also seems reasonable if we don't need to match failure-ness for that situation -- I wonder if anyone relies on that working?)

I'm not sure if this problem is related to your patch though; running "llvm-objcopy -I binary -Barm foo.txt bar" also creates an object file when it shouldn't. For now, can you just put an explicit output format (probably you want -O elf64-x86-64)


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:22
+# Check loading from raw hex file
+# RUN: llvm-objcopy -I ihex %p/Inputs/sections.hex %t-raw
+# RUN: llvm-readobj -section-headers %t-raw | FileCheck %s --check-prefix=RAW
----------------
(same for all the other places, e.g. here too)


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:41-42
+# 1. String too short
+# RUN: echo "01000000FF" > %t-bad.hex
+# RUN: not llvm-objcopy -I ihex %t-bad.hex %t-none 2>&1 | FileCheck %s --check-prefix=BAD_LENGTH
+
----------------
You should be able to avoid intermediate files by piping directly into objcopy, e.g.
echo "01000000FF" | not llvm-objcopy -I ihex - %t-none 2>&1 | FileCheck %s --check-prefix=BAD_LENGTH


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:99
+# CHECK:         Index: 1
+# CHECK-NEXT:    Name: .sec1 (35)
+# CHECK-NEXT:    Type: SHT_PROGBITS (0x1)
----------------
nit: remove the indices (i.e. `(35)` here) since they're irrelevant to the test


================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:122
+# CHECK-NEXT:    Offset: 0x49
+# CHECK-NEXT:    Size: 11
+# CHECK-NEXT:    Link: 0
----------------
Running the same scenario through GNU objcopy, there are a couple significant differences in the generated object file starting here -- looks like GNU objcopy gets confused and splits the "0123456789@" into separate sections, so size here is 8, and the next section is size 3 to handle the spill over... it looks like llvm-objcopy is correct here, but mind reviewing that to make sure llvm-objcopy is making sense here?


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

https://reviews.llvm.org/D62583