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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:142
+  support::ubig16_t SecNumOfTBSS;
+  support::ubig16_t X64bitsFlag;
+};
----------------
jasonliu wrote:
> X64bitsFlag -> XCOFF64Flags
I believe that the name @jasonliu suggested is better:
s/XCOFF64bitsFlag/XCOFF64Flags/;


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:504
+  PrintAuxMember32(Hex, "Version", Version);
+  PrintAuxMember32(Hex, "Size Of .text Section", TextSize);
+  PrintAuxMember32(Hex, "Size Of .data Section", InitDataSize);
----------------
The use of capitalization is odd. Even for title case, "of" is not capitalized in such a position.
To avoid this issue, please use lowercase following the first word (except for abbreviations or proper nouns). Please apply for all of the strings.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:528
+  PrintAuxMember32(Hex, "Stack Page Size", StackPageSize);
+  PrintAuxMember32(Hex, "FlagAndTDataAlignment", FlagAndTDataAlignment);
+  PrintAuxMember32(Number, "Section number for .tdata", SecNumOfTData);
----------------
We interpreted the packed field for the symbol alignment and type in the similar case of csect auxiliary symbol table entries. For consistency within the tool, I believe we should do it here as well.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:530
+  PrintAuxMember32(Number, "Section number for .tdata", SecNumOfTData);
+  PrintAuxMember32(Number, "Section number for .tbss", SecNumOfTBSS);
+
----------------
We should produce some warning if the auxiliary table size indicates additional content past the last field that the code understands. This is another case where I think we should also output the raw data.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:548
+      AuxiSize)                                                                \
+  W.print##H(S, AuxHeader->T)
+
----------------
Please indent the body of the `if`. Also, if `AuxiSize` is greater than the offset of the field but less than the offset past-the-end of the field, then we have a partial field and should probably produce some warning about it (in case the producer of the input file has behaviour we did not expect). I would also suggest to output the raw data in that case.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:579
+  PrintAuxMember64(Number, "Section number for .tbss", SecNumOfTBSS);
+  PrintAuxMember64(Hex, "64 bits XCOFF Flag", XCOFF64bitsFlag);
+
----------------
The adjective should be "64-bit". Also, these are only the additional XCOFF64 flags (and not all of the flags for XCOFF64).

Suggestion:
Additional flags for 64-bit XCOFF



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82549