Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[XCOFF][AIX] Enable tooling support for 64 bit symbol table parsing
ClosedPublic

Authored by jasonliu on Aug 11 2020, 1:00 PM.

Details

Summary

Add in the ability of parsing symbol table for 64 bit object.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonliu updated this revision to Diff 340857.Apr 27 2021, 8:33 AM

Address clang-tidy comment and added binary file.

jsji added a reviewer: Restricted Project.Apr 27 2021, 9:00 AM
jhenderson added inline comments.Apr 28 2021, 6:28 AM
llvm/lib/Object/XCOFFObjectFile.cpp
831

Is this error user-facing (I'm assuming so)? Assuming it is, you should record here which symbol is causing the problem. Otherwise the user will be faced with an error along the lines of this:

error: this csec symbol contains no auxiliary entry

which is not really actionable (imagine the input had 100000 symbols in - the user can't realistically go through each to find the offending one).

848

Similar to my above comment - which entry was not found? Give the user more context so that they can act on the problem.

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

This will write the error inline, rather than to stderr. Are you sure that's what you want? it isn't what most dumping tools do on failure.

jasonliu updated this revision to Diff 341230.Apr 28 2021, 9:07 AM

Address comments.

jasonliu marked 3 inline comments as done.Apr 28 2021, 9:07 AM
jhenderson added inline comments.Apr 29 2021, 12:22 AM
llvm/lib/Object/XCOFFObjectFile.cpp
831

I believe this requires std::move(Error);, as you're returning an Expected, not an Error.

835–836

I think it would make more sense to insert the name in the middle of the message to make it a bit more concise. Something like:
"csect symbol name contains no auxiliary entry"

863

I'd suggest quoting somehow the symbol name, so that any whitespace or similar that happens to end up in the name (rare, but possible to write using assembly, at least for other platforms) is easily understood to be part of the name.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
414–415

Use reportError or reportWarning so that the error is reported in a clean manner and consistent with other llvm-readobj varieties, and not report_fatal_error which looks like a crash. General rule of thumb: try to avoid using report_fatal_error, especially in tool code where it is easy to report errors properly.

In llvm-readobj for ELF, we try to avoid even using reportError where possible, as that stops the tool from continuing dumping, which can be problematic occasionally. We prefer reportWarning (or more specifically the local reportUniqueWarning which avoids reporting the same warning multiple times) and bailing out of the current routine. Take a look at ELFDumper.cpp for examples.

jasonliu updated this revision to Diff 341639.Apr 29 2021, 2:21 PM
jasonliu marked 3 inline comments as done.

Address comments.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
414–415

Thanks for the elaboration. That clears up things a lot for me. I will use reportUniqueWarning here.

jhenderson added inline comments.Apr 30 2021, 1:34 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
414–415

reportUniqueWarning can take an Error directly, so you can just do:

if (!ErrOrCsectAuxRef)
  reportUniqueWarning(ErrOrCsectAuxRef.takeError());

Also, be careful, as the program continues, so referencing ErrOrCsectAuxRef after this may result in things going wrong...

jasonliu updated this revision to Diff 341894.Apr 30 2021, 7:22 AM
jasonliu marked an inline comment as done.

Address comments.

@jhenderson @DiggerLin @Esme
Any more comments?

I'll try to take another look in the next day or two. The pre-merge bots are failing. Is that an issue with this patch?

@jhenderson @DiggerLin @Esme
Any more comments?

I'll try to take another look in the next day or two. The pre-merge bots are failing. Is that an issue with this patch?

Thanks a lot.
I think it's more of an issue that the binaries did not get into the phabricator probably. Not really an issue with the patch itself.

jhenderson accepted this revision.May 10 2021, 1:14 AM

Only some minor nits, otherwise LGTM. I haven't attempted to review the test coverage for all the new code, as I don't feel like I'm in a good position to do that, not being an XCOFF developer. I assume someone else has though.

llvm/include/llvm/Object/XCOFFObjectFile.h
483
485
llvm/lib/Object/XCOFFObjectFile.cpp
797
808

Basically any time you use consumeError, add a comment explaining why it's justified that we don't report the error to the user.

This revision is now accepted and ready to land.May 10 2021, 1:14 AM
DiggerLin added inline comments.May 10 2021, 12:37 PM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
390–391

this is only for the 32bits .
" By convention, the csect auxiliary entry in an XCOFF32 file must be the last auxiliary entry for any external symbol that has more than one auxiliary entry"

for 64bit, it maybe look for the x_auxtype ==AUX_CSECT

DiggerLin added inline comments.May 10 2021, 1:03 PM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
187

if (AuxEntPtr->AuxType != XCOFF::AUX_FILE ) , it should not be parsed as
XCOFF::AUX_FILE
it may better to print out raw data in the printSymbol()

236

if (AuxEntPtr->AuxType != XCOFF::AUX_CSECT) , it should not be parsed as
XCOFF::AUX_CSECT above
it may better to print out raw data in the printSymbol()

jasonliu updated this revision to Diff 345227.May 13 2021, 11:15 AM
jasonliu marked 4 inline comments as done.

Address comments.

jasonliu marked 2 inline comments as done.May 13 2021, 11:18 AM
jasonliu added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
187

I think it's the caller's responsibility to make sure they are passing in the right auxiliary type. So we should assume when we enter this function, we have the right auxiliary type here. So I modified it and made it an assert instead.

236

Same above. I modified it to be an assertion instead.

390–391

Good point. Updated the code.

DiggerLin added inline comments.May 13 2021, 12:07 PM
llvm/lib/Object/XCOFFObjectFile.cpp
94

SymbolAuxType is only for the 64 bits.
add assert(is64Bit() ) before return ?

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

as you mention "I think it's the caller's responsibility to make sure they are passing in the right auxiliary type. "

I think we need to check the AuxType == XCOFF::AUX_FILE for 64 bits. if not , print out the raw data as AUX_CSECT did ?

DiggerLin added inline comments.May 13 2021, 12:22 PM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
416

we have iterated auxiliary entries from line 382~396 and reiterated again in the printCsectAuxEnt(). I think we can improve on it.

And in 64bits, The auxiliary entries maybe be reordered in above implement.
it will print out all no AUX_CSECT auxiliary entries first and then AUX_CSECT entry, even if the AUX_CSECT is in the middle of the auxiliary entries
and if there are two AUX_CSECT on auxiliary entries, we only print out one.

jasonliu marked 2 inline comments as done.May 13 2021, 12:47 PM
jasonliu added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
416

I don't think we reiterated again in printCsectAuxEnt() for other auxiliary entries.
We did similar things for 382~396 in getXCOFFCsectAuxRef, but that function is specifically designed to only get XCOFFCsectAuxRef. So it's really not intended to pass information out there.
I don't really think the re-iteration should be a big concern because in theory 382~396 won't be executed all that much. If it does, I would rather have it implement properly (i.e. actually recognize the missing auxiliary entry) instead of just printing out raw datas.

jasonliu updated this revision to Diff 345261.May 13 2021, 1:12 PM
jasonliu marked an inline comment as done.
jasonliu marked an inline comment as done.
DiggerLin accepted this revision.May 13 2021, 1:59 PM

LGTM with address comment.

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

it need "continue;"
here

and please run clang-format

jasonliu updated this revision to Diff 345283.May 13 2021, 2:12 PM

Address comments.

I took a quick look at the pre-merge output. The message is that the object isn't recognised as a valid object file, not that it wasn't found, which suggests either the object or code is broken in some manner, or you're missing a dependent patch. Does this patch depend on another patch that isn't in main yet?

llvm/tools/llvm-readobj/XCOFFDumper.cpp
357–372

i -> I

(you're changing most of the loop body - you might as well fix this whilst you're here)

363

Test case?

392–394

My English language ping went off at the word "till" in these two sentences. I'd probably change it to "to". Also, use "first" rather than "1st", I suggest in both places.

Also "skips" -> "skip" for grammatical consistency.

396

i -> I

jasonliu updated this revision to Diff 345492.May 14 2021, 10:46 AM

Address comments.

I took a quick look at the pre-merge output. The message is that the object isn't recognised as a valid object file, not that it wasn't found, which suggests either the object or code is broken in some manner, or you're missing a dependent patch. Does this patch depend on another patch that isn't in main yet?

No this patch does not depend on other patches. I think this is caused by git generated binary for the patch doesn't really assemble to the same one?

I took a quick look at the pre-merge output. The message is that the object isn't recognised as a valid object file, not that it wasn't found, which suggests either the object or code is broken in some manner, or you're missing a dependent patch. Does this patch depend on another patch that isn't in main yet?

No this patch does not depend on other patches. I think this is caused by git generated binary for the patch doesn't really assemble to the same one?

Sounds vaguely plausible, I guess.

llvm/test/tools/llvm-readobj/XCOFF/file-aux-wrong64.test
2–3

Fix a couple of grammar issues and a premature line-wrap.

5–6
20

The output on this line looks incorrect to me. Possibly a bug in the code resulting from a missing newe line?

jasonliu updated this revision to Diff 345903.May 17 2021, 8:50 AM

Address comments.

jasonliu marked 2 inline comments as done.May 17 2021, 8:51 AM
jasonliu added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/file-aux-wrong64.test
20

I added an newline after all the raw bytes. Other than that, I think the output is good. We have 18 bytes per symbol table entry, and we are printing 18 raw bytes here.

jhenderson added inline comments.May 18 2021, 12:17 AM
llvm/test/tools/llvm-readobj/XCOFF/file-aux-wrong64.test
20

I don't know if it is, but you probably want 00fb indented to line up nicely with the previous line. You can then confirm that this indentation is maintained by enabling --match-full-lines and --strict-whitespace in FileCheck. (If you do that, you'll need to remove the space after CHECK-NEXT:)

jasonliu updated this revision to Diff 348533.May 28 2021, 8:20 AM

Adjustment the print out.

jasonliu added inline comments.May 28 2021, 8:21 AM
llvm/test/tools/llvm-readobj/XCOFF/file-aux-wrong64.test
20

Hi James, I adjusted the code to print 00fb in the same line, I think that actually works better because we could have multiply auxiliary entry data to print out. Putting each in the same line is easier to parse.

jhenderson accepted this revision.Jun 7 2021, 1:22 AM

Looks good again. Sorry for the delay - I was off work last week.

This revision was landed with ongoing or failed builds.Jun 7 2021, 10:25 AM
This revision was automatically updated to reflect the committed changes.

Looks good again. Sorry for the delay - I was off work last week.

No worries. Thanks for all the reviews.