[PATCH] D82549: [AIX][XCOFF] parsing xcoff object file auxiliary header
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 4 02:19:01 PDT 2021
jhenderson added inline comments.
================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:315
+XCOFF SPECIFIC OPTIONS
+---------------------------------
+
----------------
The underline here needs to match the title length or I think you'll get an error when building the doc.
================
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 | \
----------------
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.
================
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:
> 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.
================
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;
----------------
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?
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:599
+ }
+ uint16_t AuxiSize = Obj.getOptionalHeaderSize();
+ uint16_t PartialFieldOffset = AuxiSize;
----------------
I think `AuxSize` would be a more normal abbreviation, and would therefore be less likely to trip people up when typing.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:601
+ uint16_t PartialFieldOffset = AuxiSize;
+ const char *PartialFieldName;
+
----------------
I recommend explicitly initialising this to `nullptr` to avoid any risk of using an uninitialized variable.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:605
+
+#define PrintAuxMember32(H, S, T) \
+ if (offsetof(XCOFFAuxiliaryHeader32, T) + \
----------------
Could this not just be a lambda? That would play friendlier with IDEs if nothing else.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:633
+ PrintAuxMember32(Hex, "Module type", ModuleType);
+ PrintAuxMember32(Hex, "Cpu type of objects", CpuFlag);
+ PrintAuxMember32(Hex, "(Reserved)", CpuType);
----------------
CPU is an acronym, so should be all caps.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:652
+
+ // Deal with error.
+
----------------
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.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:655-670
+ std::string ErrInfo;
+ llvm::raw_string_ostream StringOS(ErrInfo);
+ StringOS << "Only partial field for " << PartialFieldName << " at offset ("
+ << PartialFieldOffset << ") Raw data:";
+ StringOS.flush();
+ W.printBinary(
+ "!!!Warning", ErrInfo,
----------------
This is a mess, in my opinion. Please use reportWarning/reportUniqueWarning to report the warning as per other warnings in llvm-readobj. Printing the other raw data is fine, but should be prefixed with simply something like "Raw data from offset 0x1234:" rather than the ugly "!!!Warning".
================
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;
+ }
----------------
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.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:682
+ }
+ uint16_t AuxiSize = Obj.getOptionalHeaderSize();
+ uint16_t PartialFieldOffset = AuxiSize;
----------------
As above.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:684
+ uint16_t PartialFieldOffset = AuxiSize;
+ const char *PartialFieldName;
+
----------------
`= nullptr;` as suggested above.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:686
+
+ DictScope DS(W, "AuxiliaryHeader");
+
----------------
This is common between the two functions: consider moving it into a higher-level calling function, along with the check above.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:688
+
+#define PrintAuxMember64(H, S, T) \
+ if (offsetof(XCOFFAuxiliaryHeader64, T) + \
----------------
Same as before: make this a lambda.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:735
+
+ if (PartialFieldOffset < AuxiSize) {
+ std::string ErrInfo;
----------------
Same as before: use `reportWarning`/`reportUniqueWarning` to report warnings.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:548
+ AuxiSize) \
+ W.print##H(S, AuxHeader->T)
+
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > DiggerLin wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > 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.
> > > > I agree your suggestion that "we have a partial field and should probably produce some warning about it " . If I implement the suggestion in the the patch, I also need to add test case to test it. It maybe cause the patch too big, I think it maybe better to put the suggestion in other patch.
> > > I would suggest implementing it in this patch and to post another patch (while this review is still up) with the specific testing for the error cases of having partial fields or trailing unrecognized fields.
> > without the test case , I do not think we can 100% guarantee the code is correct.
> > if when we write a test case for it. we find an error on the code, we would put the fix and specific test case in the some patch?
> Well, if the original patch has not already landed, I do not see why we wouldn't apply the fix in the original patch. As for the testing, I think it should be fine to have it in a separate patch that is intended to land at around the same time.
Testing should be in the same patch as the code it is testing. Please don't split them into separate patches. If the functionality can be added standalone, and there's a sensible behaviour (even if that behaviour is "do nothing") without it, then that functionality and testing should be in a separate patch.
================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:100
static bool FileHeaders;
+static bool XCOFFAuxiliaryHeader;
static bool Headers;
----------------
File-format specific options are grouped separately to the generic options (see below for examples).
================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:273
+ // XCOFF specific options.
+ opts::XCOFFAuxiliaryHeader = Args.hasArg(OPT_auxiliary_header);
+
----------------
Don't forget to clang-format your modified code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82549/new/
https://reviews.llvm.org/D82549
More information about the llvm-commits
mailing list