[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:249-250
+// Explicit extern template declarations.
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+extern template struct ExceptionSectionEntry<support::ubig32_t>;
+
----------------
DiggerLin wrote:
> 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.
Right, I understand what the extern template achieves, but your answer doesn't explain why this struct is special compared to many other classes and structs in LLVM that don't use extern template.

Anyway, like I said, this is a discussion for another time.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:457
+                                 "debug",  "typchk", "ovrflo"};
+    SmallString<256> UnknowType;
+    auto GetSectionName = [&]() {
----------------



================
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:
> > 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())
Sounds reasonable to me.


================
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:
> > 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?
If the code is simpler to understand, you should always prefer that approach to the opaque option, even if the opaque option is many fewer lines of code. The switch/case approach may well be more efficient too, since compilers can easily optimize a switch case into a single jump.

Listing them out, the advantages of switch/case are:
  - Easy to follow.
  - Clear mapping of value to name (doesn't require maintaining two parallel but intrinsically linked arrays/enums).
  - Improved compiler diagnostics (e.g. compilers can warn if no default case and not all cases in an enum are covered).
  - Potentially improved performance.

The advantage of the existing approach:
  - Fewer lines of code.

SectType doesn't have 100 values, so your argument is a false comparison. Regardless, even if it did, you'd still have to have a 100 element array and make sure that the order in that array exactly matched the order of the enum values, so it's not exactly like it's any more maintainable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133030