[PATCH] D104646: [AIX][XCOFF] llvm-readobj 64-bit relocation reading support

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:353
+        relocations<XCOFFSectionHeader64, XCOFFRelocation64>(*SectionEntPtr);
+    if (Error E = RelocationsOrErr.takeError())
+      return relocation_iterator(RelocationRef());
----------------
I believe this will cause an assertion in some configurations because you don't do anything with a failing error. You should either report it, or more likely in this case, use `consumeError()` with a TODO saying the error should be propagated up the stack, to be reported elsewhere.

You also need to make sure you've got a test case that triggers this error. Whether it is a lit test or a gtest unit test, I don't mind, although I suspect the latter may be easier.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:716
 
-Expected<ArrayRef<XCOFFRelocation32>>
-XCOFFObjectFile::relocations(const XCOFFSectionHeader32 &Sec) const {
+template <typename T, typename V>
+Expected<ArrayRef<V>> XCOFFObjectFile::relocations(const T &Sec) const {
----------------
Perhaps name these types more descriptively, e.g. `Shdr` and `Reloc`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:727
+                 sizeof(V) == XCOFF::RelocationSerializationSize32),
+                "");
+  auto RelocationOrErr = getObject<V>(Data, reinterpret_cast<void *>(RelocAddr),
----------------
It would be nice if this static assert included a message that the compiler can properly report.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:124
 
-void XCOFFDumper::printRelocations(ArrayRef<XCOFFSectionHeader32> Sections) {
+template <typename T, typename V>
+void XCOFFDumper::printRelocations(ArrayRef<T> Sections) {
----------------
As elsewhere, consider renaming these types to be more descriptive.


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

https://reviews.llvm.org/D104646