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

[Dave Bozier via Phabricator via llvm-commits]

davidb 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+{{.*}}>
----------------
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....


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1500
+                if (It != CodeSectionAddresses.begin()) {
+                  uint64_t SelectedSectionAddr = It->first;
+                  --It;
----------------
jhenderson wrote:
> 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.
So are variables used for asserts encouraged? Seems like an odd choice of warning to disable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75631