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

Fri Sep 9 01:00:33 PDT 2022

jhenderson added inline comments.

================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:231
+
+public:
+  uint32_t getSymbolIndex() const {
----------------
DiggerLin wrote:
> 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.
> just for readable to separate the data member and function.

The addition of an explicit and unnecessary `public` makes things less readable for me, not more. If you wish to explicitly call out that the method is public, put the method before the members and label the whole struct with `public`.

Also nit: add a blank line between your methods, where your methods are multi line.

================
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>;
+
----------------
DiggerLin wrote:
> 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. 
We don't bother with extern templates in many other places, so I'm not sure why you felt that this particular case needed it?

Actually, after writing that, I see it's the same for other XCOFFObjectFile declarations, so it's probably not worth me worrying about in this patch, although it's still the case that there's limited use of this feature outside this file. I'd still like to know why you feel like these structs need it specifically, when in most cases within LLVM we don't bother.

================
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())
----------------
DiggerLin wrote:
> 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.
All the more reason to split this up then: if the first patch isn't a pure refactor, and adds new functionality, that functionality should be added and tested for the loader section. If on the other hand it is not used, the additional functionality can be added in the second 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 "";
+    };
----------------
DiggerLin wrote:
> 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
Correct me if I'm wrong, but it looks to me like this is just an over-complicated way of mapping the section type values to section names. Wouldn't a switch statement be significantly clearer? Something like:

```
StringRef getSectionName(int32_t SectType) {
  switch(SectType) {
  case STYP_PAD:
    return "pad";
  case STYPE_DWARF:
    return "dwarf";
  ...
  default:
    return "<unknown: " + SectType + ">";
  }
}
```
(The default case covers the situation where a user provides a type that isn't a known type, but could be replaced by something else appropriate).

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/exception-section.test:18
+    Flags:           [ STYP_EXCEPT ]
+    SectionData:     "0000000000000000003400030000005c0002000000010000000001140002000001400002"
+Symbols:
----------------
It would probably help readers/maintainers if this had some small comments in the lines immediately below with arrows pointing up at the start of each field in the data. There are several examples of this in the yaml2obj DWARF tests.

Something like:
```
SectionData: "0000000000000000003400030000005c0002000000010000000001140002000001400002"
              ^        ^-FieldName
              +-SymIndex

```
(add more labels with field names as appropriate).

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/exception-section.test:25
+
+
+--- !XCOFF
----------------
Nit: get rid of double blank line. Same in the other test.

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:20-24
+Symbols:
+  - Name:            .bar
+    Section:         .text
+  - Name:            .foo
+    Section:         .text
----------------
Do you need any symbols at all for this test case? If so, would 1 suffice?

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:35
+    Flags:           [ STYP_EXCEPT ]
+    SectionData:     "0000000400000000000000000000000000340003000000000000005c0002000000010000000000000000000000000114000200000000000001400002"
+Symbols:
----------------
Do you really need this much data for this test case? A single entry would surely be sufficient, and you wouldn't even need any symbols at all.

================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:7
+# RUN: dd bs=1 skip=190 seek=190 count=2000 if=%p/Inputs/exception-section.o of=%t_invalid.o
+# RUN: llvm-readobj --exception-section %t_invalid.o  2>&1 |\
+# RUN:   FileCheck -DFILE=%t_invalid.o %s 
----------------
jhenderson wrote:
> 
Not addressed, and now duplicated in the invalid_sym case too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030