[Lldb-commits] [PATCH] D12079: [MIPS] microMIPS breakpoints, disassembly and compressed addresses
Greg Clayton via lldb-commits
lldb-commits at lists.llvm.org
Mon Aug 17 10:44:41 PDT 2015
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Many changes. See inlined comments.
================
Comment at: source/Core/Disassembler.cpp:1169-1187
@@ +1168,21 @@
+
+ /*
+ * MIPS:
+ * The bit #0 of an address is used for ISA mode (1 for microMIPS, 0 for MIPS).
+ * This allows processer to switch between microMIPS and MIPS without any need
+ * for special mode-control register. If the address specified in the 'range'
+ * is a microMIPS address then clear bit #0 and fetch opcode from the memory.
+ */
+ Address compressed_addr = range.GetBaseAddress();
+ if (m_arch.GetMachine() == llvm::Triple::mips64
+ || m_arch.GetMachine() == llvm::Triple::mips64el
+ || m_arch.GetMachine() == llvm::Triple::mips
+ || m_arch.GetMachine() == llvm::Triple::mipsel)
+ {
+ if ((m_arch.GetFlags() | ArchSpec::eMIPSAse_micromips) == ArchSpec::eMIPSAse_micromips
+ || (m_arch.GetFlags() | ArchSpec::eMIPSAse_mips16) == ArchSpec::eMIPSAse_mips16)
+ {
+ compressed_addr.SetOffset (compressed_addr.GetOffset() & (~1));
+ }
+ }
+
----------------
This kind of address snipping is going to be needed in many different places and this should be done in:
lldb::addr_t
Target::GetOpcodeLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const;
You will note there is already similar functionality for ARM:
```
lldb::addr_t
Target::GetOpcodeLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const
{
addr_t opcode_addr = load_addr;
switch (m_arch.GetMachine())
{
case llvm::Triple::arm:
case llvm::Triple::thumb:
switch (addr_class)
{
case eAddressClassData:
case eAddressClassDebug:
return LLDB_INVALID_ADDRESS;
case eAddressClassInvalid:
case eAddressClassUnknown:
case eAddressClassCode:
case eAddressClassCodeAlternateISA:
case eAddressClassRuntime:
opcode_addr &= ~(1ull);
break;
}
break;
default:
break;
}
return opcode_addr;
}
```
Then you would typically access this via "Address::GetCallableLoadAddress (Target *target, bool is_indirect) const".
We should probably add a new method to Address:
```
Address
Address::GetCallableAddress(Target *target, bool is_indirect) const
{
SectionSP section_sp (GetSection());
if (section_sp)
{
ModuleSP module_sp = section_sp->GetModule();
if (module_sp)
{
lldb::addr_t callable_file_addr = target->GetCallableLoadAddress (GetFileAddress(), GetAddressClass());
Address callable_addr;
if (module_sp->ResolveFileAddress (callable_file_addr, callable_addr))
return callable_addr;
}
}
return *this;
}
```
Then you should use this here:
```
const size_t bytes_read = target->ReadMemory (range.GetBaseAddress().GetCallableAddress(target, false),
```
================
Comment at: source/Core/Disassembler.cpp:1189
@@ -1169,1 +1188,3 @@
+
+ const size_t bytes_read = target->ReadMemory (compressed_addr.GetFileAddress(),
prefer_file_cache,
----------------
This is incorrect. You can't pass a file address to target->ReadMemory(...) as this will do the wrong thing if you are running. The story goes like this:
lldb_private::Address is a section offset based address that says an address is ".text + 0x1000". When target->ReadMemory() tries to read memory from this address, it can see if "prefer_file_cache" is set and if so, it will grab the section from the the address that is passed as the first parameter and then be able to get the module from that section and read data from the cached .text section contents from the object file in the module.
If you call Target::ReadMemory() with "compressed_addr.GetFileAddress()", it will get the file address (the unslid address) of 0x1000 and convert that to a Address object.
So just pass your fixed up address, in this case it will be "compressed_addr".
================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2098-2112
@@ -2093,1 +2097,17 @@
+ /*
+ * MIPS:
+ * The bit #0 of an address is used for ISA mode (1 for microMIPS, 0 for MIPS).
+ * This allows processer to switch between microMIPS and MIPS without any need
+ * for special mode-control register. However, apart from .debug_line, none of
+ * the ELF/DWARF sections set the ISA bit (for symbol or section). Use st_other
+ * flag to check whether the symbol is microMIPS and then set the ISA bit
+ * accordingly.
+ */
+ if (IS_MICROMIPS(symbol.st_other) &&
+ (arch.GetMachine() == llvm::Triple::mips64
+ || arch.GetMachine() == llvm::Triple::mips64el
+ || arch.GetMachine() == llvm::Triple::mips
+ || arch.GetMachine() == llvm::Triple::mipsel))
+ symbol_value = (symbol_value | 1); // Set ISA bit
+
----------------
I wouldn't muck with the symbol value directly, just make sure that ObjectFileELF::GetAddressClass(...) works:
AddressClass
ObjectFileELF::GetAddressClass (addr_t file_addr);
This should classify any address as either eAddressClassCode (ARM for ARM architectures) or eAddressClassCodeAlternateISA (Thumb for ARM architectures). Then any code that relies on ISA should be checking the AddressClass. for eAddressClassCode or eAddressClassCodeAlternateISA.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3172-3173
@@ -3171,3 +3171,4 @@
// Get the breakpoint address
- const addr_t addr = bp_site->GetLoadAddress();
+ const addr_t load_addr = bp_site->GetLoadAddress();
+ addr_t addr = load_addr;
----------------
This should be removed. The address in the breakpoint site should already have been sanitized by Process::CreateBreakpointSite() which will call Address::GetOpcodeLoadAddress(Target*) to get the correct address.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3187-3203
@@ -3185,1 +3186,19 @@
+ /*
+ * MIPS:
+ * If bit #0 of an address (ISA bit) is set, then this is microMIPS or MIPS16 address.
+ * Processor clears this bit before fetching the instruction from memory. Set this
+ * breakpoint at uncompressed address.
+ */
+ const ArchSpec target_arch = GetTarget().GetArchitecture();
+ if (target_arch.GetMachine() == llvm::Triple::mips || target_arch.GetMachine() == llvm::Triple::mipsel
+ || target_arch.GetMachine() == llvm::Triple::mips64 || target_arch.GetMachine() == llvm::Triple::mips64el)
+ {
+ if ((addr & 1))
+ {
+ addr = addr & (~1);
+ if (log)
+ log->Printf("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") uncompressed address = 0x%" PRIx64, site_id, (uint64_t)addr);
+ }
+ }
+
----------------
This should be removed. The address in the breakpoint site should already have been sanitized by Process::CreateBreakpointSite() which will call Address::GetOpcodeLoadAddress(Target*) to get the correct address.
================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3297-3314
@@ -3277,2 +3296,20 @@
+ /*
+ * MIPS:
+ * If bit #0 of an address (ISA bit) is set, then this is microMIPS or MIPS16 address.
+ * Processor clears this bit before fetching the instruction from memory. Use
+ * uncompressed address to disable this breakpoint.
+ */
+ const ArchSpec target_arch = GetTarget().GetArchitecture();
+ if (target_arch.GetMachine() == llvm::Triple::mips || target_arch.GetMachine() == llvm::Triple::mipsel
+ || target_arch.GetMachine() == llvm::Triple::mips64 || target_arch.GetMachine() == llvm::Triple::mips64el)
+ {
+ if ((addr & 1))
+ {
+ addr = addr & (~1);
+ if (log)
+ log->Printf("ProcessGDBRemote::DisableBreakpointSite (size_id = %" PRIu64 ") uncompressed address = 0x%" PRIx64, site_id, (uint64_t)addr);
+ }
+ }
+
if (bp_site->IsEnabled())
----------------
This should be removed. The address in the breakpoint site should already have been sanitized by Process::CreateBreakpointSite() which will call Address::GetOpcodeLoadAddress(Target*) to get the correct address.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1276-1320
@@ -1275,2 +1275,47 @@
+ /*
+ * MIPS:
+ * The bit #0 of an address is used for ISA mode (1 for microMIPS, 0 for MIPS).
+ * This allows processer to switch between microMIPS and MIPS without any need
+ * for special mode-control register. However, apart from .debug_line, none of
+ * the ELF/DWARF sections set the ISA bit (for symbol or section).
+ *
+ * Find first symbol with name func_name and type FUNC. If this is a microMIPS
+ * symbol then adjust func_range accordingly.
+ */
+ ArchSpec arch;
+ GetObjectFile()->GetArchitecture(arch);
+
+ if (arch.GetMachine() == llvm::Triple::mips64
+ || arch.GetMachine() == llvm::Triple::mips64el
+ || arch.GetMachine() == llvm::Triple::mips
+ || arch.GetMachine() == llvm::Triple::mipsel)
+ {
+ Symbol *microsym = NULL;
+ if (m_obj_file)
+ {
+ Symtab *symtab = m_obj_file->GetSymtab ();
+ if (symtab)
+ {
+ lldb::LanguageType language = ParseCompileUnitLanguage(sc);
+ microsym = symtab->FindFirstSymbolWithNameAndType (func_name.GetDemangledName(language),
+ eSymbolTypeCode,
+ Symtab::eDebugNo,
+ Symtab::eVisibilityAny);
+
+ if (microsym != NULL)
+ {
+ lldb::addr_t addr = microsym->GetFileAddress();
+
+ // If address is compressed then it is a microMIPS symbol
+ if (addr & 1)
+ {
+ Address &compressed_addr = func_range.GetBaseAddress();
+ compressed_addr.SetOffset (compressed_addr.GetOffset() | 1);
+ }
+ }
+ }
+ }
+ }
+
if (FixupAddress (func_range.GetBaseAddress()))
----------------
This should be removed and rely on ObjectFile::GetAddressClass() to do the right thing.
Repository:
rL LLVM
http://reviews.llvm.org/D12079
More information about the lldb-commits
mailing list