public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] c++, v3: Diagnose [basic.scope.block]/2 violations even in compound-stmt of function-try-block [PR52953]
Date: Fri, 1 Sep 2023 16:34:28 +0200	[thread overview]
Message-ID: <ZPH2dPVbDTDFSBpr@tucnak> (raw)
In-Reply-To: <ZPHmJl9P/CX5JViM@tucnak>

On Fri, Sep 01, 2023 at 03:24:54PM +0200, Jakub Jelinek via Gcc-patches wrote:
> So like this?
> 
> It actually changes behaviour on the
> void foo (int x) try {} catch (int x) {} case, where previously
> this triggered the
>                || (TREE_CODE (old) == PARM_DECL
>                    && (current_binding_level->kind == sk_catch
>                        || current_binding_level->level_chain->kind == sk_catch)
>                    && in_function_try_handler))
>         {
>           auto_diagnostic_group d;
>           if (permerror (DECL_SOURCE_LOCATION (decl),
>                          "redeclaration of %q#D", decl))
>             inform (DECL_SOURCE_LOCATION (old),
>                     "%q#D previously declared here", old);
> diagnostics (note, just the current_binding_level->kind == sk_catch
> case), while now it triggers already the earlier
>           if (b->kind == sk_function_parms)
>             {
>               error_at (DECL_SOURCE_LOCATION (decl),
>                         "declaration of %q#D shadows a parameter", decl);
>               inform (DECL_SOURCE_LOCATION (old),
>                       "%q#D previously declared here", old);
> error.  If you think it is important to differentiate that,
> I guess I could guard the while (b->artificial) loop with say
> +	  if (!in_function_try_handler
> +	      || current_binding_level->kind != sk_catch)
> 	    while (b->artificial)
> 	      b = b->level_chain;
> and adjust the 2 testcases.

BTW, that in_function_try_handler case doesn't work correctly
(before/after this patch),
void
foo (int x)
try {
}
catch (int)
{
  try {
  } catch (int x)
  {
  }
  try {
  } catch (int)
  {
    int x;
  }
}
(which is valid) is rejected, because
                || (TREE_CODE (old) == PARM_DECL
                    && (current_binding_level->kind == sk_catch
                        || current_binding_level->level_chain->kind == sk_catch)
                    && in_function_try_handler))
is true but nothing verified that for the first case
current_binding_level->level_chain->kind == sk_function_params
(with perhaps artificial scopes in between and in the latter case
with one extra level in between).

Here is an untested variant of the patch which does diagnostics of the
in_function_try_handler cases only if it is proven there are no intervening
non-artificial scopes, but uses the old wording + permerror for that case
like before.

Another possibility would be to use permerror for that case but the
"declaration of %q#D shadows a parameter" wording (then we'd need to adjust
both testcases, each on one line).

2023-09-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/52953
	* name-lookup.h (struct cp_binding_level): Add artificial bit-field.
	Formatting fixes.
	* name-lookup.cc (check_local_shadow): Skip artificial bindings when
	checking if parameter scope is parent scope.  Don't special case
	FUNCTION_NEEDS_BODY_BLOCK.  Diagnose the in_function_try_handler
	cases in the b->kind == sk_function_parms test, verify no
	non-artificial intervening scopes but use permerror for that case with
	different wording.  Add missing auto_diagnostic_group.
	* decl.cc (begin_function_body): Set
	current_binding_level->artificial.
	* semantics.cc (begin_function_try_block): Likewise.

	* g++.dg/diagnostic/redeclaration-3.C: New test.

--- gcc/cp/name-lookup.h.jj	2023-09-01 12:15:22.574619674 +0200
+++ gcc/cp/name-lookup.h	2023-09-01 16:11:47.838401045 +0200
@@ -292,11 +292,11 @@ struct GTY(()) cp_binding_level {
       only valid if KIND == SK_TEMPLATE_PARMS.  */
   BOOL_BITFIELD explicit_spec_p : 1;
 
-  /* true means make a BLOCK for this level regardless of all else.  */
+  /* True means make a BLOCK for this level regardless of all else.  */
   unsigned keep : 1;
 
   /* Nonzero if this level can safely have additional
-      cleanup-needing variables added to it.  */
+     cleanup-needing variables added to it.  */
   unsigned more_cleanups_ok : 1;
   unsigned have_cleanups : 1;
 
@@ -308,9 +308,13 @@ struct GTY(()) cp_binding_level {
   unsigned defining_class_p : 1;
 
   /* True for SK_FUNCTION_PARMS of a requires-expression.  */
-  unsigned requires_expression: 1;
+  unsigned requires_expression : 1;
 
-  /* 22 bits left to fill a 32-bit word.  */
+  /* True for artificial blocks which should be ignored when finding
+     parent scope.  */
+  unsigned artificial : 1;
+
+  /* 21 bits left to fill a 32-bit word.  */
 };
 
 /* The binding level currently in effect.  */
--- gcc/cp/name-lookup.cc.jj	2023-09-01 12:15:22.566619785 +0200
+++ gcc/cp/name-lookup.cc	2023-09-01 16:19:12.567335710 +0200
@@ -3146,18 +3146,34 @@ check_local_shadow (tree decl)
 	     them there.  */
 	  cp_binding_level *b = current_binding_level->level_chain;
 
-	  if (FUNCTION_NEEDS_BODY_BLOCK (current_function_decl))
-	    /* Skip the ctor/dtor cleanup level.  */
+	  if (in_function_try_handler && b->kind == sk_catch)
+	    b = b->level_chain;
+
+	  /* Skip artificially added scopes which aren't present
+	     in the C++ standard, e.g. for function-try-block or
+	     ctor/dtor cleanups.  */
+	  while (b->artificial)
 	    b = b->level_chain;
 
 	  /* [basic.scope.param] A parameter name shall not be redeclared
 	     in the outermost block of the function definition.  */
 	  if (b->kind == sk_function_parms)
 	    {
-	      error_at (DECL_SOURCE_LOCATION (decl),
-			"declaration of %q#D shadows a parameter", decl);
-	      inform (DECL_SOURCE_LOCATION (old),
-		      "%q#D previously declared here", old);
+	      auto_diagnostic_group d;
+	      bool emit = true;
+	      /* The [basic.scope.block]/(2.3) - handle of a function-try-block
+		 used to emit a different diagnostics, preserve that.  */
+	      if (in_function_try_handler
+		  && (current_binding_level->kind == sk_catch
+		      || current_binding_level->level_chain->kind == sk_catch))
+		emit = permerror (DECL_SOURCE_LOCATION (decl),
+				  "redeclaration of %q#D", decl);
+	      else
+		error_at (DECL_SOURCE_LOCATION (decl),
+			  "declaration of %q#D shadows a parameter", decl);
+	      if (emit)
+		inform (DECL_SOURCE_LOCATION (old),
+			"%q#D previously declared here", old);
 	      return;
 	    }
 	}
@@ -3194,17 +3210,10 @@ check_local_shadow (tree decl)
 	 shall not be redeclared in the outermost block of the handler.
 	 3.3.3/2:  A parameter name shall not be redeclared (...) in
 	 the outermost block of any handler associated with a
-	 function-try-block.
-	 3.4.1/15: The function parameter names shall not be redeclared
-	 in the exception-declaration nor in the outermost block of a
-	 handler for the function-try-block.  */
-      else if ((TREE_CODE (old) == VAR_DECL
-		&& old_scope == current_binding_level->level_chain
-		&& old_scope->kind == sk_catch)
-	       || (TREE_CODE (old) == PARM_DECL
-		   && (current_binding_level->kind == sk_catch
-		       || current_binding_level->level_chain->kind == sk_catch)
-		   && in_function_try_handler))
+	 function-try-block.  */
+      else if (TREE_CODE (old) == VAR_DECL
+	       && old_scope == current_binding_level->level_chain
+	       && old_scope->kind == sk_catch)
 	{
 	  auto_diagnostic_group d;
 	  if (permerror (DECL_SOURCE_LOCATION (decl),
--- gcc/cp/semantics.cc.jj	2023-09-01 12:15:22.650618611 +0200
+++ gcc/cp/semantics.cc	2023-09-01 16:11:47.844400963 +0200
@@ -1624,6 +1624,7 @@ begin_function_try_block (tree *compound
   /* This outer scope does not exist in the C++ standard, but we need
      a place to put __FUNCTION__ and similar variables.  */
   *compound_stmt = begin_compound_stmt (0);
+  current_binding_level->artificial = 1;
   r = begin_try_block ();
   FN_TRY_BLOCK_P (r) = 1;
   return r;
--- gcc/cp/decl.cc.jj	2023-09-01 15:07:40.937661100 +0200
+++ gcc/cp/decl.cc	2023-09-01 16:11:47.842400991 +0200
@@ -18002,6 +18002,7 @@ begin_function_body (void)
     keep_next_level (true);
 
   tree stmt = begin_compound_stmt (BCS_FN_BODY);
+  current_binding_level->artificial = 1;
 
   if (processing_template_decl)
     /* Do nothing now.  */;
--- gcc/testsuite/g++.dg/diagnostic/redeclaration-3.C.jj	2023-09-01 16:11:47.844400963 +0200
+++ gcc/testsuite/g++.dg/diagnostic/redeclaration-3.C	2023-09-01 16:24:21.865117426 +0200
@@ -0,0 +1,225 @@
+// PR c++/52953
+// { dg-do compile }
+// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
+
+void
+foo (int x)				// { dg-message "'int x' previously declared here" }
+{
+  int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+}
+
+void
+bar (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+  int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+}
+catch (...)
+{
+}
+
+volatile int v;
+
+void
+baz ()
+{
+#if __cplusplus >= 201103L
+  auto f = [] (int x) { int x; };	// { dg-error "declaration of 'int x' shadows a parameter" "" { target c++11 } }
+					// { dg-message "'int x' previously declared here" "" { target c++11 } .-1 }
+#endif
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  if (int x = 1)			// { dg-message "'int x' previously declared here" }
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  if (int x = 0)			// { dg-message "'int x' previously declared here" }
+    ;
+  else
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    default:;
+    }
+  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  while (int x = v)			// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  while (int x = v)			// { dg-message "'int x' previously declared here" }
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  for (; int x = v; )			// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  for (; int x = v; )			// { dg-message "'int x' previously declared here" }
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  try
+    {
+    }
+  catch (int x)				// { dg-message "'int x' previously declared here" }
+    {
+      int x;				// { dg-error "redeclaration of 'int x'" }
+    }
+  if (int x = 1)
+    if (int x = 1)
+      ;
+  if (int x = 0)
+    ;
+  else
+    if (int x = 1)
+      ;
+  if (int x = 1)
+    switch (int x = 1)
+      ;
+  if (int x = 0)
+    while (int x = v)
+      ;
+  if (int x = 0)
+    for (int x = v; x; ++x)
+      ;
+  switch (int x = 1)
+    switch (int x = 1)
+      {
+      case 1:;
+      }
+  while (int x = 0)
+    if (int x = 1)
+      ;
+  for (int x = v; x; ++x)
+    for (int x = v; x; ++x)
+      ;
+}
+
+void
+qux (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (int x)				// { dg-error "redeclaration of 'int x'" }
+{
+}
+
+void
+corge (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (...)
+{
+  int x;				// { dg-error "redeclaration of 'int x'" }
+}
+
+void
+fred (int x)				// { dg-message "'int x' previously declared here" }
+try
+{
+}
+catch (int)
+{
+}
+catch (long)
+{
+  int x;				// { dg-error "redeclaration of 'int x'" }
+}
+
+void
+garply (int x)
+{
+  try
+    {
+      int x;
+    }
+  catch (...)
+    {
+      int x;
+    }
+}
+
+struct S
+{
+  S (int x)				// { dg-message "'int x' previously declared here" }
+  try : s (x)
+  {
+    int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
+  }
+  catch (...)
+  {
+  }
+  int s;
+};
+
+struct T
+{
+  T (int x)				// { dg-message "'int x' previously declared here" }
+  try : t (x)
+  {
+  }
+  catch (...)
+  {
+    int x;				// { dg-error "redeclaration of 'int x'" }
+  }
+  int t;
+};
+
+struct U
+{
+  U (int x) : u (x)
+  {
+    try
+    {
+      int x;
+    }
+    catch (...)
+    {
+      int x;
+    }
+  }
+  int u;
+};
+
+struct V
+{
+  V (int x) : v (x)
+  {
+    {
+      int x;
+    }
+  }
+  int v;
+};
+
+void
+foobar (int x)
+try
+{
+}
+catch (int)
+{
+  try
+  {
+  }
+  catch (int x)
+  {
+  }
+  try
+  {
+  } catch (int)
+  {
+    int x;
+  }
+}


	Jakub


  reply	other threads:[~2023-09-01 14:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-31  7:20 [PATCH] c++: " Jakub Jelinek
2023-08-31 19:52 ` Jason Merrill
2023-09-01 13:24   ` [PATCH] c++, v2: " Jakub Jelinek
2023-09-01 14:34     ` Jakub Jelinek [this message]
2023-09-05 14:15     ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZPH2dPVbDTDFSBpr@tucnak \
    --to=jakub@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).