Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
The 09/23/2020 21:45, Jeff Law wrote: > 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 :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 :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
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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. Martin this is a warning with false positives that have no portable workaround and does not really help catching bugs (at least i doubt inconsistent array vs pointer declaration is causing common problems). what gcc should warn about is if there is an array style argument declaration and a caller passes an array that's provably shorter than that. i take this back: such warning only makes sense when the static keyword is used in the array declarator. i really think this issue should be fixed in gcc and not in glibc: it's not true that the argument has array type, the standard requires the parameter type to be adjusted: A declaration of a parameter as "array of type" shall be adjusted to "qualified pointer to type" this warning will just confuse users and make them believe that the two declaration styles for pointer arguments are somehow different.
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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. > > this is a warning with false positives that > have no portable workaround and does not > really help catching bugs (at least i doubt > inconsistent array vs pointer declaration > is causing common problems). > > what gcc should warn about is if there is an > array style argument declaration and a caller > passes an array that's provably shorter than > that. i take this back: such warning only makes sense when the static keyword is used in the array declarator. i really think this issue should be fixed in gcc and not in glibc: it's not true that the argument has array type, the standard requires the parameter type to be adjusted: A declaration of a parameter as "array of type" shall be adjusted to "qualified pointer to type" this warning will just confuse users and make them believe that the two declaration styles for pointer arguments are somehow different.
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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. this is a warning with false positives that have no portable workaround and does not really help catching bugs (at least i doubt inconsistent array vs pointer declaration is causing common problems). what gcc should warn about is if there is an array style argument declaration and a caller passes an array that's provably shorter than that. pointer style declaration should be possible to mix with array style (this is important for standard headers but any api with multiple implementations can end up in situations where both array and pointer style decl will be present).
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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. Martin The build/host/target setup is: Build: x86_64-linux-gnu (Ubuntu 18.04) Host: x86_64-linux-gnu Target: aarch64-none-linux-gnu, aarch64_be-none-linux-gnu, arm-none-linux-gnueabi, arm-none-linux-gnueabihf Kind regards Vasee On 20/09/2020, 01:02, "Gcc-patches on behalf of Martin Sebor via Gcc-patches" wrote: On 9/17/20 4:38 PM, Joseph Myers wrote: > On Wed, 16 Sep 2020, Martin Sebor via Gcc-patches wrote: > >> Attached is an updated revision of the patch. Besides the tweaks >> above it also contains a cosmetic change to the warning issued >> for mismatches in unspecified VLA bounds: it points at the decl >> with more of them to guide the user to specify them rather than >> make them all unspecified. > > The previous version of the patch had a while loop as previously discussed > to handle skipping multiple consecutive cdk_attrs. > > + next = pd->declarator; > + while (next && next->kind == cdk_attrs) > + next = next->declarator; > > This version is back to an "if", but I don't see anything else in the new > version of that function that actually means the "if" would skip multiple > consecutive cdk_attrs as desired. > > The patch is OK with the "while" restored there. If for some reason the > "while" breaks something, we'll need to look in more detail at exactly > what case isn't being handled correctly by "while". I guess it was the result of an experiment, trying to see if I could break it with the 'if'. I (hope I) put it back and pushed the whole series. I had to squash patches 1 and 2 because of a dependency that I had missed. Thanks for the review, by the way. I think the signature validation we've ended up with is quite a bit more comprehensive than the first attempt. Martin
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
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 :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 :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 build/host/target setup is: Build: x86_64-linux-gnu (Ubuntu 18.04) Host: x86_64-linux-gnu Target: aarch64-none-linux-gnu, aarch64_be-none-linux-gnu, arm-none-linux-gnueabi, arm-none-linux-gnueabihf Kind regards Vasee On 20/09/2020, 01:02, "Gcc-patches on behalf of Martin Sebor via Gcc-patches" wrote: On 9/17/20 4:38 PM, Joseph Myers wrote: > On Wed, 16 Sep 2020, Martin Sebor via Gcc-patches wrote: > >> Attached is an updated revision of the patch. Besides the tweaks >> above it also contains a cosmetic change to the warning issued >> for mismatches in unspecified VLA bounds: it points at the decl >> with more of them to guide the user to specify them rather than >> make them all unspecified. > > The previous version of the patch had a while loop as previously discussed > to handle skipping multiple consecutive cdk_attrs. > > + next = pd->declarator; > + while (next && next->kind == cdk_attrs) > + next = next->declarator; > > This version is back to an "if", but I don't see anything else in the new > version of that function that actually means the "if" would skip multiple > consecutive cdk_attrs as desired. > > The patch is OK with the "while" restored there. If for some reason the > "while" breaks something, we'll need to look in more detail at exactly > what case isn't being handled correctly by "while". I guess it was the result of an experiment, trying to see if I could break it with the 'if'. I (hope I) put it back and pushed the whole series. I had to squash patches 1 and 2 because of a dependency that I had missed. Thanks for the review, by the way. I think the signature validation we've ended up with is quite a bit more comprehensive than the first attempt. Martin
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 9/17/20 4:38 PM, Joseph Myers wrote: On Wed, 16 Sep 2020, Martin Sebor via Gcc-patches wrote: Attached is an updated revision of the patch. Besides the tweaks above it also contains a cosmetic change to the warning issued for mismatches in unspecified VLA bounds: it points at the decl with more of them to guide the user to specify them rather than make them all unspecified. The previous version of the patch had a while loop as previously discussed to handle skipping multiple consecutive cdk_attrs. + next = pd->declarator; + while (next && next->kind == cdk_attrs) + next = next->declarator; This version is back to an "if", but I don't see anything else in the new version of that function that actually means the "if" would skip multiple consecutive cdk_attrs as desired. The patch is OK with the "while" restored there. If for some reason the "while" breaks something, we'll need to look in more detail at exactly what case isn't being handled correctly by "while". I guess it was the result of an experiment, trying to see if I could break it with the 'if'. I (hope I) put it back and pushed the whole series. I had to squash patches 1 and 2 because of a dependency that I had missed. Thanks for the review, by the way. I think the signature validation we've ended up with is quite a bit more comprehensive than the first attempt. Martin
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Wed, 16 Sep 2020, Martin Sebor via Gcc-patches wrote: > Attached is an updated revision of the patch. Besides the tweaks > above it also contains a cosmetic change to the warning issued > for mismatches in unspecified VLA bounds: it points at the decl > with more of them to guide the user to specify them rather than > make them all unspecified. The previous version of the patch had a while loop as previously discussed to handle skipping multiple consecutive cdk_attrs. + next = pd->declarator; + while (next && next->kind == cdk_attrs) + next = next->declarator; This version is back to an "if", but I don't see anything else in the new version of that function that actually means the "if" would skip multiple consecutive cdk_attrs as desired. The patch is OK with the "while" restored there. If for some reason the "while" breaks something, we'll need to look in more detail at exactly what case isn't being handled correctly by "while". -- Joseph S. Myers jos...@codesourcery.com
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 9/15/20 5:02 PM, Joseph Myers wrote: On Wed, 9 Sep 2020, Martin Sebor via Gcc-patches wrote: Joseph, do you have any concerns with or comments on the most recent patch or is it okay as is? https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html I'm not yet convinced by the logic for extracting an array bound from a parameter declared using a typedef for an array type. Say you have typedef int A[3]; void f (A *x[*]); so an argument that is an array, using [*], of pointers to arrays, where those latter arrays are specified using the typedef. As I read the logic, first the pointer declarator is handled (ignored), then the array declarator results in [*] being stored in spec, then the "if (pd->kind == cdk_id)" handling comes into play - and because spec is "*" and vbchain is NULL_TREE, the upper bound of A gets extracted, but the upper bound of A should be irrelevant here because it's a type that's the target of a pointer. The information from parm->specs->type logically comes before, not after, the information from the declarator. I see, you're right. These arrays of pointers and pointers to arrays are going to be the death of me... Thanks for the test case. I've tweaked the function some more to handle it and added it to the test suite. As far as I can see, if one declaration gets part of the parameter type (involving VLAs) from a typedef and another declaration gets that part of the type directly in the declaration, the two spec strings constructed might differ in the number of VLA bounds mentioned in the spec strings. Is the code using those strings robust to handling the case where some of the VLA bounds are missing because they came from a typedef? Ugh. I'd completely forgotten those are possible. I've added code to the function to handle those as well, and a new test to verify. Hopefully I didn't open a can of worms by doing that. Attached is an updated revision of the patch. Besides the tweaks above it also contains a cosmetic change to the warning issued for mismatches in unspecified VLA bounds: it points at the decl with more of them to guide the user to specify them rather than make them all unspecified. Martin [2/5] - C front end support to detect out-of-bounds accesses to array parameters. gcc/c-family/ChangeLog: PR c/50584 * c-common.h (warn_parm_array_mismatch): Declare new function. (has_attribute): Move declaration of an existing function. (build_attr_access_from_parms): Declare new function. * c-warn.c (parm_array_as_string): Define new function. (plus_one): Define new function. (warn_parm_ptrarray_mismatch): Define new function. (warn_parm_array_mismatch): Define new function. (vla_bound_parm_decl): New function. * c.opt (-Warray-parameter, -Wvla-parameter): New options. * c-pretty-print.c (pp_c_type_qualifier_list): Don't print array type qualifiers here... (c_pretty_printer::direct_abstract_declarator): ...but instead print them in brackets here. Also print [static]. Strip extraneous expressions from VLA bounds. gcc/c/ChangeLog: PR c/50584 * c-decl.c (lookup_last_decl): Define new function. (c_decl_attributes): Call it. (start_decl): Add argument and use it. (finish_decl): Call build_attr_access_from_parms and decl_attributes. (get_parm_array_spec): Define new function. (push_parm_decl): Call get_parm_array_spec. (start_function): Call warn_parm_array_mismatch. Build attribute access and add it to current function. * c-parser.c (c_parser_declaration_or_fndef): Diagnose mismatches in forms of array parameters. * c-tree.h (start_decl): Add argument. gcc/ChangeLog: PR c/50584 * calls.c (initialize_argument_information): Remove assertion. * doc/invoke.texi (-Warray-parameter, -Wvla-parameter): Document. * tree-pretty-print.c (dump_generic_node): Correct handling of qualifiers. gcc/testsuite/ChangeLog: PR c/50584 * c-c++-common/Warray-bounds-6.c: Correct C++ declaration, adjust text of expected diagnostics. * gcc.dg/Wbuiltin-declaration-mismatch-9.c: Prune expected warning. * gcc.dg/Warray-parameter-2.c: New test. * gcc.dg/Warray-parameter-3.c: New test. * gcc.dg/Warray-parameter-4.c: New test. * gcc.dg/Warray-parameter-5.c: New test. * gcc.dg/Warray-parameter.c: New test. * gcc.dg/Wvla-parameter-2.c: New test. * gcc.dg/Wvla-parameter-3.c: New test. * gcc.dg/Wvla-parameter-4.c: New test. * gcc.dg/Wvla-parameter.c: New test. diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 4fc64bc4aa6..a61b25f95f1 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1321,6 +1321,7 @@ extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool); extern void warn_for_omitted_condop (location_t, tree); extern bool warn_for_restrict (unsigned, tree *, unsigned); extern void warn_for_address_or_pointer_of_packed_member (tree, tree); +extern void warn_parm_array_mismatch (location_t, tree, tree); /* Places where an lvalue, or modifiable lvalue, may be required.
Re: [PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Wed, 9 Sep 2020, Martin Sebor via Gcc-patches wrote: > Joseph, do you have any concerns with or comments on the most > recent patch or is it okay as is? > > https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html I'm not yet convinced by the logic for extracting an array bound from a parameter declared using a typedef for an array type. Say you have typedef int A[3]; void f (A *x[*]); so an argument that is an array, using [*], of pointers to arrays, where those latter arrays are specified using the typedef. As I read the logic, first the pointer declarator is handled (ignored), then the array declarator results in [*] being stored in spec, then the "if (pd->kind == cdk_id)" handling comes into play - and because spec is "*" and vbchain is NULL_TREE, the upper bound of A gets extracted, but the upper bound of A should be irrelevant here because it's a type that's the target of a pointer. The information from parm->specs->type logically comes before, not after, the information from the declarator. As far as I can see, if one declaration gets part of the parameter type (involving VLAs) from a typedef and another declaration gets that part of the type directly in the declaration, the two spec strings constructed might differ in the number of VLA bounds mentioned in the spec strings. Is the code using those strings robust to handling the case where some of the VLA bounds are missing because they came from a typedef? -- Joseph S. Myers jos...@codesourcery.com
[PING 2][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
Joseph, do you have any concerns with or comments on the most recent patch or is it okay as is? https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html Martin On 9/2/20 6:03 PM, Martin Sebor wrote: Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html On 8/25/20 12:44 PM, Martin Sebor wrote: Joseph, do you have any more comments on the rest of the most recent revision of the patch? https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html Martin On 8/19/20 6:09 PM, Joseph Myers wrote: On Wed, 19 Aug 2020, Martin Sebor via Gcc-patches wrote: I think you need a while loop there, not just an if, to account for the case of multiple consecutive cdk_attrs. At least the GNU attribute syntax direct-declarator: [...] ( gnu-attributes[opt] declarator ) should produce multiple consecutive cdk_attrs for each level of parentheses with attributes inside. I had considered a loop but couldn't find a way to trigger what you describe (or a test in the testsuite that would do it) so I didn't use one. I saw loops like that in other places but I couldn't get even those to uncover such a test case. Here's what I tried: #define A(N) __attribute__ ((aligned (N), may_alias)) int n; void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n]))); Sequences of consecutive attributes are all chained together. I've added the loop here but I have no test for it. It would be good to add one if it really is needed. The sort of thing I'm thinking of would be, where A is some attribute: void f (int (A (A (A arg; (that example doesn't involve an array, but it illustrates the syntax I'd expect to produce multiple consecutive cdk_attrs).
[PING][PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html On 8/25/20 12:44 PM, Martin Sebor wrote: Joseph, do you have any more comments on the rest of the most recent revision of the patch? https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html Martin On 8/19/20 6:09 PM, Joseph Myers wrote: On Wed, 19 Aug 2020, Martin Sebor via Gcc-patches wrote: I think you need a while loop there, not just an if, to account for the case of multiple consecutive cdk_attrs. At least the GNU attribute syntax direct-declarator: [...] ( gnu-attributes[opt] declarator ) should produce multiple consecutive cdk_attrs for each level of parentheses with attributes inside. I had considered a loop but couldn't find a way to trigger what you describe (or a test in the testsuite that would do it) so I didn't use one. I saw loops like that in other places but I couldn't get even those to uncover such a test case. Here's what I tried: #define A(N) __attribute__ ((aligned (N), may_alias)) int n; void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n]))); Sequences of consecutive attributes are all chained together. I've added the loop here but I have no test for it. It would be good to add one if it really is needed. The sort of thing I'm thinking of would be, where A is some attribute: void f (int (A (A (A arg; (that example doesn't involve an array, but it illustrates the syntax I'd expect to produce multiple consecutive cdk_attrs).
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
Joseph, do you have any more comments on the rest of the most recent revision of the patch? https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552266.html Martin On 8/19/20 6:09 PM, Joseph Myers wrote: On Wed, 19 Aug 2020, Martin Sebor via Gcc-patches wrote: I think you need a while loop there, not just an if, to account for the case of multiple consecutive cdk_attrs. At least the GNU attribute syntax direct-declarator: [...] ( gnu-attributes[opt] declarator ) should produce multiple consecutive cdk_attrs for each level of parentheses with attributes inside. I had considered a loop but couldn't find a way to trigger what you describe (or a test in the testsuite that would do it) so I didn't use one. I saw loops like that in other places but I couldn't get even those to uncover such a test case. Here's what I tried: #define A(N) __attribute__ ((aligned (N), may_alias)) int n; void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n]))); Sequences of consecutive attributes are all chained together. I've added the loop here but I have no test for it. It would be good to add one if it really is needed. The sort of thing I'm thinking of would be, where A is some attribute: void f (int (A (A (A arg; (that example doesn't involve an array, but it illustrates the syntax I'd expect to produce multiple consecutive cdk_attrs).
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 8/19/20 6:09 PM, Joseph Myers wrote: On Wed, 19 Aug 2020, Martin Sebor via Gcc-patches wrote: I think you need a while loop there, not just an if, to account for the case of multiple consecutive cdk_attrs. At least the GNU attribute syntax direct-declarator: [...] ( gnu-attributes[opt] declarator ) should produce multiple consecutive cdk_attrs for each level of parentheses with attributes inside. I had considered a loop but couldn't find a way to trigger what you describe (or a test in the testsuite that would do it) so I didn't use one. I saw loops like that in other places but I couldn't get even those to uncover such a test case. Here's what I tried: #define A(N) __attribute__ ((aligned (N), may_alias)) int n; void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n]))); Sequences of consecutive attributes are all chained together. I've added the loop here but I have no test for it. It would be good to add one if it really is needed. The sort of thing I'm thinking of would be, where A is some attribute: void f (int (A (A (A arg; (that example doesn't involve an array, but it illustrates the syntax I'd expect to produce multiple consecutive cdk_attrs). Yes, that does it, thanks. But as a result of the test: if (pd->kind != cdk_array) continue; I don't see how to write a declaration where the if rather than a loop would cause trouble. If next->kind == cdk_attrs after the test in the if statement (i.e., before I replaced it with the while loop), the test above would be true and the for loop would continue. The next test for next->kind would then skip over the attrs. Let me know if I'm missing something. Otherwise I'll just leave the loop there with no test. Martin
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Wed, 19 Aug 2020, Martin Sebor via Gcc-patches wrote: > > I think you need a while loop there, not just an if, to account for the > > case of multiple consecutive cdk_attrs. At least the GNU attribute syntax > > > > direct-declarator: > > [...] > > ( gnu-attributes[opt] declarator ) > > > > should produce multiple consecutive cdk_attrs for each level of > > parentheses with attributes inside. > > I had considered a loop but couldn't find a way to trigger what you > describe (or a test in the testsuite that would do it) so I didn't > use one. I saw loops like that in other places but I couldn't get > even those to uncover such a test case. Here's what I tried: > > #define A(N) __attribute__ ((aligned (N), may_alias)) > int n; > void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n]))); > > Sequences of consecutive attributes are all chained together. > > I've added the loop here but I have no test for it. It would be > good to add one if it really is needed. The sort of thing I'm thinking of would be, where A is some attribute: void f (int (A (A (A arg; (that example doesn't involve an array, but it illustrates the syntax I'd expect to produce multiple consecutive cdk_attrs). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 8/17/20 4:09 PM, Joseph Myers wrote:
On Thu, 13 Aug 2020, Martin Sebor via Gcc-patches wrote:
* Maybe cdk_pointer is followed by cdk_attrs before cdk_id. In this case
the code won't return.
I think I see the problem you're pointing out (I just don't see how
to trigger it or test that it doesn't happen). If the tweak in
the attached update doesn't fix it a test case would be helpful.
I think you need a while loop there, not just an if, to account for the
case of multiple consecutive cdk_attrs. At least the GNU attribute syntax
direct-declarator:
[...]
( gnu-attributes[opt] declarator )
should produce multiple consecutive cdk_attrs for each level of
parentheses with attributes inside.
I had considered a loop but couldn't find a way to trigger what you
describe (or a test in the testsuite that would do it) so I didn't
use one. I saw loops like that in other places but I couldn't get
even those to uncover such a test case. Here's what I tried:
#define A(N) __attribute__ ((aligned (N), may_alias))
int n;
void f (int (* A (2) A (4) (* A (2) A (4) (* A (2) A (4) [n])[n])));
Sequences of consecutive attributes are all chained together.
I've added the loop here but I have no test for it. It would be
good to add one if it really is needed.
* Maybe the code is correct to continue because we're in the case of an
array of pointers (cdk_array follows). But as I understand it, the intent
is to set up an "arg spec" that describes only the (multidimensional)
array that is the parameter itself - not any array pointed to. And it
looks to me like, in the case of an array of pointers to arrays, both sets
of array bounds would end up in the spec constructed.
Ideally, I'd like to check even pointers to arrays and so they should
be recorded somewhere. The middle end code doesn't do any checking
of those yet for out-of-bounds accesses. It wasn't a goal for
the first iteration so I've tweaked the code to avoid recording them.
Could you expand the comment on get_parm_array_spec to specify exactly
what you think the function should be putting in the returned attribute,
in what order, in cases where there are array declarators (constant,
empty, [*] and VLA) intermixed with other kinds of declarators and the
type from the type specifiers may or may not be an array type itself?
That will provide a basis for subsequent rounds of review of whether the
function is actually behaving as expected.
Done.
As far as I can see, the logic
+ if (TREE_CODE (nelts) == INTEGER_CST)
+ {
+ /* Skip all constant bounds except the most significant one.
+The interior ones are included in the array type. */
+ if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+ continue;
will skip constant bounds in an array that's the target of a pointer
declarator, but not any other kind of bounds. Is that what you intend -
that all the other kind of bounds in pointed-to arrays will be recorded in
this string?
The immediate goal (for the front end) is to detect differences
in the form ([] vs [N]) or value of the most significant bound
in parameters of array types (before they decay to pointers),
and differences in the form ([*] vs [n]) or (the presumed) value
(e.g., [n] vs [n + 1]) of VLA bounds.
So I need to encode every variable bound (specified or unspecified).
For ordinary arrays I want to encode just the form of the most
significant bound.
Then, the code
+ if (pd->kind == cdk_id)
+ {
+ /* Extract the upper bound from a parameter of an array type. */
also seems misplaced. If the type specifiers for the parameter are a
typedef for an array type, that array type should be processed *before*
the declarator to get the correct semantics (as if the bounds from those
type specifiers were given in the declarator), not at the end which gets
that type out of order with respect to array declarators. (Processing
before the declarator also means clearing the results of that processing
if a pointer declarator is encountered at any point, because in that case
the array type in the type specifiers is irrelevant.)
I'm not sure I follow you here. Can you show me what you mean on
a piece of code? This test case (which IIUC does what you described)
works as expected:
$ cat q.c && gcc -O2 -S -Wall q.c
typedef int A[7][9];
void f (A[3][5]);
So this is equivalent to A[3][5][7][9]. The c_declarator structures have
the one for the [3] (the top-level bound) inside the one for the [5].
The [5] bound is skipped by the "Skip all constant bounds except the most
significant one." logic. When the [3] bound is reached, the "break;" at
the end of that processing means the "Extract the upper bound from a
parameter of an array type." never gets executed. Try replacing the [3]
bound by a VLA bound. As I read the code, it will end up generating a
spec string that records first the VLA, then the [7], when it should be
first the 9
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Thu, 13 Aug 2020, Martin Sebor via Gcc-patches wrote:
> > * Maybe cdk_pointer is followed by cdk_attrs before cdk_id. In this case
> > the code won't return.
>
> I think I see the problem you're pointing out (I just don't see how
> to trigger it or test that it doesn't happen). If the tweak in
> the attached update doesn't fix it a test case would be helpful.
I think you need a while loop there, not just an if, to account for the
case of multiple consecutive cdk_attrs. At least the GNU attribute syntax
direct-declarator:
[...]
( gnu-attributes[opt] declarator )
should produce multiple consecutive cdk_attrs for each level of
parentheses with attributes inside.
> > * Maybe the code is correct to continue because we're in the case of an
> > array of pointers (cdk_array follows). But as I understand it, the intent
> > is to set up an "arg spec" that describes only the (multidimensional)
> > array that is the parameter itself - not any array pointed to. And it
> > looks to me like, in the case of an array of pointers to arrays, both sets
> > of array bounds would end up in the spec constructed.
>
> Ideally, I'd like to check even pointers to arrays and so they should
> be recorded somewhere. The middle end code doesn't do any checking
> of those yet for out-of-bounds accesses. It wasn't a goal for
> the first iteration so I've tweaked the code to avoid recording them.
Could you expand the comment on get_parm_array_spec to specify exactly
what you think the function should be putting in the returned attribute,
in what order, in cases where there are array declarators (constant,
empty, [*] and VLA) intermixed with other kinds of declarators and the
type from the type specifiers may or may not be an array type itself?
That will provide a basis for subsequent rounds of review of whether the
function is actually behaving as expected.
As far as I can see, the logic
+ if (TREE_CODE (nelts) == INTEGER_CST)
+ {
+ /* Skip all constant bounds except the most significant one.
+The interior ones are included in the array type. */
+ if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+ continue;
will skip constant bounds in an array that's the target of a pointer
declarator, but not any other kind of bounds. Is that what you intend -
that all the other kind of bounds in pointed-to arrays will be recorded in
this string?
> > Then, the code
> >
> > + if (pd->kind == cdk_id)
> > + {
> > + /* Extract the upper bound from a parameter of an array type. */
> >
> > also seems misplaced. If the type specifiers for the parameter are a
> > typedef for an array type, that array type should be processed *before*
> > the declarator to get the correct semantics (as if the bounds from those
> > type specifiers were given in the declarator), not at the end which gets
> > that type out of order with respect to array declarators. (Processing
> > before the declarator also means clearing the results of that processing
> > if a pointer declarator is encountered at any point, because in that case
> > the array type in the type specifiers is irrelevant.)
>
> I'm not sure I follow you here. Can you show me what you mean on
> a piece of code? This test case (which IIUC does what you described)
> works as expected:
>
> $ cat q.c && gcc -O2 -S -Wall q.c
> typedef int A[7][9];
>
> void f (A[3][5]);
So this is equivalent to A[3][5][7][9]. The c_declarator structures have
the one for the [3] (the top-level bound) inside the one for the [5].
The [5] bound is skipped by the "Skip all constant bounds except the most
significant one." logic. When the [3] bound is reached, the "break;" at
the end of that processing means the "Extract the upper bound from a
parameter of an array type." never gets executed. Try replacing the [3]
bound by a VLA bound. As I read the code, it will end up generating a
spec string that records first the VLA, then the [7], when it should be
first the 9 (skipped), then the 7 (skipped), then the 5 (skipped), then
the VLA. Or if it's "void f (A *[variable][5]);", it will do the same
thing (VLA, then 7, although both the 7 and the 9 are part of the
pointed-to type).
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 8/12/20 5:19 PM, Joseph Myers wrote:
On Fri, 7 Aug 2020, Martin Sebor via Gcc-patches wrote:
I don't see anything in the tests in this patch to cover this sort of case
(arrays of pointers, including arrays of pointers to arrays etc.).
I've added a few test cases and reworked the declarator parsing
(get_parm_array_spec) a bit, fixing some bugs.
I don't think get_parm_array_spec is yet logically right (and I don't see
tests of the sort of cases I'm concerned about, such as arrays of pointers
to arrays, or pointers with attributes applied to them).
Please have a look at the tests at the end of Warrary-parameter-4.c.
They should exercise the cases you brought up (as I understand them).
There are some tests for attributes in Warray-parameter.c.
You have logic
+ if (pd->kind == cdk_pointer
+ && (!next || next->kind == cdk_id))
+ {
+ /* Do nothing for the common case of a pointer. The fact that
+the parameter is one can be deduced from the absence of
+an arg spec for it. */
+ return attrs;
+ }
which is correct as far as it goes (when it returns with nothing done,
it's correct to do so, because the argument is indeed a pointer), but
incomplete:
* Maybe cdk_pointer is followed by cdk_attrs before cdk_id. In this case
the code won't return.
I think I see the problem you're pointing out (I just don't see how
to trigger it or test that it doesn't happen). If the tweak in
the attached update doesn't fix it a test case would be helpful.
* Maybe the code is correct to continue because we're in the case of an
array of pointers (cdk_array follows). But as I understand it, the intent
is to set up an "arg spec" that describes only the (multidimensional)
array that is the parameter itself - not any array pointed to. And it
looks to me like, in the case of an array of pointers to arrays, both sets
of array bounds would end up in the spec constructed.
Ideally, I'd like to check even pointers to arrays and so they should
be recorded somewhere. The middle end code doesn't do any checking
of those yet for out-of-bounds accesses. It wasn't a goal for
the first iteration so I've tweaked the code to avoid recording them.
What I think is correct is for both cdk_pointer and cdk_function to result
in the spec built up so far being cleared (regardless of what follows
cdk_pointer or cdk_function), rather than early return, so that the spec
present at the end is for the innermost sequence of array declarators
(possibly with attributes involved as well). (cdk_function shouldn't
actually be an issue, since functions can't return arrays or functions,
but logically it seems appropriate to treat it like cdk_pointer.)
I've added a test for cdk_function to the one for cdk_pointer.
Then, the code
+ if (pd->kind == cdk_id)
+ {
+ /* Extract the upper bound from a parameter of an array type. */
also seems misplaced. If the type specifiers for the parameter are a
typedef for an array type, that array type should be processed *before*
the declarator to get the correct semantics (as if the bounds from those
type specifiers were given in the declarator), not at the end which gets
that type out of order with respect to array declarators. (Processing
before the declarator also means clearing the results of that processing
if a pointer declarator is encountered at any point, because in that case
the array type in the type specifiers is irrelevant.)
I'm not sure I follow you here. Can you show me what you mean on
a piece of code? This test case (which IIUC does what you described)
works as expected:
$ cat q.c && gcc -O2 -S -Wall q.c
typedef int A[7][9];
void f (A[3][5]);
void f (A[1][5]);
void g (void)
{
A a[2][5];
f (a);
}
q.c:4:9: warning: argument 1 of type ‘int[1][5][7][9]’ with mismatched
bound [-Warray-parameter=]
4 | void f (A[1][5]);
| ^~~
q.c:3:9: note: previously declared as ‘int[3][5][7][9]’
3 | void f (A[3][5]);
| ^~~
q.c: In function ‘g’:
q.c:9:3: warning: ‘f’ accessing 3780 bytes in a region of size 2520
[-Wstringop-overflow=]
9 | f (a);
| ^
q.c:9:3: note: referencing argument 1 of type ‘int (*)[5][7][9]’
The logic
+ /* Skip all constant bounds except the most significant one.
+The interior ones are included in the array type. */
+ if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+ continue;
is another example of code that fails to look past cdk_attrs.
It should be handled by the tweak I added in the attached revision.
Martin
[2/5] - C front end support to detect out-of-bounds accesses to array parameters.
gcc/c-family/ChangeLog:
PR c/50584
* c-common.h (warn_parm_array_mismatch): Declare new function.
(has_attribute): Move declaration of an existing function.
(build_attr_access_from_parms): Declare new function.
* c.opt (-Warray-parameter, -Wvla-parameter): New
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Fri, 7 Aug 2020, Martin Sebor via Gcc-patches wrote:
> > I don't see anything in the tests in this patch to cover this sort of case
> > (arrays of pointers, including arrays of pointers to arrays etc.).
>
> I've added a few test cases and reworked the declarator parsing
> (get_parm_array_spec) a bit, fixing some bugs.
I don't think get_parm_array_spec is yet logically right (and I don't see
tests of the sort of cases I'm concerned about, such as arrays of pointers
to arrays, or pointers with attributes applied to them).
You have logic
+ if (pd->kind == cdk_pointer
+ && (!next || next->kind == cdk_id))
+ {
+ /* Do nothing for the common case of a pointer. The fact that
+the parameter is one can be deduced from the absence of
+an arg spec for it. */
+ return attrs;
+ }
which is correct as far as it goes (when it returns with nothing done,
it's correct to do so, because the argument is indeed a pointer), but
incomplete:
* Maybe cdk_pointer is followed by cdk_attrs before cdk_id. In this case
the code won't return.
* Maybe the code is correct to continue because we're in the case of an
array of pointers (cdk_array follows). But as I understand it, the intent
is to set up an "arg spec" that describes only the (multidimensional)
array that is the parameter itself - not any array pointed to. And it
looks to me like, in the case of an array of pointers to arrays, both sets
of array bounds would end up in the spec constructed.
What I think is correct is for both cdk_pointer and cdk_function to result
in the spec built up so far being cleared (regardless of what follows
cdk_pointer or cdk_function), rather than early return, so that the spec
present at the end is for the innermost sequence of array declarators
(possibly with attributes involved as well). (cdk_function shouldn't
actually be an issue, since functions can't return arrays or functions,
but logically it seems appropriate to treat it like cdk_pointer.)
Then, the code
+ if (pd->kind == cdk_id)
+ {
+ /* Extract the upper bound from a parameter of an array type. */
also seems misplaced. If the type specifiers for the parameter are a
typedef for an array type, that array type should be processed *before*
the declarator to get the correct semantics (as if the bounds from those
type specifiers were given in the declarator), not at the end which gets
that type out of order with respect to array declarators. (Processing
before the declarator also means clearing the results of that processing
if a pointer declarator is encountered at any point, because in that case
the array type in the type specifiers is irrelevant.)
The logic
+ /* Skip all constant bounds except the most significant one.
+The interior ones are included in the array type. */
+ if (next && (next->kind == cdk_array || next->kind == cdk_pointer))
+ continue;
is another example of code that fails to look past cdk_attrs.
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On 7/29/20 12:12 PM, Joseph Myers wrote: On Tue, 28 Jul 2020, Martin Sebor via Gcc-patches wrote: + /* A list of VLA variable bounds or null if not specified. */ + tree vbchain = NULL_TREE; + if (parm->declarator->kind == cdk_array) + if (pd->kind != cdk_array) + break; + /* Skip all constant bounds except the most significant +one. The interior ones are included in the array type. */ + if (next && next->kind == cdk_array) + continue; Anything working with declarators should typically have logic to skip cdk_attrs declarators. Thanks for the quick review/reply and the hints below! For example, a parameter is declared as an array using [] in that declarator if the innermost c_declarator that is not cdk_id or cdk_attrs is of kind cdk_array. (It's the innermost not the outermost because of C declarator syntax.) The array bounds for the parameter array itself (as opposed to any other bounds if the parameter is e.g. an array of pointers to arrays) are then those in all the cdk_array declarators after the last declarator (if any) that's not cdk_array, cdk_attrs or cdk_id (cdk_id only comes in the last place). If e.g. the parameter has the nested cdk_declarator sequence cdk_function cdk_pointer cdk_array cdk_attrs cdk_array cdk_pointer cdk_array cdk_attrs cdk_array cdk_array cdk_id then it's a three-dimensional array of pointers to two-dimensional arrays of pointers to functions. I don't see anything in the tests in this patch to cover this sort of case (arrays of pointers, including arrays of pointers to arrays etc.). I've added a few test cases and reworked the declarator parsing (get_parm_array_spec) a bit, fixing some bugs. While testing I also noticed a problem/limitation in the array/VLA formatting function that I couldn't think of how to fix without duplicating a lot of what the C/C++ pretty printer does. So the updated patch also includes changes to the pretty printer to do most of what I need. As may be evident from the comments, I'm not very happy with the solution but my only other idea was to add a bit to an array type to indicate whether it's [static N] or [*] and that seems too intrusive. If you find the "hack" I put in unacceptable for the initial patch I'd appreciate a suggestion for a cleaner approach. I'd like to fix that in a followup patch. The pretty printer formatting only produces [*] for the most significant unspecified VLA bound, and the whole machinery ignores [static] on VLA bounds. I'd like to fix both but I thought I'd get your suggestion for how to make [*] appear in inner bounds first. Martin [2/5] - C front end support to detect out-of-bounds accesses to array parameters. gcc/c-family/ChangeLog: PR c/50584 * c-common.h (warn_parm_array_mismatch): Declare new function. (has_attribute): Move declaration of an existing function. (build_attr_access_from_parms): Declare new function. * c.opt (-Warray-parameter, -Wvla-parameter): New options. * c-pretty-print.c (pp_c_type_qualifier_list): Don't print array type qualifiers here... (c_pretty_printer::direct_abstract_declarator): ...but instead print them in brackets here. Also print [static]. Strip extraneous expressions from VLA bounds. gcc/c/ChangeLog: PR c/50584 * c-decl.c (lookup_last_decl): Define new function. (c_decl_attributes): Call it. (start_decl): Add argument and use it. (finish_decl): Call build_attr_access_from_parms and decl_attributes. (get_parm_array_spec): Define new function. (push_parm_decl): Call get_parm_array_spec. (start_function): Call warn_parm_array_mismatch. Build attribute access and add it to current function. * c-parser.c (c_parser_declaration_or_fndef): Diagnose mismatches in forms of array parameters. * c-tree.h (start_decl): Add argument. gcc/ChangeLog: PR c/50584 * calls.c (initialize_argument_information): Remove assertion. * doc/invoke.texi (-Warray-parameter, -Wvla-parameter): Document. * tree-pretty-print.c (dump_generic_node): Correct handling of qualifiers. gcc/testsuite/ChangeLog: PR c/50584 * c-c++-common/Warray-bounds-6.c: Correct C++ declaration, adjust text of expected diagnostics. * gcc.dg/Wbuiltin-declaration-mismatch-9.c: Prune expected warning. * gcc.dg/Warray-parameter-2.c: New test. * gcc.dg/Warray-parameter-3.c: New test. * gcc.dg/Warray-parameter-4.c: New test. * gcc.dg/Warray-parameter.c: New test. * gcc.dg/Wvla-parameter-2.c: New test. * gcc.dg/Wvla-parameter.c: New test. diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 61627264e1e..8070ae7f0d1 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1319,6 +1319,7 @@ extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool); extern void warn_for_omitted_condop (location_t, tree); extern bool warn_for_restrict (unsigned, tree *, unsigned); extern void warn_for_address_or_pointer_of_packed_member (tree, tree); +extern void
Re: [PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
On Tue, 28 Jul 2020, Martin Sebor via Gcc-patches wrote: > + /* A list of VLA variable bounds or null if not specified. */ > + tree vbchain = NULL_TREE; > + if (parm->declarator->kind == cdk_array) > + if (pd->kind != cdk_array) > + break; > + /* Skip all constant bounds except the most significant > + one. The interior ones are included in the array type. */ > + if (next && next->kind == cdk_array) > + continue; Anything working with declarators should typically have logic to skip cdk_attrs declarators. For example, a parameter is declared as an array using [] in that declarator if the innermost c_declarator that is not cdk_id or cdk_attrs is of kind cdk_array. (It's the innermost not the outermost because of C declarator syntax.) The array bounds for the parameter array itself (as opposed to any other bounds if the parameter is e.g. an array of pointers to arrays) are then those in all the cdk_array declarators after the last declarator (if any) that's not cdk_array, cdk_attrs or cdk_id (cdk_id only comes in the last place). If e.g. the parameter has the nested cdk_declarator sequence cdk_function cdk_pointer cdk_array cdk_attrs cdk_array cdk_pointer cdk_array cdk_attrs cdk_array cdk_array cdk_id then it's a three-dimensional array of pointers to two-dimensional arrays of pointers to functions. I don't see anything in the tests in this patch to cover this sort of case (arrays of pointers, including arrays of pointers to arrays etc.). -- Joseph S. Myers jos...@codesourcery.com
[PATCH 2/5] C front end support to detect out-of-bounds accesses to array parameters
Patch 2 adds support for the internal attribute access specification
for array/VLA parameters to the C front end and two new options to
control warnings.
The attribute is added in two steps: first, push_parm_decl adds a new
attribute "arg spec" to each array parameter with a string describing
its form and a chain of VLA bounds, and next start_function, after
checking for mismatches with prior declarations, collects these for
each function definitionm, creates an attribute access specification
from them, and installs it in the declaration.
c_parser_declaration_or_fndef does the same for function declarations
that aren't definitions. The two-step process via attribute "arg spec"
is necessary to associate VLA bounds with VLA parameters, and because
function parameters are transient (they're not available in the middle
end).
[2/5] - C front end support to detect out-of-bounds accesses to array parameters.
gcc/c-family/ChangeLog:
PR c/50584
* c-common.h (warn_parm_array_mismatch): Declare new function.
(has_attribute): Move declaration of an existing function.
(build_attr_access_from_parms): Declare new function.
* c.opt (-Warray-parameter, -Wvla-parameter): New options.
gcc/c/ChangeLog:
PR c/50584
* c-decl.c (lookup_last_decl): Define new function.
(c_decl_attributes): Call it.
(start_decl): Add argument and use it.
(finish_decl): Call build_attr_access_from_parms and decl_attributes.
(get_parm_array_spec): Define new function.
(push_parm_decl): Call get_parm_array_spec.
(start_function): Call warn_parm_array_mismatch. Build attribute
access and add it to current function.
* c-parser.c (c_parser_declaration_or_fndef): Diagnose mismatches
in forms of array parameters.
* c-tree.h (start_decl): Add argument.
gcc/ChangeLog:
PR c/50584
* calls.c (initialize_argument_information): Remove assertion.
* doc/invoke.texi (-Warray-parameter, -Wvla-parameter): Document.
* tree-pretty-print.c (dump_generic_node): Correct handling of
qualifiers.
gcc/testsuite/ChangeLog:
PR c/50584
* c-c++-common/Warray-bounds-6.c: Correct C++ declaration, adjust
text of expected diagnostics.
* gcc.dg/Wbuiltin-declaration-mismatch-9.c: Prune expected warning.
* gcc.dg/Warray-parameter-2.c: New test.
* gcc.dg/Warray-parameter-3.c: New test.
* gcc.dg/Warray-parameter-4.c: New test.
* gcc.dg/Warray-parameter.c: New test.
* gcc.dg/Wvla-parameter-2.c: New test.
* gcc.dg/Wvla-parameter.c: New test.
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 61627264e1e..8070ae7f0d1 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1319,6 +1319,7 @@ extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool);
extern void warn_for_omitted_condop (location_t, tree);
extern bool warn_for_restrict (unsigned, tree *, unsigned);
extern void warn_for_address_or_pointer_of_packed_member (tree, tree);
+extern void warn_parm_array_mismatch (location_t, tree, tree);
/* Places where an lvalue, or modifiable lvalue, may be required.
Used to select diagnostic messages in lvalue_error and
@@ -1373,6 +1374,8 @@ extern tree find_tm_attribute (tree);
extern const struct attribute_spec::exclusions attr_cold_hot_exclusions[];
extern const struct attribute_spec::exclusions attr_noreturn_exclusions[];
extern tree handle_noreturn_attribute (tree *, tree, tree, int, bool *);
+extern bool has_attribute (location_t, tree, tree, tree (*)(tree));
+extern tree build_attr_access_from_parms (tree, bool);
/* In c-format.c. */
extern bool valid_format_string_type_p (tree);
@@ -1401,8 +1404,6 @@ extern void maybe_suggest_missing_token_insertion (rich_location *richloc,
location_t prev_token_loc);
extern tree braced_lists_to_strings (tree, tree);
-extern bool has_attribute (location_t, tree, tree, tree (*)(tree));
-
#if CHECKING_P
namespace selftest {
/* Declarations for specific families of tests within c-family,
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 2b1aca16eb4..6f3997405a1 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -338,6 +338,14 @@ Warray-bounds=
LangEnabledBy(C ObjC C++ LTO ObjC++,Wall,1,0)
; in common.opt
+Warray-parameter
+C ObjC C++ ObjC++ Warning Alias(Warray-parameter=, 2, 0)
+Warn about mismatched declarations of array parameters and unsafe accesses to them.
+
+Warray-parameter=
+C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_array_parameter) IntegerRange(0, 2) LangEnabledBy(C ObjC C++ ObjC++,Wall, 2, 0) Warning
+Warn about mismatched declarations of array parameters and unsafe accesses to them.
+
Wzero-length-bounds
C ObjC C++ ObjC++ Var(warn_zero_length_bounds) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn about accesses to interior zero-length array members.
@@ -1253,6 +1261,10 @@ Wno-vla-larger-than
C ObjC C++ LTO ObjC++ Alias(Wvla-larger-than=,18446744073709551615EiB,none) Warning
Disable Wvla-larger-than= warning. Equivalent to Wvla-larger-than= or larger.
+Wvla-parameter
+C
