[Bug 1001524] New: Cortex-M: Remote 'g' packet reply is too long

[Bug 1001524] New: Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

           Summary: Cortex-M: Remote 'g' packet reply is too long
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: normal
         Component: GDB stubs
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: john@dallaway.org.uk
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


When connecting to the Cortex-M GDB stub from recent versions of GDB, an error
is observed:

> (gdb) tar rem /dev/ttyS0
> Remote debugging using /dev/ttyS0
> Remote 'g' packet reply is too long: e14e000810000000000000001000000000000000000000000000000000000000000000000000000000000000fccf0d6800000000e8cf0d6895680008e24e00080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000021
> (gdb)

This occurs because the default register set for ARM M-profile targets has been
reduced (FPA registers have been removed) in recent GDB.

We should add a CDL option to the Cortex-M architectural HAL which allows the
stub to return the register set with or without the FPA registers.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #1 from John Dallaway <john@dallaway.org.uk> 2012-03-07 17:04:23 GMT ---
Created an attachment (id=1624)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=1624)
gdb-tdesc-cortexm-fpa-120307.xml

This target description file provides a workaround by forcing GDB to expect the
longer 'g' packet reply returned by the existing eCos GDB stub. Usage:

(gdb) set tdesc filename gdb-tdesc-cortexm-fpa.xml

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ilijak@siva.com.mk

