[PATCH] D97656: [llvm-objcopy] Initial XCOFF32 support.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:811
+  assert(NumberOfLineNumbers &&
+         "Calling LineNumber interface with a non-LineNumber section.");
+
----------------
Be consistent in the style of your assert messages - the version above has no trailing full stop, but this one.

There isn't a single style specified for asserts in LLVM, but for error messages, the agreed-upon style, where surrounding code does not differ from it, is no leading capital letter, and no trailing full stop.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:915
+  // The auxiliary header immediately follows the file header.
+  auto OptinalFileHeaderOrErr =
+      getObject<void>(Data, Base + CurOffset, Obj->getOptionalHeaderSize());
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/XCOFF/binary-copy.test:1-2
+## Replace this test when line number and aux symbol entries
+## are supported in yaml2obj.
+
----------------
Is there a particular need for you to get the llvm-objcopy implementation landed, before you have added the yaml2obj functionality? By adding a binary to the git repository, it will always stay in the repo history, even if it is later deleted, using up disk space on everybody's machines, slowing down clone times etc.


================
Comment at: llvm/test/tools/llvm-objcopy/XCOFF/invalid-section.test:8
+
+# ERROR1: The end of the file was unexpectedly encountered
+
----------------
Here and below, these error messages need improving to include additional context about what has gone wrong. For example, which section couldn't be read? What was its expected size and offset? Potentially event what was the file size?

Take a look at some examples from other tests:
llvm-objcopy\ELF\invalid-p_filesz-p_offset.test
llvm-readobj\ELF\addrsig.test (this one uses warnings, but the same message context is still important)


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Object.h:32
+  XCOFFSymbolEntry32 Sym;
+  StringRef AuxSymbolEntries; // Not yet supported. Copy content directly.
+};
----------------
Add `TODO:` to these comments.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:70-73
+    ReadSym.AuxSymbolEntries = StringRef(
+        reinterpret_cast<const char *>(SymbolDRI.p +
+                                       XCOFF::SymbolTableEntrySize),
+        XCOFF::SymbolTableEntrySize * SymbolEntRef.getNumberOfAuxEntries());
----------------
Do we need something to protect us from accidentally running off the end of the file, if it's been truncated somehow?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:90-92
+    Obj->OptionalFileHeader = StringRef(
+        reinterpret_cast<const char *>(XCOFFObj.getOptionalHeaderStart()),
+        XCOFFObj.getOptionalHeaderSize());
----------------
As above, do we need to check to make sure we don't end up reading off the end of the file?

I think it might be better to emit an error and stop, saying "optional headers are not yet supported" or something to that effect, so that we don't end up producing a potentially invalid output. Same goes with the aux symbol entries.


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Reader.cpp:94
+
+  // Read each sectiondd.
+  Obj->Sections.reserve(XCOFFObj.getNumberOfSections());
----------------
Typo?


================
Comment at: llvm/tools/llvm-objcopy/XCOFF/Writer.cpp:65
+
+  // Write the optinal header.
+  memcpy(Ptr, Obj.OptionalFileHeader.data(), Obj.OptionalFileHeader.size());
----------------
(but see also my above comment)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656