This is an archive of the discontinued LLVM Phabricator instance.

[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

DiggerLin created this revision.Aug 30 2019, 8:33 AM
sfertile added inline comments.Sep 9 2019, 9:54 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
158

This is 0x4 in the XCOFF Object Format Docs, do they need to be updated or is the patch wrong?

165

Minor nit: IIUC R_RL and 'R_RLA` are handled the same as R_POS, maybe we should keep them together.

170

Missing R_BR here.

176

Missing R_TLSML.

llvm/include/llvm/Object/XCOFFObjectFile.h
178

I imagine this (and getRelocatedLength) was templated to handle both XCOFFRelocation32 and XCOFFRelocation64 eventually? Lets just take an argument of XCOFFRelocation32 for now, and template them once we land 64-bit object file relocation support.

llvm/lib/Object/XCOFFObjectFile.cpp
513

>=

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

Minor nit: Take the concrete type, and we can change to a templated function when we land 64-bit relocation support.

DiggerLin marked 7 inline comments as done.Sep 11 2019, 8:40 AM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
158

I think it is Doc wrong, The value in the patch come from aix OS header file . I opened ticket to confirm it.

165

the enum is keep in value order, I am prefer to keep them as now.

170

good catch, added it , thanks

176

good catch, thanks

llvm/include/llvm/Object/XCOFFObjectFile.h
178

yes, all template here will be used to support XCOFFRelocation32 and XCOFFRelocation64. we support 32bits now, we will support 64bits later.

llvm/lib/Object/XCOFFObjectFile.cpp
513

yes, you are correct, symbol index are from zero.

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

we will have a 64bits support later, I am prefer to keep the templated function here. if you strong need to use a concrete type now, I will do it.

sfertile marked an inline comment as done.Sep 11 2019, 8:15 PM
sfertile added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
158

Thanks.

165

There are already relocations grouped together by functionality as opposed to value order: eg R_TOC', 'R_TRL','R_TRLA or 'R_BA`,'R_RBA','R_RBR' . Why is important to keep these grouped in value order as opposed these others that are grouped by functionality?

DiggerLin updated this revision to Diff 219907.Sep 12 2019, 7:02 AM
DiggerLin updated this revision to Diff 219914.Sep 12 2019, 7:23 AM
DiggerLin marked an inline comment as done.
DiggerLin updated this revision to Diff 219928.Sep 12 2019, 8:45 AM
hubert.reinterpretcast requested changes to this revision.Sep 12 2019, 8:51 AM
hubert.reinterpretcast added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
170

Please be consistent on whether to use lowercase hex digits or uppercase ones.

llvm/include/llvm/Object/XCOFFObjectFile.h
167

s/instuction/instruction/;

176

Add a blank line before the comment.

327

Add a hyphen: "Relocation-related".

llvm/lib/Object/XCOFFObjectFile.cpp
513

Is this value verified elsewhere against the offset of the symbol table and the buffer size? If not, then this check is not a substitute for bounds checking on the pointer arithmetic below.

709

getWithOffset does not do any bounds checking. Please ensure bounds checking.

714

This should use the logical number of relocations, not the raw field value.

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

I would prefer:

Section (index: 1) .text {
llvm/tools/llvm-readobj/XCOFFDumper.cpp
125

s/Reloation/Relocation/;

128

The number of ECases does not match the number of enumerators in the enumeration.

This revision now requires changes to proceed.Sep 12 2019, 8:51 AM
sfertile added inline comments.Sep 12 2019, 9:01 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
152

We should be documenting what each of these relocation types represents.

llvm/include/llvm/Object/XCOFFObjectFile.h
178

The problem is that we are now reviewing these functions in a context that doesn't exist yet: ie for both 32-bit relocation representation (in this patch) and 64-bit relocation representation (which is yet to be added). I think we can make an educated guess what the 64-bit relocation struct will look like, and review in that context but it wont be so obvious to other readers of the file in the meantime (unitl 64-bit support lands). To be pedantic we should either add 64-bit support along side the 32-bit support (which would mean landing 64-bit symbol table support first), or stick to the existing concrete type. (Ditto on XCOFFDumper::printRelocations).

llvm/lib/Object/XCOFFObjectFile.cpp
709

report_fatal_error() for 64-bit object files.

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

Your section index will get out of sync, since we continue (to the next section) but don't increment the index.

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

This reduction is odd. Two kinds of TLS-related relocations are represented by the .o file, but we don't check for them. Please also add in the relocation referencing the unnamed [RO] csect.

DiggerLin marked 11 inline comments as done.Sep 18 2019, 1:49 PM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
152

added

165

changed as suggestion

170

fixed

llvm/include/llvm/Object/XCOFFObjectFile.h
167

changed , thanks

176

added

327

added

llvm/lib/Object/XCOFFObjectFile.cpp
513

yes, We already check in XCOFFObjectFile::create as

// Parse symbol table.
CurOffset = Obj->fileHeader32()->SymbolTableOffset;
uint64_t SymbolTableSize = (uint64_t)(sizeof(XCOFFSymbolEntry)) *
                           Obj->getLogicalNumberOfSymbolTableEntries32();
auto SymTableOrErr =
    getObject<XCOFFSymbolEntry>(Data, Base + CurOffset, SymbolTableSize);
if (Error E = SymTableOrErr.takeError())
  return std::move(E);
Obj->SymbolTblPtr = SymTableOrErr.get();
CurOffset += SymbolTableSize;
709

I will do as suggestion

714

there is no logical number of relocation , it defined as
support::ubig16_t NumberOfRelocations; in the struct XCOFFSectionHeader32 {

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

added

128

changed as suggestion.

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

index has been increased before W.unindent();

DiggerLin updated this revision to Diff 220744.Sep 18 2019, 1:53 PM

changed code based on comment

llvm/lib/Object/XCOFFObjectFile.cpp
714

"In an XCOFF32 file, if more than 65,534 relocation entries are required, the field value will be 65535, and an STYP_OVRFLO section header will contain the actual count of relocation entries in the s_paddr field."

DiggerLin marked an inline comment as done.Sep 19 2019, 11:09 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
714

for the relocation entry or line number entry overflow. I think we need to add a new patch for it. for the interpret of fields of Overflow section also be changed. We need to display Overflow section as separation(other than use current display source code).
s_nreloc (Overflow section)
Specifies the file section number of the section header that overflowed; that is, the section header containing a value of 65535 in its s_nreloc and s_nlnno fields. This value provides a reference to the primary section header. This field must have the same value as the s_nlnno field.

s_paddr (Overflow section)
Specifies the number of relocation entries actually required. This field is used instead of the s_nreloc field of the section header that overflowed.
s_vaddr (Overflow section)
Specifies the number of line-number entries actually required. This field is used instead of the s_nlnno field of the section header that overflowed.

I added a TODO: in the code.

address comment of relocation entries overflow.

DiggerLin updated this revision to Diff 220924.Sep 19 2019, 5:14 PM
DiggerLin marked an inline comment as done.

delete the template code.

llvm/include/llvm/Object/XCOFFObjectFile.h
178

changed without template

sfertile added inline comments.Sep 20 2019, 8:16 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
146

index has been increased before W.unindent();

Yes, and that line of code will be skipped by the continue for any section that doesn't contain relocations. Any sections following a section with no relocations will be printed with the wrong section index.

DiggerLin marked an inline comment as done.Sep 20 2019, 11:33 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
146

thanks for pointout

address comment

sfertile added inline comments.Sep 23 2019, 12:34 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
152

Thanks for commenting these Digger. I see you used the descriptions from reloc.h. Those work. In some cases I think we can do a bit better on the descriptions if we crib from here. I can write up my suggestions and then run them by you before we attempt to update any though.

llvm/lib/Object/XCOFFObjectFile.cpp
714

Would it be worth while to loop over the section header table after parsing it and emit a fatal error if any sections have overflowed (either their relocation or their line number infos). I agree any handling of overflow sections must be added in a separate patch. The error I am suggesting would also need to be landed in a separate patch from this, but it should be a very quick patch to review and commit.

DiggerLin marked 2 inline comments as done.Sep 23 2019, 1:12 PM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
152

OK, thanks please share with suggestions.

llvm/lib/Object/XCOFFObjectFile.cpp
714

if a relocation is overflow , I think we still can print out part of the relocation information which is under 65535 (even if we do not know exactly number of the relocation entry at this moment)

llvm/lib/Object/XCOFFObjectFile.cpp
714

We don't know the number, and it is not immediately obvious (because the relocation might not be the field whose logical value didn't fit) that the value would be at least 65535, so the patch Sean suggested makes sense.

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

To be more clear: There are two TLS-related relocation kinds represented in the object file, and neither are checked in this test. Please check for them.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
146
DiggerLin marked 4 inline comments as done.Sep 27 2019, 1:16 PM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
158

I have confirmed from the aix OS developer, the value in the doc is wrong , they will fix it.

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.