This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] AArch64 Port of GDB and GDBSERVER


Marcus,

I started reviewing your patch before I eventually realized that your
patch is 6500 lines long. So far, I reviewed Makefile.in (OK), and
aarch64-linux-nat.c (comments below). I've stopped, because this is
A LOT of code, and very time consuming.

Can I please suggest that you split your patch in manageable (and
independent!) chunks?

I think that the following should make sense:
  1. Add bareboard aarch64 target support
     (this is mostly aarch64-tdep, and associated glue)
  2. Add aarch64-linux-tdep support
  3. Add aarch64-newlib-tdep support (what's the newlib osabi???)
  4. Add aarch64-linux-nat support
  5. Add aarch64-linux gdbserver support

It's going to help you as well, because I'll review the bits sooner,
on their own. And since I can't review the gdbserver code, it'll make
it easier for the maintainer who does to follow up on that effort.

> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c

> +static int
> +get_thread_id (ptid_t ptid)
> +{
> +  int tid = TIDGET (ptid);
> +  if (0 == tid)

Small formatting nit: We add an empty line between the local variable
declarations and the rest of the code. Can you make a pass on your
code, in case there are other instances of this issue?


> +    tid = PIDGET (ptid);
> +  return tid;
> +}
> +
> +#define GET_THREAD_ID(PTID)	get_thread_id (PTID)

Question: What's the purpose of this define? I know we do this for
arm-linux, but I don't think it brings anything.  It's probably
coming from a long time ago when 


> +static int
> +fetch_xregs_from_thread (gdb_gregset_t *regs)

Please add a comment at the start of every function that documents
what it does. For instance, it's not clear from the function
declaration what the xregs are (xreg vs reg?), and which thread
it fetches the registers from. Nor is it clear what the return
value means.

It's not the case here, but for functions that are to be registered
in the gdbarch vector, it's sufficient to just say:

    /* Implement the "bla_bla_bla" gdbarch method.  */

Thus:

> +/* Store registers back into the inferior.  Store all registers if
> +   regno == -1, otherwise store all general registers or all floating
> +   point registers depending upon the value of regno.  */
> +static void
> +aarch64_linux_store_inferior_registers (struct target_ops *ops,
> +					struct regcache *regcache, int regno)

The description above is redundant, as it would duplicate the
description that (is/should be) provided next to the hook definition
in the gdbarch.h file.

Just one additional nit: Can you add an empty line between function
description and the function itself?

> +void
> +supply_gregset (struct regcache *regcache, const gdb_gregset_t *gregsetp)
> +{
> +  fprintf (stderr, "Unimplemented: %s\n", __FUNCTION__);
> +  exit (1);
> +}

Calls to "exit" are a big no-no in general. But this is even worse
in this case, because I'm pretty sure that a legitimate user action
can cause the debugger to just abort, which is very unfriendly at
best.

Can you just implement this function instead?

As a side comment, we do not use fprintf in the GDB code. In this
case, you probably either want to use "warning", or, perhaps if
we really wanted to make it fatal, something like "internal_error".

This applies to the other regset functions that are not implemented
as well.

> +  gdb_assert ((offset + len) > 0);

I think that the extra parentheses are unnecessary. Can you remove them?

> +  if ((offset + len) >= max_wp_len)

Same here.

> +  /* The info structure we return.  This function is called repeatedly, so we
> +     return a pointer to a static variable which caches the result, rather than
> +     calling ptrace repeatedly.  */

Can you reformat the comment? The soft limit for line sizes is 70
characters, with a hard limit of 80 chars. Your command fits the hard
limit, but don't think we need to exceed the soft limit in this case.
Basically, we allow excess when we have no choice, or when it makes it
more readable.

> +/* Initialise the hardware breakpoint structure P.  The breakpoint will be
      ^^^^^^^^^^ Initialize (American spelling)
> +   enabled, and will point to the placed address of BP_TGT.  */

> +  p->address = (unsigned int) (address & ~3);

Why the cast?

> +/* Get the AArch64 hardware breakpoint type from the RW value we're given when
> +   asked to set a watchpoint.  */

Can you reformat this comment (line size).


-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]