[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm sorry I didn't come back to this - I've been struggling under an excessive review burden, and then had last week as PTO.

However, given my comments, I'd have preferred you had held off committing this a little longer. I don't believe this code is within acceptable LLVM quality at this time, and given the problem mentioned by @thakis, I think this should be reverted until the comments are addressed and the test fixed properly. (As far as I can tell, there is no reason why the test should not work on Windows, apart from some bug in this code - rGc2d2fb509306618b982bff94e1ad9acff6a41bcf <https://reviews.llvm.org/rGc2d2fb509306618b982bff94e1ad9acff6a41bcf> appears to only be a temporary fix to get the build bot green again).



================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test:125
+# XLC32OBJ-PART-NEXT: }
+
----------------
Delete trailing blank line.

The final character at EOF should be `\n`, but not a character sequence of more than one `\n`.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test:3
+RUN: llvm-readobj --auxiliary-headers %p/Inputs/xcoff-64-xlc-exec | \
+RUN:   FileCheck --check-prefix=XLC64EXEC %s
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jasonliu wrote:
> > > > Should llvm-readobj --all print out the auxiliary headers?
> > > there are several other options are not implemented, -all will be crashed.
> > I don't think that should prevent it being added at this point (although you wouldn't be able to test it). That being said, do a check of other format-specific options to see if --all dumps them.
> if add --all option in the test, it will invoke the function 
> 
> ```
>  if (opts::UnwindInfo)
>     Dumper->printUnwindInfo();
> ```
> 
> and for XCOFF. XCOFFDumper::printUnwindInfo() not implement
> 
> 
> ```
> void XCOFFDumper::printUnwindInfo() {
>   llvm_unreachable("Unimplemented functionality for XCOFFDumper");
> }
> ```
> 
> 
> It will be crash.
`llvm_unreachable` shouldn't be used where code is actually reachable. In such a case, it should just be a standard error message or similar (there are examples in LLVM/GNU style ELF output where one is implemented but not the other - look at those for the precedent). Not having a hard-error/crash would allow --all to be used.


================
Comment at: llvm/tools/llvm-readobj/Opts.td:88
+def grp_xcoff : OptionGroup<"kind">, HelpText<"OPTIONS (XCOFF specific)">;
+def auxiliary_header : FF<"auxiliary-header" , "display the auxiliary header">, Group<grp_xcoff>;
+
----------------
Existing help text style in llvm-readobj appears to be to have a leading capital (unlike error messages), so this should be "Display the auxiliary header".


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:654-655
+    llvm::raw_string_ostream StringOS(ErrInfo);
+    StringOS << "Only partial field for " << PartialFieldName << " at offset ("
+             << PartialFieldOffset << ").";
+    StringOS.flush();
----------------
Please refer to the LLVM coding standards for proper formatting of warning and error messages: such messages should start with a lower-case letter and not end with a full stop.

You don't need a string stream here, either. Just do:
```
std::string ErrInfo = (Twine("only partial field for") + PartialFieldName + " at offset (" + PartialFieldOffset)").str();
``` 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:657-659
+    reportWarning(
+        make_error<GenericBinaryError>(ErrInfo, object_error::parse_failed),
+        "-");
----------------
1) Use `createError`, not `make_error`. See existing usage throughout llvm-readobj. Same applies in all other usages of `make_error`.
2) Why are you using `reportWarning(..., "-");` That reports a warning as if the error was in a file read in from stdin, which in many cases will be incorrect. a) We encourage using `reportUniqueWarning` in new llvm-readobj code, so that the same warning isn't reported multiple times; b) use the file name for the last argument, like it should be. Same below.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:666
+    reportWarning(make_error<GenericBinaryError>(
+                      "There are extra data beyond auxiliary header",
+                      object_error::parse_failed),
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:735
+
+  if (PartialFieldOffset < AuxSize) {
+    std::string ErrInfo;
----------------
This block looks very similar to the equivalent 32-bit version. Could you move it into a function?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:605
+
+#define PrintAuxMember32(H, S, T)                                              \
+  if (offsetof(XCOFFAuxiliaryHeader32, T) +                                    \
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Could this not just be a lambda? That would play friendlier with IDEs if nothing else.
> the T is a member of class XCOFFDumper.cpp , I do not think C++11 support a template lambda.
Not sure I quite follow, but note that LLVM supports C++14 and `auto` can be used to much the same effect, if I'm not mistaken. I believe something like the following might work (note: I haven't tested, and am not particularly familiar with auto lambdas).
```
enum PrintStyle { Hex, Number };
auto PrintAuxMember32 = [&](PrintStyle P, StringRef S, auto *T) {
  ptrdiff_t Offset = T - AuxHeader;
  if (Offset + sizeof(*T) <= AuxSize) {
    switch(P) {
    case Hex:
      W.printHex(S, *T);
      break;
    case Number:
      W.printNumber(S, *T);
      break;
    }
  } else if (Offset < AuxSize) {
    PartialFieldOffset = Offset;
    PartialFieldName = S;
  }
}; 

PrintAuxMember32(Hex, "Magic", &AuxHeader->AuxMagic);
PrintAuxMember32(Hex, "Version", &AuxHeader->Version);
// etc...
```
You might be able to avoid the enum using some sort of std::bind call or similar, which would be nicer, but not essential.

Even without that, I think you should factor out the repeated `offsetof` calls into a variable, and use more meaningful variable names rather than `H`, `S` and `T`, just like you would for any function.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:652
+
+  // Deal with error.
+
----------------
jhenderson wrote:
> Delete the blank line after this comment. Also, I'd suggest you change it to something like "Handle unknown/unsupported data." or something like that.
You ignored the second part of this comment...


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:686
+
+  DictScope DS(W, "AuxiliaryHeader");
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This is common between the two functions: consider moving it into a higher-level calling function, along with the check above.
> > I do not think I can not
> > 
> > ```
> > DictScope DS(W, "AuxiliaryHeader");
> > ```
> > in higher level, 
> > if I put in the higher level as 
> > 
> > ```
> > void XCOFFDumper::printAuxiliaryHeader() {
> >   DictScope DS(W, "AuxiliaryHeader");
> >   if (Obj.is64Bit())
> >     printAuxiliaryHeader(Obj.auxiliaryHeader64());
> >   else
> >     printAuxiliaryHeader(Obj.auxiliaryHeader32());
> > }
> > ```
> > it will always print out the "AuxiliaryHeader" even the (AuxHeader == nullptr)  
> sorry. it should be "I do not think I can" in above message
I think that's fine. We do similar things in other cases in ELF, see e.g. `ELFDumper<ELFT>::printHashTable`. It shows there is no content relevant to the AuxHeader printing, even though you asked for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549