Page MenuHomePhabricator

[llvm-readobj][XCOFF]implement parsing relocation information for 32-bit xcoff objectfile
ClosedPublic

Authored by DiggerLin on Aug 30 2019, 8:33 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin added inline comments.Sep 27 2019, 1:16 PM
llvm/lib/Object/XCOFFObjectFile.cpp
714

I have added some new code to deal with the relocation entries overflow.

llvm/test/tools/llvm-readobj/xcoff-basic.test
128

added, thanks

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

changed as suggestion

DiggerLin updated this revision to Diff 222237.Sep 27 2019, 1:44 PM

added a new function getLogicalNumberOfRelocationEntries() to deal with the relocation entries overflow. and change comments of the Relocation Type.

llvm/include/llvm/Object/XCOFFObjectFile.h
173

Do these need to be declared in the header? Are they called only in one .cpp file? If so, they can be made static in the .cpp file. Otherwise, it seems odd that these aren't const member functions of XCOFFRelocation32.

llvm/lib/Object/XCOFFObjectFile.cpp
554

Please add a blank line after the function body here.

555

We can reduce the amount of background for the comment to what is necessary to understand the code here:
In an XCOFF32 file, when the field value is 65535, then an STYP_OVRFLO section header contains the actual count of relocation entries in the s_paddr field. STYP_OVRFLO headers contain the section index of their corresponding sections as their raw "NumberOfRelocations" field value.

560

No need to pass in the index. The index can be retrieved from within this function.

561

RELOC_OVERFLOW . USHRT_MAX does not need to be 65535.

563

No need for else here. Just place the else logic at the same level as the if.

570

This is not an internal-error situation. A different error handling mechanism is needed.

576

Why a is64Bit check in a file that needs an XCOFFSectionHeader32 in order to call it?

583

Write the cast out as reinterpret_cast.

584

This also needs to use the logical number of relocation entries.

llvm/include/llvm/BinaryFormat/XCOFF.h
160

s/dispacement/displacement/;

173

s/it/It/;

185

Add "(such as code used for dynamic initialization of non-local statics)" before "for which".

188

s/non modifiable/non-modifiable/;

191

s/refrenced/referenced/g;

192

s/ / /;
s/non modifiable/non-modifiable/;

193

s/refrences/references/;

195

s/Simialr/Similar/;
s/R_BR/the R_BR/;

llvm/include/llvm/Object/XCOFFObjectFile.h
163

Insert

//

between the group description and the specific description.

166

Insert

//

before the next specific description to extend the group while maintaining spacing.

169

Same here.

272

Leave the blank line that was there before this patch.

327

Insert a blank line above the comment.

llvm/lib/Object/XCOFFObjectFile.cpp
559

@sfertile, suggested that this be a separate patch. Could we land that first (with an update to how STYP_OVRFLO section headers are printed)?

DiggerLin updated this revision to Diff 222860.Oct 2 2019, 10:14 AM
DiggerLin marked 10 inline comments as done.

added a new relocation overflow test case and changed code based on comment

DiggerLin added inline comments.Oct 2 2019, 2:04 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
169

changed .

173

the llvm-readobj is using those function and obj2yaml will use them too.

272

changed

327

added

llvm/lib/Object/XCOFFObjectFile.cpp
555

added.

560

good idea.

561

changed.

563

thanks

sfertile added inline comments.Oct 3 2019, 10:08 AM
llvm/lib/Object/XCOFFObjectFile.cpp
559

Yes Please.

llvm/test/tools/llvm-readobj/reloc_overflow.ll
1 ↗(On Diff #222860)

The .ll suffix implies the test is written in LLVM IR. Use .test instead.

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

Is this specified in the docs? I wasn't able to find it specified anywhere. What about the exception section? I don't know anything about the exception implementation on AIX so I could be wrong, but I suspect it might contain relocations.

I did find the specification of the special relocations in the loader table, and that they are a different format from the 'normal' relocations implemented in this patch. Does the loader section use the relocation pointer and relocation count in the section header table for these different relocations, or do we find them through fields defined in the loader section itself?

DiggerLin marked 5 inline comments as done.Oct 7 2019, 7:20 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
559

I have created a new patch for it. https://reviews.llvm.org/D68575 . implement parsing overflow section header.

llvm/test/tools/llvm-readobj/reloc_overflow.ll
1 ↗(On Diff #222860)

I will change the name

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

from the xcoff document.
s_relptr Recognized for the .text, .data, .tdata, and STYP_DWARF sections only.

llvm/include/llvm/BinaryFormat/XCOFF.h
192

The typo, "refrenced", is still here.

193

Still missing the hyphen for "non-modifiable".

194

Either remove the "the" for this line or add "relocation" after "R_BA".

llvm/include/llvm/Object/XCOFFObjectFile.h
173

It is still odd to me that these aren't const non-static member functions of XCOFFRelocation32.

llvm/lib/Object/XCOFFObjectFile.cpp
555

I am not seeing the change.

579

Suggestion: NumRelocEntriesOrErr

583

Suggestion: NumRelocEntries

sfertile marked an inline comment as done.Oct 8 2019, 8:09 AM
sfertile added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
173

I think were these originally templated to work with both 32-bit and 64-bit relocations, which explains why they aren't member functions.

llvm/test/tools/llvm-readobj/reloc_overflow.ll
1 ↗(On Diff #222860)

I still see the .ll suffix.

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

Thanks.

DiggerLin retitled this revision from implement parsing relocation information for 32-bit xcoff objectfile to [llvm-readobj][XCOFF]implement parsing relocation information for 32-bit xcoff objectfile.Oct 8 2019, 1:55 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
173

Would using CRTP with a base class template work for that case?

DiggerLin marked 3 inline comments as done.Oct 10 2019, 12:41 PM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
173

as Sean's comment, for we only implement 32 bits relocation, we do not use any template for the relocation implement this moment.

llvm/include/llvm/Object/XCOFFObjectFile.h
173

All the more reason why these should be non-static member functions in the context of this patch.

DiggerLin updated this revision to Diff 224623.Oct 11 2019, 9:57 AM
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
173

changed to member functions

I've marked comments (all minor) that have not yet been addressed.

llvm/include/llvm/BinaryFormat/XCOFF.h
192
193
194
llvm/include/llvm/Object/XCOFFObjectFile.h
184

Blank line between the class definitions please.

llvm/lib/Object/XCOFFObjectFile.cpp
555
579
583
DiggerLin marked 6 inline comments as done.Oct 11 2019, 12:17 PM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
193

changed

194

changed

llvm/include/llvm/Object/XCOFFObjectFile.h
184

added

llvm/lib/Object/XCOFFObjectFile.cpp
583

changed

DiggerLin marked 2 inline comments as done.Oct 11 2019, 12:21 PM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
555

changed the comment as suggestion.

579

changed

Thanks @DiggerLin. I think this is almost ready.

llvm/include/llvm/Object/XCOFFObjectFile.h
156

Move the masks to the start of this class. Separate the nested types/constants from the fields using an access specifier label (e.g., public) or a comment.

161

Separate the fields from the methods using an access specified label or a comment.

163

Remove the comment and the blank line before it once the mask constants are defined in the class.

331

I would prefer a blank line between multi-line function declarations.

DiggerLin marked 4 inline comments as done.Oct 11 2019, 1:56 PM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
156

changed a suggestion.

161

changed as suggestion

163

changed as suggestion

331

added

DiggerLin updated this revision to Diff 224675.Oct 11 2019, 1:59 PM
This revision is now accepted and ready to land.Oct 11 2019, 2:16 PM
This revision was automatically updated to reflect the committed changes.