| Message ID | 20200610211148.1237792-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix ICE in check_local_shadow with enum [PR95560] | expand |
On 6/10/20 5:11 PM, Marek Polacek wrote: > Another indication that perhaps this warning is emitted too early. We > crash because same_type_p gets a null type: we have an enumerator > without a fixed underlying type and finish_enum_value_list hasn't yet > run. So check if the type is null before calling same_type_p. Hmm, I wonder why we use NULL_TREE for the type of uninitialized enumerators in a template; why not give them integer_type_node temporarily? > (This is a regression and this fix is suitable for backporting. Delaying > the warning would not be suitable to backport.) > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9? > > gcc/cp/ChangeLog: > > PR c++/95560 > * name-lookup.c (check_local_shadow): Check if types are > non-null before calling same_type_p. > > gcc/testsuite/ChangeLog: > > PR c++/95560 > * g++.dg/warn/Wshadow-local-3.C: New test. > --- > gcc/cp/name-lookup.c | 4 +++- > gcc/testsuite/g++.dg/warn/Wshadow-local-3.C | 7 +++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-3.C > > diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c > index 2ff85f1cf5e..159c98a67cd 100644 > --- a/gcc/cp/name-lookup.c > +++ b/gcc/cp/name-lookup.c > @@ -2762,7 +2762,9 @@ check_local_shadow (tree decl) > enum opt_code warning_code; > if (warn_shadow) > warning_code = OPT_Wshadow; > - else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) > + else if ((TREE_TYPE (old) > + && TREE_TYPE (decl) > + && same_type_p (TREE_TYPE (old), TREE_TYPE (decl))) > || TREE_CODE (decl) == TYPE_DECL > || TREE_CODE (old) == TYPE_DECL > || (!dependent_type_p (TREE_TYPE (decl)) > diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C > new file mode 100644 > index 00000000000..fd743eca347 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C > @@ -0,0 +1,7 @@ > +// PR c++/95560 > +// { dg-do compile { target c++11 } } > + > +template <typename> void fn1() { > + bool ready; > + enum class State { ready }; > +} > > base-commit: 3a391adf7a38780f8d01dbac08a2a143fc80b469 >
On 6/11/20 3:32 PM, Jason Merrill wrote: > On 6/10/20 5:11 PM, Marek Polacek wrote: >> Another indication that perhaps this warning is emitted too early. We >> crash because same_type_p gets a null type: we have an enumerator >> without a fixed underlying type and finish_enum_value_list hasn't yet >> run. So check if the type is null before calling same_type_p. > > Hmm, I wonder why we use NULL_TREE for the type of uninitialized > enumerators in a template; why not give them integer_type_node temporarily? But this patch is OK for 10.2. >> (This is a regression and this fix is suitable for backporting. Delaying >> the warning would not be suitable to backport.) >> >> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9? >> >> gcc/cp/ChangeLog: >> >> PR c++/95560 >> * name-lookup.c (check_local_shadow): Check if types are >> non-null before calling same_type_p. >> >> gcc/testsuite/ChangeLog: >> >> PR c++/95560 >> * g++.dg/warn/Wshadow-local-3.C: New test. >> --- >> gcc/cp/name-lookup.c | 4 +++- >> gcc/testsuite/g++.dg/warn/Wshadow-local-3.C | 7 +++++++ >> 2 files changed, 10 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >> >> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c >> index 2ff85f1cf5e..159c98a67cd 100644 >> --- a/gcc/cp/name-lookup.c >> +++ b/gcc/cp/name-lookup.c >> @@ -2762,7 +2762,9 @@ check_local_shadow (tree decl) >> enum opt_code warning_code; >> if (warn_shadow) >> warning_code = OPT_Wshadow; >> - else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) >> + else if ((TREE_TYPE (old) >> + && TREE_TYPE (decl) >> + && same_type_p (TREE_TYPE (old), TREE_TYPE (decl))) >> || TREE_CODE (decl) == TYPE_DECL >> || TREE_CODE (old) == TYPE_DECL >> || (!dependent_type_p (TREE_TYPE (decl)) >> diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >> b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >> new file mode 100644 >> index 00000000000..fd743eca347 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >> @@ -0,0 +1,7 @@ >> +// PR c++/95560 >> +// { dg-do compile { target c++11 } } >> + >> +template <typename> void fn1() { >> + bool ready; >> + enum class State { ready }; >> +} >> >> base-commit: 3a391adf7a38780f8d01dbac08a2a143fc80b469 >> >
On 6/11/20 3:34 PM, Jason Merrill wrote: > On 6/11/20 3:32 PM, Jason Merrill wrote: >> On 6/10/20 5:11 PM, Marek Polacek wrote: >>> Another indication that perhaps this warning is emitted too early. We >>> crash because same_type_p gets a null type: we have an enumerator >>> without a fixed underlying type and finish_enum_value_list hasn't yet >>> run. So check if the type is null before calling same_type_p. >> >> Hmm, I wonder why we use NULL_TREE for the type of uninitialized >> enumerators in a template; why not give them integer_type_node >> temporarily? > > But this patch is OK for 10.2. And 9. > >>> (This is a regression and this fix is suitable for backporting. >>> Delaying >>> the warning would not be suitable to backport.) >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10/9? >>> >>> gcc/cp/ChangeLog: >>> >>> PR c++/95560 >>> * name-lookup.c (check_local_shadow): Check if types are >>> non-null before calling same_type_p. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c++/95560 >>> * g++.dg/warn/Wshadow-local-3.C: New test. >>> --- >>> gcc/cp/name-lookup.c | 4 +++- >>> gcc/testsuite/g++.dg/warn/Wshadow-local-3.C | 7 +++++++ >>> 2 files changed, 10 insertions(+), 1 deletion(-) >>> create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >>> >>> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c >>> index 2ff85f1cf5e..159c98a67cd 100644 >>> --- a/gcc/cp/name-lookup.c >>> +++ b/gcc/cp/name-lookup.c >>> @@ -2762,7 +2762,9 @@ check_local_shadow (tree decl) >>> enum opt_code warning_code; >>> if (warn_shadow) >>> warning_code = OPT_Wshadow; >>> - else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) >>> + else if ((TREE_TYPE (old) >>> + && TREE_TYPE (decl) >>> + && same_type_p (TREE_TYPE (old), TREE_TYPE (decl))) >>> || TREE_CODE (decl) == TYPE_DECL >>> || TREE_CODE (old) == TYPE_DECL >>> || (!dependent_type_p (TREE_TYPE (decl)) >>> diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >>> b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >>> new file mode 100644 >>> index 00000000000..fd743eca347 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C >>> @@ -0,0 +1,7 @@ >>> +// PR c++/95560 >>> +// { dg-do compile { target c++11 } } >>> + >>> +template <typename> void fn1() { >>> + bool ready; >>> + enum class State { ready }; >>> +} >>> >>> base-commit: 3a391adf7a38780f8d01dbac08a2a143fc80b469 >>> >> >
On Thu, Jun 11, 2020 at 03:32:14PM -0400, Jason Merrill wrote: > On 6/10/20 5:11 PM, Marek Polacek wrote: > > Another indication that perhaps this warning is emitted too early. We > > crash because same_type_p gets a null type: we have an enumerator > > without a fixed underlying type and finish_enum_value_list hasn't yet > > run. So check if the type is null before calling same_type_p. > > Hmm, I wonder why we use NULL_TREE for the type of uninitialized enumerators > in a template; why not give them integer_type_node temporarily? That breaks enum22.C: template <class T> struct A { enum e_ : unsigned char { Z_, E_=sizeof(Z_) }; }; static_assert ( A<double>::E_ == 1, "E_ should be 1"); If we give 'Z_' a type, it's no longer instantiation-dependent, so sizeof(Z_) immediately evaluates to 4. Whereas if it doesn't have a type, in the template we create a SIZEOF_EXPR and only evaluate when instantiating (to 1). This sounded like a problem big enough for me not to pursue this any further. Do you want me to try anything else or is the original patch ok? Marek
On 6/15/20 9:20 PM, Marek Polacek wrote: > On Thu, Jun 11, 2020 at 03:32:14PM -0400, Jason Merrill wrote: >> On 6/10/20 5:11 PM, Marek Polacek wrote: >>> Another indication that perhaps this warning is emitted too early. We >>> crash because same_type_p gets a null type: we have an enumerator >>> without a fixed underlying type and finish_enum_value_list hasn't yet >>> run. So check if the type is null before calling same_type_p. >> >> Hmm, I wonder why we use NULL_TREE for the type of uninitialized enumerators >> in a template; why not give them integer_type_node temporarily? > > That breaks enum22.C: > > template <class T> > struct A { > enum e_ : unsigned char { Z_, E_=sizeof(Z_) }; > }; > > static_assert ( A<double>::E_ == 1, "E_ should be 1"); > > If we give 'Z_' a type, it's no longer instantiation-dependent, so sizeof(Z_) > immediately evaluates to 4. Whereas if it doesn't have a type, in the template > we create a SIZEOF_EXPR and only evaluate when instantiating (to 1). > > This sounded like a problem big enough for me not to pursue this any further. > Do you want me to try anything else or is the original patch ok? The original patch is OK, thanks. Jason
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 2ff85f1cf5e..159c98a67cd 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -2762,7 +2762,9 @@ check_local_shadow (tree decl) enum opt_code warning_code; if (warn_shadow) warning_code = OPT_Wshadow; - else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) + else if ((TREE_TYPE (old) + && TREE_TYPE (decl) + && same_type_p (TREE_TYPE (old), TREE_TYPE (decl))) || TREE_CODE (decl) == TYPE_DECL || TREE_CODE (old) == TYPE_DECL || (!dependent_type_p (TREE_TYPE (decl)) diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C new file mode 100644 index 00000000000..fd743eca347 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-3.C @@ -0,0 +1,7 @@ +// PR c++/95560 +// { dg-do compile { target c++11 } } + +template <typename> void fn1() { + bool ready; + enum class State { ready }; +}