[MIPS] microMIPS breakpoints, disassembly and compressed addresses

[jaydeep]

Summary

This patch enables setting of breakpoints and disassembly for microMIPS applications running on bare-iron targets like IASim.

MIPS uses bit #0 (ISA bit) of an address for ISA mode (1 for microMIPS/MIPS16 and 0 for MIPS). The resulting address is called as compressed address when ISA bit is set. This allows processor to switch between microMIPS and MIPS without any need for special mode-control register. This bit is then cleared by the processor while fetching the instruction from memory. However, apart from .debug_line, none of the ELF/DWARF sections set the ISA bit.

In this patch:

1. The symbol table is recorded in the form of compressed address for microMIPS symbols, so that corresponding debug_line can be decoded properly.
2. Memory read/write of compressed address has been handled

[clayborg]

Many changes. See inlined comments.

source/Core/Disassembler.cpp

1169–1187

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),

1189

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".

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

2098–2112

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.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

3172–3173

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.

3187–3203

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.

3297–3314

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.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1276–1320

This should be removed and rely on ObjectFile::GetAddressClass() to do the right thing.

[clayborg]

The main thing is, we don't want to be like other debuggers that have all this code in many many places that check address bits by checking the Architecture and litter the code with bit strips and adding bits where needed. We want to support addresses correctly by knowing that a Address has a special address class. So we use:

addr_t
Address::GetCallableLoadAddress (Target *target, bool is_indirect) const

and

lldb::addr_t
Target::GetOpcodeLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const

Target also has a counterpart that does the actual check since the target has the ArchSpec that tells us the architecture. Also if you ever need make a special address that needs to have bit zero set, there is:

lldb::addr_t
Target::GetCallableLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const

[jaydeep]

Addressed review comments.
Address conversions are handled in Address class. This is a reduced version of the original patch where address conversions are handled for breakpoints only.

[jaydeep]

Added GetCallableFileAddress for MIPS

[clayborg]

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.

include/lldb/Core/Address.h

311–312 ↗

