Re: [PATCH 3/4] arc: Add GNU/Linux support for ARC

[Tom Tromey <tom@tromey.com>]

>>>>> ">" == Shahab Vahedi via Gdb-patches <gdb-patches@sourceware.org> writes:

Thank you for the patch.
I have a few nits to pick, but no serious issues.

>> +static int
>> +arc_linux_cannot_fetch_register (struct gdbarch *gdbarch, int regnum)
>> +{
>> +  /* Assume that register is readable if it is unknown.  */
>> +  switch (regnum)
>> +    {
>> +    case ARC_ILINK_REGNUM:
>> +    case ARC_RESERVED_REGNUM:
>> +    case ARC_LIMM_REGNUM:
>> +      return TRUE;

gdb doesn't generally use the TRUE / FALSE defines -- there are
exceptions, mostly for curses code, but on the whole these should be
avoided.  You can either use 1 / 0, or true / false.

These functions only return int nowadays because C++-ification is
incomplete and nobody has gone back to fix up gdbarch to use bool
here, so I guess I'd opt for true / false.

>> +/* Implement the "sw_breakpoint_from_kind" gdbarch method.  */
>> +
>> +static const gdb_byte *
>> +arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch, int kind, int *size)

This line is too long.

>> +{
>> +
>> +  *size = kind;
>> +  return ((gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)

Spurious blank line at the start of this function.

>> +  /* Is this a delay slot?  Then next PC is in BTA register.  */
>> +  if (status32 & ARC_STATUS32_DE_MASK)

Normally in gdb this kind of expression is written out more fully, like:

    if ((status32 & ARC_STATUS32_DE_MASK) != 0)

I thought GCC had a warning that prevented the form in the patch?
Maybe I'm confusing it with something else.

>> -  /* Mask for DE bit is 0x40.  */
>> -  if (status32 & 0x40)
>> +  if (status32 & ARC_STATUS32_DE_MASK)

As long as you're touching this, you might as well change it to the
"!= 0" form.

>> +arc*-*-linux*)
>> +	# Target: ARC machine running Linux
>> +	gdb_target_obs="arc-linux-tdep.o linux-tdep.o solib-svr4.o"
>> +	build_gdbserver=yes

build_gdbserver is no longer needed in this file.  Since gdbserver was
moved to the top level, Whether or not gdbserver is built is now decided
entirely by gdbserver/configure.srv.

Tom