Re: [PATCH 1/2] gdb: bfin: new port

[Joel Brobecker <brobecker at adacore dot com>]

Mike,

First of all, I apologize of the delay in reviewing this.  Overall,
I think that this is very good work.  I can't really help with the
correctness of the code, since I don't know the bfin architecture.

> 2010-11-16  Jie Zhang  <jie.zhang@analog.com>
> 
> 	* configure.ac (bfin-*-*): Remove gdb from noconfigdirs.
> 	* configure: Regenerate.

This should be sent to gcc-patches....

> gdb/:
> 2010-11-16  Jie Zhang  <jie.zhang@analog.com>
> 
> 	* MAINTAINERS: Add Blackfin port.
> 	* Makefile.in (ALLDEPFILES): Add bfin-tdep.c.
> 	(HFILES_NO_SRCDIR): Add bfin-tdep.h.
> 	* NEWS: Mention new Blackfin port.
> 	* bfin-tdep.c, bfin-tdep.h, config/bfin/bfin.mt: New files.
> 	* configure.tgt (bfin-*-*): Handle bfin targets.

Minor comments below...

> +	bfin		--target=bfin-elf ,-Werror
> +			Mike Frysinger		vapier@gentoo.org
> +

We usually wait to see a few contributions before inviting a contributor
to become the official maintainer.  So, would you mind taking that hunk
out of your patch?

> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in

I think you will also want to add bfin-tdep.o to ALL_TARGET_OBS.
Otherwise, bfin gets left out when you build with --enable-targets=all.
I don't see a reason why we shouldn't include it.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS

This part gets approved by Eli Zaretski, our documentation maintainer.
Can you maybe ping him about it? He has always been extremely responsive,
so he probably just missed the fact that this patch had some documentation
changes for him to review.

> +#define NUM_BFIN_REGNAMES \
> +  (sizeof (bfin_register_name_strings) / sizeof (char *))

There is a macro called ARRAY_SIZE that you can use...

> +int map_gcc_gdb[] =

Can this be made static?

> +/* Return non-zero if PC points into the signal trampoline.  For the
> +   sake of bfin_linux_get_sigtramp_info.  */

This is a detail, but `nonzero' is spelled without the dash in the
middle. It's a detail because it's spelled your way everywhere...

> +static int
> +bfin_linux_pc_in_sigtramp (struct frame_info *this_frame, CORE_ADDR pc)
> +{
[...]
> +  gdb_byte buf[10];

The use of that buffer is most peculiar. You first start by reading
a 2-byte instruction and storing it at buf+4. Then, depending on
the instruction, you you extract the other instruction either at
buf or buf+6. If it works, then no need to change...

I'm also wondering whether you know about tramp-frame.h, and whether
that might be sufficient for your needs...

> +  if (ret == 1)
> +    {
> +      /* Get sigcontext address.  */
> +      info.context_addr = sp + SIGCONTEXT_OFFSET;
> +    }
> +  else
> +    internal_error (__FILE__, __LINE__, _("not a sigtramp\n"));

You should use a gdb_assert(), in this case.

> +  /* This would come after the LINK instruction in the ret_from_signal ()
> +     function, hence the frame id would be SP + 8.  */

Please remove the `()' - it is used only when talking about the function
call...

> +      /* Either there is no prologue (frameless function) or we are at
> +	 the start of a function. In short we do not have a frame.
> +	 PC is stored in rets register. FP points to previous frame.  */

Missing space after a period (2 instances).

> +      cache->saved_regs[BFIN_PC_REGNUM] = get_frame_register_unsigned (this_frame, BFIN_RETS_REGNUM);

This line is too long (over 78 characters).

> +#ifdef _DEBUG
> +      fprintf (stderr, "frameless pc case base %x\n", cache->base);
> +#endif

Can you avoid the #ifdef? I personally that debugging logs are a good
idea.  But I don't think they should only be activated (or not) at
compile time.  The typical way of doing this is to conditionalize this
on a global variable controlled by a setting.   For instance, how about

        (gdb) set debug bfin

(see `set debug infrun' for instance).

> +/* The following functions are for function prologue length calculations.  */

Each function needs to be documented.

> +static int
> +is_minus_minus_sp (int op)

Just out of curiosity: Why two 'minus' in the name?

> +CORE_ADDR
> +bfin_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)

This function should be made static.

> +	  fprintf (stderr, "Function Prologue not recognised. pc will point to ENTRY_POINT of the function\n");

No call to fprintf. In this case, I think you can use the `warning'
function.

> +/* We currently only support passing parameters in integer registers.  This
> +   conforms with GCC's default model.  Several other variants exist and
> +   we should probably support some of them based on the selected ABI.  */

I think that this comment will be less surprising if you place it at
the start of the function, inside the function body...

> +  for (i = nargs - 1; i >= 0; i--)
> +    {
> +      struct type *value_type = value_enclosing_type (args[i]);
> +      int len = TYPE_LENGTH (value_type);
> +      total_len += (len + 3) & ~3;

Empty line after local variable declarations.


> +  for (i = nargs - 1; i >= 0; i--)
> +    {
> +      struct type *value_type = value_enclosing_type (args[i]);
> +      struct type *arg_type = check_typedef (value_type);
> +      int len = TYPE_LENGTH (value_type);
> +      int container_len = (len + 3) & ~3;
> +      sp -= container_len;
> +      write_memory (sp, value_contents_writeable (args[i]), container_len);

Likewise.

> +  if (reg > sizeof (map_gcc_gdb) / sizeof (int))

You can use ARRAY_SIZE...

> +/* This function implements the BREAKPOINT_FROM_PC macro. It returns
                                                          ^^^^ missing space

> +const unsigned char *
> +bfin_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *lenptr)

Should be made static.

> +   and copy it into READBUF.  If WRITEBUF is non-zero, write the value
                                                ^^^^^^^^
> +CORE_ADDR
> +bfin_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)

Make static?

> +enum gcc_regnum {
> +  BFIN_GCC_R0_REGNUM = 0,

What is this used for?

> +/* The ABIs for Blackfin.  */
> +enum bfin_abi
> +{
> +  BFIN_ABI_FLAT
> +};
> +
> +/* Target-dependent structure in gdbarch.  */
> +struct gdbarch_tdep
> +{
> +  /* Which ABI is in use?  */
> +  enum bfin_abi bfin_abi;
> +};

Why is this used, since there is only one ABI?

> +/* Return the Blackfin ABI associated with GDBARCH.  */
> +static inline enum bfin_abi
> +bfin_abi (struct gdbarch *gdbarch)
> +{
> +  return gdbarch_tdep (gdbarch)->bfin_abi;
> +}

Please - no code in .h files. I doubt that making that function inline
is bringing any measurable benefit - is it?

-- 
Joel