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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 19 inline comments as done.
DiggerLin added a comment.

In D82549#3054179 <https://reviews.llvm.org/D82549#3054179>, @Esme wrote:

> Tests required for this patch can now be generated with D111487 <https://reviews.llvm.org/D111487>:
>
> 1. dumping full contents of the aux file header for XCOFF32 and XCOFF64.
> 2. reporting the warning message for `partial field` by specifying the value of AuxiliaryHeaderSize in the FileHeader, ex.
>
>   --- !XCOFF
>   FileHeader:
>     MagicNumber:         0x1DF
>     AuxiliaryHeaderSize: 71
>   AuxiliaryHeader:
>     Magic: 0x10B
>   ...
>
> 3. reporting the warning message for `extra data` by set the value of AuxiliaryHeaderSize bigger than AuxFileHeaderSize32/64, ex.
>
>   --- !XCOFF
>   FileHeader:
>     MagicNumber:         0x1DF
>     AuxiliaryHeaderSize: 75
>   AuxiliaryHeader:
>     Magic: 0x10B
>
> 4. dumping empty `AuxiliaryHeader {}` if AuxiliaryHeader is not set.
>
> However, tests of this patch and tests of D111487 <https://reviews.llvm.org/D111487> are interdependent. We just need to mark a TODO in this patch, and replace the tests after D111487 <https://reviews.llvm.org/D111487> has landed too.

thanks for your work.



================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:315
+XCOFF SPECIFIC OPTIONS
+---------------------------------
+
----------------
jhenderson wrote:
> The underline here needs to match the title length or I think you'll get an error when building the doc.
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:906
+    if (Error E = AuxiliaryHeaderOrErr.takeError())
+      return std::move(E);
+    Obj->AuxiliaryHeader = AuxiliaryHeaderOrErr.get();
----------------
Esme wrote:
> See D110320, it is better to report a detailed error message, i.g
> `auxiliary header with offset ** and size ** goes past the end of the file`.
when we parses the FileHeader (line 894~897) , Section header, symbol table etc in the function.  they use the same mechanism to deal with error as AuxiliaryHeader . If you strong suggestion that we need to report the detail error message , we can create a separate patch to deal with all the parse error at the same time after the patch landed.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test:1
+# This file tests the ability of llvm-readobj to display the auxiliary header for 64 bits XCOFF and 32 bits XCOFF object file.
+RUN: llvm-readobj --auxiliary-header %p/Inputs/xcoff-64-xlc-exec | \
----------------
jhenderson wrote:
> Even though it's not currently required, I'd suggest using `##` for comments and `#` for lit & FileCheck directives in this file, as likely at some point it'll contain YAML, and then you'd have to change every line in the test.
thanks


================
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
+
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:596
+  if (AuxHeader == nullptr) {
+    W.printString("The XCOFF object file do not have a auxiliary header.");
+    return;
----------------
jhenderson wrote:
> Are you sure you want to print something? I think for other formats, we often just don't do anything, or print an empty dictionary/set-like thing. E.g. what happens if you do --segments with an ELF object file?
I think print out some info will let user know what happen on the xcoff object file.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:599
+  }
+  uint16_t AuxiSize = Obj.getOptionalHeaderSize();
+  uint16_t PartialFieldOffset = AuxiSize;
----------------
jhenderson wrote:
> I think `AuxSize` would be a more normal abbreviation, and would therefore be less likely to trip people up when typing.
Ok, thanks


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:605
+
+#define PrintAuxMember32(H, S, T)                                              \
+  if (offsetof(XCOFFAuxiliaryHeader32, T) +                                    \
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:678-681
+  if (AuxHeader == nullptr) {
+    W.printString("The XCOFF object file do not have a auxiliary header.");
+    return;
+  }
----------------
jhenderson wrote:
> See my above comment for the 32-bit case.
> 
> If possible, it would be good though if this check (whether it prints or not) was in some higher-level function that then calls this and the 32-bit function, rather than duplicating it.
I think it is difficult . we call the function here 
 
```
void XCOFFDumper::printAuxiliaryHeader() {
  if (Obj.is64Bit())
    printAuxiliaryHeader(Obj.auxiliaryHeader64());
  else
    printAuxiliaryHeader(Obj.auxiliaryHeader32());
}

```


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:686
+
+  DictScope DS(W, "AuxiliaryHeader");
+
----------------
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)  


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:688
+
+#define PrintAuxMember64(H, S, T)                                              \
+  if (offsetof(XCOFFAuxiliaryHeader64, T) +                                    \
----------------
jhenderson wrote:
> Same as before: make this a lambda.
same comment as before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549