This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF][llvm-dwarfdump] support llvm-dwarfdump for XCOFF DWARF
ClosedPublic

Authored by shchenz on Feb 22 2021, 12:55 AM.

Details

Summary

Add support for XCOFF DWARF for llvm-dwarfdump tool.

The patch is made by @hubert.reinterpretcast

Diff Detail

Event Timeline

shchenz created this revision.Feb 22 2021, 12:55 AM
shchenz requested review of this revision.Feb 22 2021, 12:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 12:55 AM
shchenz edited the summary of this revision. (Show Details)Feb 22 2021, 4:19 AM

gentle ping

jasonliu accepted this revision.Mar 15 2021, 2:16 PM

I think what we usually do for test case is to upstream some pre-compiled binaries for testing purpose. But that's not the way community preferred anyway because those binaries are baked, and might be hard to reproduce later. Given this patch is small and pretty self-explanatory, I'm okay with skipping the test and let D97184 to "test" it.
FYI @hubert.reinterpretcast in case he think we should still upstream a pre-compiled binary for testing.
Otherwise, the patch LGTM.

This revision is now accepted and ready to land.Mar 15 2021, 2:16 PM
echristo requested changes to this revision.Mar 15 2021, 2:38 PM

I would prefer you use a checked in binary to test. I think it's reasonable to create a test case based on the dwarf emission support once you're convinced that's correct and go from there. For updates you can specify what the testcase is as a comment in the check. This is similar to a number of other tests FWIW :)

This revision now requires changes to proceed.Mar 15 2021, 2:38 PM
shchenz updated this revision to Diff 330885.Mar 15 2021, 11:11 PM

1: add test cases based on objects

Thanks for your review @jasonliu @echristo. Could you please help to have another look?

shchenz edited the summary of this revision. (Show Details)Mar 15 2021, 11:12 PM

@Higuoxing added more support to yaml2obj last year for DWARF emission via YAML. Maybe it would be best to wait on basic XCOFF yaml2obj support to be finished, add DWARF emission support to that, and then use yaml2obj to generate the DWARF output as required?

Also, the pre-merge checks claim your tests are failing.

Finally, how does this actually add support for llvm-dwarfdump + XCOFF? It's not particularly obvious to me, but I'm not all that familiar about what llvm-dwarfdump requires for this support.

llvm/lib/Object/XCOFFObjectFile.cpp
461–472

This list is missing DWARFv5 section names. Does XCOFF not support those sections?

