[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:249-250
+// Explicit extern template declarations.
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+
----------------
jhenderson wrote:
> 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.
in the patch, it only  llvm-readobj/XCOFFDumper.cpp used the member functions of ExceptionSectionEntry. but I instanced   the member functions of   ExceptionSectionEntry in the XCOFFObjectFile.cpp instead  other files. just because we still want to implement the decode the Exception Section in llvm-objdump later, I am not sure whether we use use the member functions of   ExceptionSectionEntry in XCOFFObjectFile.cpp later. I only instanced the  ExceptionSectionEntry in the XCOFFObjectFile.cpp explicitly and extern  instanced the  ExceptionSectionEntry explicitly, It will guarantee the members  functions  of ExceptionSectionEntry only be  instantiated once.


================
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:
> 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.
for the in the patch https://reviews.llvm.org/D110320 and https://reviews.llvm.org/D106643 , in the getLoaderSectionAddress().  there is no test case for  the code "return createError(toString(std::move(E)) +
                       ": loader section with offset 0x" +
                       Twine::utohexstr(OffsetToLoaderSection) +
                       " and size 0x" + Twine::utohexstr(SizeOfLoaderSection) +
                       " goes past the end of the file");
" 
I do not think we will used a canned invalid xcoff object file to test invalid loader Section.

I will not touch the code the getLoaderSectionAddress() in current patch.
and After the current patch landed, I will create a new NFC patch to refactor the getLoaderSectionAddress().(which will delete the function getLoaderSectionAddress())


================
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:
> 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).
yes, it is mapping the section type values to section names, I think your way is easy to understand but  much more code(at lease 30 lines codes, at least 20 lines more codes).  and if the enum of  SectType has 100 different value, that  means I have to write about 200 lines?


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


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/invalid-exception-section.test:35
+    Flags:           [ STYP_EXCEPT ]
+    SectionData:     "0000000400000000000000000000000000340003000000000000005c0002000000010000000000000000000000000114000200000000000001400002"
+Symbols:
----------------
jhenderson wrote:
> 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.
agree, thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030