[Lldb-commits] [PATCH] D12079: [MIPS] microMIPS breakpoints, disassembly and compressed addresses

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

DWARF parser should be stripping bit #0 for all addresses from mips targets: line tables, all address ranges for functions and blocks and variables should have this bit #0 stripped. The AddressClass from ObjectFileELF.cpp should help you figure out which ISA things are. This stops all sorts of extra code being added all over the debugger that needs to worry about this bit #0.


================
Comment at: include/lldb/Core/Address.h:311-312
@@ -310,1 +310,4 @@
 
+    Address
+    GetCallableFileAddress (Target *target, bool is_indirect = false) const;
+
----------------
Rename to GetCallableAddress since it returns a lldb_private::Address object and that isn't a file address. File address would be if you returned a lldb::addr_t that was a file address for a specific module, but that isn't the case here.

================
Comment at: include/lldb/Symbol/Function.h:579-580
@@ -578,4 +578,4 @@
 
     uint32_t
-    GetPrologueByteSize ();
+    GetPrologueByteSize (Target *target = nullptr);
 
----------------
This shouldn't be needed, see comment for Function::GetPrologueByteSize().

================
Comment at: include/lldb/Target/Target.h:908-909
@@ -907,1 +907,4 @@
 
+    lldb::addr_t
+    GetCallableFileAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;
+
----------------
This should return a lldb_private::Address object and "load_addr" should be a "lldb_private::Address()". There is no good way for a target to talk about file addresses since the returned "lldb::addr_t" would only make sense to a module since all modules for shared libraries share the file address zero. Load addresses are different because when a process is running and has things loaded, a load address describes a unique place in the program. A file address of zero would match all shared libraries since most shared libraries start their .text segment (or one of their segments with a file address of zero). Moving to a lldb_private::Address gives us the section + offset where the section describes which module contains the address.

The second parameter "addr_class" isn't needed if "lldb::addr_t load_addr" becomes "const lldb_private::Address &addr".

So this function should be:

```
lldb_private::Address
GetCallableAddress (const lldb_private::Address &addr);
```

================
Comment at: source/Core/Address.cpp:399-402
@@ +398,6 @@
+        {
+            lldb::addr_t callable_file_addr = target->GetCallableFileAddress (GetFileAddress(), GetAddressClass());
+            Address callable_addr;
+            if (module_sp->ResolveFileAddress (callable_file_addr, callable_addr))
+                return callable_addr;
+        }
----------------
This should become:

```
return target->GetCallableAddress(*this);
```

================
Comment at: source/Core/FormatEntity.cpp:421-424
@@ -420,6 +420,6 @@
     addr_t vaddr = LLDB_INVALID_ADDRESS;
     if (exe_ctx && !target->GetSectionLoadList().IsEmpty())
-        vaddr = addr.GetLoadAddress (target);
+        vaddr = addr.GetCallableLoadAddress (target);
     if (vaddr == LLDB_INVALID_ADDRESS)
-        vaddr = addr.GetFileAddress ();
+        vaddr = addr.GetCallableFileAddress (target).GetFileAddress ();
 
----------------
This change should probably be removed if we parse the line tables correctly right? That bit #0 for mips should be stripped when parsing the line table and the address class should be relied upon for anyone needing to know the origins of an address.

================
Comment at: source/Symbol/Function.cpp:558
@@ -557,3 +557,3 @@
 uint32_t
-Function::GetPrologueByteSize ()
+Function::GetPrologueByteSize (Target *target)
 {
----------------
Remove this param, see comment below.

================
Comment at: source/Symbol/Function.cpp:569-575
@@ -569,1 +568,9 @@
+
+            /*
+             * MIPS:
+             * The .debug_line section contains compressed addresses (bit #0 set), use callable
+             * file address to find the correct entry.
+            */
+            if (line_table->FindLineEntryByAddress(GetAddressRange().GetBaseAddress().GetCallableFileAddress(target),
+                first_line_entry, &first_line_entry_idx))
             {
----------------
This bit #0 should be sanitized before it is placed into the line table when the line table is being parsed. No one should have to worry about this, so thie "Target *target" parameter should be removed and the line table should strip bit #0 and we should rely on getting the address class correctly like you already fixed in ObjectFileELF.cpp if anyone needs to know about the address class.

================
Comment at: source/Symbol/Function.cpp:627
@@ -619,3 +626,3 @@
                 }
-                const addr_t func_start_file_addr = m_range.GetBaseAddress().GetFileAddress();
+                const addr_t func_start_file_addr = m_range.GetBaseAddress().GetCallableFileAddress(target).GetFileAddress();
                 const addr_t func_end_file_addr = func_start_file_addr + m_range.GetByteSize();
----------------
This should be removed. Bit #0 should never be left set in any public facing address and the address class should be relied upon by anyone needing to know about the ISA of the code address. So the DWARF parser needs to be fixed to strip bit #0 for all MIPS stuff.

================
Comment at: source/Target/RegisterContext.cpp:106-116
@@ -105,3 +105,13 @@
     uint32_t reg = ConvertRegisterKindToRegisterNumber (eRegisterKindGeneric, LLDB_REGNUM_GENERIC_PC);
-    return ReadRegisterAsUnsigned (reg, fail_value);
+    uint64_t pc = ReadRegisterAsUnsigned (reg, fail_value);
+
+    if (pc != fail_value)
+    {
+        TargetSP target_sp = m_thread.CalculateTarget();
+        Target *target = target_sp.get();
+        Address addr (pc);
+        pc = addr.GetOpcodeLoadAddress (target);
+    }
+
+    return pc;
 }
----------------
Bit #0 should be stripped from the PC before it is figured out and the frame might need to track the address class, so this change shouldn't be needed. We don't want extra bits floating around in our code that we have to strip everywhere. This should be done as the stack frames are being created. The frame will need to keep track of the address class in case the address doesn't map back to a shared library (JITed code might not have a module describing the code). So this code should be removed and the backtracer will need to sanitize the addresses as the PC values are unwound.

================
Comment at: source/Target/Target.cpp:2062-2063
@@ -2061,2 +2061,4 @@
 
 lldb::addr_t
+Target::GetCallableFileAddress (lldb::addr_t load_addr, AddressClass addr_class) const
+{
----------------
This should be:

```
lldb_private::Address
GetCallableAddress (const lldb_private::Address &addr);
```

================
Comment at: source/Target/ThreadPlanStepInRange.cpp:286
@@ -285,3 +285,3 @@
                     if (curr_addr == func_start_address.GetLoadAddress(m_thread.CalculateTarget().get()))
-                        bytes_to_skip = sc.function->GetPrologueByteSize();
+                        bytes_to_skip = sc.function->GetPrologueByteSize(m_thread.CalculateTarget().get());
                 }
----------------
Remove. Line tables shouldn't contain bit #0 set in any addresses in the line tables. AddressClass of an address should be relied upon for ISA.


Repository:
  rL LLVM

http://reviews.llvm.org/D12079