diff mbox

[review] Fix unused function error

Message ID gerrit.1576076440000.Iad6123d61d76d111e3ef8d24aa8c60112304c749@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Dec. 11, 2019, 3 p.m. UTC 
  Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................

Fix unused function error

Attempting to build GDB in Ubuntu 18.04.3 on x86_64, i ran into warnings
that caused the build to fail. For some reason the diagnostics macros don't
work for my case, but ATTRIBUTE_UNUSED does.

gdb/ChangeLog:

2019-12-11  Luis Machado  <luis.machado@linaro.org>

	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
	(select_strerror_r): Use ATTRIBUTED_UNUSED instead of the diagnostics
	macros.

Change-Id: Iad6123d61d76d111e3ef8d24aa8c60112304c749
---
M gdb/gdbsupport/safe-strerror.c
1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Simon Marchi (Code Review) Dec. 11, 2019, 4:04 p.m. UTC | #1 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 2:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,20 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 12:16:22 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 18.04.3 on x86_64, i ran into warnings
| +that caused the build to fail. For some reason the diagnostics macros don't
| +work for my case, but ATTRIBUTE_UNUSED does.

PS2, Line 11:

What does the warning that you get look like?

Which gcc version do you have?

Does the DIAGNOSTIC_IGNORE_UNUSED_FUNCTION not work for you because it
ends up defined as empty for you for some reason?

Or can you reproduce this by unrolling the macros and using _Pragma
directly?

(typo: i -> I)

| +
| +The compiler version is gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0.
| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTED_UNUSED instead of the diagnostics
Simon Marchi (Code Review) Dec. 11, 2019, 5:05 p.m. UTC | #2 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 2:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,20 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 12:16:22 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 18.04.3 on x86_64, i ran into warnings
| +that caused the build to fail. For some reason the diagnostics macros don't
| +work for my case, but ATTRIBUTE_UNUSED does.

PS2, Line 11:

It looks like this:

binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char*
select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-
function]  select_strerror_r (char *res, char *)

I reported a GCC version of 7.4.0, but in reality the box runs GCC
5.4.0.

As for the DIAGNOSTIC_IGNORE_UNUSED_FUNCTION macro, I gave this a try
and it seems the macros do get expanded, but they still have no effect
on the warning.

I replaced the macros with the pragmas to make sure. Still doesn't
work.

| +
| +The compiler version is gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0.
| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTED_UNUSED instead of the diagnostics
Simon Marchi (Code Review) Dec. 11, 2019, 5:17 p.m. UTC | #3 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 3:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

If this is legitimately a GCC bug, then I see two choices:

#1 - adjust to DIAGNOSTIC_IGNORE_UNUSED_FUNCTION to ignore "-Wunused"
instead of "-Wunused-function" for buggy gcc versions, assuming that
works.

#2 - switch to ATTRIBUTE_UNUSED like you have, but then we should nuke
DIAGNOSTIC_IGNORE_UNUSED_FUNCTION, since it doesn't work as
advertised.

ATTRIBUTE_UNUSED on the function like you have seems simpler for not
having to push/pop, aka not requiring global context.  So I'm fine
with option #2.

What doesn't look good to me is working around one case but letting
the bug latent for another user of DIAGNOSTIC_IGNORE_UNUSED_FUNCTION.
There's no other user of DIAGNOSTIC_IGNORE_UNUSED_FUNCTION currently,
AFAICS, so #2 should be trivial.

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 5:26 p.m. UTC | #4 
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

As a sidenote, ARI currently doesn't like ATTRIBUTE_UNUSED but tromey
is removing that in https://gnutoolchain-gerrit.osci.io/r/c/binutils-
gdb/+/742

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 5:58 p.m. UTC | #5 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

I tried #1 with "-Wunused" and it doesn't work.

I'm trying to find some more information on when exactly this ceased
to be a problem (if at all). I take it this was fixed, since Christian
didn't see this problem, neither did the buildbot after the first fix.

The warnings in include/diagnostics.h tell us to use these macros
guarded by known versions, like so:

#if __GNUC__ >= 10
#  pragma GCC diagnostic push
#  pragma GCC diagnostic ignored "-Wformat-diag"
#endif

We could have a construct like the above, guarded for known-to-work
compiler versions, and use ATTRIBUTE_UNUSED for the rest.

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 6:03 p.m. UTC | #6 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

