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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
