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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, May 18, 2018 at 9:21 AM, Pedro Alves <palves@redhat.com> wrote:
> 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?
>

Yes, gdb and binutils should share them.  Can you move gdb/common/diagnostics.h
to include?

Thanks.

-- 
H.J.