--- Comment #2 from Ilija Kocho <ilijak@siva.com.mk> 2012-03-08 08:00:55 GMT ---
(In reply to comment #0)
> When connecting to the Cortex-M GDB stub from recent versions of GDB, an error
> is observed:
> 
> > (gdb) tar rem /dev/ttyS0
> > Remote debugging using /dev/ttyS0
> > Remote 'g' packet reply is too long: e14e000810000000000000001000000000000000000000000000000000000000000000000000000000000000fccf0d6800000000e8cf0d6895680008e24e00080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000021
> > (gdb)
> 
> This occurs because the default register set for ARM M-profile targets has been
> reduced (FPA registers have been removed) in recent GDB.
> 
> We should add a CDL option to the Cortex-M architectural HAL which allows the
> stub to return the register set with or without the FPA registers.

Cortex-M arch. HAL has CDL that indicate presence of FPU
CYGINT_HAL_CORTEXM_FPU, CYGHWR_HAL_CORTEXM_FPU that we may be used if we're
happy sending FPU registers always when FPU is present.

Once we have FPU it is likely to be VFP rather than FPA. More precisely for
Cortex-M4 it is FPv4-SP-D16
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0439c/BEHFGGGE.html

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jifl@ecoscentric.com

--- Comment #3 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 03:58:46 GMT ---
A CDL option approach isn't ideal. You should not have to match programmed
stubs to particular versions of tools. You can't use certain boards with
particular stub versions with certain versions of GDB. Stubs are meant to be
persistent and long-lived and independent of toolchain versions.

I would prefer to fix GDB, hence this:

http://sourceware.org/ml/gdb-patches/2010-11/msg00142.html

(and you can see the rest of the thread for more rationale).

However it looks like the discussion died off and I didn't chase it up. I will
do so now.

There's nothing to stop an updated version of my patch going into the tools
being respun though - my GDB contributions are covered by an FSF assignment.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #4 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 04:29:32 GMT ---
(In reply to comment #3)
> However it looks like the discussion died off and I didn't chase it up. I will
> do so now.
> 
> There's nothing to stop an updated version of my patch going into the tools
> being respun though - my GDB contributions are covered by an FSF assignment.

And you can find an updated patch here, albeit against GDB trunk. It may apply
ok to 7.4 though - I haven't tried.

http://sourceware.org/ml/gdb-patches/2012-03/msg00320.html

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #5 from John Dallaway <john@dallaway.org.uk> 2012-03-09 09:18:19 GMT ---
Hi Jifl

(In reply to comment #3)

> A CDL option approach isn't ideal. You should not have to match programmed
> stubs to particular versions of tools. You can't use certain boards with
> particular stub versions with certain versions of GDB. Stubs are meant to be
> persistent and long-lived and independent of toolchain versions.

I agree. However, reducing the length of the 'g' packet reply is a good thing
in the long term. Especially when debugging multi-threaded code using an IDE
over a slow serial connection. IDEs tend to fetch the register sets for all
threads when single stepping.

> I would prefer to fix GDB, hence this:
> 
> http://sourceware.org/ml/gdb-patches/2010-11/msg00142.html
> 
> (and you can see the rest of the thread for more rationale).
> 
> However it looks like the discussion died off and I didn't chase it up. I will
> do so now.
> 
> There's nothing to stop an updated version of my patch going into the tools
> being respun though - my GDB contributions are covered by an FSF assignment.

I hope your patch is accepted.

I still think we should move with the times and have a CDL option to support
the new 'g' packet reply length in the Cortex-M stub. This would be mainly for
performance reasons, but also to support builds of GDB 7.3 and 7.4 without your
patch (eg CodeSourcery tools). The option could default to the original
behaviour, at least initially. We could change the default to the new behaviour
when most people have moved to GDB 7.3 or later. People who must stick with GDB
6.8.50.x in the long term should be able to use a target description file.

Do you see any issues with this dual approach?

<aside>
In the long term, we may also wish to support the Cortex-M4 VFP registers in
the stub.
</aside>

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #6 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-09 17:15:26 GMT ---
(In reply to comment #5)
> Hi Jifl
> 
> (In reply to comment #3)
> 
> > A CDL option approach isn't ideal. You should not have to match programmed
> > stubs to particular versions of tools. You can't use certain boards with
> > particular stub versions with certain versions of GDB. Stubs are meant to be
> > persistent and long-lived and independent of toolchain versions.
> 
> I agree. However, reducing the length of the 'g' packet reply is a good thing
> in the long term. Especially when debugging multi-threaded code using an IDE
> over a slow serial connection. IDEs tend to fetch the register sets for all
> threads when single stepping.

Sorry yes, I wasn't entirely clear here, even though I made this point in my
gdb-patches mail - by using this patch we can migrate in a more organised
manner, rather than having to use CDL to match stubs to tools. The CDL can go
in, defaulting to the present (longer) format, and a bit later on we can throw
the switch.

That was my intention at the time I wrote the patch, but then the issue got
forgotten about in GDB land which led me to forget the eCos side too.

> I still think we should move with the times and have a CDL option to support
> the new 'g' packet reply length in the Cortex-M stub. This would be mainly for
> performance reasons, but also to support builds of GDB 7.3 and 7.4 without your
> patch (eg CodeSourcery tools). The option could default to the original
> behaviour, at least initially. We could change the default to the new behaviour
> when most people have moved to GDB 7.3 or later. People who must stick with GDB
> 6.8.50.x in the long term should be able to use a target description file.
> 
> Do you see any issues with this dual approach?

As above, no - it was my original intention. I even made a patch at some point
(can't find it now). But if the GDB patch isn't accepted, I suggest we patch
our own GDB this time round regardless. I'd rather the patch was accepted
though, particularly if they would prefer an alternative solution to the same
problem, or if they spot I've missed something. I greatly prefer we should stay
aligned to upstream GDB (or keep upstream GDB aligned to us ;-)), indeed
because of other tools like Codesourcery's.

It looks like I'll be merging my patch with one come up with independently by
Pedro Alves. He's a GDB maintainer too, so chances are high now that it will go
in.

> <aside>
> In the long term, we may also wish to support the Cortex-M4 VFP registers in
> the stub.
> </aside>

That's a given - not only would stubs returning VFP registers be newer stubs so
no backward compatibility problems, but they inherently have to use a
differently formatted 'g' register anyway.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #7 from Ilija Kocho <ilijak@siva.com.mk> 2012-03-10 19:00:31 GMT ---
I am working on Cortex-M4 architectural FPU support. Having VFP registers in
GDB would be of great help, so propose that we synchronize our work and get VFP
support to Cortex-M stub.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #8 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-11 00:58:46 GMT ---
(In reply to comment #7)
> I am working on Cortex-M4 architectural FPU support. Having VFP registers in
> GDB would be of great help, so propose that we synchronize our work and get VFP
> support to Cortex-M stub.

No need. The changes here should only affect those processors without FPU
support.

If the program has been built with VFP registers, then an ELF attribute gets
set in the executable marking it as such. This makes GDB treat it differently
to the situation described in this bugzilla bug.

Or at least, that's what's meant to happen. If it doesn't, GDB'll want to be
fixed until it does.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #9 from Ilija Kocho <ilijak@siva.com.mk> 2012-03-11 13:06:41 GMT ---
(In reply to comment #8)
> (In reply to comment #7)
> > I am working on Cortex-M4 architectural FPU support. Having VFP registers in
> > GDB would be of great help, so propose that we synchronize our work and get VFP
> > support to Cortex-M stub.
> 
> No need. The changes here should only affect those processors without FPU
> support.
> 
> If the program has been built with VFP registers, then an ELF attribute gets
> set in the executable marking it as such. This makes GDB treat it differently
> to the situation described in this bugzilla bug.
> 
> Or at least, that's what's meant to happen. If it doesn't, GDB'll want to be
> fixed until it does.

Hi Jifl

Here's my rationale (though I'm not initiated in GDB stuff so it may be
incomplete).
In Cortex-M architecture HAL there are files cortexm_stub.c and cortexm_stun.h.
I assume that they provide support to GDB. There are structures with members
that correspond to FPA registers - i guess due to state of matter at moment of
eCos initial port to Cortex-M3. At present the Cortex-M floating point
architecture is defined and implemented in silicon, at least for Cortex-M4F,
and it is VFP. I realize that we have to replace FPA with VFP in cortexm_stup.*
structures and code.

Another issue is the thread context that needs to (also) keep VFP registers
when available and in use. This is what I am starting to work on. In my view
GDB stub and thread context as jobs do have common points and it would be good
to synchronize them.

Comments appreciated.
Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #10 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-11 17:21:40 GMT ---
(In reply to comment #9)
> In Cortex-M architecture HAL there are files cortexm_stub.c and cortexm_stun.h.
> I assume that they provide support to GDB. There are structures with members
> that correspond to FPA registers - i guess due to state of matter at moment of
> eCos initial port to Cortex-M3. At present the Cortex-M floating point
> architecture is defined and implemented in silicon, at least for Cortex-M4F,
> and it is VFP. I realize that we have to replace FPA with VFP in cortexm_stup.*
> structures and code.

Correct. However for a while, to support older GDBs, we will have to return
fake FPA registers (even though they don't really exist). So we have to _add_
VFP support and use it exclusively for those Cortex-M's which have it; but for
everything else, we have to retain and use this "fake FPA". For now anyway.
Later on once newer tools are well established, we flip the switch and make the
default be to drop the fake FPA registers.

> Another issue is the thread context that needs to (also) keep VFP registers
> when available and in use. This is what I am starting to work on. In my view
> GDB stub and thread context as jobs do have common points and it would be good
> to synchronize them.

You would be well advised to look at other architectural HALs which have
hardware FPU support and see what they do, especially our i386 HAL. Lazy FPU
context switching is a practical necessity.

It's only possible to synchronise GDB stub and HAL saved thread context if that
is straightforward. One thing which should *not* happen is to make any changes
which increase the context switch time in order to make the layout match the
GDB register layout (which, of course, is not in our power to change). Real
performance should not be compromised because of the requirements of a
debugger.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #11 from Jonathan Larmour <jifl@ecoscentric.com> 2012-03-30 16:53:58 BST ---
(In reply to comment #42 of bug #1001468)
> Now I have enough Cortex-M4F architectural support to do some GDB testing. I
> have applied the attachment (id=1648) and it seems to work

Yay. And indeed there's the s0-s31 registers as expected.


> with following issues:

Boo.

> 1. Remote 'g' packet reply is too long
> 
> If I connect to gdb server (RedBoot on Cortex-M4F) prior to submit the elf file
> I get the error /Remote 'g' packet reply is too long/:
[snip]

> This is probably not a problem when using gdb from command line, but it is with
> Eclipse.

So you find that Eclipse connects via GDB before providing a file to GDB? That
seems wrong to me, for exactly the reason we need it not to. What's more, I
believe with at least our (eCosCentric's) eclipse, it behaves what I consider
the correct way and doesn't connect until asked to by which point it has the
file to debug. But I don't think we did anything special to do it in that
order. So are you sure this is Eclipse's behaviour? If it didn't do it that way
with our eclipse, then we would also see problems when debugging normal (not
VFP) Cortex-M binaries, as that also uses the provided binary to work out what
to do, using the previous patch (the one I checked in to GDB recently).

> 2. Potential eCos/RedBoot issues
> 
> Since register numbers are not consecutive there may be issue with some stub
> eCos/Redboot functions: For instance stub_format_registers() requires
> consecutive numbers.

I don't think I see a real problem there. cortex-M, like many HALs, provides
its own get_register() in cortexm_stub.c. So you can just return 0s for invalid
registers. Or arguably, an improvement might apparently be to define
CYGHWR_REGISTER_VALIDITY_CHECKING in cortexm_stub.h and provide a
get_register_valid() implementation. But that probably isn't worth the effort
since either way the user should never see the result.

I presume you've already been making changes in this area anyway since you'll
have needed to change target_register_t to an unsigned long long if there's an
FPU (and stick with unsigned long if there isn't, I hope!).

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #12 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-01 12:35:40 BST ---
(In reply to comment #11)
> (In reply to comment #42 of bug #1001468)
> > Now I have enough Cortex-M4F architectural support to do some GDB testing. I
> > have applied the attachment (id=1648) [details] and it seems to work
> 
> Yay. And indeed there's the s0-s31 registers as expected.

Indeed.

> 
> > with following issues:
> 
> Boo.
> 
> > 1. Remote 'g' packet reply is too long
> > 
> > If I connect to gdb server (RedBoot on Cortex-M4F) prior to submit the elf file
> > I get the error /Remote 'g' packet reply is too long/:
> [snip]
> 
> > This is probably not a problem when using gdb from command line, but it is with
> > Eclipse.
> 
> So you find that Eclipse connects via GDB before providing a file to GDB? That
> seems wrong to me, for exactly the reason we need it not to. What's more, I
> believe with at least our (eCosCentric's) eclipse, it behaves what I consider
> the correct way and doesn't connect until asked to by which point it has the
> file to debug. But I don't think we did anything special to do it in that
> order. So are you sure this is Eclipse's behaviour? 

Well my statement was little bit too general. It's actually "C/C++GDB Hardware
Debugging" plugin that comes bundled with (some configurations of) Eclipse CDT.
When I tried to debug with Eclipse I got /Remote 'g' packet reply is too long:/
and this was the best explanation.

Now I got old good Zylin plugin, where I can manually set commands, and it
works for me (and confirms my doubt about the other one).

What matters, it's a non-issue for your GDB patch.

[snip]

> 
> > 2. Potential eCos/RedBoot issues
> > 
> > Since register numbers are not consecutive there may be issue with some stub
> > eCos/Redboot functions: For instance stub_format_registers() requires
> > consecutive numbers.
> 
> I don't think I see a real problem there. cortex-M, like many HALs, provides
> its own get_register() in cortexm_stub.c. So you can just return 0s for >invalid

I guess you mean REGSIZE() macro. I was about to ask about that as it seemed as
a way, though inefficient one. Now have I tried and looks OK. I'm happy for the
time being.

> registers. Or arguably, an improvement might apparently be to define
> CYGHWR_REGISTER_VALIDITY_CHECKING in cortexm_stub.h and provide a
> get_register_valid() implementation. But that probably isn't worth the effort
> since either way the user should never see the result.

This would probably be even less efficient by wasting bandwidth for XXXes.

> 
> I presume you've already been making changes in this area anyway since you'll
> have needed to change target_register_t to an unsigned long long if there's an
> FPU (and stick with unsigned long if there isn't, I hope!).

I keep target_register_t cyg_uint32 with defined TARGET_HAS_LARGE_REGISTERS
It's still a development code but, now that we have STM 32F4 on board, I could
put it somewhere if it could speed up the development. (FYI - Lazy context
switching is implemented.)

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #13 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-02 21:49:32 BST ---
(In reply to comment #12)
> 
> Well my statement was little bit too general. It's actually "C/C++GDB Hardware
> Debugging" plugin that comes bundled with (some configurations of) Eclipse CDT.
> When I tried to debug with Eclipse I got /Remote 'g' packet reply is too long:/
> and this was the best explanation.
> 
> Now I got old good Zylin plugin, where I can manually set commands, and it
> works for me (and confirms my doubt about the other one).
> 
> What matters, it's a non-issue for your GDB patch.

Okay. So just to be absolutely clear, do you think you have tested GDB enough
that I should go ahead and submit this patch to upstream GDB? Or should I wait
a bit longer?

> > > 2. Potential eCos/RedBoot issues
> > > 
> > > Since register numbers are not consecutive there may be issue with some stub
> > > eCos/Redboot functions: For instance stub_format_registers() requires
> > > consecutive numbers.
> > 
> > I don't think I see a real problem there. cortex-M, like many HALs, provides
> > its own get_register() in cortexm_stub.c. So you can just return 0s for >invalid
> 
> I guess you mean REGSIZE() macro. I was about to ask about that as it seemed as
> a way, though inefficient one. Now have I tried and looks OK. I'm happy for the
> time being.


Actually I did mean get_register(), which here comes from cortexm_stub.c.
That's what stub_format_registers() calls (if !TARGET_HAS_LARGE_REGISTERS). My
idea was to simply check the register number. However, using
TARGET_HAS_LARGE_REGISTERS as you say you have done means in fact this is
irrelevant.

> > registers. Or arguably, an improvement might apparently be to define
> > CYGHWR_REGISTER_VALIDITY_CHECKING in cortexm_stub.h and provide a
> > get_register_valid() implementation. But that probably isn't worth the effort
> > since either way the user should never see the result.
> 
> This would probably be even less efficient by wasting bandwidth for XXXes.

Well, the bandwidth will be exactly the same, but this is irrelevant now.

> I keep target_register_t cyg_uint32 with defined TARGET_HAS_LARGE_REGISTERS
> It's still a development code but, now that we have STM 32F4 on board, I could
> put it somewhere if it could speed up the development. (FYI - Lazy context
> switching is implemented.)

Yes, using TARGET_HAS_LARGE_REGISTERS means not having to worry about
target_register_t's size. In fact I now see that's what's done elsewhere for
FPU regs.

Feel free to put up the code, but it's not likely I'd be able to work on
adapting it for the F4 straight away - we're incredibly busy here at
eCosCentric at the moment. I may be able to visually run through the patch
though just to look for any potential problems.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #14 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-02 23:18:07 BST ---
(In reply to comment #13)
> (In reply to comment #12)
> > 
> > Well my statement was little bit too general. It's actually "C/C++GDB Hardware
> > Debugging" plugin that comes bundled with (some configurations of) Eclipse CDT.
> > When I tried to debug with Eclipse I got /Remote 'g' packet reply is too long:/
> > and this was the best explanation.
> > 
> > Now I got old good Zylin plugin, where I can manually set commands, and it
> > works for me (and confirms my doubt about the other one).
> > 
> > What matters, it's a non-issue for your GDB patch.
> 
> Okay. So just to be absolutely clear, do you think you have tested GDB enough
> that I should go ahead and submit this patch to upstream GDB? Or should I wait
> a bit longer?

Please hold on. My stub is still less then perfect so I can't confirm 100%
functionality (please see issues below). I hope to fix it in a couple of days.

> 
> > > > 2. Potential eCos/RedBoot issues
> > > > 
> > > > Since register numbers are not consecutive there may be issue with some stub
> > > > eCos/Redboot functions: For instance stub_format_registers() requires
> > > > consecutive numbers.
> > > 
> > > I don't think I see a real problem there. cortex-M, like many HALs, provides
> > > its own get_register() in cortexm_stub.c. So you can just return 0s for >invalid
> > 
> > I guess you mean REGSIZE() macro. I was about to ask about that as it seemed as
> > a way, though inefficient one. Now have I tried and looks OK. I'm happy for the
> > time being.
> 
> 
> Actually I did mean get_register(), which here comes from cortexm_stub.c.
> That's what stub_format_registers() calls (if !TARGET_HAS_LARGE_REGISTERS). My
> idea was to simply check the register number. However, using
> TARGET_HAS_LARGE_REGISTERS as you say you have done means in fact this is
> irrelevant.

I am little-bit confused since get_register() returns value not address.
Returned value for non-valid register is -1 not 0. Do i miss something?

I am asking this because last few FPU registers d13..d15 are not being
transfered and I haven't proper explanation. I hope you can give me some
insight.

Here are data definitions:
-------------------
cortexm_stubs.h:

enum regnames {
    R0, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, R11, R12, SP, LR, PC,
    XPSR = 25,
    D0 = 26, D1, D2, D3, D4, D5, D6, D7, D8, D9, D10, D11, D12, D13, D14, D15,
    FPSCR
};

#define NUMREFS (FPSCR + 1) // This counts FPA registers (16 .. 24)

#define REGSIZE( _x_ ) (_x_ <= PC ? 4 : \
                        (_x_ < XPSR ? 0 : \
                         (_x_ == XPSR ? 4 : \
                          (((_x_ >= D0) && (_x_ <= D15)) ? 8 : \
                           (_x_ == FPSCR ? 4 : 0 )))))


---------------------
hal_arch.h:

// Register layout expected by GDB VFP
typedef struct
{
    cyg_uint32 gpr[16];
    cyg_uint32 xpsr;
    cyg_uint32 s[32];
    cyg_uint32 fpscr;
} HAL_CORTEXM_GDB_Registers;

----------------------

Is above OK?

> 
> > > registers. Or arguably, an improvement might apparently be to define
> > > CYGHWR_REGISTER_VALIDITY_CHECKING in cortexm_stub.h and provide a
> > > get_register_valid() implementation. But that probably isn't worth the effort
> > > since either way the user should never see the result.
> > 
> > This would probably be even less efficient by wasting bandwidth for XXXes.
> 
> Well, the bandwidth will be exactly the same, but this is irrelevant now.
> 
> > I keep target_register_t cyg_uint32 with defined TARGET_HAS_LARGE_REGISTERS
> > It's still a development code but, now that we have STM 32F4 on board, I could
> > put it somewhere if it could speed up the development. (FYI - Lazy context
> > switching is implemented.)
> 
> Yes, using TARGET_HAS_LARGE_REGISTERS means not having to worry about
> target_register_t's size. In fact I now see that's what's done elsewhere for
> FPU regs.
> 
> Feel free to put up the code, but it's not likely I'd be able to work on
> adapting it for the F4 straight away - we're incredibly busy here at
> eCosCentric at the moment. I may be able to visually run through the patch
> though just to look for any potential problems.

Ideally the variant/platform will only need to implement
CYGINT_HAL_CORTEXM_FPU.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #15 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-03 00:37:21 BST ---
(In reply to comment #14)
> > Actually I did mean get_register(), which here comes from cortexm_stub.c.
> > That's what stub_format_registers() calls (if !TARGET_HAS_LARGE_REGISTERS). My
> > idea was to simply check the register number. However, using
> > TARGET_HAS_LARGE_REGISTERS as you say you have done means in fact this is
> > irrelevant.
> 
> I am little-bit confused since get_register() returns value not address.
> Returned value for non-valid register is -1 not 0. Do i miss something?

Ah, I've worked out we've been talking at cross-purposes. I had misunderstood
what you were asking about and as a result got myself mixed up about what bit
you were referring to. Okay, start over, reboot, forget previous thread of
conversation...

You say you are using TARGET_HAS_LARGE_REGISTERS. This is fine. You are right
that stub_format_registers() cannot skip registers. We need it to skip
registers (ditto stub_update_registers()). We will probably need to add a hook
in generic-stub.c for this which cortexm_stub.h can define. Perhaps something
like: 

#ifdef TARGET_G_PACKET_SKIPS_REGS
    if ( !reg_in_g_packet( reg ) )
        continue;
#endif


> I am asking this because last few FPU registers d13..d15 are not being
> transfered and I haven't proper explanation. I hope you can give me some
> insight.
> 
> Here are data definitions:
> -------------------
> cortexm_stubs.h:
> 
> enum regnames {
>     R0, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, R11, R12, SP, LR, PC,
>     XPSR = 25,
>     D0 = 26, D1, D2, D3, D4, D5, D6, D7, D8, D9, D10, D11, D12, D13, D14, D15,
>     FPSCR
> };
> 
> #define NUMREFS (FPSCR + 1) // This counts FPA registers (16 .. 24)
> 
> #define REGSIZE( _x_ ) (_x_ <= PC ? 4 : \
>                         (_x_ < XPSR ? 0 : \
>                          (_x_ == XPSR ? 4 : \
>                           (((_x_ >= D0) && (_x_ <= D15)) ? 8 : \
>                            (_x_ == FPSCR ? 4 : 0 )))))
> 
> 
> ---------------------
> hal_arch.h:
> 
> // Register layout expected by GDB VFP
> typedef struct
> {
>     cyg_uint32 gpr[16];
>     cyg_uint32 xpsr;
>     cyg_uint32 s[32];
>     cyg_uint32 fpscr;
> } HAL_CORTEXM_GDB_Registers;
> 
> ----------------------
> 
> Is above OK?

Assuming you are actually using NUMREGS instead of NUMREFS, which I'm sure you
must be, those look ok on the face of it, but obviously the problem may well be
elsewhere. Have you set HAL_STUB_REGISTERS_SIZE for example? And if so, to
what?

In due course, you may be better off looking at the remote protocol dump, using
either "set debug remote 1" or "set remotelogfile gdbpkts.txt" just to be sure
what is being sent matches what you expect.


> Ideally the variant/platform will only need to implement
> CYGINT_HAL_CORTEXM_FPU.

We can probably anticipate that other Cortex-Ms will come out in the future
with different VFP versions, or a different number or size of registers. This
should be anticipated. So it's unlikely it will be just be a question of FPU or
not FPU. Maybe we should follow the example of GCC and have something like
CYGINT_HAL_CORTEXM_VFP4_SP_D16? Or maybe just CYGINT_HAL_CORTEXM_VFP4, and then
variants must use a CDL 'requires' to set a specific FPU implementation, such
as SP-D16.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #16 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-04 20:45:39 BST ---
Hi Jifl

My stub-ware is not yet completed so I have enforced incremental (RAW not FP)
values in VFP "registers" in order to test GDB response. The output is pasted
below, and it seems your patch gives correct operation with VFP registers.

Ilija

---- Test output -----

Sending packet: $g#67...Ack
Packet received:
9d550000100000000000000010000000000000000000000000000000000000000000000000000000000000000000000000000000e8feff77b56d00009e550000000000210100000002000000030000000400000005000000060000000700000008000000090000000a0000000b0000000c0000000d0000000e0000000f000000100000001100000012000000130000001400000015000000160000001700000018000000190000001a0000001b0000001c0000001d0000001e0000001f0000002000000021000000
Sending packet: $m559e,4#d5...Ack
Packet received: 70472de9
0x0000559e in ?? ()
Sending packet: $qSymbol::#5b...Ack
Packet received: 
Packet qSymbol (symbol-lookup) is NOT supported
Sending packet: $qTStatus#49...Ack
Packet received: 
(gdb) info all-registers 
r0             0x559d    21917
r1             0x10    16
r2             0x0    0
r3             0x10    16
r4             0x0    0
r5             0x0    0
r6             0x0    0
r7             0x0    0
r8             0x0    0
r9             0x0    0
r10            0x0    0
r11            0x0    0
r12            0x0    0
sp             0x77fffee8    0x77fffee8
lr             0x6db5    28085
pc             0x559e    0x559e
xpsr           0x21000000    553648128
d0             4.2439915824246103e-314    (raw 0x0000000200000001)
d1             8.4879831653432862e-314    (raw 0x0000000400000003)
d2             1.2731974748261962e-313    (raw 0x0000000600000005)
d3             1.6975966331180638e-313    (raw 0x0000000800000007)
d4             2.1219957914099314e-313    (raw 0x0000000a00000009)
d5             2.546394949701799e-313    (raw 0x0000000c0000000b)
d6             2.9707941079936666e-313    (raw 0x0000000e0000000d)
d7             3.3951932662855342e-313    (raw 0x000000100000000f)
d8             3.8195924245774018e-313    (raw 0x0000001200000011)
d9             4.2439915828692694e-313    (raw 0x0000001400000013)
d10            4.6683907411611369e-313    (raw 0x0000001600000015)
d11            5.0927898994530045e-313    (raw 0x0000001800000017)
d12            5.5171890577448721e-313    (raw 0x0000001a00000019)
d13            5.9415882160367397e-313    (raw 0x0000001c0000001b)
d14            6.3659873743286073e-313    (raw 0x0000001e0000001d)
d15            6.7903865326204749e-313    (raw 0x000000200000001f)
fpscr          0x21    33
---Type <return> to continue, or q <return> to quit---
s0             1.40129846e-45    (raw 0x00000001)
s1             2.80259693e-45    (raw 0x00000002)
s2             4.20389539e-45    (raw 0x00000003)
s3             5.60519386e-45    (raw 0x00000004)
s4             7.00649232e-45    (raw 0x00000005)
s5             8.40779079e-45    (raw 0x00000006)
s6             9.80908925e-45    (raw 0x00000007)
s7             1.12103877e-44    (raw 0x00000008)
s8             1.26116862e-44    (raw 0x00000009)
s9             1.40129846e-44    (raw 0x0000000a)
s10            1.54142831e-44    (raw 0x0000000b)
s11            1.68155816e-44    (raw 0x0000000c)
s12            1.821688e-44    (raw 0x0000000d)
s13            1.96181785e-44    (raw 0x0000000e)
s14            2.1019477e-44    (raw 0x0000000f)
s15            2.24207754e-44    (raw 0x00000010)
s16            2.38220739e-44    (raw 0x00000011)
s17            2.52233724e-44    (raw 0x00000012)
s18            2.66246708e-44    (raw 0x00000013)
s19            2.80259693e-44    (raw 0x00000014)
s20            2.94272678e-44    (raw 0x00000015)
s21            3.08285662e-44    (raw 0x00000016)
s22            3.22298647e-44    (raw 0x00000017)
s23            3.36311631e-44    (raw 0x00000018)
s24            3.50324616e-44    (raw 0x00000019)
s25            3.64337601e-44    (raw 0x0000001a)
s26            3.78350585e-44    (raw 0x0000001b)
s27            3.9236357e-44    (raw 0x0000001c)
s28            4.06376555e-44    (raw 0x0000001d)
s29            4.20389539e-44    (raw 0x0000001e)
s30            4.34402524e-44    (raw 0x0000001f)
s31            4.48415509e-44    (raw 0x00000020)
(gdb)

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #17 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-11 22:04:32 BST ---
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > 
> > > Well my statement was little bit too general. It's actually "C/C++GDB Hardware
> > > Debugging" plugin that comes bundled with (some configurations of) Eclipse CDT.
> > > When I tried to debug with Eclipse I got /Remote 'g' packet reply is too long:/
> > > and this was the best explanation.
> > > 
> > > Now I got old good Zylin plugin, where I can manually set commands, and it
> > > works for me (and confirms my doubt about the other one).
> > > 
> > > What matters, it's a non-issue for your GDB patch.
> > 
> > Okay. So just to be absolutely clear, do you think you have tested GDB enough
> > that I should go ahead and submit this patch to upstream GDB? Or should I wait
> > a bit longer?
> 
> Please hold on. My stub is still less then perfect so I can't confirm 100%
> functionality (please see issues below). I hope to fix it in a couple of days.

Jifl

Now I can pretty much confirm the patch. I have fixed the stub (Bug 1001558)
and now I can see all registers updated.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #18 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-11 22:24:52 BST ---
(In reply to comment #15)
> (In reply to comment #14)
> > > Actually I did mean get_register(), which here comes from cortexm_stub.c.
> > > That's what stub_format_registers() calls (if !TARGET_HAS_LARGE_REGISTERS). My
> > > idea was to simply check the register number. However, using
> > > TARGET_HAS_LARGE_REGISTERS as you say you have done means in fact this is
> > > irrelevant.
> > 
> > I am little-bit confused since get_register() returns value not address.
> > Returned value for non-valid register is -1 not 0. Do i miss something?
> 
> Ah, I've worked out we've been talking at cross-purposes. I had misunderstood
> what you were asking about and as a result got myself mixed up about what bit
> you were referring to. Okay, start over, reboot, forget previous thread of
> conversation...
> 
> You say you are using TARGET_HAS_LARGE_REGISTERS. This is fine. You are right
> that stub_format_registers() cannot skip registers. We need it to skip
> registers (ditto stub_update_registers()). We will probably need to add a hook
> in generic-stub.c for this which cortexm_stub.h can define. Perhaps something
> like: 
> 
> #ifdef TARGET_G_PACKET_SKIPS_REGS
>     if ( !reg_in_g_packet( reg ) )
>         continue;
> #endif
> 
> 

This is a variation (Bug 1001559). Works for me and looks like solution.


> > I am asking this because last few FPU registers d13..d15 are not being
> > transfered and I haven't proper explanation. I hope you can give me some
> > insight.
> > 
> > Here are data definitions:
> > -------------------
> > cortexm_stubs.h:
> > 
> > enum regnames {
> >     R0, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, R11, R12, SP, LR, PC,
> >     XPSR = 25,
> >     D0 = 26, D1, D2, D3, D4, D5, D6, D7, D8, D9, D10, D11, D12, D13, D14, D15,
> >     FPSCR
> > };
> > 
> > #define NUMREFS (FPSCR + 1) // This counts FPA registers (16 .. 24)
> > 
> > #define REGSIZE( _x_ ) (_x_ <= PC ? 4 : \
> >                         (_x_ < XPSR ? 0 : \
> >                          (_x_ == XPSR ? 4 : \
> >                           (((_x_ >= D0) && (_x_ <= D15)) ? 8 : \
> >                            (_x_ == FPSCR ? 4 : 0 )))))
> > 
> > 
> > ---------------------
> > hal_arch.h:
> > 
> > // Register layout expected by GDB VFP
> > typedef struct
> > {
> >     cyg_uint32 gpr[16];
> >     cyg_uint32 xpsr;
> >     cyg_uint32 s[32];
> >     cyg_uint32 fpscr;
> > } HAL_CORTEXM_GDB_Registers;
> > 
> > ----------------------
> > 
> > Is above OK?
> 
> Assuming you are actually using NUMREGS instead of NUMREFS, which I'm sure you
> must be, those look ok on the face of it, but obviously the problem may well be
> elsewhere. Have you set HAL_STUB_REGISTERS_SIZE for example? And if so, to
> what?

Thank you for the lead. (Bug 1001558)

> 
> In due course, you may be better off looking at the remote protocol dump, using
> either "set debug remote 1" or "set remotelogfile gdbpkts.txt" just to be sure
> what is being sent matches what you expect.
> 
> 
> > Ideally the variant/platform will only need to implement
> > CYGINT_HAL_CORTEXM_FPU.
> 
> We can probably anticipate that other Cortex-Ms will come out in the future
> with different VFP versions, or a different number or size of registers. 

My point was supposed to be: "In ideal case you don't have to add anything to
variant other than implementing the interface in order to make it fit for FPU".
In reality you need to add appropriate CFLAGS to the platform.

> This should be anticipated. So it's unlikely it will be just be a question of > FPU or
> not FPU. Maybe we should follow the example of GCC and have something like
> CYGINT_HAL_CORTEXM_VFP4_SP_D16? Or maybe just CYGINT_HAL_CORTEXM_VFP4, and then
> variants must use a CDL 'requires' to set a specific FPU implementation, such
> as SP-D16.

I have changed CYGHWR_HAL_CORTEXM_FPU to booldata so now it can select desired
FPU.

    cdl_component CYGHWR_HAL_CORTEXM_FPU {
        display          "Floating Point Unit"
        flavor           booldata
        active_if        { CYGINT_HAL_CORTEXM_FPU }
        legal_values     { "FPV4_SP_D16" }
        default_value    { "FPV4_SP_D16" }
        description      "Floating Point Unit"
cdl_option CYGHWR_HAL_CORTEXM_FPU_SWITCH {
            display "FPU context switch"
            flavor data
            legal_values { "ALL" "LAZY" "NO" }
            default_value { "LAZY" }
         ....
    }

Please comment.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #19 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-12 18:44:17 BST ---
(In reply to comment #18)
> (In reply to comment #15)
> > We need [stub_format_registers()] to skip
> > registers (ditto stub_update_registers()). We will probably need to add a hook
> > in generic-stub.c for this which cortexm_stub.h can define. Perhaps something
> > like: 
> > 
> > #ifdef TARGET_G_PACKET_SKIPS_REGS
> >     if ( !reg_in_g_packet( reg ) )
> >         continue;
> > #endif
> 
> This is a variation (Bug 1001559). Works for me and looks like solution.

Setting REGSIZE for them to 0 is good thinking.


> > Have you set HAL_STUB_REGISTERS_SIZE for example? And if so, to
> > what?
> 
> Thank you for the lead. (Bug 1001558)

Glad I guessed right :-).


> > > Ideally the variant/platform will only need to implement
> > > CYGINT_HAL_CORTEXM_FPU.
> > 
> > We can probably anticipate that other Cortex-Ms will come out in the future
> > with different VFP versions, or a different number or size of registers. 
> 
> My point was supposed to be: "In ideal case you don't have to add anything to
> variant other than implementing the interface in order to make it fit for FPU".
> In reality you need to add appropriate CFLAGS to the platform.

Not necessarily. We potentially want to treat it like ARM's thumb mode, and
have a higher layer manage adding/removing the appropriate flags to avoid
unnecessary duplication of CDL into multiple platforms.

This is also because the user has ultimate control over whether the FPU support
is included or not. Just because the hardware has an FPU doesn't mean they
definitely want to use it. After all, it will use up space for the registers
(even if they are only used lazily). And indeed your CDL below does allow it to
be enabled/disabled, which is good, but rather than every relevant platform's
CFLAGS having to test the value to decide how to construct the CFLAGS, arguably
it's better for this option to use a 'requires' to set it in the CFLAGS, e.g.
with the CDL you provided below (not my alternative):

   requires { (CYGHWR_HAL_CORTEXM_FPU == "FPV4_SP_D16") implies
is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
   requires { !is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies
!is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfpu=fpv4-sp-d16") }


> > This should be anticipated. So it's unlikely it will be just be a question of > FPU or
> > not FPU. Maybe we should follow the example of GCC and have something like
> > CYGINT_HAL_CORTEXM_VFP4_SP_D16? Or maybe just CYGINT_HAL_CORTEXM_VFP4, and then
> > variants must use a CDL 'requires' to set a specific FPU implementation, such
> > as SP-D16.
> 
> I have changed CYGHWR_HAL_CORTEXM_FPU to booldata so now it can select desired
> FPU.
> 
>     cdl_component CYGHWR_HAL_CORTEXM_FPU {
>         display          "Floating Point Unit"
>         flavor           booldata
>         active_if        { CYGINT_HAL_CORTEXM_FPU }
>         legal_values     { "FPV4_SP_D16" }
>         default_value    { "FPV4_SP_D16" }
>         description      "Floating Point Unit"

This doesn't feel quite right. Somewhere, something will have to do both:
 implements CYGINT_HAL_CORTEXM_FPU
_and_
 requires { is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies (CYGHWR_HAL_CORTEXM_FPU
== "FPV4_SP_D16" }

This seems redundant.

For that matter, is there anything to be gained from CYGINT_HAL_CORTEXM_FPU
anyway? I would have thought the HAL code would basically always have to be
customised according to the specific register layout anyway, but you are better
placed to decide that than I am.

I think it would be better to have something like:

   cdl_component CYGPKG_HAL_CORTEXM_FPU {
       display    "Hardware floating point"
       flavor     none
       no_define

       cdl_interface CYGINT_HAL_CORTEXM_FPU {
           display   "FPU present"
       }

       cdl_interface CYGINT_HAL_CORTEXM_VFP4_SP_D16 {
           display    "FPU uses vfp4-sp-d16 layout"
           implements CYGINT_HAL_CORTEXM_FPU
       }

       cdl_option CYGHWR_HAL_CORTEXM_FPU {
           display   "Use hardware FP"
           active_if CYGINT_HAL_CORTEXM_FPU
       }

       requires   { 1 >= CYGINT_HAL_CORTEXM_FPU }
       requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
== is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }


(With appropriate "description" fields added of course).

In the last line, hopefully the == will make the CDL inference engine do the
right thing, but if not, change it to:
       requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
implies is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
       requires   { !(CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
implies !is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }


This way, HALs only need to implement CYGINT_HAL_CORTEXM_VFP4_SP_D16, and
everything else is handled here at the arch HAL level, and presented to the
user all in one place.

What do you think?


> cdl_option CYGHWR_HAL_CORTEXM_FPU_SWITCH {
>             display "FPU context switch"
>             flavor data
>             legal_values { "ALL" "LAZY" "NO" }
>             default_value { "LAZY" }
>          ....
>     }

I'd say "NONE" instead of "NO".

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #20 from Ilija Kocho <ilijak@siva.com.mk> 2012-04-19 08:11:47 BST ---
Jifl,

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)

> > > Okay. So just to be absolutely clear, do you think you have tested GDB enough
> > > that I should go ahead and submit this patch to upstream GDB? Or should I wait
> > > a bit longer?
> > 
> > Please hold on. My stub is still less then perfect so I can't confirm 100%
> > functionality (please see issues below). I hope to fix it in a couple of days.
> 
> Jifl
> 
> Now I can pretty much confirm the patch. I have fixed the stub (Bug 1001558)
> and now I can see all registers updated.
> 
> Ilija

Perhaps I should have been more explicit, sorry. This message reminded me:
http://sourceware.org/ml/gdb/2012-04/msg00149.html
Yes, you can submit the GDB patch. Would you mind to CC to me?

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #21 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-19 16:13:51 BST ---
(In reply to comment #20)
> 
> Perhaps I should have been more explicit, sorry. This message reminded me:
> http://sourceware.org/ml/gdb/2012-04/msg00149.html
> Yes, you can submit the GDB patch. Would you mind to CC to me?

Responded to that mail here: http://sourceware.org/ml/gdb/2012-04/msg00151.html
and submitted the patch here:
http://sourceware.org/ml/gdb-patches/2012-04/msg00628.html

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #22 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-26 16:29:29 BST ---
(In reply to comment #21)
> (In reply to comment #20)
> > 
> > Perhaps I should have been more explicit, sorry. This message reminded me:
> > http://sourceware.org/ml/gdb/2012-04/msg00149.html
> > Yes, you can submit the GDB patch. Would you mind to CC to me?
> 
> Responded to that mail here: http://sourceware.org/ml/gdb/2012-04/msg00151.html
> and submitted the patch here:
> http://sourceware.org/ml/gdb-patches/2012-04/msg00628.html

The patch has now been committed, and will now be in future GDB releases.

http://sourceware.org/ml/gdb-patches/2012-04/msg00918.html

So all it needs now is for whoever is rebuilding the GDB (7.4?) to take the
following patches and apply them and rebuild:

http://sourceware.org/ml/gdb-patches/2012-03/msg00377.html
http://sourceware.org/ml/gdb-patches/2012-04/msg00705.html
http://sourceware.org/ml/gdb-patches/2012-04/msg00912.html

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1001468

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

--- Comment #23 from Ilija Kocho <ilijak@siva.com.mk> 2012-06-03 14:57:22 BST ---
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > We need [stub_format_registers()] to skip
> > > registers (ditto stub_update_registers()). We will probably need to add a hook
> > > in generic-stub.c for this which cortexm_stub.h can define. Perhaps something
> > > like: 
> > > 
> > > #ifdef TARGET_G_PACKET_SKIPS_REGS
> > >     if ( !reg_in_g_packet( reg ) )
> > >         continue;
> > > #endif
> > 
> > This is a variation (Bug 1001559). Works for me and looks like solution.
> 
> Setting REGSIZE for them to 0 is good thinking.
> 
> 
> > > Have you set HAL_STUB_REGISTERS_SIZE for example? And if so, to
> > > what?
> > 
> > Thank you for the lead. (Bug 1001558)
> 
> Glad I guessed right :-).
> 
> 
> > > > Ideally the variant/platform will only need to implement
> > > > CYGINT_HAL_CORTEXM_FPU.
> > > 
> > > We can probably anticipate that other Cortex-Ms will come out in the future
> > > with different VFP versions, or a different number or size of registers. 
> > 
> > My point was supposed to be: "In ideal case you don't have to add anything to
> > variant other than implementing the interface in order to make it fit for FPU".
> > In reality you need to add appropriate CFLAGS to the platform.
> 
> Not necessarily. We potentially want to treat it like ARM's thumb mode, and
> have a higher layer manage adding/removing the appropriate flags to avoid
> unnecessary duplication of CDL into multiple platforms.
> 
> This is also because the user has ultimate control over whether the FPU support
> is included or not. Just because the hardware has an FPU doesn't mean they
> definitely want to use it. After all, it will use up space for the registers
> (even if they are only used lazily). And indeed your CDL below does allow it to
> be enabled/disabled, which is good, but rather than every relevant platform's
> CFLAGS having to test the value to decide how to construct the CFLAGS, arguably
> it's better for this option to use a 'requires' to set it in the CFLAGS, e.g.
> with the CDL you provided below (not my alternative):
> 
>    requires { (CYGHWR_HAL_CORTEXM_FPU == "FPV4_SP_D16") implies
> is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
>    requires { !is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies
> !is_substr(CYGBLD_GLOBAL_CFLAGS, "-mfpu=fpv4-sp-d16") }
> 
> 
> > > This should be anticipated. So it's unlikely it will be just be a question of > FPU or
> > > not FPU. Maybe we should follow the example of GCC and have something like
> > > CYGINT_HAL_CORTEXM_VFP4_SP_D16? Or maybe just CYGINT_HAL_CORTEXM_VFP4, and then
> > > variants must use a CDL 'requires' to set a specific FPU implementation, such
> > > as SP-D16.
> > 
> > I have changed CYGHWR_HAL_CORTEXM_FPU to booldata so now it can select desired
> > FPU.
> > 
> >     cdl_component CYGHWR_HAL_CORTEXM_FPU {
> >         display          "Floating Point Unit"
> >         flavor           booldata
> >         active_if        { CYGINT_HAL_CORTEXM_FPU }
> >         legal_values     { "FPV4_SP_D16" }
> >         default_value    { "FPV4_SP_D16" }
> >         description      "Floating Point Unit"
> 
> This doesn't feel quite right. Somewhere, something will have to do both:
>  implements CYGINT_HAL_CORTEXM_FPU
> _and_
>  requires { is_enabled(CYGHWR_HAL_CORTEXM_FPU) implies (CYGHWR_HAL_CORTEXM_FPU
> == "FPV4_SP_D16" }
> 
> This seems redundant.
> 
> For that matter, is there anything to be gained from CYGINT_HAL_CORTEXM_FPU
> anyway? I would have thought the HAL code would basically always have to be
> customised according to the specific register layout anyway, but you are better
> placed to decide that than I am.
> 
> I think it would be better to have something like:
> 
>    cdl_component CYGPKG_HAL_CORTEXM_FPU {
>        display    "Hardware floating point"
>        flavor     none
>        no_define
> 
>        cdl_interface CYGINT_HAL_CORTEXM_FPU {
>            display   "FPU present"
>        }
> 
>        cdl_interface CYGINT_HAL_CORTEXM_VFP4_SP_D16 {
>            display    "FPU uses vfp4-sp-d16 layout"
>            implements CYGINT_HAL_CORTEXM_FPU
>        }
> 
>        cdl_option CYGHWR_HAL_CORTEXM_FPU {
>            display   "Use hardware FP"
>            active_if CYGINT_HAL_CORTEXM_FPU
>        }
> 
>        requires   { 1 >= CYGINT_HAL_CORTEXM_FPU }
>        requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> == is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
> 
> 
> (With appropriate "description" fields added of course).
> 
> In the last line, hopefully the == will make the CDL inference engine do the
> right thing, but if not, change it to:
>        requires   { (CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> implies is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
>        requires   { !(CYGHWR_HAL_CORTEXM_FPU && CYGINT_HAL_CORTEXM_VFP4_SP_D16)
> implies !is_substr(CYGBLD_GLOBAL_CFLAGS, " -mfpu=fpv4-sp-d16") }
> 
> 
> This way, HALs only need to implement CYGINT_HAL_CORTEXM_VFP4_SP_D16, and
> everything else is handled here at the arch HAL level, and presented to the
> user all in one place.
> 
> What do you think?
> 

For the record, some of your suggestions are accepted and integrated in Bug
1001607.

Thanks.

Ilija

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug 1001524] New: Cortex-M: Remote 'g' packet reply is too long

[bugzilla-daemon]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001524

           Summary: Cortex-M: Remote 'g' packet reply is too long
           Product: eCos
           Version: CVS
          Platform: All
        OS/Version: Cortex-M
            Status: NEW
          Severity: enhancement
          Priority: normal
         Component: GDB stubs
        AssignedTo: unassigned@bugs.ecos.sourceware.org
        ReportedBy: john@dallaway.org.uk
                CC: ecos-bugs@ecos.sourceware.org
             Class: Advice Request


When connecting to the Cortex-M GDB stub from recent versions of GDB, an error
is observed:

> (gdb) tar rem /dev/ttyS0
> Remote debugging using /dev/ttyS0
> Remote 'g' packet reply is too long: e14e000810000000000000001000000000000000000000000000000000000000000000000000000000000000fccf0d6800000000e8cf0d6895680008e24e00080000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000021
> (gdb)

This occurs because the default register set for ARM M-profile targets has been
reduced (FPA registers have been removed) in recent GDB.

We should add a CDL option to the Cortex-M architectural HAL which allows the
stub to return the register set with or without the FPA registers.

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.