Page MenuHomePhabricator

[llvm-readobj][xcoff] implement parsing overflow section header.
ClosedPublic

Authored by DiggerLin on Oct 7 2019, 6:58 AM.

Details

Summary

in the xcoff, if the number of relocation entries or line number entries is overflow(large than or equal 65535) , there will be overflow section for it. The interpret of overflow section is different with generic section header, the patch implement parsing the overflow section.

Diff Detail

Event Timeline

DiggerLin created this revision.Oct 7 2019, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:59 AM
DiggerLin retitled this revision from implement parsing overflow section to implement parsing overflow section header..Oct 7 2019, 6:59 AM
llvm/test/tools/llvm-readobj/xcoff-overflow-section.test
38

The same information, when conveyed not via an overflow header, is expressed in decimal format (and not hexadecimal).

39

Ditto.

42

According to the XCOFF documentation: The s_relptr and s_lnnoptr fields must have the same values as in the corresponding primary section header.

43

See above.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
446–447

Since we removed the context of the code line from here, the comment is ambiguous. This should say that, for now, we dump only the section type (low order 16 bits).

447

Can we encode this into a function (getSectionType(const T &Section)) or constant (SectionFlagsTypeMask) with the comment:
The low order 16 bits of s_flags denotes the section type.

Also, the variable should be named SectionType or similar.

451

I think we should not be printing a "warning" to the same stream as the output.

452

"64-bit XCOFF object file".

458

s/TODO : /TODO /;
s/interpret/interpretation/;
s/, type check section header/, and type check section headers/;
s/from/from that of/;
s/generic section header/generic section headers/;
s/generic seciton header now/generic section headers for now/;

lebedev.ri retitled this revision from implement parsing overflow section header. to [llvm-readobj][xcoff] implement parsing overflow section header..Oct 8 2019, 8:36 AM
DiggerLin marked 7 inline comments as done.Oct 8 2019, 1:01 PM
DiggerLin added inline comments.
llvm/test/tools/llvm-readobj/xcoff-overflow-section.test
38

changed as suggestion.

39

changed as suggestion.

42

changed raw data and test case

llvm/tools/llvm-readobj/XCOFFDumper.cpp
446–447

changed as suggestions.

451

changed

458

changed as suggestion.

DiggerLin updated this revision to Diff 223929.Oct 8 2019, 1:05 PM
DiggerLin marked an inline comment as done.

address some comments

llvm/tools/llvm-readobj/XCOFFDumper.cpp
59

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).

456

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.

464

Suggestion: "For now we just dump the section type portion of the flags."

DiggerLin marked 3 inline comments as done.Oct 9 2019, 7:22 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
59

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

456

changed as suggestion

464

changed as suggestion.

DiggerLin updated this revision to Diff 224055.Oct 9 2019, 7:37 AM
hubert.reinterpretcast marked 2 inline comments as done.Oct 9 2019, 11:51 AM

LGTM to land as-is. Not sure if other people have an opinion about the const.
@DiggerLin, I believe you have had a number of patches committed into the project. I think you can request commit access and land this yourself. Thanks.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
46

I am not sure that I see a meaningful difference between the functions here that are const and the ones that are not. Given that there are already precedent cases of printing methods of ObjDumper subclasses that are const, I am okay with adding new ones that are const if we have reason to believe they will remain const.

59

Okay, I agree. Would you mind posting such an NFC patch after this patch lands?

456

For future reference, I believe we have been using "Oxford commas". That is, a comma before the "and" before (in this case) the third list item, would be appropriate.

This revision is now accepted and ready to land.Oct 9 2019, 11:52 AM
DiggerLin marked 3 inline comments as done.Oct 10 2019, 11:00 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
46

when I create a new function , I make as many functions const as possible so that accidental changes to objects are avoided.

59

OK, I will do it.

456

OK, got it , thanks

This revision was automatically updated to reflect the committed changes.