| Message ID | d65cacfb9cbc7bf1a94791bf7213169b77ec213e.camel@tugraz.at |
|---|---|
| State | New |
| Headers | show |
| Series | Less warnings for parameters declared as arrays [PR98541, PR98536] | expand |
On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > With the relatively new warnings (11..) affecting VLA bounds, > I now get a lot of false positives with -Wall. In general, I find > the new warnings very useful, but they seem a bit too > aggressive and some minor tweaks are needed, otherwise they are > too noisy. This patch suggests two changes: > > 1. For VLA bounds non-null is implied only when 'static' is > used (similar to clang) and not already when a bound > 0 is > specified: > > int foo(int n, char buf[static n]); > > int foo(10, 0); // warning with 'static' but not without. > > > (It also seems problematic to require a size of 0 to indicate > that the pointer may be null, because 0 is not allowed in > ISO C as a size. It is also inconsistent to how arrays with > static bound behave.) > > There seems to be agreement about this change in PR98541. > > > 2. GCC always warns when the number of unspecified > bounds is different between two declarations: > > int foo(int n, char buf[*]); > int foo(int n, char buf[n]); > > or > > int foo(int n, char buf[n]); > int foo(int n, char buf[*]); > > But the first version is useful if the size expression > can not be specified in a header (e.g. because it uses > a macro or variable not available there) and there is > currently no easy way to avoid this. The warning for > both cases was by design, but I suggest to limit the > warning to the second case. > > Note that the logic currently applied by GCC is too > simplistic anyway, as GCC does not warn for > > int foo(int x, int y, double m[*][y]); > int foo(int x, int y, double m[x][*]); > > because the number of specified / unspecified bounds > is the same. So I suggest to go with the attached > patch now and add more precise warnings later > if there is more experience with these warning > in gernal and if this then still seems desirable. > > > Martin > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > To avoid false positivies, tune the warnings for parameters declared > as arrays with size expressions. Only warn about null arguments with > 'static'. Also do not warn when more bounds are specified in the new > declaration than before. > > PR c/98541 > PR c/98536 > > c-family/ > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > bounds are specified. > > gcc/ > * gimple-ssa-warn-access.cc > (pass_waccess::maybe_check_access_sizes): For VLA bounds > in parameters, only warn about null pointers with 'static'. > > gcc/testsuite: > * gcc.dg/Wnonnull-4: Adapt test. > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > * gcc.dg/Wvla-parameter-4.c: Adapt test. > * gcc.dg/attr-access-2.c: Adapt test. Neither appears to be a regression. Seems like it should defer to gcc-14. jeff
Am Dienstag, dem 04.04.2023 um 19:31 -0600 schrieb Jeff Law: > > On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > > > > With the relatively new warnings (11..) affecting VLA bounds, > > I now get a lot of false positives with -Wall. In general, I find > > the new warnings very useful, but they seem a bit too > > aggressive and some minor tweaks are needed, otherwise they are > > too noisy. This patch suggests two changes: > > > > 1. For VLA bounds non-null is implied only when 'static' is > > used (similar to clang) and not already when a bound > 0 is > > specified: > > > > int foo(int n, char buf[static n]); > > > > int foo(10, 0); // warning with 'static' but not without. > > > > > > (It also seems problematic to require a size of 0 to indicate > > that the pointer may be null, because 0 is not allowed in > > ISO C as a size. It is also inconsistent to how arrays with > > static bound behave.) > > > > There seems to be agreement about this change in PR98541. > > > > > > 2. GCC always warns when the number of unspecified > > bounds is different between two declarations: > > > > int foo(int n, char buf[*]); > > int foo(int n, char buf[n]); > > > > or > > > > int foo(int n, char buf[n]); > > int foo(int n, char buf[*]); > > > > But the first version is useful if the size expression > > can not be specified in a header (e.g. because it uses > > a macro or variable not available there) and there is > > currently no easy way to avoid this. The warning for > > both cases was by design, but I suggest to limit the > > warning to the second case. > > > > Note that the logic currently applied by GCC is too > > simplistic anyway, as GCC does not warn for > > > > int foo(int x, int y, double m[*][y]); > > int foo(int x, int y, double m[x][*]); > > > > because the number of specified / unspecified bounds > > is the same. So I suggest to go with the attached > > patch now and add more precise warnings later > > if there is more experience with these warning > > in gernal and if this then still seems desirable. > > > > > > Martin > > > > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > > > > > > > > > > > > > > > > > To avoid false positivies, tune the warnings for parameters declared > > as arrays with size expressions. Only warn about null arguments with > > 'static'. Also do not warn when more bounds are specified in the new > > declaration than before. > > > > > > > > > > > > > > > > > > PR c/98541 > > PR c/98536 > > > > > > > > > > > > > > > > > > c-family/ > > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > > bounds are specified. > > > > > > > > > > > > > > > > > > gcc/ > > * gimple-ssa-warn-access.cc > > (pass_waccess::maybe_check_access_sizes): For VLA bounds > > in parameters, only warn about null pointers with 'static'. > > > > > > > > > > > > > > > > > > gcc/testsuite: > > * gcc.dg/Wnonnull-4: Adapt test. > > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > > * gcc.dg/Wvla-parameter-4.c: Adapt test. > > * gcc.dg/attr-access-2.c: Adapt test. > Neither appears to be a regression. Seems like it should defer to gcc-14. The false positives are a regression relative to earlier version of GCC because the warnings are completely new (since GCC 11). I could bring this back later, but this means there is another version of GCC where one might need to turn off these warnings. My concern is also that an otherwise very useful feature that brings some safety benefits gets underused. Martin
Am Dienstag, dem 04.04.2023 um 19:31 -0600 schrieb Jeff Law: > > On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > > > > With the relatively new warnings (11..) affecting VLA bounds, > > I now get a lot of false positives with -Wall. In general, I find > > the new warnings very useful, but they seem a bit too > > aggressive and some minor tweaks are needed, otherwise they are > > too noisy. This patch suggests two changes: > > > > 1. For VLA bounds non-null is implied only when 'static' is > > used (similar to clang) and not already when a bound > 0 is > > specified: > > > > int foo(int n, char buf[static n]); > > > > int foo(10, 0); // warning with 'static' but not without. > > > > > > (It also seems problematic to require a size of 0 to indicate > > that the pointer may be null, because 0 is not allowed in > > ISO C as a size. It is also inconsistent to how arrays with > > static bound behave.) > > > > There seems to be agreement about this change in PR98541. > > > > > > 2. GCC always warns when the number of unspecified > > bounds is different between two declarations: > > > > int foo(int n, char buf[*]); > > int foo(int n, char buf[n]); > > > > or > > > > int foo(int n, char buf[n]); > > int foo(int n, char buf[*]); > > > > But the first version is useful if the size expression > > can not be specified in a header (e.g. because it uses > > a macro or variable not available there) and there is > > currently no easy way to avoid this. The warning for > > both cases was by design, but I suggest to limit the > > warning to the second case. > > > > Note that the logic currently applied by GCC is too > > simplistic anyway, as GCC does not warn for > > > > int foo(int x, int y, double m[*][y]); > > int foo(int x, int y, double m[x][*]); > > > > because the number of specified / unspecified bounds > > is the same. So I suggest to go with the attached > > patch now and add more precise warnings later > > if there is more experience with these warning > > in gernal and if this then still seems desirable. > > > > > > Martin > > > > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > > > > > > > > > To avoid false positivies, tune the warnings for parameters declared > > as arrays with size expressions. Only warn about null arguments with > > 'static'. Also do not warn when more bounds are specified in the new > > declaration than before. > > > > > > > > > > PR c/98541 > > PR c/98536 > > > > > > > > > > c-family/ > > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > > bounds are specified. > > > > > > > > > > gcc/ > > * gimple-ssa-warn-access.cc > > (pass_waccess::maybe_check_access_sizes): For VLA bounds > > in parameters, only warn about null pointers with 'static'. > > > > > > > > > > gcc/testsuite: > > * gcc.dg/Wnonnull-4: Adapt test. > > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > > * gcc.dg/Wvla-parameter-4.c: Adapt test. > > * gcc.dg/attr-access-2.c: Adapt test. > Neither appears to be a regression. Seems like it should defer to gcc-14. Then ok for trunk now? Martin
Ok for gcc-14 now? Am Dienstag, dem 04.04.2023 um 19:31 -0600 schrieb Jeff Law: > > > On 4/3/23 13:34, Martin Uecker via Gcc-patches wrote: > > > > > > With the relatively new warnings (11..) affecting VLA bounds, > > I now get a lot of false positives with -Wall. In general, I find > > the new warnings very useful, but they seem a bit too > > aggressive and some minor tweaks are needed, otherwise they are > > too noisy. This patch suggests two changes: > > > > 1. For VLA bounds non-null is implied only when 'static' is > > used (similar to clang) and not already when a bound > 0 is > > specified: > > > > int foo(int n, char buf[static n]); > > > > int foo(10, 0); // warning with 'static' but not without. > > > > > > (It also seems problematic to require a size of 0 to indicate > > that the pointer may be null, because 0 is not allowed in > > ISO C as a size. It is also inconsistent to how arrays with > > static bound behave.) > > > > There seems to be agreement about this change in PR98541. > > > > > > 2. GCC always warns when the number of unspecified > > bounds is different between two declarations: > > > > int foo(int n, char buf[*]); > > int foo(int n, char buf[n]); > > > > or > > > > int foo(int n, char buf[n]); > > int foo(int n, char buf[*]); > > > > But the first version is useful if the size expression > > can not be specified in a header (e.g. because it uses > > a macro or variable not available there) and there is > > currently no easy way to avoid this. The warning for > > both cases was by design, but I suggest to limit the > > warning to the second case. > > > > Note that the logic currently applied by GCC is too > > simplistic anyway, as GCC does not warn for > > > > int foo(int x, int y, double m[*][y]); > > int foo(int x, int y, double m[x][*]); > > > > because the number of specified / unspecified bounds > > is the same. So I suggest to go with the attached > > patch now and add more precise warnings later > > if there is more experience with these warning > > in gernal and if this then still seems desirable. > > > > > > Martin > > > > > > Less warnings for parameters declared as arrays [PR98541, > > PR98536] > > > > To avoid false positivies, tune the warnings for parameters > > declared > > as arrays with size expressions. Only warn about null > > arguments with > > 'static'. Also do not warn when more bounds are specified in > > the new > > declaration than before. > > > > PR c/98541 > > PR c/98536 > > > > c-family/ > > * c-warn.cc (warn_parm_array_mismatch): Do not warn if > > more > > bounds are specified. > > > > gcc/ > > * gimple-ssa-warn-access.cc > > (pass_waccess::maybe_check_access_sizes): For VLA > > bounds > > in parameters, only warn about null pointers with > > 'static'. > > > > gcc/testsuite: > > * gcc.dg/Wnonnull-4: Adapt test. > > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > > * gcc.dg/Wvla-parameter-4.c: Adapt test. > > * gcc.dg/attr-access-2.c: Adapt test. > Neither appears to be a regression. Seems like it should defer to > gcc-14. > jeff
Joseph, I would appreciate if you could take a look at this? This fixes the remaining issues which requires me to turn the warnings off with -Wno-vla-parameter and -Wno-nonnull in my projects. Am Montag, dem 03.04.2023 um 21:34 +0200 schrieb Martin Uecker: > > With the relatively new warnings (11..) affecting VLA bounds, > I now get a lot of false positives with -Wall. In general, I find > the new warnings very useful, but they seem a bit too > aggressive and some minor tweaks are needed, otherwise they are > too noisy. This patch suggests two changes: > > 1. For VLA bounds non-null is implied only when 'static' is > used (similar to clang) and not already when a bound > 0 is > specified: > > int foo(int n, char buf[static n]); > > int foo(10, 0); // warning with 'static' but not without. > > > (It also seems problematic to require a size of 0 to indicate > that the pointer may be null, because 0 is not allowed in > ISO C as a size. It is also inconsistent to how arrays with > static bound behave.) > > There seems to be agreement about this change in PR98541. > > > 2. GCC always warns when the number of unspecified > bounds is different between two declarations: > > int foo(int n, char buf[*]); > int foo(int n, char buf[n]); > > or > > int foo(int n, char buf[n]); > int foo(int n, char buf[*]); > > But the first version is useful if the size expression > can not be specified in a header (e.g. because it uses > a macro or variable not available there) and there is > currently no easy way to avoid this. The warning for > both cases was by design, but I suggest to limit the > warning to the second case. > > Note that the logic currently applied by GCC is too > simplistic anyway, as GCC does not warn for > > int foo(int x, int y, double m[*][y]); > int foo(int x, int y, double m[x][*]); > > because the number of specified / unspecified bounds > is the same. So I suggest to go with the attached > patch now and add more precise warnings later > if there is more experience with these warning > in gernal and if this then still seems desirable. > > > Martin > > > Less warnings for parameters declared as arrays [PR98541, PR98536] > > To avoid false positivies, tune the warnings for parameters declared > as arrays with size expressions. Only warn about null arguments with > 'static'. Also do not warn when more bounds are specified in the new > declaration than before. > > PR c/98541 > PR c/98536 > > c-family/ > * c-warn.cc (warn_parm_array_mismatch): Do not warn if more > bounds are specified. > > gcc/ > * gimple-ssa-warn-access.cc > (pass_waccess::maybe_check_access_sizes): For VLA bounds > in parameters, only warn about null pointers with 'static'. > > gcc/testsuite: > * gcc.dg/Wnonnull-4: Adapt test. > * gcc.dg/Wstringop-overflow-40.c: Adapt test. > * gcc.dg/Wvla-parameter-4.c: Adapt test. > * gcc.dg/attr-access-2.c: Adapt test. > > > diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc > index 9ac43a1af6e..f79fb876142 100644 > --- a/gcc/c-family/c-warn.cc > +++ b/gcc/c-family/c-warn.cc > @@ -3599,23 +3599,13 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) > continue; > } > > - if (newunspec != curunspec) > + if (newunspec > curunspec) > { > location_t warnloc = newloc, noteloc = origloc; > const char *warnparmstr = newparmstr.c_str (); > const char *noteparmstr = curparmstr.c_str (); > unsigned warnunspec = newunspec, noteunspec = curunspec; > > - if (newunspec < curunspec) > - { > - /* If the new declaration has fewer unspecified bounds > - point the warning to the previous declaration to make > - it clear that that's the one to change. Otherwise, > - point it to the new decl. */ > - std::swap (warnloc, noteloc); > - std::swap (warnparmstr, noteparmstr); > - std::swap (warnunspec, noteunspec); > - } > if (warning_n (warnloc, OPT_Wvla_parameter, warnunspec, > "argument %u of type %s declared with " > "%u unspecified variable bound", > @@ -3641,16 +3631,11 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) > continue; > } > } > - > /* Iterate over the lists of VLA variable bounds, comparing each > - pair for equality, and diagnosing mismatches. The case of > - the lists having different lengths is handled above so at > - this point they do . */ > - for (tree newvbl = newa->size, curvbl = cura->size; newvbl; > + pair for equality, and diagnosing mismatches. */ > + for (tree newvbl = newa->size, curvbl = cura->size; newvbl && curvbl; > newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl)) > { > - gcc_assert (curvbl); > - > tree newpos = TREE_PURPOSE (newvbl); > tree curpos = TREE_PURPOSE (curvbl); > > @@ -3663,7 +3648,6 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) > and both are the same expression they are necessarily > the same. */ > continue; > - > pretty_printer pp1, pp2; > const char* const newbndstr = expr_to_str (pp1, newbnd, "*"); > const char* const curbndstr = expr_to_str (pp2, curbnd, "*"); > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index b3de4b77924..a405f830fb5 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -3478,27 +3478,14 @@ pass_waccess::maybe_check_access_sizes (rdwr_map *rwm, tree fndecl, tree fntype, > > if (integer_zerop (ptr)) > { > - if (sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0) > + if (!access.second.internal_p > + && sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0) > { > /* Warn about null pointers with positive sizes. This is > different from also declaring the pointer argument with > attribute nonnull when the function accepts null pointers > only when the corresponding size is zero. */ > - if (access.second.internal_p) > - { > - const std::string argtypestr > - = access.second.array_as_string (ptrtype); > - > - if (warning_at (loc, OPT_Wnonnull, > - "argument %i of variable length " > - "array %s is null but " > - "the corresponding bound argument " > - "%i value is %s", > - ptridx + 1, argtypestr.c_str (), > - sizidx + 1, sizstr)) > - arg_warned = OPT_Wnonnull; > - } > - else if (warning_at (loc, OPT_Wnonnull, > + if (warning_at (loc, OPT_Wnonnull, > "argument %i is null but " > "the corresponding size argument " > "%i value is %s", > diff --git a/gcc/testsuite/gcc.dg/Wnonnull-4.c b/gcc/testsuite/gcc.dg/Wnonnull-4.c > index 2c1c45a9856..1f14fbba45d 100644 > --- a/gcc/testsuite/gcc.dg/Wnonnull-4.c > +++ b/gcc/testsuite/gcc.dg/Wnonnull-4.c > @@ -27,9 +27,9 @@ void test_fca_n (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > } > > > @@ -55,9 +55,9 @@ void test_fsa_x_n (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > } > > > @@ -83,9 +83,9 @@ void test_fia_1_n (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > } > > > @@ -111,9 +111,9 @@ void test_fla_3_n (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > } > > > @@ -139,9 +139,9 @@ void test_fda_n_5 (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is \\d+" } > } > > > @@ -167,7 +167,7 @@ void test_fca_n_n (int r_m1) > T ( 0); > > // Verify positive bounds. > - T ( 1); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > - T ( 9); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > - T (max); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > + T ( 1); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 1" } > + T ( 9); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 9" } > + T (max); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } > } > diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c > index 386c92dc7a8..9e0ad1f3aff 100644 > --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c > +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c > @@ -11,6 +11,7 @@ void fxa2 (int16_t[2]) __attribute__ ((nonnull)); > void fas2 (int16_t[static 2]); > > void fvla (unsigned n, int16_t[n]); > +void fvlaS (unsigned n, int16_t[static n]); > > void test_array_1_dim (void) > { > @@ -33,7 +34,8 @@ void test_array_1_dim (void) > fas2 (a1); // { dg-warning "'fas2' accessing 4 bytes in a region of size 2 " } > fas2 (&i); // { dg-warning "'fas2' accessing 4 bytes in a region of size 2 " } > > - fvla (1, 0); // { dg-warning "\\\[-Wnonnull" } > + fvla (1, 0); > + fvlaS (1, 0); // { dg-warning "\\\[-Wnonnull" } > fvla (1, &i); > fvla (2, a2); > fvla (2, a1); // { dg-warning "'fvla' accessing 4 bytes in a region of size 2 " } > @@ -47,6 +49,7 @@ void fxac2 (const int16_t[2]) __attribute__ ((nonnull)); > void facs2 (const int16_t[static 2]); > > void fvlac (unsigned n, const int16_t[n]); > +void fvlacS (unsigned n, const int16_t[static n]); > > void test_const_array_1_dim (void) > { > @@ -69,7 +72,8 @@ void test_const_array_1_dim (void) > facs2 (a1); // { dg-warning "'facs2' reading 4 bytes from a region of size 2 " } > facs2 (&i); // { dg-warning "'facs2' reading 4 bytes from a region of size 2 " } > > - fvlac (1, 0); // { dg-warning "\\\[-Wnonnull" } > + fvlac (1, 0); > + fvlacS (1, 0); // { dg-warning "\\\[-Wnonnull" } > fvlac (1, &i); > fvlac (2, a2); > fvlac (2, a1); // { dg-warning "'fvlac' reading 4 bytes from a region of size 2 " } > diff --git a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c > index 599ad19a3e4..79f72a94c7f 100644 > --- a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c > +++ b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c > @@ -9,12 +9,9 @@ extern int m, n; > > typedef int IA3[3]; > > -/* Verify the warning points to the declaration with more unspecified > - bounds, guiding the user to specify them rather than making them all > - unspecified. */ > -void* f_pIA3ax (IA3 *x[*]); // { dg-warning "argument 1 of type 'int \\\(\\\*\\\[\\\*]\\\)\\\[3]' .aka '\[^\n\r\}\]+'. declared with 1 unspecified variable bound" } > void* f_pIA3ax (IA3 *x[*]); > -void* f_pIA3ax (IA3 *x[n]); // { dg-message "subsequently declared as 'int \\\(\\\*\\\[n]\\\)\\\[3]' with 0 unspecified variable bounds" "note" } > +void* f_pIA3ax (IA3 *x[*]); > +void* f_pIA3ax (IA3 *x[n]); > void* f_pIA3ax (IA3 *x[n]) { return x; } > > > diff --git a/gcc/testsuite/gcc.dg/attr-access-2.c b/gcc/testsuite/gcc.dg/attr-access-2.c > index 76baddffc9f..616b7a9527c 100644 > --- a/gcc/testsuite/gcc.dg/attr-access-2.c > +++ b/gcc/testsuite/gcc.dg/attr-access-2.c > @@ -60,16 +60,6 @@ RW (2, 1) void f10 (int n, char a[n]) // { dg-warning "attribute 'access *\\\( > // { dg-warning "argument 2 of type 'char\\\[n]' declared as a variable length array" "" { target *-*-* } .-1 } > { (void)&n; (void)&a; } > > - > -/* The following is diagnosed to point out declarations with the T[*] > - form in headers where specifying the bound is just as important as > - in the definition (to detect bugs). */ > - void f11 (int, char[*]); // { dg-warning "argument 2 of type 'char\\\[\\\*\\\]' declared with 1 unspecified variable bound" } > - void f11 (int m, char a[m]); // { dg-message "subsequently declared as 'char\\\[m]' with 0 unspecified variable bounds" "note" } > -RW (2, 1) void f11 (int n, char arr[n]) // { dg-message "subsequently declared as 'char\\\[n]' with 0 unspecified variable bounds" "note" } > -{ (void)&n; (void)&arr; } > - > - > /* Verify that redeclaring a function with attribute access applying > to an array parameter of any form is not diagnosed. */ > void f12__ (int, int[]) RW (2, 1); > > > >
On Mon, 31 Jul 2023, Martin Uecker via Gcc-patches wrote: > Joseph, I would appreciate if you could take a look at this? > > This fixes the remaining issues which requires me to turn the > warnings off with -Wno-vla-parameter and -Wno-nonnull in my > projects. The front-end changes are OK.
diff --git a/gcc/c-family/c-warn.cc b/gcc/c-family/c-warn.cc index 9ac43a1af6e..f79fb876142 100644 --- a/gcc/c-family/c-warn.cc +++ b/gcc/c-family/c-warn.cc @@ -3599,23 +3599,13 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) continue; } - if (newunspec != curunspec) + if (newunspec > curunspec) { location_t warnloc = newloc, noteloc = origloc; const char *warnparmstr = newparmstr.c_str (); const char *noteparmstr = curparmstr.c_str (); unsigned warnunspec = newunspec, noteunspec = curunspec; - if (newunspec < curunspec) - { - /* If the new declaration has fewer unspecified bounds - point the warning to the previous declaration to make - it clear that that's the one to change. Otherwise, - point it to the new decl. */ - std::swap (warnloc, noteloc); - std::swap (warnparmstr, noteparmstr); - std::swap (warnunspec, noteunspec); - } if (warning_n (warnloc, OPT_Wvla_parameter, warnunspec, "argument %u of type %s declared with " "%u unspecified variable bound", @@ -3641,16 +3631,11 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) continue; } } - /* Iterate over the lists of VLA variable bounds, comparing each - pair for equality, and diagnosing mismatches. The case of - the lists having different lengths is handled above so at - this point they do . */ - for (tree newvbl = newa->size, curvbl = cura->size; newvbl; + pair for equality, and diagnosing mismatches. */ + for (tree newvbl = newa->size, curvbl = cura->size; newvbl && curvbl; newvbl = TREE_CHAIN (newvbl), curvbl = TREE_CHAIN (curvbl)) { - gcc_assert (curvbl); - tree newpos = TREE_PURPOSE (newvbl); tree curpos = TREE_PURPOSE (curvbl); @@ -3663,7 +3648,6 @@ warn_parm_array_mismatch (location_t origloc, tree fndecl, tree newparms) and both are the same expression they are necessarily the same. */ continue; - pretty_printer pp1, pp2; const char* const newbndstr = expr_to_str (pp1, newbnd, "*"); const char* const curbndstr = expr_to_str (pp2, curbnd, "*"); diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index b3de4b77924..a405f830fb5 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -3478,27 +3478,14 @@ pass_waccess::maybe_check_access_sizes (rdwr_map *rwm, tree fndecl, tree fntype, if (integer_zerop (ptr)) { - if (sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0) + if (!access.second.internal_p + && sizidx >= 0 && tree_int_cst_sgn (sizrng[0]) > 0) { /* Warn about null pointers with positive sizes. This is different from also declaring the pointer argument with attribute nonnull when the function accepts null pointers only when the corresponding size is zero. */ - if (access.second.internal_p) - { - const std::string argtypestr - = access.second.array_as_string (ptrtype); - - if (warning_at (loc, OPT_Wnonnull, - "argument %i of variable length " - "array %s is null but " - "the corresponding bound argument " - "%i value is %s", - ptridx + 1, argtypestr.c_str (), - sizidx + 1, sizstr)) - arg_warned = OPT_Wnonnull; - } - else if (warning_at (loc, OPT_Wnonnull, + if (warning_at (loc, OPT_Wnonnull, "argument %i is null but " "the corresponding size argument " "%i value is %s", diff --git a/gcc/testsuite/gcc.dg/Wnonnull-4.c b/gcc/testsuite/gcc.dg/Wnonnull-4.c index 2c1c45a9856..1f14fbba45d 100644 --- a/gcc/testsuite/gcc.dg/Wnonnull-4.c +++ b/gcc/testsuite/gcc.dg/Wnonnull-4.c @@ -27,9 +27,9 @@ void test_fca_n (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'char\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } } @@ -55,9 +55,9 @@ void test_fsa_x_n (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'short int\\\[]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } } @@ -83,9 +83,9 @@ void test_fia_1_n (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'int\\\[1]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } } @@ -111,9 +111,9 @@ void test_fla_3_n (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'long int\\\[3]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } } @@ -139,9 +139,9 @@ void test_fda_n_5 (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'double\\\[n]\\\[5]' is null but the corresponding bound argument 1 value is \\d+" } } @@ -167,7 +167,7 @@ void test_fca_n_n (int r_m1) T ( 0); // Verify positive bounds. - T ( 1); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 1" } - T ( 9); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 9" } - T (max); // { dg-warning "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } + T ( 1); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 1" } + T ( 9); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is 9" } + T (max); // { dg-bogus "argument 2 of variable length array 'char\\\[n]\\\[n]' is null but the corresponding bound argument 1 value is \\d+" } } diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c index 386c92dc7a8..9e0ad1f3aff 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-40.c @@ -11,6 +11,7 @@ void fxa2 (int16_t[2]) __attribute__ ((nonnull)); void fas2 (int16_t[static 2]); void fvla (unsigned n, int16_t[n]); +void fvlaS (unsigned n, int16_t[static n]); void test_array_1_dim (void) { @@ -33,7 +34,8 @@ void test_array_1_dim (void) fas2 (a1); // { dg-warning "'fas2' accessing 4 bytes in a region of size 2 " } fas2 (&i); // { dg-warning "'fas2' accessing 4 bytes in a region of size 2 " } - fvla (1, 0); // { dg-warning "\\\[-Wnonnull" } + fvla (1, 0); + fvlaS (1, 0); // { dg-warning "\\\[-Wnonnull" } fvla (1, &i); fvla (2, a2); fvla (2, a1); // { dg-warning "'fvla' accessing 4 bytes in a region of size 2 " } @@ -47,6 +49,7 @@ void fxac2 (const int16_t[2]) __attribute__ ((nonnull)); void facs2 (const int16_t[static 2]); void fvlac (unsigned n, const int16_t[n]); +void fvlacS (unsigned n, const int16_t[static n]); void test_const_array_1_dim (void) { @@ -69,7 +72,8 @@ void test_const_array_1_dim (void) facs2 (a1); // { dg-warning "'facs2' reading 4 bytes from a region of size 2 " } facs2 (&i); // { dg-warning "'facs2' reading 4 bytes from a region of size 2 " } - fvlac (1, 0); // { dg-warning "\\\[-Wnonnull" } + fvlac (1, 0); + fvlacS (1, 0); // { dg-warning "\\\[-Wnonnull" } fvlac (1, &i); fvlac (2, a2); fvlac (2, a1); // { dg-warning "'fvlac' reading 4 bytes from a region of size 2 " } diff --git a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c index 599ad19a3e4..79f72a94c7f 100644 --- a/gcc/testsuite/gcc.dg/Wvla-parameter-4.c +++ b/gcc/testsuite/gcc.dg/Wvla-parameter-4.c @@ -9,12 +9,9 @@ extern int m, n; typedef int IA3[3]; -/* Verify the warning points to the declaration with more unspecified - bounds, guiding the user to specify them rather than making them all - unspecified. */ -void* f_pIA3ax (IA3 *x[*]); // { dg-warning "argument 1 of type 'int \\\(\\\*\\\[\\\*]\\\)\\\[3]' .aka '\[^\n\r\}\]+'. declared with 1 unspecified variable bound" } void* f_pIA3ax (IA3 *x[*]); -void* f_pIA3ax (IA3 *x[n]); // { dg-message "subsequently declared as 'int \\\(\\\*\\\[n]\\\)\\\[3]' with 0 unspecified variable bounds" "note" } +void* f_pIA3ax (IA3 *x[*]); +void* f_pIA3ax (IA3 *x[n]); void* f_pIA3ax (IA3 *x[n]) { return x; } diff --git a/gcc/testsuite/gcc.dg/attr-access-2.c b/gcc/testsuite/gcc.dg/attr-access-2.c index 76baddffc9f..616b7a9527c 100644 --- a/gcc/testsuite/gcc.dg/attr-access-2.c +++ b/gcc/testsuite/gcc.dg/attr-access-2.c @@ -60,16 +60,6 @@ RW (2, 1) void f10 (int n, char a[n]) // { dg-warning "attribute 'access *\\\( // { dg-warning "argument 2 of type 'char\\\[n]' declared as a variable length array" "" { target *-*-* } .-1 } { (void)&n; (void)&a; } - -/* The following is diagnosed to point out declarations with the T[*] - form in headers where specifying the bound is just as important as - in the definition (to detect bugs). */ - void f11 (int, char[*]); // { dg-warning "argument 2 of type 'char\\\[\\\*\\\]' declared with 1 unspecified variable bound" } - void f11 (int m, char a[m]); // { dg-message "subsequently declared as 'char\\\[m]' with 0 unspecified variable bounds" "note" } -RW (2, 1) void f11 (int n, char arr[n]) // { dg-message "subsequently declared as 'char\\\[n]' with 0 unspecified variable bounds" "note" } -{ (void)&n; (void)&arr; } - - /* Verify that redeclaring a function with attribute access applying to an array parameter of any form is not diagnosed. */ void f12__ (int, int[]) RW (2, 1);
With the relatively new warnings (11..) affecting VLA bounds, I now get a lot of false positives with -Wall. In general, I find the new warnings very useful, but they seem a bit too aggressive and some minor tweaks are needed, otherwise they are too noisy. This patch suggests two changes: 1. For VLA bounds non-null is implied only when 'static' is used (similar to clang) and not already when a bound > 0 is specified: int foo(int n, char buf[static n]); int foo(10, 0); // warning with 'static' but not without. (It also seems problematic to require a size of 0 to indicate that the pointer may be null, because 0 is not allowed in ISO C as a size. It is also inconsistent to how arrays with static bound behave.) There seems to be agreement about this change in PR98541. 2. GCC always warns when the number of unspecified bounds is different between two declarations: int foo(int n, char buf[*]); int foo(int n, char buf[n]); or int foo(int n, char buf[n]); int foo(int n, char buf[*]); But the first version is useful if the size expression can not be specified in a header (e.g. because it uses a macro or variable not available there) and there is currently no easy way to avoid this. The warning for both cases was by design, but I suggest to limit the warning to the second case. Note that the logic currently applied by GCC is too simplistic anyway, as GCC does not warn for int foo(int x, int y, double m[*][y]); int foo(int x, int y, double m[x][*]); because the number of specified / unspecified bounds is the same. So I suggest to go with the attached patch now and add more precise warnings later if there is more experience with these warning in gernal and if this then still seems desirable. Martin Less warnings for parameters declared as arrays [PR98541, PR98536] To avoid false positivies, tune the warnings for parameters declared as arrays with size expressions. Only warn about null arguments with 'static'. Also do not warn when more bounds are specified in the new declaration than before. PR c/98541 PR c/98536 c-family/ * c-warn.cc (warn_parm_array_mismatch): Do not warn if more bounds are specified. gcc/ * gimple-ssa-warn-access.cc (pass_waccess::maybe_check_access_sizes): For VLA bounds in parameters, only warn about null pointers with 'static'. gcc/testsuite: * gcc.dg/Wnonnull-4: Adapt test. * gcc.dg/Wstringop-overflow-40.c: Adapt test. * gcc.dg/Wvla-parameter-4.c: Adapt test. * gcc.dg/attr-access-2.c: Adapt test.