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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:231
+
+public:
+  uint32_t getSymbolIndex() const {
----------------
jhenderson wrote:
> Why the `public` directive? It makes it look like the fields are `private`, except they aren't because this is a `struct`.
just for readable to separate the data member and 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>;
+
----------------
jhenderson wrote:
> Why the duplication? In fact, why the `extern template` at all?
"extern template" means the template is initiated somewhere. 
for the template has member function. without the  "extern template". there maybe member function maybe be initiate in different compile unit. it waste the the code. 


================
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())
----------------
jhenderson wrote:
> 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.
the function getLoaderSectionAddress is mapped into getSectionFileOffsetToRawData(XCOFF::STYP_LOADER).

and  the function getSectionFileOffsetToRawData has more functionality than only get getLoaderSectionAddress. if split into two patch. I do not think it is NFC patch.


================
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 "";
+    };
----------------
jhenderson wrote:
> 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).
the functionality of the mapping the value SectionTypeFlags into string.

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/BinaryFormat/XCOFF.h


================
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;
+      }
----------------
jhenderson wrote:
> Could a template function (or `auto` lambda) allow you to avoid this duplication (passing in the `sections32/sections64` functions as parameters)?
good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030