gdb support for Atmel AVR

gdb support for Atmel AVR

[Theodore A. Roth]

Hi,

I have a patch for gdb-5.1 which adds support for Atmel's AVR 
microprocessors. It's getting to the point where it works well and is 
closing in on being ready for submission for inclusion.

Before the patch can be considered for inclusion, I know I need to sign
some documents. Thus, can someone point me at them? I've searched the web
and have only found references to the them, but not the actual docs. I
would also need to know where to send the signed docs.

I was also wondering if someone would be kind enough to review the patch
as it stands now and comment on it. I'm sure it will need some more work
to clean up loose ends before it can be accepted for inclusion. It might
need broken up since it's a rather large patch right now.

Here's a link to the patch:
  http://freesoftware.fsf.org/download/simulavr/gdb-patches/


Thanks,

Ted Roth

Re: gdb support for Atmel AVR

[Andrew Cagney]

> Hi,
> 
> I have a patch for gdb-5.1 which adds support for Atmel's AVR 
> microprocessors. It's getting to the point where it works well and is 
> closing in on being ready for submission for inclusion.


Nice.  Are the specs online somewhere?  Be interesting to look at this 
beast.

Is there a simulator as well?


> Before the patch can be considered for inclusion, I know I need to sign
> some documents. Thus, can someone point me at them? I've searched the web
> and have only found references to the them, but not the actual docs. I
> would also need to know where to send the signed docs.
> 
> I was also wondering if someone would be kind enough to review the patch
> as it stands now and comment on it. I'm sure it will need some more work
> to clean up loose ends before it can be accepted for inclusion. It might
> need broken up since it's a rather large patch right now.
> 
> Here's a link to the patch:
>   http://freesoftware.fsf.org/download/simulavr/gdb-patches/
> 


(Expect an e-mail from GDB assignment's clerk with the assignment info 
you need).

To make the process easier, below is a brief checklist of the things I 
look for in reviewing a target:

The first is multi-arch.  Have a look at xstormy16.  Old targets are 
being converted to multi-arch and new targets should be multi-arched 
from day one (well at least as far as possible - shlibs are not yet 
finished).  Pragmatics: multi-arch makes it much easier for developers 
to work on GDB.

Next is GNU's coding style and indentation.  Here, the best I can 
suggest, is run your code through the gdb_indent.sh script.  Pragmatics: 
  it is objective.

After that is the ARI.  Check http://sources.redhat.com/gdb/ari/ and 
have a look at the items listed under Regressions, Errors and Deprecated 
(als glance through the Warnings for things that are comming down the 
pipeline and might also raise an eyebrow).  Just remember that the ARI 
is a moving target (more things get deleted all the time) so no one 
expects things to be right from day one.  You'll just need to check for 
problems once the code is in.  Pragmatics: the intention is to stop the 
code (unintentionally) getting worse.  Eg: people adding new references 
to registers[] when we're trying to eliminate all the existing ones (1).

Finally, there is -Werror.  Your target should be buildable with:
	--target=<your-target> --enable-gdb-build-warnings=,-Werror
and then start.  You need to select your host carefully.  Pragmatics: 
anyone making changes across GDB should try rebuilding and running all 
targets.  -Werror makes this much easier.

The one thing you'll notice isn't on the check list is test results.  On 
that count, far as I'm conceerned, as soon as your target is showing 
reasonable signs of life it is ready for acceptance.

enjoy,
Andrew

(1) Things to do today is add a gdb_style.sh.

Re: gdb support for Atmel AVR

[Theodore A. Roth]

On Fri, 8 Feb 2002, Andrew Cagney wrote:

:)Nice.  Are the specs online somewhere?  Be interesting to look at this 
:)beast.

The patch is available here (latest is pre9): 
  <http://freesoftware.fsf.org/download/simulavr/gdb-patches/>

:)
:)Is there a simulator as well?

I'm also writing a simulator which can be used standalone or with gdb as a 
remote target. There's still a lot of work to be done on it, but it has 
been very useful in getting avr-gdb working.
  <http://savannah.gnu.org/projects/simulavr/>

There's another project using Atmel's JTAG ICE box as a remote target with 
the same patch.
  <http://avarice.sourceforge.net/>

