[PATCH] D68575: [llvm-readobj][xcoff] implement parsing overflow section header.
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 07:30:34 PDT 2019
DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:58
+ static constexpr unsigned SectionFlagsTypeMask = 0xffffu;
const XCOFFObjectFile &Obj;
};
----------------
hubert.reinterpretcast wrote:
> Add a blank line here. Also, I am wondering if this should be part of `llvm/BinaryFormat/XCOFF.h` (perhaps in `SectionHeader32`, or in a base class thereof when 64-bit support lands).
for consistent with SectionFlagsReservedMask, puting define SectionFlagsTypeMask here too, I think we maybe need to create a NFC patch to put SectionFlagsReservedMask and SectionFlagsTypeMask in the xcoff.h
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:455
+ case XCOFF::STYP_TYPCHK:
+ // TODO : The interpretation of loader, exception, type check section
+ // headers are different from that of generic section header. We will
----------------
hubert.reinterpretcast wrote:
> The "TODO" still has a colon surrounded by spaces on both sides after it. I do not think that we have been using colons after "TODO".
>
> Still missing "and" before "type check section headers".
>
> Still missing "s" after "generic section header".
>
> Typo "seciton" is still present.
changed as suggestion
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:463
+ }
+ // For now we just dump the section type flags.
+ if (SectionType & SectionFlagsReservedMask)
----------------
hubert.reinterpretcast wrote:
> Suggestion: "For now we just dump the section type portion of the flags."
changed as suggestion.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68575/new/
https://reviews.llvm.org/D68575
More information about the llvm-commits
mailing list