Re: [RFC] New GDB Port CR16

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: Text/Plain, Size: 2246 bytes --]

On Monday 27 August 2012 02:36:15 Kaushik Phatak wrote:
> Please find attached a patch that adds support for the National
> Semiconductor CR16 architecture to GDB. The sim patch is already present
> in the sources. This patch enhances the sim to support breakpoints for sim
> based debugging.

this one diff has multiple unrelated changes.  might be nice if you were to 
split them up and commit them separately.  the sim/common/ changes could be 
merged now for example.  the other cr16 sim changes should be split up from 
the gdb changes so that they may be merged independently.

> --- gdb_src.orig/sim/common/gennltvals.sh
> +++ ./gdb_src/sim/common/gennltvals.sh
> 
> --- gdb_src.orig/sim/common/nltvals.def
> +++ ./gdb_src/sim/common/nltvals.def
> 
> --- gdb_src.orig/sim/cr16/cr16_sim.h
> +++ ./gdb_src/sim/cr16/cr16_sim.h
> 
> --- gdb_src.orig/sim/cr16/interp.c
> +++ ./gdb_src/sim/cr16/interp.c

changes to these files look fine

> --- gdb_src.orig/sim/cr16/simops.c
> +++ ./gdb_src/sim/cr16/simops.c
> void
> OP_C_C ()

unrelated, but that should be "(void)".  i imagine this sim port probably has 
a bunch of those bugs lurking though.

> @@ -5465,9 +5467,24 @@ OP_C_C ()
>  #endif
>  	    
>  	  default:
> +	    a = OP[0];
> +	    switch(a)
> +	    {
> +	  case  TRAP_BREAKPOINT:
> +	    State.exception = SIGTRAP;
> +	    tmp = (PC);
> +	    JMP(tmp);
> +	    trace_output_void ();
> +	  break;
> +	  case  SIGTRAP:  // supervisor call ?
> +	    State.exception = SIG_CR16_EXIT;
> +	    trace_output_void ();
> +	  break;
> +	  default:
>  	    cr16_callback->error (cr16_callback, "Unknown syscall %d", FUNC);
> +	  break;
> +	    }

pretty sure the indentation here is incorrect.  the case statements inside 
this new switch statement are not indented far enough.

also, please do not use C++ style comments //.  use /* */ instead.

> --- gdb_src.orig/opcodes/cr16-dis.c
> +++ ./gdb_src/opcodes/cr16-dis.c
>  
> -static int
> +int match_opcode (void);
> +
> +int
>  match_opcode (void)

this new local prototype makes no sense.  just delete it.

> -static void
> +void make_instruction (void);
> +
> +void
>  make_instruction (void)

same here
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]