[PATCH] The kalimba Platform plugin

Hi people,

As some of you are already aware I have been working towards a lldb port
which debugs CSRs Kalimba chips. I have recently added a PlatformKalimba
plugin. With some slight tweaking to the .note parsing section of the
ObjectFileELF and a few Makefiles, my lldb build now lists "kalimba" as
an available Platform and associates any kalimba binary Targets with this Platform.

Now I do not currently have submit access for lldb (though we are
planning on adding lldb buildbots and running the testsuites soon, so
perhaps this situation will change in the future), so I've built a patch
which comprises this work. The impact of the patch on the rest of lldb
is minimal, and since I am not running the testsuites yet, I have spent
some time testing my build against a linux binary, and can claim that
all the basic run, paused, step, backtrace, inspect variables commands
still work. Indeed the platform commands continue to work as expected
for the host linux platform.

So I'd appreciate if anyone could cast their eyes over this patch and
see if it looks ok to submit or if it is missing anything. I've based the PlatformKalimba on PlatformLinux and basically stripped out anything
which I don't think is immediately applicable. I've also removed any
copied comments, since for the most part they are historic, and in order
to try to keep the source reasonably self-documenting. I'll also add
that the platform is deliberately incomplete, and will be fleshed
subsequently. I hope I made no CMake mistakes, I have little experience with CMake, and so my CMakeLists.txt files are all based on copying from other CMakeLists.txts.

Anyway,
I look forward to your feedback.

thanks
Matthew Gardiner

Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, Qualcomm Technology News & Trends | Qualcomm, CSR people blog, www.csr.com/people, YouTube, CSR Decomission - YouTube, Facebook, Facebook, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.

PlatformKalimba.patch (21.7 KB)

Hi Mathew,
I had a quick look and nothing jumped at me. Although I think you are missing call
to PlatformKalimba::Terminate().

Regards,
Abid

Abid, Hafiz wrote:

Hi Mathew,
I had a quick look and nothing jumped at me. Although I think you are missing call
to PlatformKalimba::Terminate().

Regards,
Abid

Thanks for the review, Abid.

I've just attached a corrected patch with the call to PlatformKalimba::Terminate() made from lldb_private::Terminate().

thanks
Matt

Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, Qualcomm Technology News & Trends | Qualcomm, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, Facebook, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.

PlatformKalimba2.patch (21.9 KB)

A few questions:

In PlatformKalimba::GetProcessInfo() the code is:

bool
PlatformKalimba::GetProcessInfo (lldb::pid_t pid, ProcessInstanceInfo &process_info)
{
    bool success = false;
    if (IsHost())
    {
        success = Platform::GetProcessInfo (pid, process_info);
    }
    else
    {
        if (m_remote_platform_sp)
            success = m_remote_platform_sp->GetProcessInfo (pid, process_info);
    }
    return success;
}

Can "kalimba" processes actually run locally on your host machine as native apps? I would guess that PlatformKalimba is for remote debugging only and would expect your code to return false if "IsHost()" is true in that case. If you are running a simulator that uses local processes on the host machine, then this is correct.

In your GetSupportedArchitectureAtIndex you should only return true if "idx == 0":

bool
PlatformKalimba::GetSupportedArchitectureAtIndex (uint32_t idx, ArchSpec &arch)
{
    if (idx == 0)
    {
        arch = ArchSpec("kalimba-csr-unknown");
        return true;
    }
    return false;
}

GetSupportedArchitectureAtIndex gets used when you select your platform and then load an executable. It will search for valid architectures by iterating through the architectures using an increasing index. This is needed for cases like iOS where the iOS device might be "armv7s", but when we load a binary, we will start with "armv7s" and then back up to looking for "armv7", armv6", etc.

You will need to fill out:

size_t
PlatformDarwin::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
{
}

You mainly need to figure out which opcode your target will use as a breakpoint instruction and return its size and fill in the bytes for the instruction, see PlatformDarwin::GetSoftwareBreakpointTrapOpcode() for an example.

In PlatformKalimba::LaunchProcess() you end up launching a host process after checking IsHost(), is this your intention? Are you running a simulator? Same thing for PlatformKalimba::Attach().

