[PATCH] D50203: Find PLT entries for x86, x86_64, and AArch64

[Joel Galenson via Phabricator via llvm-commits]

jgalenson added inline comments.


================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp:165
+         Byte += 4) {
+      uint32_t Insn = (PltContents[Byte + 3] << 24) |
+                      (PltContents[Byte + 2] << 16) |
----------------
pcc wrote:
> You could use the `support::endian::read32le` function here (and elsewhere in the patch).
Ooh, I didn't know about that.  Thanks!


================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp:170
+      // Check for adrp
+      if (Insn >> 31 == 1 && ((Insn >> 24) & 0x1f) == 0x10) {
+        Imm = (((PltSectionVA + Byte) >> 12) << 12) +
----------------
pcc wrote:
> To avoid excessive indentation I'd write this with an early continue.
> ```
> if (Insn >> 31 != 1 || ((Insn >> 24) & 0x1f) != 0x10)
>   continue;
> uint64_t Imm = ...;
> ```
> I might also simplify the condition like this:
> ```
> if ((Insn & 0x9f000000) != 0x90000000)
>   continue;
> ```
Good ideas.  Thanks.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:449
+  for (uint64_t Byte = 0, End = PltContents.size(); Byte + 6 < End; ) {
+    // Recognize a jmp followed by a push.
+    if (PltContents[Byte] == 0xff && PltContents[Byte + 1] == 0xa3 &&
----------------
pcc wrote:
> I wouldn't try to match the push here (and in the 64-bit version) since it's not essential for decoding the plt entry. It's also theoretically possible for it to be missing, e.g. a linker would be able to avoid emitting it when passed `-z now`.
I didn't know that.  Thanks.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:450
+    // Recognize a jmp followed by a push.
+    if (PltContents[Byte] == 0xff && PltContents[Byte + 1] == 0xa3 &&
+        PltContents[Byte + 6] == 0x68) {
----------------
pcc wrote:
> I think you also need to be able to decode the `0xff 0x25` form here, it's used in non-PIC executables.
I extended the condition to handle that case.  But does anything else change with that form, or just these bits of the opcode?


https://reviews.llvm.org/D50203