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

Simon Marchi
2018-07-12T22:18:25Z

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