[2/3] Add DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION

  On Fri, Jun 1, 2018 at 3:19 AM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Jun 01, 2018 at 08:57:20AM +0100, Nick Clifton wrote:
>> Hi H.J.
>>
>> > +# if __GNUC__ >= 8
>> > +#  define DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION \
>> > +  DIAGNOSTIC_IGNORE ("-Wstringop-truncation")
>> > +# endif
>> > +#endif
>>
>> Presumably the Wstringop-truncation bug will be fixed in gcc 8.1
>> so shouldn't the test check the revision number as well ?
>
> Yes, it has already been fixed.
>

The bug is in GCC 8.1 and will be fixed in GCC 8.2.  Here is the
updated patch with

+# if __GNUC__ == 8 && __GNUC_MINOR__ < 2
+#  define DIAGNOSTIC_IGNORE_STRINGOP_TRUNCATION \
+  DIAGNOSTIC_IGNORE ("-Wstringop-truncation")
+# endif

OK for master?
  

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.

Cheers
  Nick
  

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.

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

Thanks,
Pedro Alves
  

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?

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

On Mon, Jun 4, 2018 at 5:46 AM, H.J. Lu <hjl.tools@gmail.com> 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?
>
>> Also, you can use GCC_VERSION to make the version check
>> a little simpler.
>>
>> Thanks,
>> Pedro Alves

My second patch is needed for Darwin.   I will check it in as is
and update it with a follow up patch to support GCC version.
  

On 06/04/2018 02:30 PM, H.J. Lu wrote:
> On Mon, Jun 4, 2018 at 5:46 AM, H.J. Lu <hjl.tools@gmail.com> 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?
>>
>>> Also, you can use GCC_VERSION to make the version check
>>> a little simpler.
> 
> My second patch is needed for Darwin.   I will check it in as is
> and update it with a follow up patch to support GCC version.

For the record, I think it would have been better procedure
to split and push the DIAGNOSTIC_STRINGIFY bit only [1], while
giving people reasonable time to respond to the rest.

https://sourceware.org/ml/gdb-patches/2018-06/msg00062.html

Thanks,
Pedro Alves
  

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