[Lldb-commits] [PATCH] D107213: Disassemble AArch64 pc-relative address calculations & symbolicate

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1328
+            (m_adrp_insn & 0x9f000000) == 0x90000000 &&
+            (m_adrp_insn & 0x1f) == ((value >> 5) & 0x1f)) {
+          // Bitmasking lifted from MachODump.cpp's SymbolizerSymbolLookUp.
----------------
DavidSpickett wrote:
> Add comments to these two lines saying what they're checking for.
> 
> (looks correct from what the ARMARM says)
There's a comment right before the conditional expr which says what it's checking.  The check that m_adrp_insn is actually an adrp instruction is unnecessary; I cribbed that from the MachODump file, but the only way I set the ivar to the instruction is if we had type_ptr==LLVMDisassembler_ReferenceType_In_ARM64_ADRP in the previous instruction.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1332-1335
+          adrp_imm =
+              ((m_adrp_insn & 0x00ffffe0) >> 3) | ((m_adrp_insn >> 29) & 0x3);
+          if (m_adrp_insn & 0x0200000)
+            adrp_imm |= 0xfffffffffc000000LL;
----------------
DavidSpickett wrote:
> Comments here too. The second one is sign extension, correct?
Yeah, it was being done incorrectly.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1339
+          addxri_imm = (addxri_inst >> 10) & 0xfff;
+          if (((addxri_inst >> 22) & 0x3) == 1)
+            addxri_imm <<= 12;
----------------
DavidSpickett wrote:
> "sh" is a single bit field so you could just:
> ```
> if ((addrxi_inst >> 22) & 1)
> ```
Yeah, I rewrote it the way you suggest.  I don't know why it was written that way in MachODump.cpp (incorrectly), but also why would the sh bit be set on the ADD when that just shifts the value up 12 bits like the ADRP instruction did already.  It's like a less rangey version of ADRP as soon as the sh bit is set.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213