:)(Expect an e-mail from GDB assignment's clerk with the assignment info 
:)you need).

Thanks.

:)
:)To make the process easier, below is a brief checklist of the things I 
:)look for in reviewing a target:
:)
:)The first is multi-arch.  Have a look at xstormy16.  Old targets are 
:)being converted to multi-arch and new targets should be multi-arched 
:)from day one (well at least as far as possible - shlibs are not yet 
:)finished).  Pragmatics: multi-arch makes it much easier for developers 
:)to work on GDB.

It uses multi-arch already.

:)
:)Next is GNU's coding style and indentation.  Here, the best I can 
:)suggest, is run your code through the gdb_indent.sh script.  Pragmatics: 
:)  it is objective.

I'm using emacs with gnu C indentation style, so that should be good.

:)
:)After that is the ARI.  Check http://sources.redhat.com/gdb/ari/ and 
:)have a look at the items listed under Regressions, Errors and Deprecated 
:)(als glance through the Warnings for things that are comming down the 
:)pipeline and might also raise an eyebrow).  Just remember that the ARI 
:)is a moving target (more things get deleted all the time) so no one 
:)expects things to be right from day one.  You'll just need to check for 
:)problems once the code is in.  Pragmatics: the intention is to stop the 
:)code (unintentionally) getting worse.  Eg: people adding new references 
:)to registers[] when we're trying to eliminate all the existing ones (1).

I'll take a look at that site.

:)
:)Finally, there is -Werror.  Your target should be buildable with:
:)	--target=<your-target> --enable-gdb-build-warnings=,-Werror
:)and then start.  You need to select your host carefully.  Pragmatics: 
:)anyone making changes across GDB should try rebuilding and running all 
:)targets.  -Werror makes this much easier.

That should be easy to do.

:)
:)The one thing you'll notice isn't on the check list is test results.  On 
:)that count, far as I'm conceerned, as soon as your target is showing 
:)reasonable signs of life it is ready for acceptance.

Some people are already using it for real work so I think it's got a heart 
beat already. ;)

