2012/12/22 Jeremy Bennett <[email protected]>:
> On Sat, 2012-12-22 at 01:00 +0100, Franck Jullien wrote:
>> I'm currently working on GDB updates.
>> The idea is to get rid of the readspr/writespr commands to access spr
>> registers.
>
> Hi Franck,
>
> This is a great piece of work. I've taken a first look through your
> patches, and here are some comments.
>
>> As a matter of fact those commands use qRcmd RSP packets to get data from
>> the GDB client but this is not compliant with the RSP protocol.
>
>> This is not a really big deal when you are working with openrisc specific
>> gdb clients versions. However, if we want to get our OpenOCD port to be
>> mainstream, we can't use this trick.
>
> Just being picky, but qRcmd is compliant with RSP - the whole point of
> qRcmd is to allow the client to make arbitrary communication with the
> server. I think the point is that it would be much better if we could
> avoid needing custom commands in the server. It also sounds like OpenOCD
> only supports a restricted subset of RSP.
I wasn't sure qRcmd can receive registers data. In the GDB manual,
one can read: "Before the final result packet, the target may also
respond with a number of intermediate `Ooutput' console output
packets.". However, we agree we should avoid using it.
>
> Where I have seen qRcmd used is to provide additional commands in the
> GDB client to access facilities like flashing an FPGA. Looks like those
> architectures would not be able to use OpenOCD.
>
>> This series, implement spr access using p/P RSP commands and registers
>> definition using remote target description file.
>
> I am very interested in this, because I'm working on another
> architecture with a large auxiliary register set. I hadn't heard of
> restricting the range of registers that GDB can access using g/G packets
> and using p/P packets for other registers.
>
In remote.c: "A 'g' reply that doesn't include a register's value
implies either that the register is not available, or that the 'p'
packet must be used"
Then after receiving a 'g' reply, in_g_packet value is updated
according to the size of the 'g' reply.
for (i = 0; i < gdbarch_num_regs (gdbarch); i++)
{
.....
if (rsa->regs[i].offset >= rsa->sizeof_g_packet)
rsa->regs[i].in_g_packet = 0;
else
rsa->regs[i].in_g_packet = 1;
}
}
> Have you run this idea past the GDB mailing list? While GDB generally
> uses g/G to cache registers, it sometimes uses p/P as well, so you'll
> need to make sure p/P still work with the GPRs.
I can't see why p/P GPR access wouldn't work anymore.
If the GDB community
> agree this works, I'd like to add it to the GDB internals manual to help
> future implementers of architectures with auxiliary registers.
>
>> This patch keeps compatibility with "classic" gdb clients.
>>
>> I already modified OpenOCD to send the target descriptor file and get/set
>> spr registers using p/P packets (https://github.com/Franck79/openOCD,
>> auto_tdesc branch, work in progress).
>>
>> As a matter of fact, OpenOCD generates the target descriptor file at run time
>> based on the target registers (we can still tells it tu use an existing file,
>> this is just a TCL config).
>> So adding a register (and handle it in gdb) is very easy. We could imagine a
>> simple TCL command to add a register definition if we don't want to recompile
>> OpenOCD nor GBD...
>
> What happens to GDB users who are not connecting to an OpenOCD target?
> Where do they get their register description? I see other architectures
> providing feature sets in the gdb/features directory. Should there be a
> fallback to allow a feature set XML file to be supplied to the client?
>
Yes I saw that two. I think it is a default targe description provided
here. However,
one can use "set tdesc filename" to provide a tdesc file to the client.
> One big omission. You haven't provided ChangeLog entries. GNU coding
> standards compliant ChangeLog's are an absolute must have if this code
> is ever to be committed to the FSF. OpenRISC specific entries should go
> into ChangeLog.or32 files, which are merged into the main ChangeLog for
> FSF submission.
>
> (I know the argument that git logs replace ChangeLogs, but a) the FSF
> set the rules and b) I've yet to see any git log usage with the
> discipline of GNU compliant ChangeLogs. The best approach is to write
> your ChangeLog entries and paste those into your git log).
>
Yeah I know. This series has been sent to get some feedback about the
way I took.
I'll take care of the documentation.
> On a secondary note, some of your patches don't seem to follow GNU
> layout rules for indentation. You can use the "indent" command to apply
> GNU rules automatically to your source.
>
This GNU coding style is not the prettiest thing I've ever seen :)
Is mixing white space and tab when starting a new line is part of the
coding style ?
I did try to copy the coding style while writing my code....
I'll try to do my best.
> Final thought. How do the regression results compare. If you get any
> tests that pass with the old approach and fail with the new, they will
> be important to investigate.
>
I've never run tests. Need to see how it works.
> Best wishes,
>
>
> Jeremy
>
>> Franck Jullien (3):
>> target-description: add tdesc/feature functions
>> or32: set gdbarch num regs to gpr + spr regs
>> or32: add target descriptor support
>>
>> gdb/or32-tdep.c | 160
>> +++++++++++++++++++++++++++++++++++++++++++--
>> gdb/or32-tdep.h | 4 +-
>> gdb/target-descriptions.c | 69 +++++++++++++++++++
>> gdb/target-descriptions.h | 24 +++++++
>> 4 files changed, 251 insertions(+), 6 deletions(-)
>>
>> _______________________________________________
>> Openrisc mailing list
>> [email protected]
>> http://lists.opencores.org/listinfo/openrisc
>
> --
> Tel: +44 (1590) 610184
> Cell: +44 (7970) 676050
> SkypeID: jeremybennett
> Email: [email protected]
> Web: www.embecosm.com
>
> _______________________________________________
> OpenRISC mailing list
> [email protected]
> http://lists.openrisc.net/listinfo/openrisc
_______________________________________________
OpenRISC mailing list
[email protected]
http://lists.openrisc.net/listinfo/openrisc
