RE: [RFA 3/5] New port: CR16: gdb port

Kaushik Phatak
2012-10-23T13:03:24Z

Hi Joel,
Thanks for your detailed review. I will make another pass over my code and
take care of the formatting nits.
To answer some of your questions,

>> These types of globals are a big no-no, Can you adjust your code to avoid
>> those globals?
These globals are actually declared in /opcodes/cr16-dis.c and my earlier
version of this patch had declared them extern here. Converting the disassembler
piece of code would be bit tricky as print_insn_cr16 and print_arg seem to use
these globals directly. Would it be OK just to leave them as extern here with
appropriate comments?

>> > + ULONGEST pc;
>> > +
>> > + pc = frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM);
>> > + return pc;
>> Just curious: Why not just:
>> return frame_unwind_register_unsigned (this_frame, CR16_PC_REGNUM);

I will make that change for unwind_pc and unwind_sp as well. The locals helped
me debug my code better, but I can remove them now.

>> > + /* If we don't pass the option -mint32
>> > + FIXME: add if else case depending on the option passed,
>> Is there any way of handling this fixme easily?
I think I can remove this fixme as the issue is actually at the sim end.
The host side gdb gets correct information about data types directly from
the symbol table. The simulator totally gets lost when -mint32 is passed.

>> you can add a gdb_assert_not_reached if you know you've covered
>> all registers, or else return any type, like builtin_int32.
OK, I think I will return builtin_int32 here as there are some targets with
additional debug registers.

Regards,
Kaushik