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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett 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.
----------------
jasonmolenda wrote:
> 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.
> The check that m_adrp_insn is actually an adrp instruction is unnecessary

In that we could have an Optional<uint32_t> and assume if set, it's an adrp. But this is functionally the same.

Comment bikeshedding, put the conditions in the same order as the text? So readers know that 0x1f is the register?
(though if you were questioning this code the first port of call would be the arm manuals but regardless)
```
// If previous instruction was ADRP and this is ADD, and it's to
// the same register, this is pc-relative address calculation.
if (m_adrp_address == pc - 4 &&
    (m_adrp_insn & 0x9f000000) == 0x90000000 && 
    *type_ptr == LLVMDisassembler_ReferenceType_In_ARM64_ADDXri &&
    (m_adrp_insn & 0x1f) == ((value >> 5) & 0x1f)) {
```


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:15-27
+        src_dir = self.getSourceDir()
+        yaml_path = os.path.join(src_dir, "a.out-arm64.yaml")
+        obj_path = self.getBuildArtifact("a.out-arm64")
+        self.yaml2obj(yaml_path, obj_path)
+
+        target = self.dbg.CreateTarget(obj_path)
+        self.assertTrue(target, VALID_TARGET)
----------------
Could move most of this into a utility fn too.


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:29
+
+    def test_arm64_32(self):
+        src_dir = self.getSourceDir()
----------------
`no_debug_info_test` here?


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/TestAArch64AdrpAdd.py:60
+        if found_hi_string == False or found_foo == False:
+            print('Did not find "HI" string or "foo" in disassembly symbolication in %s' % binaryname)
+            if self.TraceOn():
----------------
Shouldn't this be within the if Trace?


================
Comment at: lldb/test/API/functionalities/disassemble/aarch64-adrp-add/main.c:260
+  asm("nop");asm("nop");asm("nop");asm("nop");
+  asm("nop");asm("nop");asm("nop");asm("nop");
+  return 5;
----------------
I admit a bit of a "wat" moment here, but I assume this is setting things up so that foo is some distance behind (at a lower address) than main?
So we have foo, which is before main and HI which is after for a negative, positive offset.

Could do with a comment for how many instructions this is. Maybe you could #define something for maybe 64 nops at a time just to make it a little less verbose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107213