Re: [1/2] C-SKY Port

[Tom Tromey <tom@tromey.com>]

>>>>> ">" == Hafiz Abid Qadeer <abid_qadeer@mentor.com> writes:

>> Add support for new target 'csky'.

>> 2018-07-25  Jiangshuai Li  <jiangshuai_li@c-sky.com>
>> 	    Hafiz Abid Qadeer  <abidh@codesourcery.com>
>> 	    Don Breazeal  <donb@codesourcery.com>

I didn't check the copyright assignment situation, but for a patch of
this size, assignments are needed.  I assume codesourcery has some kind
of blanket assignment, but what about Jiangshuai Li?

>> diff --git a/gdb/configure.tgt b/gdb/configure.tgt

>> +csky*-linux*)

>> +csky*-*)

It's more normal to mention all the "-"s, so "csky*-*-linux*" and "csky*-*-*".

I don't know anything about C-SKY, so I didn't really look deeply at the
arch-specific bits.  I assume those pass your testing.

>> diff --git a/gdb/csky-tdep.c b/gdb/csky-tdep.c

>> +struct stack_item
>> +{
>> +  int len;
>> +  struct stack_item *prev;
>> +  void *data;

Seems to me that this could be gdb_byte instead of void here, and in
push_stack_item and pop_stack_item.

>> +/* Implement the push_dummy_call gdbarch method.  */
>> +
>> +static CORE_ADDR
>> +csky_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>> +		      struct regcache *regcache, CORE_ADDR bp_addr,
>> +		      int nargs, struct value **args, CORE_ADDR sp,
>> +		      int struct_return, CORE_ADDR struct_addr)
>> +{
[...]
>> +      arg_type = check_typedef (value_type (args[argnum]));
>> +      len = TYPE_LENGTH (arg_type);
>> +      val = value_contents (args[argnum]);

I wonder if this can ever throw.

In theory of course it can.  But maybe in practice it isn't possible?
I don't know.

The issue is, if it can throw, then the stack_items will be leaked.
A way around that is to redo the stack as self-managing C++ objects and
use a local std::vector or the like to hold them.

>> +  /* Transfer the dummy stack frame to the target.  */
>> +  while (si)
>> +    {
>> +      sp -= si->len;
>> +      write_memory (sp, (const bfd_byte *) si->data, si->len);

A similar consideration applies here.


This all seems pretty reasonable to me.

Tom