diff mbox

V2: Fix compile warning building testcase

Message ID a2ed58c1-4606-4505-a615-4a84cca93963@codethink.co.uk
State New
Headers show

Commit Message

Sam Thursfield March 8, 2017, 12:55 p.m. UTC 
Hi Ian

Thanks a lot for reviewing the patch. I've attached a new version with 
some comments below.

On 08/03/17 04:03, Ian Lance Taylor via gcc-patches wrote:
> Thanks.  The patch submission is fine but 1) you didn't see which
> version of GCC you are using; 2) I don't understand why it works.  If
> BACKTRACE_SUPPORTED is 0, then I would expect that you would see a
> warning for test1, test2, test3, and test4.  Your patch doesn't fix
> those, so it sounds like you are only seeing the warning for test5, so
> BACKTRACE_SUPPORTED must be 1..  test5 is not compiled if
> BACKTRACE_SUPPORTS_DATA is 0, so BACKTRACE_SUPPORTS_DATA must be 1.
> So both BACKTRACE_SUPPORTED and BACKTRACE_SUPPORTS_DATA must be 1, so
> I don't understand what your patch fixes.

I'm using GCC 6.3.0 here to compile.

I see your point about the fix not making sense. The values of the
defines are this:

     #define BACKTRACE_SUPPORTED 0
     #define BACKTRACE_SUPPORTS_DATA 1

I noticed that test1(), test2(), test3() and test4() all have the
"unused" attribute:

     static int test1 (void) __attribute__ ((noinline, unused));
     static inline int test2 (void) __attribute__ ((always_inline, unused));
     static int test3 (void) __attribute__ ((noinline, unused));
     static inline int test4 (void) __attribute__ ((always_inline, unused));

So that will be why they don't trigger -Wunused-function :-)

It seems to me now that it'd be neater to add that same attribute to 
test5(), instead of using an #ifdef guard. Attached is a new patch that 
does so.

Again, I've tested this using GCC 6.3.0 on powerpc-ibm-aix7.2.0.0.

Sam

Comments

Li, Pan2 via Gcc-patches March 8, 2017, 2:21 p.m. UTC | #1 
On Wed, Mar 8, 2017 at 4:55 AM, Sam Thursfield
<sam.thursfield@codethink.co.uk> wrote:
>
> Thanks a lot for reviewing the patch. I've attached a new version with some
> comments below.
>
> On 08/03/17 04:03, Ian Lance Taylor via gcc-patches wrote:
>>
>> Thanks.  The patch submission is fine but 1) you didn't see which
>> version of GCC you are using; 2) I don't understand why it works.  If
>> BACKTRACE_SUPPORTED is 0, then I would expect that you would see a
>> warning for test1, test2, test3, and test4.  Your patch doesn't fix
>> those, so it sounds like you are only seeing the warning for test5, so
>> BACKTRACE_SUPPORTED must be 1..  test5 is not compiled if
>> BACKTRACE_SUPPORTS_DATA is 0, so BACKTRACE_SUPPORTS_DATA must be 1.
>> So both BACKTRACE_SUPPORTED and BACKTRACE_SUPPORTS_DATA must be 1, so
>> I don't understand what your patch fixes.
>
>
> I'm using GCC 6.3.0 here to compile.
>
> I see your point about the fix not making sense. The values of the
> defines are this:
>
>     #define BACKTRACE_SUPPORTED 0
>     #define BACKTRACE_SUPPORTS_DATA 1
>
> I noticed that test1(), test2(), test3() and test4() all have the
> "unused" attribute:
>
>     static int test1 (void) __attribute__ ((noinline, unused));
>     static inline int test2 (void) __attribute__ ((always_inline, unused));
>     static int test3 (void) __attribute__ ((noinline, unused));
>     static inline int test4 (void) __attribute__ ((always_inline, unused));
>
> So that will be why they don't trigger -Wunused-function :-)
>
> It seems to me now that it'd be neater to add that same attribute to
> test5(), instead of using an #ifdef guard. Attached is a new patch that does
> so.

Makes sense.  Thanks.  Committed to mainline.

Ian
diff mbox

Patch

From 87b52dee5ca3b8feca379877c873c22b673f1b7b Mon Sep 17 00:00:00 2001
From: Sam Thursfield <sam.thursfield@codethink.co.uk>
Date: Wed, 8 Mar 2017 06:48:30 -0600
Subject: [PATCH] libbacktrace: Fix compile warning building testcase

This warning occurred when the BACKTRACE_SUPPORTED flag is defined to 0.

    ../../../gcc/libbacktrace/btest.c:624:1: error: 'test5' defined but not used [-Werror=unused-function]
     test5 (void)
     ^~~~~
    cc1: all warnings being treated as errors

The other test functions have the 'unused' attribute, presumably to
avoid similar errors, so let's handle test5() the same way. instead
of using a #ifdef guard.

libbacktrace/:

2017-03-08  Sam Thursfield  <sam.thursfield@codethink.co.uk>

       * btest.c (test5): Replace #ifdef guard with 'unused' attribute
       to fix compile warning when BACKTRACE_SUPPORTED isn't defined.
---
 libbacktrace/btest.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libbacktrace/btest.c b/libbacktrace/btest.c
index e28a3d8..92cb325 100644
--- a/libbacktrace/btest.c
+++ b/libbacktrace/btest.c
@@ -616,7 +616,7 @@  f33 (int f1line, int f2line)
   return failures;
 }
 
-#if BACKTRACE_SUPPORTS_DATA
+static int test5 (void) __attribute__ ((unused));
 
 int global = 1;
 
@@ -686,8 +686,6 @@  test5 (void)
   return failures;
 }
 
-#endif /* BACKTRACE_SUPPORTS_DATA  */
-
 static void
 error_callback_create (void *data ATTRIBUTE_UNUSED, const char *msg,
 		       int errnum)
-- 
2.2.2