Re: [PATCH 2/3] Add DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION

[Pedro Alves <palves at redhat dot com>]

On 06/04/2018 01:46 PM, H.J. Lu wrote:
> On Mon, Jun 4, 2018 at 5:19 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/04/2018 01:13 PM, Nick Clifton wrote:
>>> Hi H.J.
>>>
>>>> +# if __GNUC__ == 8 && __GNUC_MINOR__ < 2
>>>> +#  define DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION \
>>>> +  DIAGNOSTIC_IGNORE ("-Wstringop-truncation")
>>>> +# endif
>>>>
>>>> OK for master?
>>>
>>> Approved - please apply.
>>
>> Please don't.  This is again going against the intention of
>> the header.  The GCC version checks should be put in the
>> places where the warning needs to be suppressed.
>> The current patch makes all current and future uses of
>> DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION only apply to
>> GCC 8.1.  That is incorrect.  Consider what you will
>> have to do to suppress some -Wstrinop-truncation warning
>> with DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION on GCC 8.2 or
>> GCC 9 or whatever.
> 

> This is what my original patch intended to do.  But diagnostics.h from
> GDB doesn't support GCC version. > Should I extend it to match glibc?

diagnostics.h doesn't support a GCC version parameter on purpose, as I
explained before.  A single GCC version argument like glibc does
only works for GCC, and is in fact not used by the macro at all,
it is only there for conveniently grepping.  It's a nop argument:

#define DIAG_IGNORE_NEEDS_COMMENT(version, option)     \
  _Pragma (_DIAG_STR (GCC diagnostic ignored option))

I've detailed more here on why I don't think that's right:

 https://sourceware.org/ml/binutils/2018-05/msg00191.html



We mainly want to use DIAGNOSTIC_IGNORE_XXX to disable warnings 
around code that even though is seemingly smelly, is actually
what we want to write.  See these examples:

 https://sourceware.org/ml/gdb-patches/2017-06/msg00622.html
 https://sourceware.org/ml/gdb-patches/2017-12/msg00529.html

The case in question is a little different, as the warning is
a false positive in these particular spots.  So I think we should
disable it in these particular spots, only.  I.e., I think a version
check in the client side (like your original patch, IIRC, though
implemented differently) is appropriate, like:

+	DIAGNOSTIC_PUSH;
+	/* GCC 8.1 warns about 80 equals destination size with
+	   -Wstringop-truncation:
+	   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85643
+	 */
+#if GCC_VERSION == 8001
+	DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION;
+#endif
 	strncpy (data + 44, va_arg (ap, const char *), 80);
+	DIAGNOSTIC_POP;

It would be fine with me to add the GCC version check to diagnostics.h,
if, when you consider what to do when you need to disable 
-Wstringop-truncation in some future GCC version, in some
other spot in the code, you decide that if that happens,
it's OK to disable -Wstringop-truncation everywhere 
DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION is used at that point
in time, even if the current spots that use
DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION won't trigger false positive
with the newer GCC version that is forcing you to change the
version check.  That seems to go counter to the expressed desire
to only disable -Wstringop-truncation in these spots with 8.1
but not 8.2, though.  That's a design conflict.

>> Also, you can use GCC_VERSION to make the version check
>> a little simpler.

Thanks,
Pedro Alves