This is an archive of the discontinued LLVM Phabricator instance.

[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

Repository
rL LLVM

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
37

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

38

Ditto.

41

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

42

See above.

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

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.

441

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

442

"64-bit XCOFF object file".

448

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/;

456

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

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
37

changed as suggestion.

38

changed as suggestion.

41

changed raw data and test case

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

changed

448

changed as suggestion.

456

changed as suggestions.

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
55

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

467

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.

475

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
55

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

467

changed as suggestion

475

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.

55

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

467

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.

55

OK, I will do it.

467

OK, got it , thanks

This revision was automatically updated to reflect the committed changes.