[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:
> 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.


================
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());
----------------
jhenderson wrote:
> jhenderson wrote:
> > Do we need something to protect us from accidentally running off the end of the file, if it's been truncated somehow?
> I'm probably being dumb, but where is this checked?
> Do we need something to protect us from accidentally running off the end of the file, if it's been truncated somehow?

It was a comment in reading auxiliary symbols. Now I read the data by calling XCOFFObjectFile::getRawData, which has an offset check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97656