diff mbox series

c++: Fix ENABLE_SCOPE_CHECKING printing

Message ID 65a4fdbe.050a0220.b4d36.7277@mx.google.com
State Accepted
Headers
Series c++: Fix ENABLE_SCOPE_CHECKING printing |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Nathaniel Shead Jan. 15, 2024, 9:41 a.m. UTC 
  While working on another bug, I noticed the ENABLE_SCOPE_CHECKING macro
and thought to try it out. It caused selftest to ICE. This patch is a
minimal fix to get it working again.

Probably this should use a test to stop this regressing again in the
future the next time new scope-kinds are added, but given it's dependent
on a (almost certainly rarely-used) build-time macro I'm not sure
exactly how you would do that?

Or alternatively I could add a `sk_count` to the end of the scope kind
list and `static_assert` that the size of the descriptor list matches?

(Also not sure if this would be appropriate for stage 4 or if it should
wait till next stage 1. I suppose this fixes a regression but I suspect
this has been broken for a very long time.)

-- >8 --

The lists of scope kinds used by ENABLE_SCOPE_CHECKING don't seem to
have been updated in a long while, causing ICEs and confusing output.
This patch brings the list into line.

gcc/cp/ChangeLog:

	* name-lookup.cc (cp_binding_level_descriptor): Add missing
	scope kinds.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/name-lookup.cc | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Jason Merrill Jan. 15, 2024, 9:04 p.m. UTC | #1 
On 1/15/24 04:41, Nathaniel Shead wrote:
> While working on another bug, I noticed the ENABLE_SCOPE_CHECKING macro
> and thought to try it out. It caused selftest to ICE. This patch is a
> minimal fix to get it working again.
> 
> Probably this should use a test to stop this regressing again in the
> future the next time new scope-kinds are added, but given it's dependent
> on a (almost certainly rarely-used) build-time macro I'm not sure
> exactly how you would do that?
> 
> Or alternatively I could add a `sk_count` to the end of the scope kind
> list and `static_assert` that the size of the descriptor list matches?

That sounds good.

> (Also not sure if this would be appropriate for stage 4 or if it should
> wait till next stage 1. I suppose this fixes a regression but I suspect
> this has been broken for a very long time.)

I think it's OK now since it doesn't affect the normal codepath.

> -- >8 --
> 
> The lists of scope kinds used by ENABLE_SCOPE_CHECKING don't seem to
> have been updated in a long while, causing ICEs and confusing output.
> This patch brings the list into line.
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.cc (cp_binding_level_descriptor): Add missing
> 	scope kinds.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/name-lookup.cc | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index d827d337d3b..2e93ed183f1 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -4464,11 +4464,16 @@ cp_binding_level_descriptor (cp_binding_level *scope)
>       "try-scope",
>       "catch-scope",
>       "for-scope",
> +    "cond-init-scope",
> +    "stmt-expr-scope",
>       "function-parameter-scope",
>       "class-scope",
> +    "enum-scope",
>       "namespace-scope",
>       "template-parameter-scope",
> -    "template-explicit-spec-scope"
> +    "template-explicit-spec-scope",
> +    "transaction-scope",
> +    "openmp-scope"
>     };
>     const scope_kind kind = scope->explicit_spec_p
>       ? sk_template_spec : scope->kind;
Nathaniel Shead Jan. 16, 2024, 11:59 a.m. UTC | #2 
On Mon, Jan 15, 2024 at 04:04:49PM -0500, Jason Merrill wrote:
> On 1/15/24 04:41, Nathaniel Shead wrote:
> > While working on another bug, I noticed the ENABLE_SCOPE_CHECKING macro
> > and thought to try it out. It caused selftest to ICE. This patch is a
> > minimal fix to get it working again.
> > 
> > Probably this should use a test to stop this regressing again in the
> > future the next time new scope-kinds are added, but given it's dependent
> > on a (almost certainly rarely-used) build-time macro I'm not sure
> > exactly how you would do that?
> > 
> > Or alternatively I could add a `sk_count` to the end of the scope kind
> > list and `static_assert` that the size of the descriptor list matches?
> 
> That sounds good.
> 
> > (Also not sure if this would be appropriate for stage 4 or if it should
> > wait till next stage 1. I suppose this fixes a regression but I suspect
> > this has been broken for a very long time.)
> 
> I think it's OK now since it doesn't affect the normal codepath.
> 