I'm especially interested in someone reviewing the changes I had to make 
outside of avr-tdep.c and config/avr/*. These changes may affect other 
targets. I've tried really hard to keep those changes to a minimum though.

Ted Roth

Re: gdb support for Atmel AVR

[Daniel Jacobowitz]

On Fri, Feb 08, 2002 at 10:59:58AM -0700, Theodore A. Roth wrote:
> :)
> :)The one thing you'll notice isn't on the check list is test results.  On 
> :)that count, far as I'm conceerned, as soon as your target is showing 
> :)reasonable signs of life it is ready for acceptance.
> 
> Some people are already using it for real work so I think it's got a heart 
> beat already. ;)
> 
> I'm especially interested in someone reviewing the changes I had to make 
> outside of avr-tdep.c and config/avr/*. These changes may affect other 
> targets. I've tried really hard to keep those changes to a minimum though.

I really do not think that TARGET_REMOTE_ADDR_BIT should be
necessary... in what way was TARGET_ADDR_BIT/TARGET_POINTER_BIT
inadequate?  Do you have different sized code and data pointers?


Oh, reading further down the patch I see that AVR is a Harvard
architecture.  There is support for this in GDB, with
CODE_SPACE/DATA_SPACE that were recently introduced (for the d10v, I
think).  You may have some problems if they are of different size, I
suppose.



Also, I think (.avrgdbinit aside...) that you should not have a
tm-avr.h at all.  You can set multi-arch from configure.tgt.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: gdb support for Atmel AVR

[Andrew Cagney]

> :)Finally, there is -Werror.  Your target should be buildable with:
> :)	--target=<your-target> --enable-gdb-build-warnings=,-Werror
> :)and then start.  You need to select your host carefully.  Pragmatics: 
> :)anyone making changes across GDB should try rebuilding and running all 
> :)targets.  -Werror makes this much easier.
> 
> That should be easy to do.
> 
> :)
> :)The one thing you'll notice isn't on the check list is test results.  On 
> :)that count, far as I'm conceerned, as soon as your target is showing 
> :)reasonable signs of life it is ready for acceptance.
> 
> Some people are already using it for real work so I think it's got a heart 
> beat already. ;)


Sounds good.


> I'm especially interested in someone reviewing the changes I had to make 
> outside of avr-tdep.c and config/avr/*. These changes may affect other 
> targets. I've tried really hard to keep those changes to a minimum though.


(Mutter something about needing to avoid contamination until the 
assignment is in place :-/ :-)

I'll personally avoid looking at the changes however it looks like 
you're getting the feedback you need.

good luck!
Andrew

Re: gdb support for Atmel AVR

[Theodore A. Roth]

On Fri, 8 Feb 2002, Daniel Jacobowitz wrote:

:)I really do not think that TARGET_REMOTE_ADDR_BIT should be
:)necessary... in what way was TARGET_ADDR_BIT/TARGET_POINTER_BIT
:)inadequate?  Do you have different sized code and data pointers?

Code and data pointers are both 16-bit. The problem is we use some of the
bits 31-16 to flag whether gdb is asking for code (flash) or data (sram)
space.  Using "remote_address_size = TARGET_ADDR_BIT;" in remote.c causes
gdb to mask off the upper 16 bits thus removing the flag. Without the
flag, the target will always think it is accessing code space.

Basically, I've tricked gdb into storing ptrs and addresses into 32 bit
numbers while it still thinks that they are both 16 bits. I need all 32
bits sent to the target, but when gdb issues an 'm' packet for say a
struct, it must request the right number of bytes from the remote target. 

I got burned by this when I set remoteaddresssize to 32. Gdb would ask for 
4 bytes at some address and then dereference the return value thinking the 
value was a ptr. Needless to say, the 32 ptr pointed to the wrong data.

:)
:)Oh, reading further down the patch I see that AVR is a Harvard
:)architecture.  There is support for this in GDB, with
:)CODE_SPACE/DATA_SPACE that were recently introduced (for the d10v, I
:)think).  You may have some problems if they are of different size, I
:)suppose.

I modeled some of my work on the d10v-tdep.c file in gdb-5.1. Looking at 
the d10v from cvs, I see the use of TYPE_CODE_SPACE in the 
d10v_pointer_to_address function. This may help with some other things in 
my code, but I don't think it stops the problem described above.

:)Also, I think (.avrgdbinit aside...) that you should not have a
:)tm-avr.h at all.  You can set multi-arch from configure.tgt.

Easy enough to change. Done.

Is there any other way to set GDBINIT_FILENAME? Shouldn't that be part of 
multi-arch? If it could be set via multi-arch, I wouldn't need tm-avr.h. 
Since avr-gdb is always going to be a cross tool, I think I should be 
using .avrgdbinit to allow native gdb to use .gdbinit, right?

I'd also like a critic on my changes to 
eval.c/printcmd.c/value.h/values.c which where necessary to get the 
disassemble command to function properly.

Ted Roth

Re: gdb support for Atmel AVR

[Daniel Jacobowitz]

On Fri, Feb 08, 2002 at 03:26:28PM -0700, Theodore A. Roth wrote:
> On Fri, 8 Feb 2002, Daniel Jacobowitz wrote:
> 
> :)I really do not think that TARGET_REMOTE_ADDR_BIT should be
> :)necessary... in what way was TARGET_ADDR_BIT/TARGET_POINTER_BIT
> :)inadequate?  Do you have different sized code and data pointers?
> 
> Code and data pointers are both 16-bit. The problem is we use some of the
> bits 31-16 to flag whether gdb is asking for code (flash) or data (sram)
> space.  Using "remote_address_size = TARGET_ADDR_BIT;" in remote.c causes
> gdb to mask off the upper 16 bits thus removing the flag. Without the
> flag, the target will always think it is accessing code space.
> 
> Basically, I've tricked gdb into storing ptrs and addresses into 32 bit
> numbers while it still thinks that they are both 16 bits. I need all 32
> bits sent to the target, but when gdb issues an 'm' packet for say a
> struct, it must request the right number of bytes from the remote target. 
> 
> I got burned by this when I set remoteaddresssize to 32. Gdb would ask for 
> 4 bytes at some address and then dereference the return value thinking the 
> value was a ptr. Needless to say, the 32 ptr pointed to the wrong data.
> 
> :)
> :)Oh, reading further down the patch I see that AVR is a Harvard
> :)architecture.  There is support for this in GDB, with
> :)CODE_SPACE/DATA_SPACE that were recently introduced (for the d10v, I
> :)think).  You may have some problems if they are of different size, I
> :)suppose.
> 
> I modeled some of my work on the d10v-tdep.c file in gdb-5.1. Looking at 
> the d10v from cvs, I see the use of TYPE_CODE_SPACE in the 
> d10v_pointer_to_address function. This may help with some other things in 
> my code, but I don't think it stops the problem described above.

I think it ought to, but again I'll defer to someone with a better
understanding of how d10v handles this.  It appears to be rather
different.

> :)Also, I think (.avrgdbinit aside...) that you should not have a
> :)tm-avr.h at all.  You can set multi-arch from configure.tgt.
> 
> Easy enough to change. Done.
> 
> Is there any other way to set GDBINIT_FILENAME? Shouldn't that be part of 
> multi-arch? If it could be set via multi-arch, I wouldn't need tm-avr.h. 
> Since avr-gdb is always going to be a cross tool, I think I should be 
> using .avrgdbinit to allow native gdb to use .gdbinit, right?

No other port bothers.  It's never been a real problem for me; .gdbinit
goes in object directories generally.

> I'd also like a critic on my changes to 
> eval.c/printcmd.c/value.h/values.c which where necessary to get the 
> disassemble command to function properly.

You'd really need a comment from one of the d10v folks on this.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: gdb support for Atmel AVR

[Andrew Cagney]

> On Fri, 8 Feb 2002, Daniel Jacobowitz wrote:
> 
> :)I really do not think that TARGET_REMOTE_ADDR_BIT should be
> :)necessary... in what way was TARGET_ADDR_BIT/TARGET_POINTER_BIT
> :)inadequate?  Do you have different sized code and data pointers?
> 
> Code and data pointers are both 16-bit. The problem is we use some of the
> bits 31-16 to flag whether gdb is asking for code (flash) or data (sram)
> space.  Using "remote_address_size = TARGET_ADDR_BIT;" in remote.c causes
> gdb to mask off the upper 16 bits thus removing the flag. Without the
> flag, the target will always think it is accessing code space.


TARGET_ADDR_BIT is the number of significant bits in a CORE_ADDR.  For 
your target that is 32.  The remote protocol will use those 32 bits when 
  requesting raw memory.

Separatly, you've got 16 bit pointers you need TARGET_PTR_BIT=16.  GDB 
uses the functions pointer_to_address() and address_to_pointer() when 
converting a C code/data pointer to/from a CORE_ADDR.

BTW, the d10v is even more fun.  Data pointers are 16 bits and point to 
an 8 bit byte.  Code pointers are also 16 bits but point to a 16 bit 
word.  Consequently some shifting also occures when converting to/from 
CORE_ADDRS.  This all works with out cpu specific changes to core GDB.


> Basically, I've tricked gdb into storing ptrs and addresses into 32 bit
> numbers while it still thinks that they are both 16 bits. I need all 32
> bits sent to the target, but when gdb issues an 'm' packet for say a
> struct, it must request the right number of bytes from the remote target. 


The d10v was doing something like that but has since been fixed.


> I got burned by this when I set remoteaddresssize to 32. Gdb would ask for 
> 4 bytes at some address and then dereference the return value thinking the 
> value was a ptr. Needless to say, the 32 ptr pointed to the wrong data.


I'd try the above.

enjoy,
Andrew

Re: gdb support for Atmel AVR

[Theodore A. Roth]

Looks like I need to do some more work then. Thanks for the explanation.

Ted Roth

On Fri, 8 Feb 2002, Andrew Cagney wrote:


:)> Code and data pointers are both 16-bit. The problem is we use some of the
:)> bits 31-16 to flag whether gdb is asking for code (flash) or data (sram)
:)> space.  Using "remote_address_size = TARGET_ADDR_BIT;" in remote.c causes
:)> gdb to mask off the upper 16 bits thus removing the flag. Without the
:)> flag, the target will always think it is accessing code space.
:)
:)
:)TARGET_ADDR_BIT is the number of significant bits in a CORE_ADDR.  For 
:)your target that is 32.  The remote protocol will use those 32 bits when 
:)  requesting raw memory.
:)
:)Separatly, you've got 16 bit pointers you need TARGET_PTR_BIT=16.  GDB 
:)uses the functions pointer_to_address() and address_to_pointer() when 
:)converting a C code/data pointer to/from a CORE_ADDR.
:)
:)BTW, the d10v is even more fun.  Data pointers are 16 bits and point to 
:)an 8 bit byte.  Code pointers are also 16 bits but point to a 16 bit 
:)word.  Consequently some shifting also occures when converting to/from 
:)CORE_ADDRS.  This all works with out cpu specific changes to core GDB.
:)
:)
:)> Basically, I've tricked gdb into storing ptrs and addresses into 32 bit
:)> numbers while it still thinks that they are both 16 bits. I need all 32
:)> bits sent to the target, but when gdb issues an 'm' packet for say a
:)> struct, it must request the right number of bytes from the remote target. 
:)
:)
:)The d10v was doing something like that but has since been fixed.
:)
:)
:)> I got burned by this when I set remoteaddresssize to 32. Gdb would ask for 
:)> 4 bytes at some address and then dereference the return value thinking the 
:)> value was a ptr. Needless to say, the 32 ptr pointed to the wrong data.
:)
:)
:)I'd try the above.
:)
:)enjoy,
:)Andrew
:)
:)
:)
:)

Re: gdb support for Atmel AVR

[Theodore A. Roth]

On Fri, 8 Feb 2002, Andrew Cagney wrote:

:)TARGET_ADDR_BIT is the number of significant bits in a CORE_ADDR.  For 
:)your target that is 32.  The remote protocol will use those 32 bits when 
:)  requesting raw memory.
:)
:)Separatly, you've got 16 bit pointers you need TARGET_PTR_BIT=16.  GDB 
:)uses the functions pointer_to_address() and address_to_pointer() when 
:)converting a C code/data pointer to/from a CORE_ADDR.

<snip>

:)I'd try the above.

I set TARGET_ADDR_BIT to 32 and TARGET_PTR_BIT to 16 and initial testing 
looks like it works. I think had the two swapped when I was playing with 
this months ago.

Now I have avr support with only the following modifications to gdb from 
cvs:
	modify: configure.tgt
	add:    avr-tdep.c
	add:    config/avr/avr.mt
	add:    config/avr/tm-avr.h

This makes me much happier than my previous patch with all the other files 
it touched.

I'll continue testing and make up a new patch to send out to the avr 
community for testing while I wait for copyright assignment papers to show 
up.

Thanks for the help.

Ted Roth

Re: gdb support for Atmel AVR

[Andrew Cagney]

> <snip>
> 
> :)I'd try the above.
> 
> I set TARGET_ADDR_BIT to 32 and TARGET_PTR_BIT to 16 and initial testing 
> looks like it works. I think had the two swapped when I was playing with 
> this months ago.
> 
> Now I have avr support with only the following modifications to gdb from 
> cvs:
> 	modify: configure.tgt
> 	add:    avr-tdep.c
> 	add:    config/avr/avr.mt
> 	add:    config/avr/tm-avr.h
> 
> This makes me much happier than my previous patch with all the other files 
> it touched.


(Unless you've got shared libraries you should be able to zap tm-avr.h 
as well.)

The above news is really good.  I suspect ``months ago'' all the 
framework you needed may not have been in place.  Perhaphs also have a 
look over the relevant sections of the gdbint.texinfo file - there is 
plenty of oportunity there for improvement.

nice,
Andrew

Re: gdb support for Atmel AVR

[Theodore A. Roth]

I still haven't heard from the clerk yet. Once I get the papers turned,
how long does it take before the avr patch can be accepted?

Ted Roth

On Fri, 8 Feb 2002, Andrew Cagney wrote:

:)(Expect an e-mail from GDB assignment's clerk with the assignment info 
:)you need).