[PATCH] D48554: Handle absolute symbols as branch targets in disassembly.

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: test/tools/llvm-objdump/call-absolute-symbol-elf.test:1
+// RUN: llvm-objdump --disassemble %p/Inputs/call-absolute-symbol.elf-x86_64 | FileCheck %s
+
----------------
MaskRay wrote:
> saugustine wrote:
> > MaskRay wrote:
> > > Should this test be synthesized from a `.s` file using llvm-mc?
> > This is a good question, and I debated. But only about five of the ~100 files in llbm-objdump/Inputs are normal ASCII source of one kind or another. The rest are binaries. So I went that way. Totally willing to change it if you think it is important.
> Adding  @enderby @ruiu who have touched `test/Object/Inputs/*` binaries.
> 
> There are also utilities `obj2yaml` and `yaml2obj` (introduced in 2012) to construct tests involving binary files. I wonder why the tests here are organized in such a way.
I can elaborate here:

- Some of the tests predate obj2yaml/yaml2obj - especially since it didn't see widespread use until recently.

- For some tests you'll definitely want an existing object file because you've either got malformed output that you were trying to look at, or something that might change in the future.

- Otherwise a .s file is fine too.

My preference here, I think, is going to be:

1) yaml2obj if it's simple
2) .s file if possible
3) checked in object file


================
Comment at: test/tools/llvm-objdump/call-absolute-symbol-elf.test:3
+
+CHECK: 201000:	e8 fb f0 df ff 	callq	-2100997 <foo>
----------------
MaskRay wrote:
> saugustine wrote:
> > MaskRay wrote:
> > > I find that on AArch64 PowerPC x86_64 ... all these call/branch instructions print the immediate value without taking account of PC (what they print are relative addresses, instead of absolute addresses as printed by `objdump`)
> > > 
> > > The PowerPC one at least appends `.+` before the immediate to tell you it is a relative address.
> > > 
> > > https://github.com/llvm-mirror/llvm/tree/master/lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp#L386
> > > void PPCInstPrinter::printBranchOperand(const MCInst *MI, unsigned OpNo,
> > >                                         raw_ostream &O) {
> > >   if (!MI->getOperand(OpNo).isImm())
> > >     return printOperand(MI, OpNo, O);
> > > 
> > >   // Branches can take an immediate operand.  This is used by the branch
> > >   // selection pass to print .+8, an eight byte displacement from the PC.
> > >   O << ".+";
> > >   printAbsBranchOperand(MI, OpNo, O);
> > > }
> > > 
> > This patch doesn't change the printed value, it just adds the symbol it targets. This is sufficient for my needs, and I'm not sure what the dependencies of the current format are, or if it is a principled decision.
> > 
> > Before this patch:
> > 
> > 201000:  e8 fb f0 df ff   callq  -2100997
> > 
> > After:
> > 
> > 201000:  e8 fb f0 df ff   callq  -2100997 <foo>
> I should be clear that my comment was asking why pc-relative addresses are printed in the current way... This patch definitely improves the view so you can just ignore my comment..
Almost assuredly no particular planning on the format of objdump output here.


Repository:
  rL LLVM

https://reviews.llvm.org/D48554