(On Diff #33793)

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.

include/lldb/Symbol/Function.h

579–580 ↗

(On Diff #33793)

This shouldn't be needed, see comment for Function::GetPrologueByteSize().

include/lldb/Target/Target.h

908–909 ↗

(On Diff #33793)

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);

source/Core/Address.cpp

399–402 ↗

(On Diff #33793)

This should become:

return target->GetCallableAddress(*this);

source/Core/FormatEntity.cpp

421–424 ↗

(On Diff #33793)

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.

source/Symbol/Function.cpp

558 ↗

(On Diff #33793)

Remove this param, see comment below.

569–575 ↗

(On Diff #33793)

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.

627 ↗

(On Diff #33793)

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.

source/Target/RegisterContext.cpp

106–116 ↗

(On Diff #33793)

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.

source/Target/Target.cpp

2062–2063 ↗

(On Diff #33793)

This should be:

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

source/Target/ThreadPlanStepInRange.cpp

286 ↗

(On Diff #33793)

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.

[jaydeep]

source/Target/RegisterContext.cpp

106–116 ↗

(On Diff #33793)

The breakpoint is set on OpcodeAddress (bit #0 clear), but target returns CallableAddress (bit #0 set) when breakpoint is hit.

Set using:
StoppointLocation (loc_id, addr.GetOpcodeLoadAddress(&owner.GetTarget()), hardware)

Find using:
addr_t pc = thread_sp->GetRegisterContext()->GetPC() + m_breakpoint_pc_offset;
lldb::BreakpointSiteSP bp_site_sp = thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);

We either need to clear bit #0 from the PC we get or need to set the breakpoint on CallableAddress (which would need LineTable in CallableAddress form).

[jaydeep]

In this patch:

The bit #0 has been cleared from addresses in the line tables. However we are relying upon ArchSpec instead of Target while clearing this bit in ParseDWARFLineTableCallback because SymbolContext may not have a valid target to call Address::GetOpcodeLoadAddress().

Bare-iron targets (like YAMON, IASim, Qemu) return compressed address (bit #0 set) when process is stopped in microMIPS address space. For example: bit #0 of PC is set when a breakpoint is hit. This bit has been cleared while reading the PC in RegisterContext::GetPC(). This would help us find breakpoints set using GetOpcodeLoadAddress (bit #0 clear),

DisassemblerLLVMC::DisassemblerLLVMC has been modified to create m_alternate_disasm_ap for microMIPS. This would display disassembly in either compressed (bit #0 set) or uncompressed (bit #0 clear) address space based on ISA mode.

[clayborg]

Open issues:

• In FormatEntity.cpp we probably don't need any changes to DumpAddress()
• Remove MIPS comment from generic code and let the "Target::GetOpcodeLoadAddress (...) const" document what is happening.
• Call Target::GetOpcodeLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const to fixup the PC.

source/Core/FormatEntity.cpp

421–429 ↗

(On Diff #34199)

I repeat this concern: do we still need to do this? There should be no changes needed for this function if the bit #0 has been stripped.

source/Target/RegisterContext.cpp

110–116 ↗

(On Diff #34199)

Probably no need for this MIPS specific comment in here, it should be documented once in the Target functions that strip the bit zero.

117–120 ↗

(On Diff #34199)

We don't need to make a section + offset address here, we can just use:

lldb::addr_t
Target::GetOpcodeLoadAddress (lldb::addr_t load_addr, AddressClass addr_class) const;

So this code should be:

TargetSP target_sp = m_thread.CalculateTarget();
if (target_sp)
    pc = target->GetOpcodeLoadAddress (pc, eAddressClassCode);

[jaydeep]

In this patch:

• Removed MIPS comment from generic code
• Used Target::GetOpcodeLoadAddress to fixup the PC

Regarding change in FormatEntity.cpp:

We still need to do this. The bit #0 of ‘addr’ has already been striped and thus it does not represent its true address space (microMIPS or MIPS). We need to call GetCallableLoadAddress here because we want to set the bit #0 of this address if it belongs to eAddressClassCodeAlternateISA.

This change displays the microMIPS disassembly (and other addresses) in compact address space:

0x8020067d <+0>:  addiusp -16
0x8020067f <+2>:  sw     $fp, 12($sp)
0x80200681 <+4>:  move   $fp, $sp

• thread #1: tid = 0x0001, 0x802006c5 micro.elf`foo(a=0, b=0) + 16 at micro.c:19, stop reason = breakpoint 2.1 frame #0: 0x802006c5 micro.elf`foo(a=0, b=0) + 16 at micro.c:19

Without this change the microMIPS disassembly would be displayed in uncompact (MIPS) address space:

0x8020067c <+0>:  addiusp -16
0x8020067e <+2>:  sw     $fp, 12($sp)
0x80200680 <+4>:  move   $fp, $sp

• thread #1: tid = 0x0001, 0x802006c4 micro.elf`foo(a=0, b=0) + 16 at micro.c:19, stop reason = breakpoint 2.1 frame #0: 0x802006c4 micro.elf`foo(a=0, b=0) + 16 at micro.c:19

[clayborg]

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

[clayborg]

Actually not a new format type, but an extra arg will need to be passed to DumpAddress like "bool addr_is_callable".

Can you explain something to me? In the following example:

0x8020067d <+0>:  addiusp -16
0x8020067f <+2>:  sw     $fp, 12($sp)
0x80200681 <+4>:  move   $fp, $sp

Is the addiusp actually at 0x8020067c in memory? Then we just display 0x8020067d to let people know this is MicroMIPS?

[jaydeep]

In D12079#242751, @clayborg wrote:

Actually not a new format type, but an extra arg will need to be passed to DumpAddress like "bool addr_is_callable".

Can you explain something to me? In the following example:

0x8020067d <+0>:  addiusp -16
0x8020067f <+2>:  sw     $fp, 12($sp)
0x80200681 <+4>:  move   $fp, $sp

Is the addiusp actually at 0x8020067c in memory? Then we just display 0x8020067d to let people know this is MicroMIPS?

Yes, addiusp is actually at 0x8020067c, but processor (when running in microMIPS mode) strips bit #0 while fetching it from memory. We should display it at 0x8020067d to let user know that this is microMIPS.

[jaydeep]

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

[clayborg]

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

[jaydeep]

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

Yes.

[jaydeep]

In D12079#244059, @jaydeep wrote:

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

Yes.

Instead of modifying Address::Dump() we should modify DumpAddress() so that

In D12079#244059, @jaydeep wrote:

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

In D12079#243390, @clayborg wrote:

In D12079#242998, @jaydeep wrote:

In D12079#242742, @clayborg wrote:

So DumpAddress() in FormatEntity.cpp is a generic "dump any address by describing it". You can't just change the code to suit your needs for MIPS. This address could be any address: code or data. If you want something that can take an address like 0x1000 and you ask for its AddressClass and it sees that its address class is eAddressClassCodeAlternateISA, and then you change it to be "0x1001", this will need to be a new format type.

DumpAddress in FormatEntity.cpp is called for the following entities:

case Entry::Type::LineEntryStartAddress:
case Entry::Type::LineEntryEndAddress:
case Entry::Type::AddressFile:
case Entry::Type::AddressLoad:
case Entry::Type::AddressLoadOrFile:
case Entry::Type::FrameRegisterPC

So only the LineEntry ones should actually do what you did.

We need to display all these entities in compressed address format. How about a new MIPS specific function in Address and Target class which would do this.

Address Address::GetCallableAddress(Target *target);
lldb::addr_t Target::GetCallableAddress (lldb::addr_t load_addr, AddressClass addr_class);

We already have this in Target:

lldb::addr_t
GetCallableLoadAddress (lldb::addr_t load_addr, lldb::AddressClass addr_class = lldb::eAddressClassInvalid) const;

So the solution here will be to modify Address::Dump() such that it detects when an address is eAddressClassCodeAlternateISA and when that happens it checks if the ExecutionContext parameter is non NULL, and if so, extract the target, and check the target's architecture is MIPS, then add the extra bit when displaying this address. As it seems that we would always want to describe a section offset address (lldb_private::Address object) in this way to show the MicroMIPS address space bit, right?

Yes.

Change in Address::Dump() would display microMIPS address only for Entry::Type::AddressLoadOrFile entity. (when "print_file_addr_or_load_addr" is true in DumpAddress()). However we would like to display microMIPS addresses for

Entry::Type::AddressFile
Entry::Type::AddressLoad
Entry::Type::FrameRegisterPC
Entry::Type::LineEntryStartAddress
Entry::Type::LineEntryEndAddress

entities as well (when "print_file_addr_or_load_addr" is false). The suggested change should be moved to DumpAddress().

[jaydeep]

In this patch:
Modified DumpAddress() to print compressed address for microMIPS.

[jaydeep]

Hi Greg,
Could you please find some time to review this?
Thanks

[clayborg]

So everywhere that we want to display a code address for MicroMIPS needs to now add code that creates a callable address? I still say that Address::Dump() is what should be modified, not DumpAddress if FormatEntity.cpp. I don't want any other code anywhere in the debugger to have to make this check. It should be just Address::Dump() that knows about this.

[jaydeep]

Addressed review comments

[clayborg]

One last change to make line table parsing more efficient by not having to check the arch for every line table entry.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

1442

Maybe this should be a "lldb:addr_t addr_mask;" instead of the architecture. Then you determine the mask one time before you parse a line table and fill it in.

1470–1480

Move this code to where the ParseDWARFLineTableCallbackInfo is filled in and fill in "addr_mask" as described above. Otherwise each time we append a line entry to a sequence we will be checking the arch over and over and over and over....

1471

change this line to:

file_addr & info->addr_mask,

1511

Fill in "info.addr_mask" here:

/*
 * MIPS:
 * The SymbolContext may not have a valid target, thus we may not be able
 * to call Address::GetOpcodeLoadAddress() which would clear the bit #0
 * for MIPS. Use ArchSpec to clear the bit #0.
*/
ArchSpec arch;
GetObjectFile()->GetArchitecture(arch);
switch (arch.GetMachine())
{
case llvm::Triple::mips:
case llvm::Triple::mipsel:
case llvm::Triple::mips64:
case llvm::Triple::mips64el:
    info.addr_mask = ~((lldb::addr_t)1);
    break;
default:
    info.addr_mask = ~((lldb::addr_t)0);
    break;
}

[jaydeep]

Addressed review comments

[clayborg]

A few more little things with respect to not calling accessors multiple times in if statements and this will be good to go.

source/Core/Address.cpp

474 ↗

(On Diff #34873)

Change this to be:

const llvm::Triple::ArchType llvm_arch = target->GetArchitecture().GetMachine();

and use llvm_arch in if statement below instead of calling accessor 4 times.

source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp

750–763 ↗

(On Diff #34873)

We should store the llvm::Triple::ArchType into a local const variable up on line 750 and use it on line 750 and in this "else if". Also note that we have "triple" which is already the "arch.GetTriple()".

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

2054–2055

const llvm::Triple::ArchType llvm_arch = target->GetArchitecture().GetMachine();

Then use llvm_arch in the if statement.

[jaydeep]

Addressed review comments.

[clayborg]

Looks good.

[jaydeep]

Closed by commit http://reviews.llvm.org/rL248248

source/Core/Disassembler.cpp

if (exe_ctx)
         if (target == NULL || byte_size == 0 || !range.GetBaseAddress().IsValid())
             return 0;
 
         DataBufferHeap *heap_buffer = new DataBufferHeap (byte_size, '\0');
         DataBufferSP data_sp(heap_buffer);
 
         Error error;
         lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
-        const size_t bytes_read = target->ReadMemory (range.GetBaseAddress(),
+
+        /*
+         * 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));
+            }
+        }
+
+        const size_t bytes_read = target->ReadMemory (compressed_addr.GetFileAddress(),
                                                       prefer_file_cache, 
                                                       heap_buffer->GetBytes(), 
                                                       heap_buffer->GetByteSize(), 
                                                       error,
                                                       &load_addr);
         
         if (bytes_read > 0)
         {

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

if (m_sections_ap.get())
         }
         else
         {
             unified_section_list = *m_sections_ap;
         }
     }
 }
 
+#define STO_MIPS_ISA            (3 << 6)
+#define STO_MICROMIPS           (2 << 6)
+#define IS_MICROMIPS(ST_OTHER)  (((ST_OTHER) & STO_MIPS_ISA) == STO_MICROMIPS)
+
 // private
 unsigned
 ObjectFileELF::ParseSymbols (Symtab *symtab,
                              user_id_t start_id,
                              SectionList *section_list,
                              const size_t num_symbols,
                              const DataExtractor &symtab_data,
                              const DataExtractor &strtab_data)
for (i = 0; i < num_symbols; ++i)
         // symbol_value_offset may contain 0 for ARM symbols or -1 for
         // THUMB symbols. See above for more details.
         uint64_t symbol_value = symbol.st_value + symbol_value_offset;
         if (symbol_section_sp && CalculateType() != ObjectFile::Type::eTypeObjectFile)
             symbol_value -= symbol_section_sp->GetFileAddress();
 
         if (symbol_section_sp)
         {
             ModuleSP module_sp(GetModule());
             if (module_sp)
             {
                 SectionList *module_section_list = module_sp->GetSectionList();
                 if (module_section_list && module_section_list != section_list)
                 {
                     const ConstString &sect_name = symbol_section_sp->GetName();
                     lldb::SectionSP section_sp (module_section_list->FindSectionByName (sect_name));
                     if (section_sp && section_sp->GetFileSize())
                     {
for (i = 0; i < num_symbols; ++i)
                 mangled.SetMangledName( ConstString((mangled_name + suffix).str()) );
 
             ConstString demangled = mangled.GetDemangledName(lldb::eLanguageTypeUnknown);
             llvm::StringRef demangled_name = demangled.GetStringRef();
             if (!demangled_name.empty())
                 mangled.SetDemangledName( ConstString((demangled_name + suffix).str()) );
         }
 
+        /*
+         * 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
+
         Symbol dc_symbol(
             i + start_id,       // ID is the original symbol table index.
             mangled,
             symbol_type,        // Type of this symbol
             is_global,          // Is this globally visible?
             false,              // Is this symbol debug info?
             false,              // Is this symbol a trampoline?
             false,              // Is this symbol artificial?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

ProcessGDBRemote::EnableBreakpointSite (BreakpointSite *bp_site)
     Error error;
     assert(bp_site != NULL);
 
     // Get logging info
     Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_BREAKPOINTS));
     user_id_t site_id = bp_site->GetID();
 
     // Get the breakpoint address
-    const addr_t addr = bp_site->GetLoadAddress();
+    const addr_t load_addr = bp_site->GetLoadAddress();
+    addr_t addr = load_addr;
 
     // Log that a breakpoint was requested
     if (log)
         log->Printf("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64, site_id, (uint64_t)addr);
 
     // Breakpoint already exists and is enabled
     if (bp_site->IsEnabled())
     {
         if (log)
             log->Printf("ProcessGDBRemote::EnableBreakpointSite (size_id = %" PRIu64 ") address = 0x%" PRIx64 " -- SUCCESS (already enabled)", site_id, (uint64_t)addr);
         return error;
     }
 
+    /*
+     * 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);
+        }
+    }
+
     // Get the software breakpoint trap opcode size
     const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode(bp_site);
 
     // SupportsGDBStoppointPacket() simply checks a boolean, indicating if this breakpoint type
     // is supported by the remote stub. These are set to true by default, and later set to false
     // only after we receive an unimplemented response when sending a breakpoint packet. This means
     // initially that unless we were specifically instructed to use a hardware breakpoint, LLDB will
     // attempt to set a software breakpoint. HardwareRequired() also queries a boolean variable which
ProcessGDBRemote::DisableBreakpointSite (BreakpointSite *bp_site)
     Error error;
     assert (bp_site != NULL);
     addr_t addr = bp_site->GetLoadAddress();
     user_id_t site_id = bp_site->GetID();
     Log *log (ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_BREAKPOINTS));
     if (log)
         log->Printf ("ProcessGDBRemote::DisableBreakpointSite (site_id = %" PRIu64 ") addr = 0x%8.8" PRIx64, site_id, (uint64_t)addr);
 
+    /*
+     * 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())
     {
         const size_t bp_op_size = GetSoftwareBreakpointTrapOpcode (bp_site);
 
         BreakpointSite::Type bp_type = bp_site->GetType();
         switch (bp_type)
         {
         case BreakpointSite::eSoftware:

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (die->GetDIENamesAndRanges (this,
                                                decl_line, 
                                                decl_column));
 
             // Supply the type _only_ if it has already been parsed
             Type *func_type = m_die_to_type.lookup (die);
 
             assert(func_type == NULL || func_type != DIE_IS_BEING_PARSED);
 
+            /*
+             * 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()))
             {
                 const user_id_t func_user_id = MakeUserID(die->GetOffset());
                 func_sp.reset(new Function (sc.comp_unit,
                                             MakeUserID(func_user_id),       // UserID is the DIE offset
                                             MakeUserID(func_user_id),
                                             func_name,
                                             func_type,
SymbolFileDWARF::ParseImportedModules (const lldb_private::SymbolContext &sc, std::vector<lldb_private::ConstString> &imported_modules)
     }
     return false;
 }
 
 struct ParseDWARFLineTableCallbackInfo
 {
     LineTable* line_table;
     std::unique_ptr<LineSequence> sequence_ap;
 };
 
 //----------------------------------------------------------------------
 // ParseStatementTableCallback
 //----------------------------------------------------------------------
 static void
 ParseDWARFLineTableCallback(dw_offset_t offset, const DWARFDebugLine::State& state, void* userData)
 {
     if (state.row == DWARFDebugLine::State::StartParsingLineTable)
else
 
         // If this is our first time here, we need to create a
         // sequence container.
         if (!info->sequence_ap.get())
         {
             info->sequence_ap.reset(line_table->CreateLineSequenceContainer());
             assert(info->sequence_ap.get());
         }
         line_table->AppendLineEntryToSequence (info->sequence_ap.get(),
                                                state.address,
                                                state.line,
                                                state.column,
                                                state.file,
                                                state.is_stmt,
                                                state.basic_block,
                                                state.prologue_end,
                                                state.epilogue_begin,
                                                state.end_sequence);
         if (state.end_sequence)
         {
             // First, put the current sequence into the line table.
             line_table->InsertSequence(info->sequence_ap.get());
             // Then, empty it to prepare for the next sequence.
             info->sequence_ap->Clear();
         }
     }
 }
if (dwarf_cu)
             const dw_offset_t cu_line_offset = dwarf_cu_die->GetAttributeValueAsUnsigned(this, dwarf_cu, DW_AT_stmt_list, DW_INVALID_OFFSET);
             if (cu_line_offset != DW_INVALID_OFFSET)
             {
                 std::unique_ptr<LineTable> line_table_ap(new LineTable(sc.comp_unit));
                 if (line_table_ap.get())
                 {
                     ParseDWARFLineTableCallbackInfo info;
                     info.line_table = line_table_ap.get();
                     lldb::offset_t offset = cu_line_offset;
                     DWARFDebugLine::ParseStatementTable(get_debug_line_data(), &offset, ParseDWARFLineTableCallback, &info);
                     if (m_debug_map_symfile)
                     {
                         // We have an object file that has a line table with addresses
                         // that are not linked. We need to link the line table and convert
                         // the addresses that are relative to the .o file into addresses
                         // for the main executable.
                         sc.comp_unit->SetLineTable (m_debug_map_symfile->LinkOSOLineTable (this, line_table_ap.get()));