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

Tom Tromey
2020-04-24T14:00:06Z

>>>>> ">" == 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