[PATCH] D149817: [llvm-objdump][X86] Add @plt symbols for .plt.got

[ben via Phabricator via llvm-commits]

bd1976llvm accepted this revision.
bd1976llvm added a comment.
This revision is now accepted and ready to land.

Thanks for working on this and sorry for the lack of response.

Generally looks good, LGTM from me.

I'm happy with the missing corner case for x86-32, given that you mentioned that it is extremely unlikely to occur. Do the GNU tools support that corner case? If they have gone to the trouble to support it I'm wondering if they have a use-case that hasn't occurred to us?
In the description, I think we should have a link to the optimisation in the PS ABI documents in addition to your blog entry.
I wonder if the LLDB folks are aware of this? Might be worth informing them?
I have mentioned a few improvements that seemed worthwhile to me. Feel free to ignore these if they are not helpful.



================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:672
+  if (RelaPlt) {
+    for (const auto &Relocation : RelaPlt->relocations()) {
+      if (Relocation.getType() != JumpSlotReloc)
----------------
R instead of Relocation to match the RelaDyn code?

Also the RelaPlt code here looks very similar to the RelaDyn code below. I would be tempted to factor out the common parts?


================
Comment at: llvm/test/tools/llvm-objdump/X86/plt-got.test:2
+## If a symbol needs both JUMP_SLOT and GLOB_DAT relocations, GNU ld places the
+## PLT entry in .plt.got. Test that we synthesize @plt symbols for the PLT entries.
+# RUN: yaml2obj --docnum=1 %s -o %t.x86-64
----------------
Would be nice to expand this comment to also mention more details of the optimisation e.g. dropping the JUMP_SLOT entry.


================
Comment at: llvm/test/tools/llvm-objdump/X86/plt-got.test:39
+# 32:      <_start>:
+# 32-NEXT:   movl
+# 32-NEXT:   movl
----------------
The check for the x86-64 case includes the registers?


================
Comment at: llvm/test/tools/llvm-objdump/X86/plt-got.test:61
+## b.s
+##
+## .globl foo0, foo1, combined0, combined1
----------------
don't need this empty line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149817