This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][llvm-objdump] enable --symbolize-operands for PowerPC ELF/XCOFF.
ClosedPublic

Authored by Esme on Nov 23 2021, 6:08 PM.

Details

Summary
--symbolize-operands
When disassembling, symbolize a branch target operand to print a label instead of a real address.

Diff Detail

Event Timeline

Esme created this revision.Nov 23 2021, 6:08 PM
Esme requested review of this revision.Nov 23 2021, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 6:08 PM
MaskRay added inline comments.Nov 23 2021, 6:53 PM
llvm/test/tools/llvm-objdump/ELF/PowerPC/elf-disassemble-symbolize-operands.yaml
7 ↗(On Diff #389361)

Add -NEXT:

43 ↗(On Diff #389361)

obj2yaml created test file has many redundant things which are noise, e.g. eh_frame, .note.GNU-stack, STT_FILE. They can all be deleted.

How about using assembly with llvm-mc -filetype=obj? The main thing to test is the code sequence which is opaque as the yaml test (Content). An assembly test is easier to update.

llvm/tools/llvm-objdump/llvm-objdump.cpp
981–982

You can order PowerPC before X86 for an alphabetical order.

jhenderson added inline comments.Nov 25 2021, 1:04 AM
llvm/test/tools/llvm-objdump/ELF/PowerPC/elf-disassemble-symbolize-operands.yaml
1 ↗(On Diff #389361)

Don't prefix test names with elf- if they're in an ELF subdirectory.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.yaml
17–18 ↗(On Diff #389361)

Similar comment to MaskRay's above - the input probably wants to just be IR or assembly.

Esme updated this revision to Diff 392618.Dec 7 2021, 7:28 PM

Address comments.

shchenz added inline comments.Dec 8 2021, 7:04 PM
lld/test/ELF/ppc64-toc-call-to-pcrel.s
33

The change is not so straightforward to me.

Could we use <callee>:, <caller>:, <__toc_save_callee>: as the check label content? So it would not be confused with the new added branch target like <__toc_save_callee>.

llvm/test/tools/llvm-objdump/ELF/PowerPC/disassemble-symbolize-operands.ll
2

Can we add a case with internal/external function calls?

4

And maybe another case for CTR loop too?(for bdnz)

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
3

Can we show the leading-addr? As it can show whether the label matches the addr or not.

Esme updated this revision to Diff 394497.Dec 15 2021, 2:24 AM
  1. Address comments.
  2. Skip recording branches for function calls in collectLocalBranchTargets();

My comments have been addressed. Looks good, but leaving approval to someone who actually knows about the disassembly.

shchenz added inline comments.Dec 15 2021, 5:06 AM
llvm/test/tools/llvm-objdump/ELF/PowerPC/disassemble-symbolize-operands.ll
30

we can add an internal attribute for the internal function to make sure it is internally called in the function foo. For now, it is not internally called. I saw two external calls there.

Linux system objdump can dump the internal function call as:

78:   89 ff ff 4b     bl      0 <internal>

So it's better llvm-objdump can also dump the internal function call like this.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1002

s/PowerPc/PowerPC/g

1006

Can this logic be added in PPCMCInstrAnalysis? So we don't need the target check here.

Esme updated this revision to Diff 395061.Dec 17 2021, 12:07 AM

Address comments.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1006

I think we should return true for such cases in evaluateBranch(), because they are indeed branch operands, and their targets should be printed just like the behavior of system objdump when no --symbolize-operands is requires.

88:      	bl 0x88 <foo+0x48>
8c:      	nop
shchenz accepted this revision as: shchenz.Dec 17 2021, 12:43 AM

LGTM. Thanks for adding this support.

One minor comment: need to update the doc for PowerPC for --symbolize-operands option in llvm-objdump.rst.

Better to wait for some days before committing.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
7

This seems not right, expect .internal here. I guess it should be a legacy issue. Should be ok to do this in another patch.

This revision is now accepted and ready to land.Dec 17 2021, 12:43 AM
Esme updated this revision to Diff 395387.Dec 20 2021, 12:22 AM
Esme added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbolize-operands.ll
7

I will leave a TODO for this.

This revision was landed with ongoing or failed builds.Dec 20 2021, 8:18 PM
This revision was automatically updated to reflect the committed changes.