This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] microMIPS breakpoints, disassembly and compressed addresses
ClosedPublic

Authored by jaydeep on Aug 17 2015, 4:13 AM.

Details

Reviewers
clayborg
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

Diff Detail

Repository
rL LLVM

Event Timeline

jaydeep retitled this revision from to [MIPS] microMIPS breakpoints, disassembly and compressed addresses.Aug 17 2015, 4:13 AM
jaydeep updated this revision to Diff 32289.Aug 17 2015, 4:13 AM
jaydeep updated this object.
jaydeep added a reviewer: clayborg.
jaydeep set the repository for this revision to rL LLVM.
jaydeep added subscribers: lldb-commits, bhushan, slthakur and 2 others.
clayborg requested changes to this revision.Aug 17 2015, 10:44 AM
clayborg edited edge metadata.

Many changes. See inlined comments.

source/Core/Disassembler.cpp
1169–1187 ↗(On Diff #32289)

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 ↗(On Diff #32289)

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
2128–2142

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 ↗(On Diff #32289)

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 ↗(On Diff #32289)

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 ↗(On Diff #32289)

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.

This revision now requires changes to proceed.Aug 17 2015, 10:44 AM

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 updated this revision to Diff 33301.Aug 26 2015, 10:58 PM
jaydeep edited edge metadata.

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 updated this revision to Diff 33793.Sep 2 2015, 3:22 AM
jaydeep edited edge metadata.

Added GetCallableFileAddress for MIPS

clayborg requested changes to this revision.Sep 2 2015, 10:04 AM
clayborg edited edge metadata.

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

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
107–121

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

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.

This revision now requires changes to proceed.Sep 2 2015, 10:04 AM
jaydeep added inline comments.Sep 4 2015, 5:07 AM
source/Target/RegisterContext.cpp
107–121

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 updated this revision to Diff 34199.Sep 8 2015, 1:43 AM
jaydeep edited edge metadata.

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 requested changes to this revision.Sep 8 2015, 9:24 AM
clayborg edited edge metadata.

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

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
111–117

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

118–121

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);
This revision now requires changes to proceed.Sep 8 2015, 9:24 AM
jaydeep updated this revision to Diff 34292.Sep 8 2015, 9:08 PM
jaydeep edited edge metadata.

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 requested changes to this revision.Sep 9 2015, 2:51 PM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.Sep 9 2015, 2:51 PM

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?

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.

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

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?

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?

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.

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?

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

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?

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 updated this revision to Diff 34532.Sep 10 2015, 10:37 PM
jaydeep edited edge metadata.

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

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

clayborg requested changes to this revision.Sep 14 2015, 11:31 AM
clayborg edited edge metadata.

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.

This revision now requires changes to proceed.Sep 14 2015, 11:31 AM
jaydeep updated this revision to Diff 34782.Sep 14 2015, 10:35 PM
jaydeep edited edge metadata.

Addressed review comments

clayborg requested changes to this revision.Sep 15 2015, 2:01 PM
clayborg edited edge metadata.

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
1397

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.

1426–1436

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

1439

change this line to:

file_addr & info->addr_mask,
1479

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;
}
This revision now requires changes to proceed.Sep 15 2015, 2:01 PM
jaydeep updated this revision to Diff 34873.Sep 15 2015, 9:50 PM
jaydeep edited edge metadata.

Addressed review comments

clayborg requested changes to this revision.Sep 16 2015, 10:28 AM
clayborg edited edge metadata.

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
698–727

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.

This revision now requires changes to proceed.Sep 16 2015, 10:28 AM
jaydeep updated this revision to Diff 35059.Sep 17 2015, 9:25 PM
jaydeep edited edge metadata.

Addressed review comments.

clayborg accepted this revision.Sep 18 2015, 9:30 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Sep 18 2015, 9:30 AM