Re: [RFA 06/13] Remove dead code from m32c-tdep.c

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-07-12 04:52 PM, Tom Tromey wrote:
> This removes some dead code from m32c-tdep.c.  I broke this out into a
> separate patch because the dead code seemed unusual, as if perhaps it
> had not been completed; and so I thought it deserved extra scrutiny.
> 
> gdb/ChangeLog
> 2018-07-12  Tom Tromey  <tom@tromey.com>
> 
> 	* m32c-tdep.c (mark_dma): Remove.
> 	(make_regs): Remove dead code.
> ---
>  gdb/ChangeLog   |  5 +++++
>  gdb/m32c-tdep.c | 23 -----------------------
>  2 files changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/gdb/m32c-tdep.c b/gdb/m32c-tdep.c
> index f696568e3a7..6b1d1e83121 100644
> --- a/gdb/m32c-tdep.c
> +++ b/gdb/m32c-tdep.c
> @@ -689,15 +689,6 @@ mark_general (struct m32c_reg *reg)
>  }
>  
>  
> -/* Mark REG as a DMA register, and return it.  */
> -static struct m32c_reg *
> -mark_dma (struct m32c_reg *reg)
> -{
> -  reg->dma_p = 1;
> -  return reg;
> -}
> -
> -
>  /* Mark REG as a SYSTEM register, and return it.  */
>  static struct m32c_reg *
>  mark_system (struct m32c_reg *reg)
> @@ -839,20 +830,6 @@ make_regs (struct gdbarch *arch)
>    struct m32c_reg *pc          = G (RC (pc));
>    struct m32c_reg *flg         = G (R16U (flg));
>  
> -  if (mach == bfd_mach_m32c)
> -    {
> -      struct m32c_reg *svf     = S (R16U (svf));
> -      struct m32c_reg *svp     = S (RC (svp));
> -      struct m32c_reg *vct     = S (RC (vct));
> -
> -      struct m32c_reg *dmd01   = DMA (RP (dmd, tdep->uint8));
> -      struct m32c_reg *dct01   = DMA (RP (dct, tdep->uint16));
> -      struct m32c_reg *drc01   = DMA (RP (drc, tdep->uint16));
> -      struct m32c_reg *dma01   = DMA (RP (dma, tdep->data_addr_reg_type));
> -      struct m32c_reg *dsa01   = DMA (RP (dsa, tdep->data_addr_reg_type));
> -      struct m32c_reg *dra01   = DMA (RP (dra, tdep->data_addr_reg_type));
> -    }
> -
>    num_raw_regs = tdep->num_regs;
>  
>    r0 	      = G (CB (r0, raw_r0_pair));
> 

I'm unsure about this one.  If you expand the macros, it looks something
like this:

mark_dma((add_reg (arch, "dmd" "0", tdep->uint8, m32c_sim_reg_dmd0, m32c_raw_read, m32c_raw_write, 0, 0, 0),
          add_reg (arch, "dmd" "1", tdep->uint8, m32c_sim_reg_dmd1, m32c_raw_read, m32c_raw_write, 0, 0, 0) - 1))

On one hand, the add_reg calls look important to me, because they add registers
to the gdbarch_tdep structure, so I would keep them.  But the mark_dma call looks
really fishy.  It only receives the first register, because this expression actually
ends up using the "comma" operator, returning the value on the right.  A little bit like
in this small program:

#include <stdio.h>

int foo(int a) {
  return a;
}

int main()
{
  int val = foo((1, 2));
  printf("%d\n", val);
}

$ gcc test.c -Wall
test.c: In function ‘main’:
test.c:9:19: warning: left-hand operand of comma expression has no effect [-Wunused-value]
   int val = foo((1, 2));
                   ^
$ ./a.out
2

And the expression on the right actually returns the first register (because of the -1).
So we end up marking only one of the two registers as DMA.  I have no idea if that's
intended or not.

So maybe the safe thing to do would be to make mark_dma return void, and get
rid of the local variables and assignments (but keep the DMA(...) calls).

Simon