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

[Esme Yi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:702
 
+Expected<StringRef> XCOFFObjectFile::getRawData(uintptr_t Start,
+                                                uint64_t Size) const {
----------------
jhenderson wrote:
> 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.
Nice suggestion, thx!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656