> Patch Set 4:
> 
> (1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

> We could have a construct like the above, guarded for known-to-work compiler versions, and use ATTRIBUTE_UNUSED for the rest.

Can't see how that is better than the option #2 I gave?

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 6:19 p.m. UTC | #7 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

Ok. Let's go with #2 then.

I actually found the PR and the fix here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64079

Should we at least put a comment in place in case someone tries to put
this particular macro up again?

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 6:23 p.m. UTC | #8 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +1,30 @@ 
| +Parent:     1d61b032 (Remove more shifts for sign/zero extension)
| +Author:     Luis Machado <luis.machado@linaro.org>
| +AuthorDate: 2019-12-11 11:55:49 -0300
| +Commit:     Luis Machado <luis.machado@linaro.org>
| +CommitDate: 2019-12-11 14:06:52 -0300
| +
| +Fix unused function error
| +
| +Attempting to build GDB in Ubuntu 16.04.6 LTS on x86_64, I ran into warnings
| +that caused the build to fail:
| +
| +binutils-gdb/gdb/gdbsupport/safe-strerror.c:44:1: error: ‘char* select_strerror_r(char*, char*)’ defined but not used [-Werror=unused-function]  select_strerror_r (char *res, char *)
| +
| +The diagnostics macros seem to expand correctly to their respective pragmas,
| +but it doesn't seem to have an effect on the warning. I tried to use the
| +pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.

PS3, Line 21:

> I actually found the PR and the fix here:

Thanks for digging.

> Should we at least put a comment in place in case someone tries to put this particular macro up again?

Sure.

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 11, 2019, 6:25 p.m. UTC | #9 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 4:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +17,15 @@ pragmas explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case, if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.
| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdb/gdbsupport/safe-strerror.c: Remove diagnostics.h

PS4, Line 27:

Drop the leading "gdb/".

Missing period.

But while at it, say "Don't include diagnostics.h.".
"remove foo.h" sounds like you're deleting the file to me. :-)

| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
| +Change-Id: Iad6123d61d76d111e3ef8d24aa8c60112304c749
Simon Marchi (Code Review) Dec. 11, 2019, 7:54 p.m. UTC | #10 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 5:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +16,22 @@ correctly to its respective pragma, but this doesn't seem to have an effect on
| +the warning. I tried to use the pragma explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.
| +
| +This is likely the result of PR64079 in GCC, which was fixed by commit
| +9e96f1e1b9731c4e1ef4fbbbf0997319973f0537.
| +
| +To prevent other developers from attempting to use this macro, only to get
| +confused by it not working as expected, it seems better to not define this
| +particular macro when the compiler is GCC.

PS5, Line 28:

No, remove the macro for all compilers.  Eliminate it completely.
There's no point in using it when we can use ATTRIBUTE_UNUSED instead.

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdbsupport/safe-strerror.c: Don't include diagnostics.h.
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 12, 2019, 11:15 a.m. UTC | #11 
Luis Machado has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 6:

(1 comment)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +16,22 @@ correctly to its respective pragma, but this doesn't seem to have an effect on
| +the warning. I tried to use the pragma explicitly and got the same result.
| +
| +ATTRIBUTE_UNUSED works fine in this case if you put it in both functions,
| +which should fix warnings for both gdb and gdbserver builds.
| +
| +The compiler version is gcc (Ubuntu 5.4.0-6ubuntu1~16.04.11) 5.4.0 20160609.
| +
| +This is likely the result of PR64079 in GCC, which was fixed by commit
| +9e96f1e1b9731c4e1ef4fbbbf0997319973f0537.
| +
| +To prevent other developers from attempting to use this macro, only to get
| +confused by it not working as expected, it seems better to not define this
| +particular macro when the compiler is GCC.

PS5, Line 28:

Got it. Done in this updated version now.

| +
| +gdb/ChangeLog:
| +
| +2019-12-11  Luis Machado  <luis.machado@linaro.org>
| +
| +	* gdbsupport/safe-strerror.c: Don't include diagnostics.h.
| +	(select_strerror_r): Use ATTRIBUTE_UNUSED instead of the diagnostics
| +	macros.
| +
Simon Marchi (Code Review) Dec. 12, 2019, 11:19 a.m. UTC | #12 
Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/753
......................................................................


Patch Set 6: Code-Review+2
diff mbox

Patch

diff --git a/gdb/gdbsupport/safe-strerror.c b/gdb/gdbsupport/safe-strerror.c
index 9973fa6..9126507 100644
--- a/gdb/gdbsupport/safe-strerror.c
+++ b/gdb/gdbsupport/safe-strerror.c
@@ -18,7 +18,6 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "common-defs.h"
-#include "diagnostics.h"
 #include <string.h>
 
 /* There are two different versions of strerror_r; one is GNU-specific, the
@@ -29,25 +28,21 @@ 
 
 /* We only ever use one of the two overloads, so suppress the warning for
    an unused function.  */
-DIAGNOSTIC_PUSH
-DIAGNOSTIC_IGNORE_UNUSED_FUNCTION
 
 /* Called if we have a XSI-compliant strerror_r.  */
-static char *
+ATTRIBUTE_UNUSED static char *
 select_strerror_r (int res, char *buf)
 {
   return res == 0 ? buf : nullptr;
 }
 
 /* Called if we have a GNU strerror_r.  */
-static char *
+ATTRIBUTE_UNUSED static char *
 select_strerror_r (char *res, char *)
 {
   return res;
 }
 
-DIAGNOSTIC_POP
-
 /* Implementation of safe_strerror as defined in common-utils.h.  */
 
 const char *