* [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
@ 2010-05-04 13:49 Pierre Muller
2010-05-04 15:38 ` Joel Brobecker
0 siblings, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2010-05-04 13:49 UTC (permalink / raw)
To: gdb; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 13:49 [ARI] Status of ATTRIBUTE_UNUSED in gdb sources Pierre Muller
@ 2010-05-04 15:38 ` Joel Brobecker
2010-05-04 16:05 ` [RFA] Get rid " Pierre Muller
2010-05-04 16:23 ` [ARI] Status " Jan Kratochvil
0 siblings, 2 replies; 9+ messages in thread
From: Joel Brobecker @ 2010-05-04 15:38 UTC (permalink / raw)
To: Pierre Muller; +Cc: gdb, gdb-patches
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 15:38 ` Joel Brobecker
@ 2010-05-04 16:05 ` Pierre Muller
2010-05-05 14:42 ` Joel Brobecker
2010-05-04 16:23 ` [ARI] Status " Jan Kratochvil
1 sibling, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2010-05-04 16:05 UTC (permalink / raw)
To: 'Joel Brobecker'; +Cc: gdb, gdb-patches
> -----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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 15:38 ` Joel Brobecker
2010-05-04 16:05 ` [RFA] Get rid " Pierre Muller
@ 2010-05-04 16:23 ` Jan Kratochvil
2010-05-04 20:59 ` Pierre Muller
1 sibling, 1 reply; 9+ messages in thread
From: Jan Kratochvil @ 2010-05-04 16:23 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Pierre Muller, gdb, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 16:23 ` [ARI] Status " Jan Kratochvil
@ 2010-05-04 20:59 ` Pierre Muller
2010-05-04 21:15 ` Jan Kratochvil
0 siblings, 1 reply; 9+ messages in thread
From: Pierre Muller @ 2010-05-04 20:59 UTC (permalink / raw)
To: 'Jan Kratochvil', 'Joel Brobecker'; +Cc: gdb, gdb-patches
> -----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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 20:59 ` Pierre Muller
@ 2010-05-04 21:15 ` Jan Kratochvil
2010-05-05 14:40 ` Joel Brobecker
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kratochvil @ 2010-05-04 21:15 UTC (permalink / raw)
To: Pierre Muller; +Cc: 'Joel Brobecker', gdb, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [ARI] Status of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 21:15 ` Jan Kratochvil
@ 2010-05-05 14:40 ` Joel Brobecker
0 siblings, 0 replies; 9+ messages in thread
From: Joel Brobecker @ 2010-05-05 14:40 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Pierre Muller, gdb, gdb-patches
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources
2010-05-04 16:05 ` [RFA] Get rid " Pierre Muller
@ 2010-05-05 14:42 ` Joel Brobecker
2010-05-05 15:09 ` Pierre Muller
0 siblings, 1 reply; 9+ messages in thread
From: Joel Brobecker @ 2010-05-05 14:42 UTC (permalink / raw)
To: Pierre Muller; +Cc: gdb, gdb-patches
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFA] Get rid of ATTRIBUTE_UNUSED in gdb sources
2010-05-05 14:42 ` Joel Brobecker
@ 2010-05-05 15:09 ` Pierre Muller
0 siblings, 0 replies; 9+ messages in thread
From: Pierre Muller @ 2010-05-05 15:09 UTC (permalink / raw)
To: 'Joel Brobecker'; +Cc: gdb, gdb-patches
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-05-05 15:09 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-04 13:49 [ARI] Status of ATTRIBUTE_UNUSED in gdb sources Pierre Muller
2010-05-04 15:38 ` Joel Brobecker
2010-05-04 16:05 ` [RFA] Get rid " Pierre Muller
2010-05-05 14:42 ` Joel Brobecker
2010-05-05 15:09 ` Pierre Muller
2010-05-04 16:23 ` [ARI] Status " Jan Kratochvil
2010-05-04 20:59 ` Pierre Muller
2010-05-04 21:15 ` Jan Kratochvil
2010-05-05 14:40 ` Joel Brobecker
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox