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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

The test is with MachO but I assume this would apply to ELF, COFF etc, since it's a general disassembly feature?



================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1315
+      if (target->GetArchitecture().GetMachine() == llvm::Triple::aarch64 ||
+          target->GetArchitecture().GetMachine() == llvm::Triple::aarch64_32) {
+        if (*type_ptr == LLVMDisassembler_ReferenceType_In_ARM64_ADRP) {
----------------
Add `aarch64_be` here.

Triple does have a `isAArch64` which does this but I think you just get the `ArchType` enum here so you can't use it which is a shame.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1325
+        // the same register, this is pc-relative address calculation.
+        if (*type_ptr == LLVMDisassembler_ReferenceType_In_ARM64_ADDXri &&
+            m_adrp_address == pc - 4 &&
----------------
There is a 32 bit variant of ADD with W registers, I assume `LLVMDisassembler_ReferenceType_In_ARM64_ADDXri` is specific to the X version though, correct?

Which saves you checking the "sf" field later. (plus I don't know that any compiler would bother using the W version for this sort of thing anyway)


================
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.
----------------
Add comments to these two lines saying what they're checking for.

(looks correct from what the ARMARM says)


================
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;
----------------
Comments here too. The second one is sign extension, correct?


================
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;
----------------
"sh" is a single bit field so you could just:
```
if ((addrxi_inst >> 22) & 1)
```


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1342
+          value = (m_adrp_address & 0xfffffffffffff000LL) + (adrp_imm << 12) +
+                  addxri_imm;
+        }
----------------
ditto on the comments, but the math seems fine.


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1347
+      }
+
       if (m_inst->UsingFileAddress()) {
----------------
This method is pretty long as it is, does it make sense to make the aarch64 specific part of it another private method?


================
Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h:77
+  lldb::addr_t m_adrp_address;
+  uint32_t m_adrp_insn;
 
----------------
Just to check I understand, these are member vars because SymbolLookup will first be called on the adrp. So you write these values, then it's called again for the add, which is where you use them?

If so I would add a comment along those lines.


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:35
+                    if "HI" in i.GetComment(target):
+                        found_hi_string = True
+                if found_hi_string == False and self.TraceOn():
----------------
You could `break` after finding it.


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:36
+                        found_hi_string = True
+                if found_hi_string == False and self.TraceOn():
+                    strm = lldb.SBStream()
----------------
`if not found_hi_string and...`


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:41
+                    print(strm.GetData())
+                self.assertTrue(found_hi_string)
----------------
A message would help here, like `"Did not find \"HI\" string in disassembly."`.

Of course if you want to debug it the logging is actually going to help but just for anyone seeing it as a random failure in a general test suite run.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213