| Message ID | VI1PR03MB45289E8FD7DD111FE1F0E2BAE49E0@VI1PR03MB4528.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix -Wshadow=local warnings in genautomata.c | expand |
Sorry for the slow reply. Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > Hi, > > this is probably on the border to obvious. > > The REGEXP_xxx macros in genautomata are invoked > recursively, and the local values are all named _regexp > and shadow each other. > > > Fixed by using different names _regexp1..6 for each > macro. Sorry to repeat the complaint about numerical suffixes, but I think we'd need better names. E.g. _regexp_unit or _re_unit for REGEXP_UNIT and similarly for the other macros. But a similar fix to rtl.h might be better. Thanks, Richard > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. > > 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * genautomata.c (REGEXP_UNIT, REGEXP_RESERV, REGEXP_SEQUENCE, > REGEXP_REPEAT, REGEXP_ALLOF, REGEXP_ONEOF): Rename local vars. > > Index: gcc/genautomata.c > =================================================================== > --- gcc/genautomata.c (revision 276484) > +++ gcc/genautomata.c (working copy) > @@ -984,46 +984,46 @@ decl_mode_check_failed (enum decl_mode mode, const > > > #define REGEXP_UNIT(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_unit) \ > - regexp_mode_check_failed (_regexp->mode, "rm_unit", \ > +(({ struct regexp *const _regex1 = (r); \ > + if (_regex1->mode != rm_unit) \ > + regexp_mode_check_failed (_regex1->mode, "rm_unit", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.unit; })) > + &(_regex1)->regexp.unit; })) > > #define REGEXP_RESERV(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_reserv) \ > - regexp_mode_check_failed (_regexp->mode, "rm_reserv", \ > +(({ struct regexp *const _regex2 = (r); \ > + if (_regex2->mode != rm_reserv) \ > + regexp_mode_check_failed (_regex2->mode, "rm_reserv", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.reserv; })) > + &(_regex2)->regexp.reserv; })) > > #define REGEXP_SEQUENCE(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_sequence) \ > - regexp_mode_check_failed (_regexp->mode, "rm_sequence", \ > +(({ struct regexp *const _regex3 = (r); \ > + if (_regex3->mode != rm_sequence) \ > + regexp_mode_check_failed (_regex3->mode, "rm_sequence", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.sequence; })) > + &(_regex3)->regexp.sequence; })) > > #define REGEXP_REPEAT(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_repeat) \ > - regexp_mode_check_failed (_regexp->mode, "rm_repeat", \ > +(({ struct regexp *const _regex4 = (r); \ > + if (_regex4->mode != rm_repeat) \ > + regexp_mode_check_failed (_regex4->mode, "rm_repeat", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.repeat; })) > + &(_regex4)->regexp.repeat; })) > > #define REGEXP_ALLOF(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_allof) \ > - regexp_mode_check_failed (_regexp->mode, "rm_allof", \ > +(({ struct regexp *const _regex5 = (r); \ > + if (_regex5->mode != rm_allof) \ > + regexp_mode_check_failed (_regex5->mode, "rm_allof", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.allof; })) > + &(_regex5)->regexp.allof; })) > > #define REGEXP_ONEOF(r) __extension__ \ > -(({ struct regexp *const _regexp = (r); \ > - if (_regexp->mode != rm_oneof) \ > - regexp_mode_check_failed (_regexp->mode, "rm_oneof", \ > +(({ struct regexp *const _regex6 = (r); \ > + if (_regex6->mode != rm_oneof) \ > + regexp_mode_check_failed (_regex6->mode, "rm_oneof", \ > __FILE__, __LINE__, __FUNCTION__); \ > - &(_regexp)->regexp.oneof; })) > + &(_regex6)->regexp.oneof; })) > > static const char *regexp_name (enum regexp_mode); > static void regexp_mode_check_failed (enum regexp_mode, const char *,
On 10/16/19 9:11 AM, Richard Sandiford wrote: > Sorry for the slow reply. > > Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >> Hi, >> >> this is probably on the border to obvious. >> >> The REGEXP_xxx macros in genautomata are invoked >> recursively, and the local values are all named _regexp >> and shadow each other. >> >> >> Fixed by using different names _regexp1..6 for each >> macro. > > Sorry to repeat the complaint about numerical suffixes, but I think > we'd need better names. E.g. _regexp_unit or _re_unit for REGEXP_UNIT > and similarly for the other macros. But a similar fix to rtl.h might > be better. Should the warning trigger when the shadowing name results from macro expansion? The author of a macro can't (in general) know what context it's going to be used, and when different macros come from two different third party headers, it would seem pointless to force their users to jump through hoops just to avoid the innocuous shadowing. Such as in this example: #define Abs(x) \ __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) #define Min(x, y) \ __extension__ (({ __typeof__ (x) _x = x; __typeof__ (y) _y = y; _x < _y ? _x : _y; })) int f (int x, int y) { return Abs (Min (x, y)); // -Wshadow for _x? } Martin > Thanks, > Richard > >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? >> >> >> Thanks >> Bernd. >> >> 2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> * genautomata.c (REGEXP_UNIT, REGEXP_RESERV, REGEXP_SEQUENCE, >> REGEXP_REPEAT, REGEXP_ALLOF, REGEXP_ONEOF): Rename local vars. >> >> Index: gcc/genautomata.c >> =================================================================== >> --- gcc/genautomata.c (revision 276484) >> +++ gcc/genautomata.c (working copy) >> @@ -984,46 +984,46 @@ decl_mode_check_failed (enum decl_mode mode, const >> >> >> #define REGEXP_UNIT(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_unit) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_unit", \ >> +(({ struct regexp *const _regex1 = (r); \ >> + if (_regex1->mode != rm_unit) \ >> + regexp_mode_check_failed (_regex1->mode, "rm_unit", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.unit; })) >> + &(_regex1)->regexp.unit; })) >> >> #define REGEXP_RESERV(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_reserv) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_reserv", \ >> +(({ struct regexp *const _regex2 = (r); \ >> + if (_regex2->mode != rm_reserv) \ >> + regexp_mode_check_failed (_regex2->mode, "rm_reserv", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.reserv; })) >> + &(_regex2)->regexp.reserv; })) >> >> #define REGEXP_SEQUENCE(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_sequence) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_sequence", \ >> +(({ struct regexp *const _regex3 = (r); \ >> + if (_regex3->mode != rm_sequence) \ >> + regexp_mode_check_failed (_regex3->mode, "rm_sequence", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.sequence; })) >> + &(_regex3)->regexp.sequence; })) >> >> #define REGEXP_REPEAT(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_repeat) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_repeat", \ >> +(({ struct regexp *const _regex4 = (r); \ >> + if (_regex4->mode != rm_repeat) \ >> + regexp_mode_check_failed (_regex4->mode, "rm_repeat", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.repeat; })) >> + &(_regex4)->regexp.repeat; })) >> >> #define REGEXP_ALLOF(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_allof) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_allof", \ >> +(({ struct regexp *const _regex5 = (r); \ >> + if (_regex5->mode != rm_allof) \ >> + regexp_mode_check_failed (_regex5->mode, "rm_allof", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.allof; })) >> + &(_regex5)->regexp.allof; })) >> >> #define REGEXP_ONEOF(r) __extension__ \ >> -(({ struct regexp *const _regexp = (r); \ >> - if (_regexp->mode != rm_oneof) \ >> - regexp_mode_check_failed (_regexp->mode, "rm_oneof", \ >> +(({ struct regexp *const _regex6 = (r); \ >> + if (_regex6->mode != rm_oneof) \ >> + regexp_mode_check_failed (_regex6->mode, "rm_oneof", \ >> __FILE__, __LINE__, __FUNCTION__); \ >> - &(_regexp)->regexp.oneof; })) >> + &(_regex6)->regexp.oneof; })) >> >> static const char *regexp_name (enum regexp_mode); >> static void regexp_mode_check_failed (enum regexp_mode, const char *,
On Wed, Oct 16, 2019 at 09:43:49AM -0600, Martin Sebor wrote: > Should the warning trigger when the shadowing name results from > macro expansion? The author of a macro can't (in general) know > what context it's going to be used, and when different macros > come from two different third party headers, it would seem > pointless to force their users to jump through hoops just to > avoid the innocuous shadowing. Such as in this example: > > #define Abs(x) \ > __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) > > #define Min(x, y) \ > __extension__ (({ __typeof__ (x) _x = x; __typeof__ (y) _y = y; _x < _y ? > _x : _y; })) > > int f (int x, int y) > { > return Abs (Min (x, y)); // -Wshadow for _x? > } The counter example would be: #define F(x) \ __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) #define G(x) \ __extension__ (({ __typeof__ (x) _x = x; F(_x); })) where a -Wshadow diagnostics could point the author at a serious bug, because in the expansion it will be __typeof__ (_x) _x = _x; ... Jakub
On 10/16/19 9:50 AM, Jakub Jelinek wrote: > On Wed, Oct 16, 2019 at 09:43:49AM -0600, Martin Sebor wrote: >> Should the warning trigger when the shadowing name results from >> macro expansion? The author of a macro can't (in general) know >> what context it's going to be used, and when different macros >> come from two different third party headers, it would seem >> pointless to force their users to jump through hoops just to >> avoid the innocuous shadowing. Such as in this example: >> >> #define Abs(x) \ >> __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) >> >> #define Min(x, y) \ >> __extension__ (({ __typeof__ (x) _x = x; __typeof__ (y) _y = y; _x < _y ? >> _x : _y; })) >> >> int f (int x, int y) >> { >> return Abs (Min (x, y)); // -Wshadow for _x? >> } > > The counter example would be: > #define F(x) \ > __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) > #define G(x) \ > __extension__ (({ __typeof__ (x) _x = x; F(_x); })) > where a -Wshadow diagnostics could point the author at a serious bug, > because in the expansion it will be __typeof__ (_x) _x = _x; ... True. I don't suppose there is a way to make it so the warning triggers for the counter example and not for the original, is there? Martin PS The counterexample nicely illustrates why -Wself-init should be in -Wall like in Clang or MSVC, or at least in -Wextra like in ICC. Let me take it as a reminder to submit a patch for GCC 10.
On Wed, 16 Oct 2019, Jakub Jelinek wrote: > The counter example would be: > #define F(x) \ > __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) > #define G(x) \ > __extension__ (({ __typeof__ (x) _x = x; F(_x); })) > where a -Wshadow diagnostics could point the author at a serious bug, > because in the expansion it will be __typeof__ (_x) _x = _x; ... And this is not theoretical, <https://sourceware.org/bugzilla/show_bug.cgi?id=15667> <https://sourceware.org/ml/libc-alpha/2013-06/msg00851.html> was a real bug in glibc soft-fp where shadowing of variables called _c1 and _c2 in two macros resulted in wrong code.
On Wed, Oct 16, 2019 at 10:03:51AM -0600, Martin Sebor wrote: > > The counter example would be: > > #define F(x) \ > > __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) > > #define G(x) \ > > __extension__ (({ __typeof__ (x) _x = x; F(_x); })) > > where a -Wshadow diagnostics could point the author at a serious bug, > > because in the expansion it will be __typeof__ (_x) _x = _x; ... > > True. I don't suppose there is a way to make it so the warning > triggers for the counter example and not for the original, is > there? Maybe look through the macro nesting context and if the shadowing declaration comes from the same macro as shadowed declaration or macro included directly or indirectly from the macro with shadowed declaration, warn, otherwise not? This might still not warn in case where the scope of the shadowing declaration is created from multiple macros ({ coming from one, }) from another one, but otherwise could work. Perhaps -Wshadow-local needs multiple modes, the default one that will have this macro handling and full one (=2) which would warn regardless of macro definitions. Jakub
On Wed, Oct 16, 2019 at 10:03:51AM -0600, Martin Sebor wrote: > PS The counterexample nicely illustrates why -Wself-init should > be in -Wall like in Clang or MSVC, or at least in -Wextra like in > ICC. Let me take it as a reminder to submit a patch for GCC 10. c-family/c-gimplify.c says: /* This is handled mostly by gimplify.c, but we have to deal with not warning about int x = x; as it is a GCC extension to turn off this warning but only if warn_init_self is zero. */ A lot of code will start to warn if you turn on -Winit-self by default (in -Wall or -W), since forever we have had this GCC extension, and people expect it. (Or so I fear, feel free to prove me wrong :-) ) Segher
On 10/16/19, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Oct 16, 2019 at 10:03:51AM -0600, Martin Sebor wrote: >> > The counter example would be: >> > #define F(x) \ >> > __extension__ (({ __typeof__ (x) _x = x; _x < 0 ? -_x : _x; })) >> > #define G(x) \ >> > __extension__ (({ __typeof__ (x) _x = x; F(_x); })) >> > where a -Wshadow diagnostics could point the author at a serious bug, >> > because in the expansion it will be __typeof__ (_x) _x = _x; ... >> >> True. I don't suppose there is a way to make it so the warning >> triggers for the counter example and not for the original, is >> there? > > Maybe look through the macro nesting context and if the shadowing > declaration comes from the same macro as shadowed declaration > or macro included directly or indirectly from the macro with shadowed > declaration, warn, otherwise not? > This might still not warn in case where the scope of the shadowing > declaration is created from multiple macros ({ coming from one, > }) from another one, but otherwise could work. > Perhaps -Wshadow-local needs multiple modes, the default one that > will have this macro handling and full one (=2) which would warn > regardless of macro definitions. > I'm worried about the proliferation of the number of '=' signs here... there's already confusion as to whether the first '=' represents levels or just a different spelling of names, adding a second would only compound the confusion. > Jakub >
On 10/16/19 9:43 AM, Martin Sebor wrote: > On 10/16/19 9:11 AM, Richard Sandiford wrote: >> Sorry for the slow reply. >> >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >>> Hi, >>> >>> this is probably on the border to obvious. >>> >>> The REGEXP_xxx macros in genautomata are invoked >>> recursively, and the local values are all named _regexp >>> and shadow each other. >>> >>> >>> Fixed by using different names _regexp1..6 for each >>> macro. >> >> Sorry to repeat the complaint about numerical suffixes, but I think >> we'd need better names. E.g. _regexp_unit or _re_unit for REGEXP_UNIT >> and similarly for the other macros. But a similar fix to rtl.h might >> be better. > > Should the warning trigger when the shadowing name results from > macro expansion? The author of a macro can't (in general) know > what context it's going to be used, and when different macros > come from two different third party headers, it would seem > pointless to force their users to jump through hoops just to > avoid the innocuous shadowing. Such as in this example: One could make the argument that if you want to avoid shadowing, then you should avoid code within macros for this precise reason. And if you're getting code within macros from 3rd parties, then well, you're in for a world of pain if you're going to try to be shadow-free. jeff
2019-10-04 Bernd Edlinger <bernd.edlinger@hotmail.de> * genautomata.c (REGEXP_UNIT, REGEXP_RESERV, REGEXP_SEQUENCE, REGEXP_REPEAT, REGEXP_ALLOF, REGEXP_ONEOF): Rename local vars. Index: gcc/genautomata.c =================================================================== --- gcc/genautomata.c (revision 276484) +++ gcc/genautomata.c (working copy) @@ -984,46 +984,46 @@ decl_mode_check_failed (enum decl_mode mode, const #define REGEXP_UNIT(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_unit) \ - regexp_mode_check_failed (_regexp->mode, "rm_unit", \ +(({ struct regexp *const _regex1 = (r); \ + if (_regex1->mode != rm_unit) \ + regexp_mode_check_failed (_regex1->mode, "rm_unit", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.unit; })) + &(_regex1)->regexp.unit; })) #define REGEXP_RESERV(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_reserv) \ - regexp_mode_check_failed (_regexp->mode, "rm_reserv", \ +(({ struct regexp *const _regex2 = (r); \ + if (_regex2->mode != rm_reserv) \ + regexp_mode_check_failed (_regex2->mode, "rm_reserv", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.reserv; })) + &(_regex2)->regexp.reserv; })) #define REGEXP_SEQUENCE(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_sequence) \ - regexp_mode_check_failed (_regexp->mode, "rm_sequence", \ +(({ struct regexp *const _regex3 = (r); \ + if (_regex3->mode != rm_sequence) \ + regexp_mode_check_failed (_regex3->mode, "rm_sequence", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.sequence; })) + &(_regex3)->regexp.sequence; })) #define REGEXP_REPEAT(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_repeat) \ - regexp_mode_check_failed (_regexp->mode, "rm_repeat", \ +(({ struct regexp *const _regex4 = (r); \ + if (_regex4->mode != rm_repeat) \ + regexp_mode_check_failed (_regex4->mode, "rm_repeat", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.repeat; })) + &(_regex4)->regexp.repeat; })) #define REGEXP_ALLOF(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_allof) \ - regexp_mode_check_failed (_regexp->mode, "rm_allof", \ +(({ struct regexp *const _regex5 = (r); \ + if (_regex5->mode != rm_allof) \ + regexp_mode_check_failed (_regex5->mode, "rm_allof", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.allof; })) + &(_regex5)->regexp.allof; })) #define REGEXP_ONEOF(r) __extension__ \ -(({ struct regexp *const _regexp = (r); \ - if (_regexp->mode != rm_oneof) \ - regexp_mode_check_failed (_regexp->mode, "rm_oneof", \ +(({ struct regexp *const _regex6 = (r); \ + if (_regex6->mode != rm_oneof) \ + regexp_mode_check_failed (_regex6->mode, "rm_oneof", \ __FILE__, __LINE__, __FUNCTION__); \ - &(_regexp)->regexp.oneof; })) + &(_regex6)->regexp.oneof; })) static const char *regexp_name (enum regexp_mode); static void regexp_mode_check_failed (enum regexp_mode, const char *,