[PATCH] D133030: [AIX] llvm-readobj support a new option --exception-section for xcoff object file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:231
+
+public:
+  uint32_t getSymbolIndex() const {
----------------
Why the `public` directive? It makes it look like the fields are `private`, except they aren't because this is a `struct`.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:238
+  uint64_t getTrapInstAddr() const {
+    assert(Reason && " Zero is not a valid trap exception reason code.");
+    return TrapInstAddr;
----------------
I'd also consider replacing `Reason` with `Reason != 0`. This makes it a mirror of the `getSymbolIndex` function.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:249-250
+// Explicit extern template declarations.
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+
----------------
Why the duplication? In fact, why the `extern template` at all?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:408
 
-Expected<uintptr_t> XCOFFObjectFile::getLoaderSectionAddress() const {
-  uint64_t OffsetToLoaderSection = 0;
-  uint64_t SizeOfLoaderSection = 0;
+uint64_t XCOFFObjectFile::getSectionFileOffsetToRawData(DataRefImpl Sec) const {
+  if (is64Bit())
----------------
I think it would help the understandability of these changes if the refactoring was split into a separate prerequisite patch. You'd thus have two patches (well three if you include the yaml2obj patch discussed out of line):

1) NFC refactoring to make the loader section code reusable.
2) Implementation of the exception section.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:419
+
+  if (DRI.p == 0) // No section is not error.
     return 0;
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:422-423
 
-  uintptr_t LoderSectionStart =
-      reinterpret_cast<uintptr_t>(base() + OffsetToLoaderSection);
-  if (Error E =
-          Binary::checkOffset(Data, LoderSectionStart, SizeOfLoaderSection))
-    return createError(toString(std::move(E)) +
-                       ": loader section with offset 0x" +
-                       Twine::utohexstr(OffsetToLoaderSection) +
-                       " and size 0x" + Twine::utohexstr(SizeOfLoaderSection) +
+  uint64_t OffsetToSection = getSectionFileOffsetToRawData(DRI);
+  uint64_t SizeOfSection = getSectionSize(DRI);
+
----------------
I'd use the slightly shorter names `SectionOffset`  and `SectionSize`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:428
+  if (Error E = Binary::checkOffset(Data, SectionStart, SizeOfSection)) {
+
+    const char *SectNameMap[] = {"pad",    "dwarf",  "text",  "data", "bss",
----------------
Don't start blocks with a blank line.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:432-437
+    auto getSectName = [&]() {
+      for (int I = 3; I < 16; I++)
+        if (SectType & (1 << I))
+          return SectNameMap[I - 3];
+      return "";
+    };
----------------
All the magic numbers make this code completely opaque. Why "3", why "16", why "1" etc. I think you'd be better off assigning them to const variables so you can name them.

Also, I think the consensus is that lambdas should have name style like variables, because they are function objects rather than pure functions. In other words, I'd name this `GetSectionName` (no need to abbreviate).


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:767-771
+    for (const auto &Sec32 : sections32())
+      if (Sec32.getSectionType() == SectType) {
+        DRI.p = reinterpret_cast<uintptr_t>(&Sec32);
+        break;
+      }
----------------
Could a template function (or `auto` lambda) allow you to avoid this duplication (passing in the `sections32/sections64` functions as parameters)?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1006
+  DataRefImpl DRI = getSectionByType(XCOFF::STYP_EXCEPT);
+
+  if (DRI.p == 0)
----------------
Probably delete this blank line.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1012-1013
+
+  assert(is64Bit() && sizeof(ExceptEnt) == sizeof(ExceptionSectionEntry64) ||
+         !is64Bit() && sizeof(ExceptEnt) == sizeof(ExceptionSectionEntry32));
+
----------------
This assertion is independent of finding the section, so should probably the first line of the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030