Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters

[Jeff Law <law@redhat.com>]

On 9/23/20 11:45 AM, Martin Sebor via Gcc-patches wrote:
> On 9/23/20 9:44 AM, Szabolcs Nagy wrote:
>> The 09/23/2020 09:22, Szabolcs Nagy wrote:
>>> The 09/21/2020 12:45, Martin Sebor via Gcc-patches wrote:
>>>> On 9/21/20 12:20 PM, Vaseeharan Vinayagamoorthy wrote:
>>>>> After this patch, I am seeing this -Warray-parameter error:
>>>>>
>>>>> In file included from ../include/pthread.h:1,
>>>>>                    from ../sysdeps/nptl/thread_db.h:25,
>>>>>                    from ../nptl/descr.h:32,
>>>>>                    from ../sysdeps/aarch64/nptl/tls.h:44,
>>>>>                    from ../include/errno.h:25,
>>>>>                    from ../sysdeps/unix/sysv/linux/sysdep.h:23,
>>>>>                    from 
>>>>> ../sysdeps/unix/sysv/linux/generic/sysdep.h:22,
>>>>>                    from 
>>>>> ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:24,
>>>>>                    from <stdin>:1:
>>>>> ../sysdeps/nptl/pthread.h:734:47: error: argument 1 of type 
>>>>> ‘struct __jmp_buf_tag *’ declared as a pointer 
>>>>> [-Werror=array-parameter=]
>>>>>     734 | extern int __sigsetjmp (struct __jmp_buf_tag *__env, int 
>>>>> __savemask) __THROWNL;
>>>>>         | ~~~~~~~~~~~~~~~~~~~~~~^~~~~
>>>>> In file included from ../include/setjmp.h:2,
>>>>>                    from ../nptl/descr.h:24,
>>>>>                    from ../sysdeps/aarch64/nptl/tls.h:44,
>>>>>                    from ../include/errno.h:25,
>>>>>                    from ../sysdeps/unix/sysv/linux/sysdep.h:23,
>>>>>                    from 
>>>>> ../sysdeps/unix/sysv/linux/generic/sysdep.h:22,
>>>>>                    from 
>>>>> ../sysdeps/unix/sysv/linux/aarch64/sysdep.h:24,
>>>>>                    from <stdin>:1:
>>>>> ../setjmp/setjmp.h:54:46: note: previously declared as an array 
>>>>> ‘struct __jmp_buf_tag[1]’
>>>>>      54 | extern int __sigsetjmp (struct __jmp_buf_tag __env[1], 
>>>>> int __savemask) __THROWNL;
>>>>>         | ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
>>>>> cc1: all warnings being treated as errors
>>>>
>>>> The warning flags differences between the forms of array parameters
>>>> in redeclarations of the same function, including pointers vs arrays
>>>> as in this instance.  It needs to be suppressed in glibc, either by
>>>> making the function declarations consistent, or by #pragma diagnostic.
>>>> (IIRC, the pointer declaration comes before struct __jmp_buf_tag has
>>>> been defined so simply using the array form there doesn't work without
>>>> defining the type first.)
>>>>
>>>> I would expect the warning to be suppressed when using the installed
>>>> library thanks to -Wno-system-headers.
>>>
>>> why is this a warning? i'm not convinced it
>>> should be in -Wall.
>
> The main motivation for the warning is to detect unintentional
> inconsistencies between function redeclarations that make deriving
> true true intent difficult or impossible  (e.g, T[3] vs T[1], or
> T[] vs T[1], or equivalently T* vs T[1]).
>
> One goal is to support the convention where a constant array bound
> in a function array parameter is used in lieu of the [static N]
> notation (i.e., the minimum number of elements the caller is
> expected to  make available).  The [static N] notation is little
> known, used only exceedingly rarely, and isn't available in C++.
> The array notation is used more often, although by no means common.
>
> The ultimate goal is to motivate users to take advantage of GCC's
> ability to check ordinary functions for out-of-bounds accesses to
> array arguments.  The checking is only feasible if all declarations
> of the same function, including its definition, use a consistent
> notation to specify the same bound.  Including the strict
> -Warray-parameter=2 setting in -Wall helps support this goal
> (-Warray-parameter=1 doesn't warn for mismatches in the forms
> of ordinary array bounds without [static].)
>
> I mentioned the results of testing the patch with a number of
> packages, including Glibc, Binutils/GDB, Glibc, and the kernel,
> in the overview of the patch series:
> https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550920.html
> It explains why I chose not to relax the warning to accommodate
> the Glibc use case.
>
> Based on my admittedly limited testing I'm not concerned about
> the warning having adverse effects.  But if broader excposure
> shows that it is prone to some it can certainly be adjusted.
> Jeff does periodic mass rebuilds of all of Fedora with the top
> of GCC trunk so we should know soon.

Yea.  If the patch was in last week's snapshot, then it should start 
spinning the 9000 Fedora packages later tonight alongside some Ranger 
bits we're testing.  I'm on PTO till Monday though, so I won't be 
looking at the results until Tuesday probably...


jeff