[PATCH] D75631: [llvm-objdump] Fix reliability of call target disassembling

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked 2 inline comments as done.
thopre added inline comments.


================
Comment at: lld/test/ELF/pre_init_fini_array.s:140-145
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
+// DISASM-NEXT:   callq   {{.*}} <_start+{{.*}}>
----------------
davidb wrote:
> jhenderson wrote:
> > davidb wrote:
> > > thopre wrote:
> > > > Suggestion welcome on how to get better output here. I tried replacing the call by leaq with some .align here and there to avoid overlap of __start and __end symbols but that only works for GNU objdump, llvm-objdump doesn't print the target of lea.
> > > Should be an lea right? from my host libc. Changing it to lea will make this test unrelated to your fix, as that only does symbol lookups for calls/branches.
> > > 
> > > ```
> > >    2:	4c 8d 3d 00 00 00 00 	lea    0x0(%rip),%r15        # 9 <__libc_csu_init+0x9>
> > > 			5: R_X86_64_PC32	__preinit_array_start-0x4
> > >    9:	41 56                	push   %r14
> > >    b:	4c 8d 35 00 00 00 00 	lea    0x0(%rip),%r14        # 12 <__libc_csu_init+0x12>
> > > 			e: R_X86_64_PC32	__preinit_array_end-0x4
> > > ```
> > I think here the instruction chosen isn't important to the test. What is important is to show that the references to the start/end symbols have been patched up correctly, which can be done by using the disassembly as it used to. In your latest version, this isn't the case any more. Using another instruction would be the way to fix it, assuming that there's another instruction where the disassembler does look ups of symbols.
> > 
> > > llvm-objdump doesn't print the target of lea.
> > 
> > This sounds like something that should be fixed in llvm-objdump to me.
> The instruction chosen should be important as this change only affects calls/branches and shouldn't affect the symbol resolution of general address loads (lea), as the former checks isText(), and the latter shouldn't....
> 
> Though as mentioned above, llvm-objdump doesn't appear to dump symbols for data....
This specific test is an existing test which is not aimed at testing the disassembly of a call instruction. It just happen to start failing with the patch because it's calling something that is data, not code. What @jhenderson meant is that this test could use leaq instead of call and still test what it's supposed to test (that a reference to those initializer and finalizers are resolved correctly).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75631/new/

https://reviews.llvm.org/D75631