Greg

Greg Clayton wrote:

A few questions:

In PlatformKalimba::GetProcessInfo() the code is:

bool
PlatformKalimba::GetProcessInfo (lldb::pid_t pid, ProcessInstanceInfo &process_info)
{
     bool success = false;
     if (IsHost())
     {
         success = Platform::GetProcessInfo (pid, process_info);
     }
     else
     {
         if (m_remote_platform_sp)
             success = m_remote_platform_sp->GetProcessInfo (pid, process_info);
     }
     return success;
}

Can "kalimba" processes actually run locally on your host machine as native apps? I would guess that PlatformKalimba is for remote debugging only and would expect your code to return false if "IsHost()" is true in that case. If you are running a simulator that uses local processes on the host machine, then this is correct.

We can't run kalimba ELFs on the host. We do have a simulator (which acts on a file resulting from turning an ELF into a bespoke target memory view) to run on the host, but we'd connect to the sim, in the "same" way as a real device, except that the CSR-type SPI wire packets, move over TCP/IP not on a real wire as such. So debugging kalimba on a simulator is still remote debugging. I'll modify my implementation as follows:

      bool success = false;
      if (IsHost())
      {
- success = Platform::GetProcessInfo (pid, process_info);
+ success = false;
      }
      else
      {

which I believe to be your suggestion.

In your GetSupportedArchitectureAtIndex you should only return true if "idx == 0":

bool
PlatformKalimba::GetSupportedArchitectureAtIndex (uint32_t idx, ArchSpec &arch)
{
     if (idx == 0)
     {
         arch = ArchSpec("kalimba-csr-unknown");
         return true;
     }
     return false;
}

GetSupportedArchitectureAtIndex gets used when you select your platform and then load an executable. It will search for valid architectures by iterating through the architectures using an increasing index. This is needed for cases like iOS where the iOS device might be "armv7s", but when we load a binary, we will start with "armv7s" and then back up to looking for "armv7", armv6", etc.

Ok, thanks for this, I'll modify my next patch attempt accordingly.

You will need to fill out:

size_t
PlatformDarwin::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
{
}

You mainly need to figure out which opcode your target will use as a breakpoint instruction and return its size and fill in the bytes for the instruction, see PlatformDarwin::GetSoftwareBreakpointTrapOpcode() for an example.

Software breakpoints aren't properly supported on kalimba. We did have them in the earlier variants, but I'm told that these never worked well: there were issues in the pipeline implementation and so on.

So I think that my original implementation of "return 0" was correct.

Kalimba architectures do, however, support setting up to 8 hardware breakpoints. That is, we have dedicated debug registers in the hardware.

By the way what sort of thing should PlatformKalimba do for CalculateTrapHandlerSymbolNames? Should that be a no-op, not a m_trap_handlers.push_back (ConstString ("_sigtramp"));?

In PlatformKalimba::LaunchProcess() you end up launching a host process after checking IsHost(), is this your intention? Are you running a simulator?

Ok. So is it correct to merely do something like this:

     if (IsHost())
     {
         error.SetErrorString ("native execution is not possible");
     }

?

Same thing for PlatformKalimba::Attach().

Ok. So I'll make a similar change to the one made to LaunchProcess.

In response to Greg's review and my current thoughts I'm attaching the latest version of the PlatformKalimba.cpp. Is this file now a good enough first attempt? Or are there any further areas to complete?

In summary I've

1. Modified GetProcessInfo to return false if IsHost()==true.
2. Implemented GetSupportedArchitectureAtIndex as per Greg's suggestion.
3. Modified LaunchProcess and Attach to merely set up an error if IsHost()==true.
4. Retained my previous implementation for GetSoftwareBreakpointTrapOpcode with reasoning.
5. Remove m_trap_handlers.push_back (ConstString ("_sigtramp")); from CalculateTrapHandlerSymbolNames.

I look forward to further comment.

thanks
Matt

Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, Qualcomm Technology News & Trends | Qualcomm, CSR people blog, www.csr.com/people, YouTube, CSR Decomission - YouTube, Facebook, Facebook, or follow us on Twitter at www.twitter.com/CSR_plc.
New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.

PlatformKalimba.cpp (9.86 KB)

Greg Clayton wrote:

A few questions:

In PlatformKalimba::GetProcessInfo() the code is:

bool
PlatformKalimba::GetProcessInfo (lldb::pid_t pid, ProcessInstanceInfo &process_info)
{
    bool success = false;
    if (IsHost())
    {
        success = Platform::GetProcessInfo (pid, process_info);
    }
    else
    {
        if (m_remote_platform_sp)
            success = m_remote_platform_sp->GetProcessInfo (pid, process_info);
    }
    return success;
}

Can "kalimba" processes actually run locally on your host machine as native apps? I would guess that PlatformKalimba is for remote debugging only and would expect your code to return false if "IsHost()" is true in that case. If you are running a simulator that uses local processes on the host machine, then this is correct.

We can't run kalimba ELFs on the host. We do have a simulator (which acts on a file resulting from turning an ELF into a bespoke target memory view) to run on the host, but we'd connect to the sim, in the "same" way as a real device, except that the CSR-type SPI wire packets, move over TCP/IP not on a real wire as such. So debugging kalimba on a simulator is still remote debugging. I'll modify my implementation as follows:

    bool success = false;
    if (IsHost())
    {
- success = Platform::GetProcessInfo (pid, process_info);
+ success = false;
    }
    else
    {

which I believe to be your suggestion.

In your GetSupportedArchitectureAtIndex you should only return true if "idx == 0":

bool
PlatformKalimba::GetSupportedArchitectureAtIndex (uint32_t idx, ArchSpec &arch)
{
    if (idx == 0)
    {
        arch = ArchSpec("kalimba-csr-unknown");
        return true;
    }
    return false;
}

GetSupportedArchitectureAtIndex gets used when you select your platform and then load an executable. It will search for valid architectures by iterating through the architectures using an increasing index. This is needed for cases like iOS where the iOS device might be "armv7s", but when we load a binary, we will start with "armv7s" and then back up to looking for "armv7", armv6", etc.

Ok, thanks for this, I'll modify my next patch attempt accordingly.

You will need to fill out:

size_t
PlatformDarwin::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
{
}

You mainly need to figure out which opcode your target will use as a breakpoint instruction and return its size and fill in the bytes for the instruction, see PlatformDarwin::GetSoftwareBreakpointTrapOpcode() for an example.

Software breakpoints aren't properly supported on kalimba. We did have them in the earlier variants, but I'm told that these never worked well: there were issues in the pipeline implementation and so on.

So I think that my original implementation of "return 0" was correct.

Kalimba architectures do, however, support setting up to 8 hardware breakpoints. That is, we have dedicated debug registers in the hardware.

By the way what sort of thing should PlatformKalimba do for CalculateTrapHandlerSymbolNames? Should that be a no-op, not a m_trap_handlers.push_back (ConstString ("_sigtramp"));?

If there are functions that can asynchronously interrupt any function at any instruction, you should list them. Why? Because stack backtracking rules get broken if you don't. You can just leave them empty if you don't know.

In PlatformKalimba::LaunchProcess() you end up launching a host process after checking IsHost(), is this your intention? Are you running a simulator?

Ok. So is it correct to merely do something like this:

   if (IsHost())
   {
       error.SetErrorString ("native execution is not possible");
   }

?

Same thing for PlatformKalimba::Attach().

Ok. So I'll make a similar change to the one made to LaunchProcess.

Sounds good.

In response to Greg's review and my current thoughts I'm attaching the latest version of the PlatformKalimba.cpp. Is this file now a good enough first attempt? Or are there any further areas to complete?

In summary I've

1. Modified GetProcessInfo to return false if IsHost()==true.
2. Implemented GetSupportedArchitectureAtIndex as per Greg's suggestion.
3. Modified LaunchProcess and Attach to merely set up an error if IsHost()==true.
4. Retained my previous implementation for GetSoftwareBreakpointTrapOpcode with reasoning.
5. Remove m_trap_handlers.push_back (ConstString ("_sigtramp")); from CalculateTrapHandlerSymbolNames.

Should be good to go after your above changes.