[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:704
+                                                uint64_t Size,
+                                                std::string Name) const {
+  uintptr_t StartPtr = reinterpret_cast<uintptr_t>(Start);
----------------
`Name` should be a `StringRef`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:710
+                       " and size 0x" + Twine::utohexstr(Size) +
+                       " go/goes past the end of the file");
+  return StringRef(Start, Size);
----------------
Rather than this awkward "go/goes", could you use "goes" only, and use singular terms for the `Name` parameter, e.g. "relocation data" or "symbol data" rather than "relocations" or "symbols"?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:702
 
+Expected<StringRef> XCOFFObjectFile::getRawData(uintptr_t Start,
+                                                uint64_t Size) const {
----------------
Esme wrote:
> jhenderson wrote:
> > Why is `Start` a `uintptr_t` rather than either a `const char *` or `const uint8_t *` (or even a `StringRef` or `ArrayRef<uint8_t>`)?
> > 
> > I also don't see this test being tested.
> Hmm, the function is called by objcopy when reading aux symbols. It looks like we can test this by making the `StartOffset + SymbolEntrySize * NumberOfAuxEntries` exceed the end of the file, where the NumberOfAuxEntries is user definable in the yaml, but I still can't construct such a case because we use zeros padding when the defined number is greater than the actual number of aux symbols in yaml2obj.
Fair enough. I'd add a TODO comment saying this path is untested.

ELF yaml2obj has support for raw "Content" fields that allow overriding the contents of a section. It may be worth considering something similar for XCOFF to test this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656