Re: [PATCH] Support new target S+core 3, a variant of S+core

[Joel Brobecker <brobecker@adacore.com>]

> The S+core team in our company have ported GDB to S+core 3, a variant
> of S+core, now we want to contribute some patch. So here it is (see 
> attachment).

Thanks, Don.

> Please review it, and any suggestions are most welcome. 

Note that Qinwei is also capable of reviewing your code.
I can take a look, but it's better if Qinwei does it, because he has
actual knowledge of the target.

> At now, I'm quite new to GDB, so if you fond something I did wrong, 
> please point it out.

If you haven't done so, the gdb/CONTRIBUTE file contains valuable
information. You may also want to give gdb/MAINTAINERS a quick read.

The first thing that struck me was the fact that the patch was produced
in the wrong format, so "context" is missing. Could you resubmit your
patch using the "-u" option? For instance "cvs diff -u ...".

Also, it looks like you are submitting several changes in one patch,
which we prefer not to. For instance, your change to gdb/configure.tgt
can be submitted as is, as it is independent of the target; it just
tells gdb about the the simulator.

> --- src/gdb/MAINTAINERS 2009-03-04 10:26:35.000000000 +0800
> +++ src.mod/gdb/MAINTAINERS     2009-03-04 10:29:15.000000000 +0800
> @@ -307,6 +307,7 @@
>  
>         score   --target=score-elf
>                         Qinwei          qinwei@sunnorth.com.cn
> +                       Don Lee         don.lee@sunplusct.com
>  
>         sh              --target=sh-elf ,-Werror
>                         --target=sh64-elf ,-Werror

As I said on the gdb@ mailing-list, I would prefer to hold off on
that change, but others might have a different opinion. In any case,
this is also an independent change, so this can be submitted separately
from the rest.

> Index: gdb/configure.tgt
> ===================================================================
> RCS file: /cvs/src/src/gdb/configure.tgt,v
> retrieving revision 1.213
> diff -r1.213 configure.tgt
> 378c378,382
> < 	gdb_target_obs="score-tdep.o"
> ---
> > 	gdb_target_obs="score-tdep.o corelow.o"
> > 	if test -f ../sim/score/Makefile; then
> > 	  gdb_sim=../sim/score/libsim.a
> > 	fi
> > 	build_gdbserver=yes

There is no score subdirectory in sim/. Are you going to submit
a simulator for that target?

The rest of the patch is very hard to read! Could you explain a little
bit the actual changes you made? At first sight, what the patch says
is that you replaced pretty much everything, which I doubt is the case.

-- 
Joel