Page MenuHomePhabricator

[llvm-objdump][XCOFF][AIX] Enable the -l (--line-numbers) option.
ClosedPublic

Authored by Esme on Apr 25 2021, 6:27 PM.

Details

Summary

Added support for dumping line number information in llvm-objdump.

Diff Detail

Event Timeline

Esme created this revision.Apr 25 2021, 6:27 PM
Esme requested review of this revision.Apr 25 2021, 6:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2021, 6:27 PM
Esme updated this revision to Diff 340438.Apr 25 2021, 11:19 PM

Updated test cases.

The llvm-objdump test looks good to me. Instead of # REQUIRES: powerpc-registered-target, you can add a lit.local.cfg with if not 'PowerPC' in config.root.targets:\n config.unsupported = True

Esme updated this revision to Diff 341074.Apr 27 2021, 10:26 PM

Addressed Mask's comment, i.e added the lit.local.cfg.

Higuoxing accepted this revision.Apr 29 2021, 8:00 PM

Only minor suggestions, otherwise LGTM. But please give somebody else a chance to review this change :)

llvm/lib/Object/XCOFFObjectFile.cpp
836–837
849–854

I would suggest putting the code block

if (!getNumberOfAuxEntries())
    return false;

in the front to avoid potential useless computation.

This revision is now accepted and ready to land.Apr 29 2021, 8:00 PM
Esme updated this revision to Diff 341771.Apr 29 2021, 10:03 PM

Thanks for your review @Higuoxing.

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
2

Add a simple comment to the start of this file describing what it is testing.

Esme added a comment.May 9 2021, 11:43 PM

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

For the parts that aren't required and can't be exercised at all, it seems like you should be able to just leave them out? For those parts that are required, you could add gtest unit testing to cover them. See llvm/unittest/Object for some examples.

Esme updated this revision to Diff 345338.May 13 2021, 8:00 PM

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

For the parts that aren't required and can't be exercised at all, it seems like you should be able to just leave them out? For those parts that are required, you could add gtest unit testing to cover them. See llvm/unittest/Object for some examples.

The return values of these two hooks do not affect the result of line number. I removed these paths in the patch and will post another patch to implement them.

One nit remaining, otherwise looks good.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
2

Ping this coment.

Esme updated this revision to Diff 345740.May 16 2021, 7:52 PM

Addressed comment.

jhenderson accepted this revision.May 17 2021, 1:07 AM

LGTM, once my last comment has been addressed.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
2

As per other comments, use ## for the marker. Also, no need to mention llvm-objdump (since we're in the llvm-objdump test directory).

This revision was landed with ongoing or failed builds.Jun 9 2021, 9:38 PM
This revision was automatically updated to reflect the committed changes.