Re: [PATCH] Fix PR c++/92024
On 10/12/19 2:10 PM, Bernd Edlinger wrote: On 10/11/19 6:31 PM, Jason Merrill wrote: On 10/10/19 2:06 PM, Bernd Edlinger wrote: On 10/10/19 7:49 PM, Jason Merrill wrote: On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C Why does that flag cause this crash? gcc/cp/name-lookup.c: if (warn_shadow) warning_code = OPT_Wshadow; else if (warn_shadow_local) warning_code = OPT_Wshadow_local; else if (warn_shadow_compatible_local && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) || (!dependent_type_p (TREE_TYPE (decl)) && !dependent_type_p (TREE_TYPE (old)) /* If the new decl uses auto, we don't yet know its type (the old type cannot be using auto at this point, without also being dependent). This is an indication we're (now) doing the shadow checking too early. */ && !type_uses_auto (TREE_TYPE (decl)) && can_convert (TREE_TYPE (old), TREE_TYPE (decl), tf_none warning_code = OPT_Wshadow_compatible_local; if -Wshadow=compatible-local is used, the can_convert function crashes in instantiate_class_template_1. Right, checking can_convert is problematic here, as it can cause template instantiations that change the semantics of the program. Or, in this case, crash. So I try to make C++ behave more consistently with the code in c-decl.c, thus dependent on warn_shadow but not on warn_shadow_local and/or warn_shadow_compatible_local: if (warn_shadow) warning_code = OPT_Wshadow; else if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) warning_code = OPT_Wshadow_compatible_local; else warning_code = OPT_Wshadow_local; warned = warning_at (DECL_SOURCE_LOCATION (new_decl), warning_code, "declaration of %qD shadows a parameter", new_decl); I cannot remove the if (warn_shadow) since this breaks gcc.dg/pr48062.c which uses: #pragma GCC diagnostic ignored "-Wshadow" to disable a -Wshadow=compatible-local warning, but while -Wno-shadow on the command line disables also dependent warnings the pragma does not (always) do that. So instead I'd like to adjust the doc of -Wshadow to reflect the implementation and remove the if(warn_shadow_local) to have C and C++ behave identical and hopefully now in sync with the doc. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? OK, thanks. Jason
[PING**2] [PATCH] Fix PR c++/92024
Ping... On 10/18/19 3:19 PM, Bernd Edlinger wrote: > Ping... > > for the c++ FE and testsuite changes in the updated patch > here: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00916.html > > > Thanks > Bernd. > > > > > On 10/12/19 8:10 PM, Bernd Edlinger wrote: >> On 10/11/19 6:31 PM, Jason Merrill wrote: >>> On 10/10/19 2:06 PM, Bernd Edlinger wrote: On 10/10/19 7:49 PM, Jason Merrill wrote: if -Wshadow=compatible-local is used, the can_convert function crashes in instantiate_class_template_1. >>> >>> Right, checking can_convert is problematic here, as it can cause template >>> instantiations that change the semantics of the program. Or, in this case, >>> crash. >>> >> >> So I try to make C++ behave more consistently with the code in c-decl.c, >> thus dependent on warn_shadow but not on warn_shadow_local and/or >> warn_shadow_compatible_local: >> >>if (warn_shadow) >> warning_code = OPT_Wshadow; >> else if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) >> warning_code = OPT_Wshadow_compatible_local; >> else >> warning_code = OPT_Wshadow_local; >> warned = warning_at (DECL_SOURCE_LOCATION (new_decl), >> warning_code, >> "declaration of %qD shadows a parameter", >> new_decl); >> >> I cannot remove the if (warn_shadow) since this breaks gcc.dg/pr48062.c >> which uses: >> >> #pragma GCC diagnostic ignored "-Wshadow" >> >> to disable a -Wshadow=compatible-local warning, but while -Wno-shadow on the >> command line disables also dependent warnings the pragma does not (always) >> do that. >> >> So instead I'd like to adjust the doc of -Wshadow to reflect the >> implementation >> and remove the if(warn_shadow_local) to have C and C++ behave identical and >> hopefully now in sync with the doc. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? >> >> >> Thanks >> Bernd. >>
Re: [PATCH] Fix PR c++/92024
Ping... for the c++ FE and testsuite changes in the updated patch here: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg00916.html Thanks Bernd. On 10/12/19 8:10 PM, Bernd Edlinger wrote: > On 10/11/19 6:31 PM, Jason Merrill wrote: >> On 10/10/19 2:06 PM, Bernd Edlinger wrote: >>> On 10/10/19 7:49 PM, Jason Merrill wrote: >>> >>> if -Wshadow=compatible-local is used, the can_convert function crashes >>> in instantiate_class_template_1. >> >> Right, checking can_convert is problematic here, as it can cause template >> instantiations that change the semantics of the program. Or, in this case, >> crash. >> > > So I try to make C++ behave more consistently with the code in c-decl.c, > thus dependent on warn_shadow but not on warn_shadow_local and/or > warn_shadow_compatible_local: > >if (warn_shadow) > warning_code = OPT_Wshadow; > else if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) > warning_code = OPT_Wshadow_compatible_local; > else > warning_code = OPT_Wshadow_local; > warned = warning_at (DECL_SOURCE_LOCATION (new_decl), > warning_code, > "declaration of %qD shadows a parameter", > new_decl); > > I cannot remove the if (warn_shadow) since this breaks gcc.dg/pr48062.c > which uses: > > #pragma GCC diagnostic ignored "-Wshadow" > > to disable a -Wshadow=compatible-local warning, but while -Wno-shadow on the > command line disables also dependent warnings the pragma does not (always) do > that. > > So instead I'd like to adjust the doc of -Wshadow to reflect the > implementation > and remove the if(warn_shadow_local) to have C and C++ behave identical and > hopefully now in sync with the doc. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > Is it OK for trunk? > > > Thanks > Bernd. >
Re: [PATCH] Fix PR c++/92024
On 10/12/19 12:10 PM, Bernd Edlinger wrote: [snip snip] So instead I'd like to adjust the doc of -Wshadow to reflect the implementation and remove the if(warn_shadow_local) to have C and C++ behave identical and hopefully now in sync with the doc. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? The documentation changes look good to me. -Sandra
Re: [PATCH] Fix PR c++/92024
On 10/11/19 6:31 PM, Jason Merrill wrote: > On 10/10/19 2:06 PM, Bernd Edlinger wrote: >> On 10/10/19 7:49 PM, Jason Merrill wrote: >>> On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C >>> >>> Why does that flag cause this crash? >>> >> >> gcc/cp/name-lookup.c: >> >> if (warn_shadow) >> warning_code = OPT_Wshadow; >> else if (warn_shadow_local) >> warning_code = OPT_Wshadow_local; >> else if (warn_shadow_compatible_local >> && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) >> || (!dependent_type_p (TREE_TYPE (decl)) >> && !dependent_type_p (TREE_TYPE (old)) >> /* If the new decl uses auto, we don't yet know >> its type (the old type cannot be using auto >> at this point, without also being >> dependent). This is an indication we're >> (now) doing the shadow checking too >> early. */ >> && !type_uses_auto (TREE_TYPE (decl)) >> && can_convert (TREE_TYPE (old), TREE_TYPE (decl), >> tf_none >> warning_code = OPT_Wshadow_compatible_local; >> >> if -Wshadow=compatible-local is used, the can_convert function crashes >> in instantiate_class_template_1. > > Right, checking can_convert is problematic here, as it can cause template > instantiations that change the semantics of the program. Or, in this case, > crash. > So I try to make C++ behave more consistently with the code in c-decl.c, thus dependent on warn_shadow but not on warn_shadow_local and/or warn_shadow_compatible_local: if (warn_shadow) warning_code = OPT_Wshadow; else if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl))) warning_code = OPT_Wshadow_compatible_local; else warning_code = OPT_Wshadow_local; warned = warning_at (DECL_SOURCE_LOCATION (new_decl), warning_code, "declaration of %qD shadows a parameter", new_decl); I cannot remove the if (warn_shadow) since this breaks gcc.dg/pr48062.c which uses: #pragma GCC diagnostic ignored "-Wshadow" to disable a -Wshadow=compatible-local warning, but while -Wno-shadow on the command line disables also dependent warnings the pragma does not (always) do that. So instead I'd like to adjust the doc of -Wshadow to reflect the implementation and remove the if(warn_shadow_local) to have C and C++ behave identical and hopefully now in sync with the doc. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. 2019-10-12 Bernd Edlinger * doc/invoke.texi (-Wshadow, -Wshadow=global -Wshadow=local, -Wshadow=compatible-local): Update documentation. cp: 2019-10-12 Bernd Edlinger PR c++/92024 * name-lookup.c (check_local_shadow): Shadowing TYPE_DECLs is always a -Wshadow=compatible-local warning, unless -Wshadow is used. testsuite: 2019-10-12 Bernd Edlinger PR c++/92024 * g++.dg/parse/crash70.C: New test. * c-c++-common/Wshadow-1.c: New test. Index: gcc/cp/name-lookup.c === --- gcc/cp/name-lookup.c (revision 276893) +++ gcc/cp/name-lookup.c (working copy) @@ -2750,29 +2750,31 @@ check_local_shadow (tree decl) parameter, more than often they would use the variable thinking (mistakenly) it's still the parameter. It would be rare that users would use the variable in the place that - expects the parameter but thinking it's a new decl. */ + expects the parameter but thinking it's a new decl. + If either object is a TYPE_DECL, '-Wshadow=compatible-local' + warns regardless of whether one of the types involved + is a subclass of the other, since that is never okay. */ enum opt_code warning_code; if (warn_shadow) warning_code = OPT_Wshadow; - else if (warn_shadow_local) - warning_code = OPT_Wshadow_local; - else if (warn_shadow_compatible_local - && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) - || (!dependent_type_p (TREE_TYPE (decl)) - && !dependent_type_p (TREE_TYPE (old)) - /* If the new decl uses auto, we don't yet know - its type (the old type cannot be using auto - at this point, without also being - dependent). This is an indication we're - (now) doing the shadow checking too - early. */ - && !type_uses_auto (TREE_TYPE (decl)) - && can_convert (TREE_TYPE (old), TREE_TYPE (decl), - tf_none + else if (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) + || TREE_CODE (decl) == TYPE_DECL + || TREE_CODE (old) == TYPE_DECL +
Re: [PATCH] Fix PR c++/92024
On 10/11/19 6:31 PM, Jason Merrill wrote: > On 10/10/19 2:06 PM, Bernd Edlinger wrote: >> On 10/10/19 7:49 PM, Jason Merrill wrote: >>> On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C >>> >>> Why does that flag cause this crash? >>> >> >> gcc/cp/name-lookup.c: >> >> if (warn_shadow) >> warning_code = OPT_Wshadow; >> else if (warn_shadow_local) >> warning_code = OPT_Wshadow_local; >> else if (warn_shadow_compatible_local >> && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) >> || (!dependent_type_p (TREE_TYPE (decl)) >> && !dependent_type_p (TREE_TYPE (old)) >> /* If the new decl uses auto, we don't yet know >> its type (the old type cannot be using auto >> at this point, without also being >> dependent). This is an indication we're >> (now) doing the shadow checking too >> early. */ >> && !type_uses_auto (TREE_TYPE (decl)) >> && can_convert (TREE_TYPE (old), TREE_TYPE (decl), >> tf_none >> warning_code = OPT_Wshadow_compatible_local; >> >> if -Wshadow=compatible-local is used, the can_convert function crashes >> in instantiate_class_template_1. > > Right, checking can_convert is problematic here, as it can cause template > instantiations that change the semantics of the program. Or, in this case, > crash. > Ah, alright. I think shadowing one type with another type of the same name is always a no no. How about always warning (and not asking for can_convert) when either decl is a TYPE_DECL? like this: Index: gcc/cp/name-lookup.c === --- gcc/cp/name-lookup.c(Revision 276886) +++ gcc/cp/name-lookup.c(Arbeitskopie) @@ -2741,8 +2741,7 @@ return; } - /* If '-Wshadow=compatible-local' is specified without other --Wshadow= flags, we will warn only when the type of the + /* -Wshadow=compatible-local warns only when the type of the shadowing variable (DECL) can be converted to that of the shadowed parameter (OLD_LOCAL). The reason why we only check if DECL's type can be converted to OLD_LOCAL's type (but not the @@ -2750,29 +2749,29 @@ parameter, more than often they would use the variable thinking (mistakenly) it's still the parameter. It would be rare that users would use the variable in the place that -expects the parameter but thinking it's a new decl. */ +expects the parameter but thinking it's a new decl. +If either object is a TYPE_DECL -Wshadow=compatible-local +warns regardless of whether one of the types involved +is a subclass of the other, since that is never okay. */ enum opt_code warning_code; - if (warn_shadow) - warning_code = OPT_Wshadow; - else if (warn_shadow_local) - warning_code = OPT_Wshadow_local; - else if (warn_shadow_compatible_local - && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) - || (!dependent_type_p (TREE_TYPE (decl)) - && !dependent_type_p (TREE_TYPE (old)) - /* If the new decl uses auto, we don't yet know - its type (the old type cannot be using auto - at this point, without also being - dependent). This is an indication we're - (now) doing the shadow checking too - early. */ - && !type_uses_auto (TREE_TYPE (decl)) - && can_convert (TREE_TYPE (old), TREE_TYPE (decl), - tf_none + if (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)) + && !dependent_type_p (TREE_TYPE (old)) + /* If the new decl uses auto, we don't yet know +its type (the old type cannot be using auto +at this point, without also being +dependent). This is an indication we're +(now) doing the shadow checking too +early. */ + && !type_uses_auto (TREE_TYPE (decl)) + && can_convert (TREE_TYPE (old), TREE_TYPE (decl), + tf_none))) warning_code = OPT_Wshadow_compatible_local; else - return; + warning_code = OPT_Wshadow_local; const char *msg; if (TREE_CODE (old) == PARM_DECL) Bernd.
Re: [PATCH] Fix PR c++/92024
On 10/10/19 2:06 PM, Bernd Edlinger wrote: On 10/10/19 7:49 PM, Jason Merrill wrote: On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C Why does that flag cause this crash? gcc/cp/name-lookup.c: if (warn_shadow) warning_code = OPT_Wshadow; else if (warn_shadow_local) warning_code = OPT_Wshadow_local; else if (warn_shadow_compatible_local && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) || (!dependent_type_p (TREE_TYPE (decl)) && !dependent_type_p (TREE_TYPE (old)) /* If the new decl uses auto, we don't yet know its type (the old type cannot be using auto at this point, without also being dependent). This is an indication we're (now) doing the shadow checking too early. */ && !type_uses_auto (TREE_TYPE (decl)) && can_convert (TREE_TYPE (old), TREE_TYPE (decl), tf_none warning_code = OPT_Wshadow_compatible_local; if -Wshadow=compatible-local is used, the can_convert function crashes in instantiate_class_template_1. Right, checking can_convert is problematic here, as it can cause template instantiations that change the semantics of the program. Or, in this case, crash. Jason
Re: [PATCH] Fix PR c++/92024
On Thu, Oct 10, 2019 at 06:06:40PM +, Bernd Edlinger wrote:
> On 10/10/19 7:49 PM, Jason Merrill wrote:
> > On 10/10/19 10:42 AM, Bernd Edlinger wrote:
> >> Hi,
> >>
> >> this fixes a crash when -Wshadow=compatible-local is
> >> enabled in the testcase g++.dg/parse/crash68.C
> >
> > Why does that flag cause this crash?
> >
>
> gcc/cp/name-lookup.c:
>
> if (warn_shadow)
> warning_code = OPT_Wshadow;
> else if (warn_shadow_local)
> warning_code = OPT_Wshadow_local;
> else if (warn_shadow_compatible_local
>&& (same_type_p (TREE_TYPE (old), TREE_TYPE (decl))
>|| (!dependent_type_p (TREE_TYPE (decl))
>&& !dependent_type_p (TREE_TYPE (old))
>/* If the new decl uses auto, we don't yet know
> its type (the old type cannot be using auto
> at this point, without also being
> dependent). This is an indication we're
> (now) doing the shadow checking too
> early. */
>&& !type_uses_auto (TREE_TYPE (decl))
>&& can_convert (TREE_TYPE (old), TREE_TYPE (decl),
>tf_none
> warning_code = OPT_Wshadow_compatible_local;
>
> if -Wshadow=compatible-local is used, the can_convert function crashes
> in instantiate_class_template_1.
>
> The problem there is that CLASSTYPE_TI_TEMPLATE (type)
> uses CLASSTYPE_TEMPLATE_INFO (type) but that is NULL.
... because the warning is called from pushtag which ran before building the
template info for 'c':
9926 // Build template info for the new specialization.
9927 SET_TYPE_TEMPLATE_INFO (t, build_template_info (found, arglist));
Looks like another indication that the shadow checking is done too early.
> Since other errors may return error_mark_node as well
> and the template is totally erroneous anyway, I figured
> we might get away with that simple fix.
But it doesn't have to be broken; this valid test crashes with that option too
template
struct S {
S () {
struct c;
{
struct c {};
}
}
};
S s;
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Re: [PATCH] Fix PR c++/92024
On 10/10/19 7:49 PM, Jason Merrill wrote: > On 10/10/19 10:42 AM, Bernd Edlinger wrote: >> Hi, >> >> this fixes a crash when -Wshadow=compatible-local is >> enabled in the testcase g++.dg/parse/crash68.C > > Why does that flag cause this crash? > gcc/cp/name-lookup.c: if (warn_shadow) warning_code = OPT_Wshadow; else if (warn_shadow_local) warning_code = OPT_Wshadow_local; else if (warn_shadow_compatible_local && (same_type_p (TREE_TYPE (old), TREE_TYPE (decl)) || (!dependent_type_p (TREE_TYPE (decl)) && !dependent_type_p (TREE_TYPE (old)) /* If the new decl uses auto, we don't yet know its type (the old type cannot be using auto at this point, without also being dependent). This is an indication we're (now) doing the shadow checking too early. */ && !type_uses_auto (TREE_TYPE (decl)) && can_convert (TREE_TYPE (old), TREE_TYPE (decl), tf_none warning_code = OPT_Wshadow_compatible_local; if -Wshadow=compatible-local is used, the can_convert function crashes in instantiate_class_template_1. The problem there is that CLASSTYPE_TI_TEMPLATE (type) uses CLASSTYPE_TEMPLATE_INFO (type) but that is NULL. Since other errors may return error_mark_node as well and the template is totally erroneous anyway, I figured we might get away with that simple fix. Bernd.
Re: [PATCH] Fix PR c++/92024
On 10/10/19 10:42 AM, Bernd Edlinger wrote: Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C Why does that flag cause this crash? Jason
[PATCH] Fix PR c++/92024
Hi, this fixes a crash when -Wshadow=compatible-local is enabled in the testcase g++.dg/parse/crash68.C Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. patch-pr92024.diff Description: patch-pr92024.diff
