Re: [PATCH v2] Use DIAG_IGNORE_NEEDS_COMMENT to silence -Wstringop-truncation

[Pedro Alves <palves at redhat dot com>]

On 05/18/2018 03:56 PM, H.J. Lu wrote:
> On Wed, May 16, 2018 at 7:33 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 05/16/2018 09:54 PM, Alan Modra wrote:
>>> On Mon, May 14, 2018 at 10:09:53PM -0400, Carlos O'Donell wrote:
>>>> On 05/07/2018 11:32 PM, Alan Modra wrote:
>>>>> Also, not all binutils users have glibc installed.  We can't depend on
>>>>> a macro defined in glibc's /usr/include/features.h, __GNUC_PREREQ.
>>>>
>>>> Just to be clear the patch doesn't depend on glibc being installed, it
>>>> just copies the presently used macros from glibc for use in binutils.
>>>>
>>>> ... and then uses them to fix the gcc 8 issues that crop up with -Werror.
>>>
>>> I checked HJ's patch again, and stand by my comment about
>>> __GNUC_PREREQ.  This macro is not defined anywhere in the binutils
>>> sources, nor is it defined by HJ's patch.
>>
>> My apologies, I misread your original statement. I completely agree.
>>
>> Why do we need __GNUC_PREREQ macro?
>>
>> You can unconditionally use the DIAG_* macros, and if you're
>> __GNUC__ >= 8 then they evaluate to meaningful things, otherwise
>> nothing.
> 
> Here is the patch without __GNUC_PREREQ.  OK for master branch?

> +#if __GNUC__ >= 8
> +/* These are copied from glibc 2.27.  */

That doesn't seems right to me.

The __GNUC__ >= 8 check should control disabling this specific
instance of the warning disablement, not what the DIAG_IGNORE_NEEDS_COMMENT
macro expands too.  Older GCCs support GCC diagnostic push/pop too.

Consider what we'd have to do to use DIAG_IGNORE_NEEDS_COMMENT
in some other case where we'll want to disable a warning on
older GCC versions.  E.g., if you wanted to do:

  DIAG_IGNORE_NEEDS_COMMENT (5, "-Wmaybe-uninitialized");

you'd then have to lower the __GNUC__ >= 8 in the
definition of DIAG_IGNORE_NEEDS_COMMENT to __GNUC__ >= 5
at least.  That seems like a design issue to me.

Also, DIAG_IGNORE_NEEDS_COMMENT's "version" parameter is
GCC biased.  That is fine for glibc, I guess, which basically
requires GCC, but in binutils, what if we want to handle
disabling a warning for clang, say?

We handle this in gdb a bit differently, and I think in a
better way for projects other than glibc.  See 
gdb/common/diagnostics.h:

  https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/diagnostics.h;hb=HEAD

We have DIAGNOSTIC_PUSH/DIAGNOSTIC_POP/DIAGNOSTIC_IGNORE
macros, too, but we don't use those directly in "client" code.
Instead, we define warning-class-specific macros on top of
the base DIAGNOSTIC_{PUSH,POP,IGNORE} ones, and use those
higher level ones throughout.  E.g., in this case, we'd add a new:

 #define DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION \
    DIAGNOSTIC_IGNORE ("-Wstringop-truncation")

in diagnostics.h, defined appropriately for the different compilers,
and then use DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION throughout instead.

I wonder whether it'd make sense to share the code between
gdb and binutils?

Also, if we stick to a bfd-only solution, shouldn't the macros
be prefixed with BFD_ ?

Thanks,
Pedro Alves