llvm/test/tools/llvm-dwarfdump/XCOFF/lit.local.cfg
1–2 ↗(On Diff #330885)

Why do you need this? As far as I can tell, the behaviour under test is target agnostic.

@Higuoxing added more support to yaml2obj last year for DWARF emission via YAML. Maybe it would be best to wait on basic XCOFF yaml2obj support to be finished, add DWARF emission support to that, and then use yaml2obj to generate the DWARF output as required?

Also, the pre-merge checks claim your tests are failing.

Finally, how does this actually add support for llvm-dwarfdump + XCOFF? It's not particularly obvious to me, but I'm not all that familiar about what llvm-dwarfdump requires for this support.

I'm very happy to help implement DWARF support for yaml2xcoff (if needed), but I'm a little busy these days. I think I'm able to work on it in the next few months :-)

shchenz marked an inline comment as done.Mar 16 2021, 6:17 AM

Maybe it would be best to wait on basic XCOFF yaml2obj support to be finished, add DWARF emission support to that, and then use yaml2obj to generate the DWARF output as required?

I totally agree with your suggestions, committing an object directly is not so good.
Due to the limitation of XCOFF yaml2obj support for now in D95505 (for example, it only supports 32-bit) and the time when we need llvm-dwarfdump in, I firstly added this patch without test and I thought it can be verified together with patch D97184, in that patch, the object is generated from ll. After address @echristo comments, I added these tests based on XCOFF objects.

Finally, how does this actually add support for llvm-dwarfdump + XCOFF? It's not particularly obvious to me, but I'm not all that familiar about what llvm-dwarfdump requires for this support.

llvm-dwarfdump calls object::createBinary( to recognize a specific form object, we already added XCOFF support in object::createBinary()-> identify_magic(), so in this patch, we only need to override some functions called by DWARFContext class. llvm-dwarfdump calls DWARFContext to dump DWARF info.

I'm very happy to help implement DWARF support for yaml2xcoff (if needed), but I'm a little busy these days. I think I'm able to work on it in the next few months :-)

Thanks for your kindly help. If possible, could you please help to review the XCOFF yam2obj support patch D95505? Thanks @Higuoxing

llvm/lib/Object/XCOFFObjectFile.cpp
461–472

Yes, XCOFF does not support DWARF 5 sections for now.

llvm/test/tools/llvm-dwarfdump/XCOFF/lit.local.cfg
1–2 ↗(On Diff #330885)

Do you think is it necessary to run these tests on other targets? These are very basic DWARF info tests and I think other targets should already cover these tests by general/their tests?

shchenz updated this revision to Diff 330962.Mar 16 2021, 6:18 AM
shchenz marked an inline comment as done.
jasonliu added inline comments.Mar 16 2021, 6:55 AM
llvm/test/tools/llvm-dwarfdump/XCOFF/lit.local.cfg
2 ↗(On Diff #330962)

If these tests are runnable on other target, then I'd prefer to let them run on other targets as well.
People might change some staff and cause behavior change and they wouldn't know they need to update XCOFF test case if this test case is only runnable for powerpc target.

jasonliu added inline comments.Mar 16 2021, 7:01 AM
llvm/test/tools/llvm-dwarfdump/XCOFF/empty.test
181

Could you add the empty.c test case as comment here and the compile command as well so that people know how to reproduce if needed?
Also, could we use a public available compiler (xlclang 16.1 / gcc) instead of a self-built compiler to compile this test case for better reproducibility later as well?

shchenz updated this revision to Diff 331158.Mar 16 2021, 9:51 PM

1: update the testcases

echristo added inline comments.Mar 16 2021, 10:07 PM
llvm/test/tools/llvm-dwarfdump/XCOFF/empty.test
181

Couple of small things:

a) Can you name it something other than empty? Maybe basic or something?

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

shchenz updated this revision to Diff 331272.Mar 17 2021, 8:22 AM

1: rename the binary names

shchenz updated this revision to Diff 331276.Mar 17 2021, 8:29 AM

use correct file mode

Thanks for review @echristo

a) Can you name it something other than empty? Maybe basic or something?

Done

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

Yeah, sort of auto-generating. But do we need to loosen up the checks? This DWARF info should be settled as the result of the tests comes from object files.

jasonliu added inline comments.Mar 17 2021, 9:00 AM
llvm/test/tools/llvm-dwarfdump/XCOFF/basic.test
7–8 ↗(On Diff #331276)

The compiler version comment and the compile command is not consistent.

Harbormaster completed remote builds in B94249: Diff 331276.
echristo accepted this revision.Mar 17 2021, 10:03 AM

Thanks for review @echristo

a) Can you name it something other than empty? Maybe basic or something?

Done

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

Yeah, sort of auto-generating. But do we need to loosen up the checks? This DWARF info should be settled as the result of the tests comes from object files.

Yeah, it's tough. Right now if anything changes even slightly in the output the test will fail, but isn't necessarily everything you need to be checking if that makes sense? That said, it's probably on the small order of update probability so it's fine if you'd like to leave it this way and we can update it later if it becomes an issue.

Accepting (though fix any other feedback first).

-eric

This revision is now accepted and ready to land.Mar 17 2021, 10:03 AM

Thanks for review @echristo

a) Can you name it something other than empty? Maybe basic or something?

Done

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

Yeah, sort of auto-generating. But do we need to loosen up the checks? This DWARF info should be settled as the result of the tests comes from object files.

Yeah, it's tough. Right now if anything changes even slightly in the output the test will fail, but isn't necessarily everything you need to be checking if that makes sense? That said, it's probably on the small order of update probability so it's fine if you'd like to leave it this way and we can update it later if it becomes an issue.

Accepting (though fix any other feedback first).

-eric

Yeah. Agree, at least there is no need for the requirement of the DIE attributes' order and the fixed offset in the .dwinfo sections. This will be too tough if we regenerate the object file for every new commit. But for this case, since we directly test the object files, so I think it should be ok. Yeah, we may still have some changes in llvm-dwarfdump tool which makes the output different, but that should be not often.

Thanks for your review

llvm/test/tools/llvm-dwarfdump/XCOFF/basic.test
7–8 ↗(On Diff #331276)

Will update it in the commit. Thanks

shchenz updated this revision to Diff 331432.Mar 17 2021, 6:07 PM

1: correct xlc version

This revision was landed with ongoing or failed builds.Mar 17 2021, 6:22 PM
This revision was automatically updated to reflect the committed changes.