Thanks. Here's an updated version.

Bootstrapped on x86_64-pc-linux-gnu, OK for trunk if regtesting passes?

I've also built stage 1 with `-DENABLE_SCOPE_CHECKING` and verified it
didn't immediately fail, but regtesting fails due to all the extra
output. As an option specifically for developing GCC itself I assume
that's OK.

-- >8 --

The lists of scope kinds used by ENABLE_SCOPE_CHECKING don't seem to
have been updated in a long while, causing ICEs and confusing output.
This patch brings the list into line.

Additionally, the comment on 'explicit_spec_p' says that the flag is
only valid if kind is 'sk_template_parms', so we rewrite the condition
to be more obviously correct here.

gcc/cp/ChangeLog:

	* name-lookup.h (enum scope_kind): Add 'sk_count'.
	* name-lookup.cc (cp_binding_level_descriptor): Add missing
	scope kinds. Add assertion that the list is up to date. Fix
	handling of explicit_spec_p.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/name-lookup.cc | 15 ++++++++++++---
 gcc/cp/name-lookup.h  |  3 ++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index d827d337d3b..15b5fba6297 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -4464,14 +4464,23 @@ cp_binding_level_descriptor (cp_binding_level *scope)
     "try-scope",
     "catch-scope",
     "for-scope",
+    "cond-init-scope",
+    "stmt-expr-scope",
     "function-parameter-scope",
     "class-scope",
+    "enum-scope",
     "namespace-scope",
     "template-parameter-scope",
-    "template-explicit-spec-scope"
+    "template-explicit-spec-scope",
+    "transaction-scope",
+    "openmp-scope"
   };
-  const scope_kind kind = scope->explicit_spec_p
-    ? sk_template_spec : scope->kind;
+  static_assert (ARRAY_SIZE (scope_kind_names) == sk_count,
+		 "must keep names aligned with scope_kind enum");
+
+  scope_kind kind = scope->kind;
+  if (kind == sk_template_parms && scope->explicit_spec_p)
+    kind = sk_template_spec;
 
   return scope_kind_names[kind];
 }
diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index 4f8454ee35e..d2371323337 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -213,7 +213,8 @@ enum scope_kind {
 			explicit specialization is introduced by
 			"template <>", this scope is always empty.  */
   sk_transaction,    /* A synchronized or atomic statement.  */
-  sk_omp	     /* An OpenMP structured block.  */
+  sk_omp,	     /* An OpenMP structured block.  */
+  sk_count	     /* Number of scope_kind enumerations.  */
 };
 
 struct GTY(()) cp_class_binding {
Jason Merrill Jan. 16, 2024, 12:10 p.m. UTC | #3 
On 1/16/24 06:59, Nathaniel Shead wrote:
> On Mon, Jan 15, 2024 at 04:04:49PM -0500, Jason Merrill wrote:
>> On 1/15/24 04:41, Nathaniel Shead wrote:
>>> While working on another bug, I noticed the ENABLE_SCOPE_CHECKING macro
>>> and thought to try it out. It caused selftest to ICE. This patch is a
>>> minimal fix to get it working again.
>>>
>>> Probably this should use a test to stop this regressing again in the
>>> future the next time new scope-kinds are added, but given it's dependent
>>> on a (almost certainly rarely-used) build-time macro I'm not sure
>>> exactly how you would do that?
>>>
>>> Or alternatively I could add a `sk_count` to the end of the scope kind
>>> list and `static_assert` that the size of the descriptor list matches?
>>
>> That sounds good.
>>
>>> (Also not sure if this would be appropriate for stage 4 or if it should
>>> wait till next stage 1. I suppose this fixes a regression but I suspect
>>> this has been broken for a very long time.)
>>
>> I think it's OK now since it doesn't affect the normal codepath.
>>
> 
> Thanks. Here's an updated version.
> 
> Bootstrapped on x86_64-pc-linux-gnu, OK for trunk if regtesting passes?

OK.

> I've also built stage 1 with `-DENABLE_SCOPE_CHECKING` and verified it
> didn't immediately fail, but regtesting fails due to all the extra
> output. As an option specifically for developing GCC itself I assume
> that's OK.
> 
> -- >8 --
> 
> The lists of scope kinds used by ENABLE_SCOPE_CHECKING don't seem to
> have been updated in a long while, causing ICEs and confusing output.
> This patch brings the list into line.
> 
> Additionally, the comment on 'explicit_spec_p' says that the flag is
> only valid if kind is 'sk_template_parms', so we rewrite the condition
> to be more obviously correct here.
> 
> gcc/cp/ChangeLog:
> 
> 	* name-lookup.h (enum scope_kind): Add 'sk_count'.
> 	* name-lookup.cc (cp_binding_level_descriptor): Add missing
> 	scope kinds. Add assertion that the list is up to date. Fix
> 	handling of explicit_spec_p.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/name-lookup.cc | 15 ++++++++++++---
>   gcc/cp/name-lookup.h  |  3 ++-
>   2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index d827d337d3b..15b5fba6297 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -4464,14 +4464,23 @@ cp_binding_level_descriptor (cp_binding_level *scope)
>       "try-scope",
>       "catch-scope",
>       "for-scope",
> +    "cond-init-scope",
> +    "stmt-expr-scope",
>       "function-parameter-scope",
>       "class-scope",
> +    "enum-scope",
>       "namespace-scope",
>       "template-parameter-scope",
> -    "template-explicit-spec-scope"
> +    "template-explicit-spec-scope",
> +    "transaction-scope",
> +    "openmp-scope"
>     };
> -  const scope_kind kind = scope->explicit_spec_p
> -    ? sk_template_spec : scope->kind;
> +  static_assert (ARRAY_SIZE (scope_kind_names) == sk_count,
> +		 "must keep names aligned with scope_kind enum");
> +
> +  scope_kind kind = scope->kind;
> +  if (kind == sk_template_parms && scope->explicit_spec_p)
> +    kind = sk_template_spec;
>   
>     return scope_kind_names[kind];
>   }
> diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
> index 4f8454ee35e..d2371323337 100644
> --- a/gcc/cp/name-lookup.h
> +++ b/gcc/cp/name-lookup.h
> @@ -213,7 +213,8 @@ enum scope_kind {
>   			explicit specialization is introduced by
>   			"template <>", this scope is always empty.  */
>     sk_transaction,    /* A synchronized or atomic statement.  */
> -  sk_omp	     /* An OpenMP structured block.  */
> +  sk_omp,	     /* An OpenMP structured block.  */
> +  sk_count	     /* Number of scope_kind enumerations.  */
>   };
>   
>   struct GTY(()) cp_class_binding {
diff mbox series

Patch

diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index d827d337d3b..2e93ed183f1 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -4464,11 +4464,16 @@  cp_binding_level_descriptor (cp_binding_level *scope)
     "try-scope",
     "catch-scope",
     "for-scope",
+    "cond-init-scope",
+    "stmt-expr-scope",
     "function-parameter-scope",
     "class-scope",
+    "enum-scope",
     "namespace-scope",
     "template-parameter-scope",
-    "template-explicit-spec-scope"
+    "template-explicit-spec-scope",
+    "transaction-scope",
+    "openmp-scope"
   };
   const scope_kind kind = scope->explicit_spec_p
     ? sk_template_spec : scope->kind;