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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Thanks for sending this! I assigned PR39841 <https://bugs.llvm.org/show_bug.cgi?id=39841> to you since this should fix that bug.

I ran this on some internal ihex files we have, and it seems to be dropping one of the sections, so I'll have to poke around to see what the bug is. But some ihex support is better than none, so it's not necessarily blocking :)



================
Comment at: test/tools/llvm-objcopy/ELF/ihex-reader.test:2-4
+# RUN: yaml2obj %p/Inputs/ihex-elf-sections.yaml -o %t
+# RUN: llvm-objcopy -O ihex %t %t.hex
+# RUN: llvm-objcopy -I ihex %t.hex %t2
----------------
The only ihex reading test (besides the error cases) is just consuming ihex that llvm-objcopy produces with -O ihex. Could you add a .hex test file for a more stable test? And then it would be easier to verify, e.g., llvm-objcopy -I ihex -O binary <test> is identical to objcopy -I ihex -O binary <test>


================
Comment at: tools/llvm-objcopy/ELF/ELFObjcopy.cpp:638
+  const ElfType OutputElfType = getOutputElfType(
+      Config.OutputArch ? Config.OutputArch.getValue() : Config.BinaryArch);
+  if (Error E = handleArgs(Config, *Obj, Reader, OutputElfType))
----------------
A more succinct way (and same in executeObjcopyOnRawBinary):
```
const ElfType OutputElfType = getOutputElfType(
    Config.OutputArch.getValueOr(Config.BinaryArch));
```


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:159-160
+  T Value;
+  bool Fail = S.getAsInteger(16, Value);
+  assert(!Fail);
+  return Value;
----------------
`Fail` is unused in release builds, so you need to add a `(void)Fail;` to silence the error/warning in release builds.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1050
+  OwnedDataSection *Section = nullptr;
+  uint64_t RecAddr, SegmentAddr = 0, BaseAddr = 0;
+  uint32_t SecNo = 1;
----------------
RecAddr should be defined in the loop, where it is used


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1450-1460
+Error IHexReader::checkChars(StringRef Line, size_t LineNo) const {
+  if (Line[0] != ':')
+    return parseError("line %zu: missing ':' in the beginning of line.",
+                      LineNo);
+
+  for (size_t Pos = 1; Pos < Line.size(); ++Pos)
+    if (hexDigitValue(Line[Pos]) == -1U)
----------------
WDYT about just using llvm::Regex here instead of this method? It may be easier to read code if it just attempts to match ":[0-9A-F]+".  It would produce less precise error messages, though.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1451
+Error IHexReader::checkChars(StringRef Line, size_t LineNo) const {
+  if (Line[0] != ':')
+    return parseError("line %zu: missing ':' in the beginning of line.",
----------------
I think this will crash (or be UB) on an empty line?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1469-1470
+    break;
+  case IHexRecord::EndOfFile:
+    break;
+  case IHexRecord::SegmentAddr:
----------------
I think there should be validation (somewhere) that there are no more records after this


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1510
+
+  MemBuf->getBuffer().split(Lines, '\n');
+  for (size_t LineNo = 1; LineNo <= Lines.size(); ++LineNo) {
----------------
as a tiny optimization, call Records.reserve(Lines.size()) once you know how many lines there are.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1525-1532
+    size_t DataLen = checkedGetHex<uint8_t>(Line.substr(1, 2));
+    if (Line.size() != IHexRecord::getLength(DataLen))
+      return parseError("line %zu: invalid line length %zu (should be %zu)",
+                        LineNo, Line.size(), IHexRecord::getLength(DataLen));
+
+    Rec.Addr = checkedGetHex<uint16_t>(Line.substr(3, 4));
+    Rec.Type = checkedGetHex<uint8_t>(Line.substr(7, 2));
----------------
Once we've validated it, can we convert the whole hex string to separate ArrayRef<uint8/16_t> fields for each record, so we don't have to worry about it being valid everywhere (i.e. using checkedGetHex)?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1534-1537
+    if (IHexRecord::getChecksum(Line.drop_front(1)) != 0)
+      return parseError("line %zu: incorrect checksum.", LineNo);
+    if (Error E = checkRecord(Rec, LineNo))
+      return std::move(E);
----------------
How about creating a static method to convert a line into an Expected<IHexRecord>, so we can return an error if it's invalid instead of making the user call getChecksum/checkRecord?


================
Comment at: tools/llvm-objcopy/ELF/Object.h:195-198
+  // Memory address of the record.
+  uint32_t Addr : 16;
+  // Record type (see below).
+  uint32_t Type : 16;
----------------
Why not just uint16_t fields?


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

https://reviews.llvm.org/D60270