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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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:
> 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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1500
+                if (It != CodeSectionAddresses.begin()) {
+                  uint64_t SelectedSectionAddr = It->first;
+                  --It;
----------------
davidb wrote:
> This will cause an unused variable warning in release? (it is only used in assert).
> 
> I'm not sure this assert is really valuable. In. fact, this might trigger for binaries linked with OVERLAY segments (See D44780).
FWIW, @MaskRay was saying in another review that llvm builds with the unused variable warning off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75631