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

Message ID 20180712205208.32646-7-tom@tromey.com
State New
Headers show
Series
  • Add -Wunused-variable
Related show

Commit Message

Tom Tromey July 12, 2018, 8:52 p.m.
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(-)

-- 
2.13.6

Comments

Simon Marchi July 12, 2018, 10:17 p.m. | #1
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
Tom Tromey July 13, 2018, 8:49 p.m. | #2
>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:


Simon> I'm unsure about this one.

Boy, I really misread this code.
Good thing I broke this one out.

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

Yep.

Tom

Patch

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));