[ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Pierre Muller]

  The Awk Regression Index page:
http://sourceware.org/gdb/current/ari/

Only has one Critical entry at top before the big table:

BUG ATTRIBUTE_UNUSED with 18 occurences.
Description is:
	Do not use ATTRIBUTE_UNUSED, do not bother (GDB is compiled with
-Werror and, consequently, is not able to tolerate false warnings. Since
-Wunused-param produces such warnings, neither that warning flag nor
ATTRIBUTE_UNUSED are used by GDB.

  Is this comment still valid, from the compiler point of view?
Does gcc still have such problems?

  If yes, is it OK if I remove all
ATTRIBUTE_UNUSED from sources?
 
  If no, shouldn't we change that policy?


Pierre Muller
as ARI maintainer for GDB

Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Joel Brobecker]

> Is this comment still valid, from the compiler point of view?
> Does gcc still have such problems?

I don't know.

>   If yes, is it OK if I remove all ATTRIBUTE_UNUSED from sources?
>   If no, shouldn't we change that policy?

I haven't thought much about it but, to me ATTRIBUTE_UNUSED does not
make sense unless we use -Wunused-param. And I really don't see how
this switch could help us increase code quality - only extra hassle
trying to silence warnings. We have so many functions used as callbacks
with defined APIs that we probably have unused parameters everywhere.
So, at this point, I'm leaning towards getting rid of ATTRIBUTE_UNUSED
and keeping the ARI (or perhaps poison it too).


-- 
Joel

[RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Tuesday, May 04, 2010 5:38 PM
> À : Pierre Muller
> Cc : gdb@sourceware.org; gdb-patches@sourceware.org
> Objet : Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
> 
> > Is this comment still valid, from the compiler point of view?
> > Does gcc still have such problems?
> 
> I don't know.
> 
> >   If yes, is it OK if I remove all ATTRIBUTE_UNUSED from sources?
> >   If no, shouldn't we change that policy?
> 
> I haven't thought much about it but, to me ATTRIBUTE_UNUSED does not
> make sense unless we use -Wunused-param. And I really don't see how
> this switch could help us increase code quality - only extra hassle
> trying to silence warnings. We have so many functions used as callbacks
> with defined APIs that we probably have unused parameters everywhere.
> So, at this point, I'm leaning towards getting rid of ATTRIBUTE_UNUSED
> and keeping the ARI (or perhaps poison it too).

  In that case, may I commit the following?

  Tested on gcc16, no changes found.

Pierre  


2010-05-04  Pierre Muller  <muller@ics.u-strasbg.fr>

	ARI fix: Remove ATTRIBUTE_UNUSED throughout.
	* arm-linux-tdep.c (arm_linux_cleanup_svc): Remove 
	ATTRIBUTE_UNUSED.
	(cleanup_kernel_helper_return): Likewise.
	* arm-tdep.c (copy_unmodified): Likewise.
	(copy_preload): Likewise.
	(copy_copro_load_store): Likewise.
	(cleanup_branch): Likewise.
	(copy_b_bl_blx): Likewise.
	(copy_bx_blx_reg): Likewise.
	(copy_alu_imm): Likewise.
	(copy_alu_reg): Likewise.
	(copy_alu_shifted_reg): Likewise.
	(cleanup_load): Likewise.
	(cleanup_store): Likewise.
	(cleanup_block_load_pc): Likewise.
	(cleanup_svc): Likewise.
	(copy_undef): Likewise.
	(copy_unpred): Likewise.
	* remote.c (register_remote_support_xml): Likewise.

Index: src/gdb/arm-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-linux-tdep.c,v
retrieving revision 1.75
diff -u -p -r1.75 arm-linux-tdep.c
--- src/gdb/arm-linux-tdep.c	29 Apr 2010 16:34:25 -0000	1.75
+++ src/gdb/arm-linux-tdep.c	4 May 2010 15:45:39 -0000
@@ -655,7 +655,7 @@ arm_linux_software_single_step (struct f
 /* Support for displaced stepping of Linux SVC instructions.  */
 
 static void
-arm_linux_cleanup_svc (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+arm_linux_cleanup_svc (struct gdbarch *gdbarch,
 		       struct regcache *regs,
 		       struct displaced_step_closure *dsc)
 {
@@ -776,7 +776,7 @@ arm_linux_copy_svc (struct gdbarch *gdba
    would have been called from the non-displaced location).  */
 
 static void
-cleanup_kernel_helper_return (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_kernel_helper_return (struct gdbarch *gdbarch,
 			      struct regcache *regs,
 			      struct displaced_step_closure *dsc)
 {
Index: src/gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.302
diff -u -p -r1.302 arm-tdep.c
--- src/gdb/arm-tdep.c	31 Mar 2010 22:10:07 -0000	1.302
+++ src/gdb/arm-tdep.c	4 May 2010 15:45:40 -0000
@@ -3702,7 +3702,7 @@ insn_references_pc (uint32_t insn, uint3
    matter what address they are executed at: in those cases, use this.  */
 
 static int
-copy_unmodified (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, uint32_t insn,
+copy_unmodified (struct gdbarch *gdbarch, uint32_t insn,
 		 const char *iname, struct displaced_step_closure *dsc)
 {
   if (debug_displaced)
@@ -3718,7 +3718,7 @@ copy_unmodified (struct gdbarch *gdbarch
 /* Preload instructions with immediate offset.  */
 
 static void
-cleanup_preload (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_preload (struct gdbarch *gdbarch,
 		 struct regcache *regs, struct displaced_step_closure *dsc)
 {
   displaced_write_reg (regs, dsc, 0, dsc->tmp[0], CANNOT_WRITE_PC);
@@ -3803,7 +3803,7 @@ copy_preload_reg (struct gdbarch *gdbarc
 /* Copy/cleanup coprocessor load and store instructions.  */
 
 static void
-cleanup_copro_load_store (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_copro_load_store (struct gdbarch *gdbarch,
 			  struct regcache *regs,
 			  struct displaced_step_closure *dsc)
 {
@@ -3857,7 +3857,7 @@ copy_copro_load_store (struct gdbarch *g
    PC).  */
 
 static void
-cleanup_branch (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, struct regcache
*regs,
+cleanup_branch (struct gdbarch *gdbarch, struct regcache *regs,
 		struct displaced_step_closure *dsc)
 {
   ULONGEST from = dsc->insn_addr;
@@ -3881,7 +3881,7 @@ cleanup_branch (struct gdbarch *gdbarch 
 /* Copy B/BL/BLX instructions with immediate destinations.  */
 
 static int
-copy_b_bl_blx (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, uint32_t insn,
+copy_b_bl_blx (struct gdbarch *gdbarch, uint32_t insn,
 	       struct regcache *regs, struct displaced_step_closure *dsc)
 {
   unsigned int cond = bits (insn, 28, 31);
@@ -3928,7 +3928,7 @@ copy_b_bl_blx (struct gdbarch *gdbarch A
 /* Copy BX/BLX with register-specified destinations.  */
 
 static int
-copy_bx_blx_reg (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, uint32_t insn,
+copy_bx_blx_reg (struct gdbarch *gdbarch, uint32_t insn,
 		 struct regcache *regs, struct displaced_step_closure *dsc)
 {
   unsigned int cond = bits (insn, 28, 31);
@@ -3966,7 +3966,7 @@ copy_bx_blx_reg (struct gdbarch *gdbarch
 /* Copy/cleanup arithmetic/logic instruction with immediate RHS. */
 
 static void
-cleanup_alu_imm (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_alu_imm (struct gdbarch *gdbarch,
 		 struct regcache *regs, struct displaced_step_closure *dsc)
 {
   ULONGEST rd_val = displaced_read_reg (regs, dsc->insn_addr, 0);
@@ -4027,7 +4027,7 @@ copy_alu_imm (struct gdbarch *gdbarch, u
 /* Copy/cleanup arithmetic/logic insns with register RHS.  */
 
 static void
-cleanup_alu_reg (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_alu_reg (struct gdbarch *gdbarch,
 		 struct regcache *regs, struct displaced_step_closure *dsc)
 {
   ULONGEST rd_val;
@@ -4096,7 +4096,7 @@ copy_alu_reg (struct gdbarch *gdbarch, u
 /* Cleanup/copy arithmetic/logic insns with shifted register RHS.  */
 
 static void
-cleanup_alu_shifted_reg (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_alu_shifted_reg (struct gdbarch *gdbarch,
 			 struct regcache *regs,
 			 struct displaced_step_closure *dsc)
 {
@@ -4170,7 +4170,7 @@ copy_alu_shifted_reg (struct gdbarch *gd
 /* Clean up load instructions.  */
 
 static void
-cleanup_load (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, struct regcache
*regs,
+cleanup_load (struct gdbarch *gdbarch, struct regcache *regs,
 	      struct displaced_step_closure *dsc)
 {
   ULONGEST rt_val, rt_val2 = 0, rn_val;
@@ -4200,7 +4200,7 @@ cleanup_load (struct gdbarch *gdbarch AT
 /* Clean up store instructions.  */
 
 static void
-cleanup_store (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, struct regcache
*regs,
+cleanup_store (struct gdbarch *gdbarch, struct regcache *regs,
 	       struct displaced_step_closure *dsc)
 {
   CORE_ADDR from = dsc->insn_addr;
@@ -4533,7 +4533,7 @@ cleanup_block_store_pc (struct gdbarch *
    must undo that here.  */
 
 static void
-cleanup_block_load_pc (struct gdbarch *gdbarch ATTRIBUTE_UNUSED,
+cleanup_block_load_pc (struct gdbarch *gdbarch,
 		       struct regcache *regs,
 		       struct displaced_step_closure *dsc)
 {
@@ -4729,7 +4729,7 @@ copy_block_xfer (struct gdbarch *gdbarch
    for Linux, where some SVC instructions must be treated specially.  */
 
 static void
-cleanup_svc (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, struct regcache
*regs,
+cleanup_svc (struct gdbarch *gdbarch, struct regcache *regs,
 	     struct displaced_step_closure *dsc)
 {
   CORE_ADDR from = dsc->insn_addr;
@@ -4773,7 +4773,7 @@ copy_svc (struct gdbarch *gdbarch, uint3
 /* Copy undefined instructions.  */
 
 static int
-copy_undef (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, uint32_t insn,
+copy_undef (struct gdbarch *gdbarch, uint32_t insn,
 	    struct displaced_step_closure *dsc)
 {
   if (debug_displaced)
@@ -4788,7 +4788,7 @@ copy_undef (struct gdbarch *gdbarch ATTR
 /* Copy unpredictable instructions.  */
 
 static int
-copy_unpred (struct gdbarch *gdbarch ATTRIBUTE_UNUSED, uint32_t insn,
+copy_unpred (struct gdbarch *gdbarch, uint32_t insn,
 	     struct displaced_step_closure *dsc)
 {
   if (debug_displaced)
Index: src/gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.403
diff -u -p -r1.403 remote.c
--- src/gdb/remote.c	3 May 2010 02:13:01 -0000	1.403
+++ src/gdb/remote.c	4 May 2010 15:45:42 -0000
@@ -3482,7 +3482,7 @@ static char *remote_support_xml;
 /* Register string appended to "xmlRegisters=" in qSupported query.  */
 
 void
-register_remote_support_xml (const char *xml ATTRIBUTE_UNUSED)
+register_remote_support_xml (const char *xml)
 {
 #if defined(HAVE_LIBEXPAT)
   if (remote_support_xml == NULL)

Re: [RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources

[Joel Brobecker]

> 2010-05-04  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	ARI fix: Remove ATTRIBUTE_UNUSED throughout.
> 	* arm-linux-tdep.c (arm_linux_cleanup_svc): Remove 
> 	ATTRIBUTE_UNUSED.
> 	(cleanup_kernel_helper_return): Likewise.
> 	* arm-tdep.c (copy_unmodified): Likewise.
> 	(copy_preload): Likewise.
> 	(copy_copro_load_store): Likewise.
> 	(cleanup_branch): Likewise.
> 	(copy_b_bl_blx): Likewise.
> 	(copy_bx_blx_reg): Likewise.
> 	(copy_alu_imm): Likewise.
> 	(copy_alu_reg): Likewise.
> 	(copy_alu_shifted_reg): Likewise.
> 	(cleanup_load): Likewise.
> 	(cleanup_store): Likewise.
> 	(cleanup_block_load_pc): Likewise.
> 	(cleanup_svc): Likewise.
> 	(copy_undef): Likewise.
> 	(copy_unpred): Likewise.
> 	* remote.c (register_remote_support_xml): Likewise.

Yes, please go ahead. If anyone ends up disagreeing, it's easy to
revert if this is where the discussion leads us.

-- 
Joel

RE: [RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources

[Pierre Muller]

> > 2010-05-04  Pierre Muller  <muller@ics.u-strasbg.fr>
> >
> > 	ARI fix: Remove ATTRIBUTE_UNUSED throughout.
> > 	* arm-linux-tdep.c (arm_linux_cleanup_svc): Remove
> > 	ATTRIBUTE_UNUSED.
> > 	(cleanup_kernel_helper_return): Likewise.
> > 	* arm-tdep.c (copy_unmodified): Likewise.
> > 	(copy_preload): Likewise.
> > 	(copy_copro_load_store): Likewise.
> > 	(cleanup_branch): Likewise.
> > 	(copy_b_bl_blx): Likewise.
> > 	(copy_bx_blx_reg): Likewise.
> > 	(copy_alu_imm): Likewise.
> > 	(copy_alu_reg): Likewise.
> > 	(copy_alu_shifted_reg): Likewise.
> > 	(cleanup_load): Likewise.
> > 	(cleanup_store): Likewise.
> > 	(cleanup_block_load_pc): Likewise.
> > 	(cleanup_svc): Likewise.
> > 	(copy_undef): Likewise.
> > 	(copy_unpred): Likewise.
> > 	* remote.c (register_remote_support_xml): Likewise.
> 
> Yes, please go ahead. If anyone ends up disagreeing, it's easy to
> revert if this is where the discussion leads us.

  Thanks, patch committed.

Pierre

Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Jan Kratochvil]

On Tue, 04 May 2010 17:37:52 +0200, Joel Brobecker wrote:
> make sense unless we use -Wunused-param. And I really don't see how
> this switch could help us increase code quality - only extra hassle
> trying to silence warnings.

While I agree ATTRIBUTE_UNUSED is more hassle than benefit I have to bring up
recently -Wunused-param would catch a real bug in an Archer patch
(missing "bp->thread = tp->num;"):
	http://sourceware.org/ml/archer/2010-q2/msg00017.html


Regards,
Jan

RE: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la
> part de Jan Kratochvil
> Envoyé : Tuesday, May 04, 2010 6:23 PM
> À : Joel Brobecker
> Cc : Pierre Muller; gdb@sourceware.org; gdb-patches@sourceware.org
> Objet : Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
> 
> On Tue, 04 May 2010 17:37:52 +0200, Joel Brobecker wrote:
> > make sense unless we use -Wunused-param. And I really don't see how
> > this switch could help us increase code quality - only extra hassle
> > trying to silence warnings.
> 
> While I agree ATTRIBUTE_UNUSED is more hassle than benefit I have to
> bring up
> recently -Wunused-param would catch a real bug in an Archer patch
> (missing "bp->thread = tp->num;"):
> 	http://sourceware.org/ml/archer/2010-q2/msg00017.html

  Did you really use -Wunused-param to catch this,
or is it just a case where it could have been useful 
to have this warning?

  I found no easy way to add some extra -Wunused option to
the compiler flags...
  Adding it to CFLAGS has no effects as there is a -Wno-unused
inside WARN_CFLAGS ...

  Maybe we could add a EXTRA_WARN_CFLAGS to Makefile.in

Pierre 

Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Jan Kratochvil]

On Tue, 04 May 2010 22:58:58 +0200, Pierre Muller wrote:
> > -----Message d'origine-----
> > De : gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] De la
> > part de Jan Kratochvil
> > recently -Wunused-param would catch a real bug in an Archer patch
> > (missing "bp->thread = tp->num;"):
> > 	http://sourceware.org/ml/archer/2010-q2/msg00017.html
> 
>   Did you really use -Wunused-param to catch this,
> or is it just a case where it could have been useful 
> to have this warning?

I haven't tried to use -Wunused-param; it was spotted just by eyes.


Regards,
Jan

Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources

[Joel Brobecker]

> >   Did you really use -Wunused-param to catch this,
> > or is it just a case where it could have been useful 
> > to have this warning?
> 
> I haven't tried to use -Wunused-param; it was spotted just by eyes.

What I think Jan and I wanted to say is that -Wunused-param is not
useless; but that it comes at too high a price for too little gain.
The example that Jan provided was meant to be a real-life example
where the warning would have been useful (assuming that the high
noise/signal ration haven't conditioned us to simply silence it without
looking further).

-- 
Joel