Patchwork [DOC] Clarify docs about stmt exprs (PR c/51088)

login
register
mail settings
Submitter Marek Polacek
Date April 8, 2014, 4:23 p.m.
Message ID <20140408162322.GA11972@redhat.com>
Download mbox | patch
Permalink /patch/337696/
State New
Headers show

Comments

Marek Polacek - April 8, 2014, 4:23 p.m.
On Fri, Mar 28, 2014 at 02:44:21PM +0000, Joseph S. Myers wrote:
> On Fri, 28 Mar 2014, Marek Polacek wrote:
> 
> > PR51088 contains some Really Bizzare code.  We should tell users
> > not to do any shenanigans like that.
> > 
> > Ok for trunk?
> 
> I don't think a doc patch resolves this bug.  The compiler should never 
> generate code with an undefined reference to a local label like that; 
> either the code should get a compile-time error (that's what I suggest), 
> or it should generate output that links but has undefined behavior at 
> runtime.

Ok, with this patch the compiler should issue an error if someone's
trying to take an address of a label defined in a statement expression
outside of that statement expression.
I admit this was very tricky; I had to completely revamp the patch
several times, this one is the least disrupting and simplest one
I could come up with.  It works by marking labels that are declared
outside of stmt expr while we're entering a stmt expr (but we mustn't
do this for nested stmt exprs).  If we're then defining the label in
stmt expr and it was referenced outside of this stmt expr, raise an error.
This patch doesn't catch cases like ({ A:0; }); &&A;, in that case the
behavior is just undefined.
Does this approach make sense?

Regtested/bootstrapped on x86_64-linux.  I don't think it's stage4 material,
so ok for next stage1?

2014-04-08  Marek Polacek  <polacek@redhat.com>

	PR c/51088
	* c-decl.c (stmt_expr_depth): New variable.
	(struct c_label_vars): Add seen_outside_stmt_expr variable.
	(c_bindings_start_stmt_expr): Bump stmt_expr_depth.  Mark labels
	declared outside of statement expressions.
	(c_bindings_end_stmt_expr): Decrement stmt_expr_depth.
	(make_label): Set seen_outside_stmt_expr.
	(check_earlier_gotos): Return true if error was issued.
	(define_label): Give error if taking an address of a label defined
	in statement expression outside of the statement expression.

	* doc/extend.texi (Statement Exprs): Add note about taking
	addresses of labels inside of statement expressions.

	* gcc.c-torture/compile/pr17913.c (f): Add dg-error.
	* gcc.dg/pr51088.c: New test.


	Marek
Joseph S. Myers - May 1, 2014, 5:21 p.m.
On Tue, 8 Apr 2014, Marek Polacek wrote:

> This patch doesn't catch cases like ({ A:0; }); &&A;, in that case the
> behavior is just undefined.

Those cases can produce link failures just like the cases where the 
address is taken before the statement expression, so I think they need 
errors as well.

int
main ()
{
  1 || ({ A: 0; });
  void *L[] = { &&A };
}
Jeff Law - July 17, 2014, 2:21 a.m.
On 04/08/14 10:23, Marek Polacek wrote:
> On Fri, Mar 28, 2014 at 02:44:21PM +0000, Joseph S. Myers wrote:
>> On Fri, 28 Mar 2014, Marek Polacek wrote:
>>
>>> PR51088 contains some Really Bizzare code.  We should tell users
>>> not to do any shenanigans like that.
>>>
>>> Ok for trunk?
>>
>> I don't think a doc patch resolves this bug.  The compiler should never
>> generate code with an undefined reference to a local label like that;
>> either the code should get a compile-time error (that's what I suggest),
>> or it should generate output that links but has undefined behavior at
>> runtime.
>
> Ok, with this patch the compiler should issue an error if someone's
> trying to take an address of a label defined in a statement expression
> outside of that statement expression.
> I admit this was very tricky; I had to completely revamp the patch
> several times, this one is the least disrupting and simplest one
> I could come up with.  It works by marking labels that are declared
> outside of stmt expr while we're entering a stmt expr (but we mustn't
> do this for nested stmt exprs).  If we're then defining the label in
> stmt expr and it was referenced outside of this stmt expr, raise an error.
> This patch doesn't catch cases like ({ A:0; }); &&A;, in that case the
> behavior is just undefined.
> Does this approach make sense?
>
> Regtested/bootstrapped on x86_64-linux.  I don't think it's stage4 material,
> so ok for next stage1?
>
> 2014-04-08  Marek Polacek  <polacek@redhat.com>
>
> 	PR c/51088
> 	* c-decl.c (stmt_expr_depth): New variable.
> 	(struct c_label_vars): Add seen_outside_stmt_expr variable.
> 	(c_bindings_start_stmt_expr): Bump stmt_expr_depth.  Mark labels
> 	declared outside of statement expressions.
> 	(c_bindings_end_stmt_expr): Decrement stmt_expr_depth.
> 	(make_label): Set seen_outside_stmt_expr.
> 	(check_earlier_gotos): Return true if error was issued.
> 	(define_label): Give error if taking an address of a label defined
> 	in statement expression outside of the statement expression.
>
> 	* doc/extend.texi (Statement Exprs): Add note about taking
> 	addresses of labels inside of statement expressions.
>
> 	* gcc.c-torture/compile/pr17913.c (f): Add dg-error.
> 	* gcc.dg/pr51088.c: New test.
So this seems OK if you just want to warn when we take the address of 
the label at function scope, but don't you want to warn when the depth 
of the address taken operation is different (lower) then the depth of 
the when the label is defined?

Am I missing something here?

jeff

Patch

diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index df84980..15663ae 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -157,6 +157,9 @@  enum machine_mode c_default_pointer_mode = VOIDmode;
 /* If non-zero, implicit "omp declare target" attribute is added into the
    attribute lists.  */
 int current_omp_declare_target_attribute;
+
+/* Remember how many statement expressions we've entered.  */
+static int stmt_expr_depth;
 
 /* Each c_binding structure describes one binding of an identifier to
    a decl.  All the decls in a scope - irrespective of namespace - are
@@ -313,6 +316,8 @@  struct GTY(()) c_label_vars {
      goto statements seen before the label was defined, so that we can
      issue appropriate warnings for them.  */
   vec<c_goto_bindings_p, va_gc> *gotos;
+  /* Whether this label is referenced outside of statement expression.  */
+  bool seen_outside_stmt_expr;
 };
 
 /* Each c_scope structure describes the complete contents of one
@@ -1360,6 +1365,16 @@  c_bindings_start_stmt_expr (struct c_spot_bindings* switch_bindings)
 {
   struct c_scope *scope;
 
+  /* We entered a statement expression.  */
+  stmt_expr_depth++;
+
+  if (stmt_expr_depth == 1)
+    /* Remember which labels are declared outside of statement exprs.  */
+    for (struct c_binding *b = current_function_scope->bindings;
+	 b != NULL; b = b->prev)
+      if (TREE_CODE (b->decl) == LABEL_DECL)
+	b->u.label->seen_outside_stmt_expr = true;
+
   for (scope = current_scope; scope != NULL; scope = scope->outer)
     {
       struct c_binding *b;
@@ -1393,6 +1408,9 @@  c_bindings_end_stmt_expr (struct c_spot_bindings *switch_bindings)
 {
   struct c_scope *scope;
 
+  /* We're leaving a statement expression.  */
+  stmt_expr_depth--;
+
   for (scope = current_scope; scope != NULL; scope = scope->outer)
     {
       struct c_binding *b;
@@ -3074,6 +3092,7 @@  make_label (location_t location, tree name, bool defining,
   set_spot_bindings (&label_vars->label_bindings, defining);
   label_vars->decls_in_scope = make_tree_vector ();
   label_vars->gotos = NULL;
+  label_vars->seen_outside_stmt_expr = false;
   *p_label_vars = label_vars;
 
   return label;
@@ -3228,11 +3247,12 @@  declare_label (tree name)
 /* When we define a label, issue any appropriate warnings if there are
    any gotos earlier in the function which jump to this label.  */
 
-static void
+static bool
 check_earlier_gotos (tree label, struct c_label_vars* label_vars)
 {
   unsigned int ix;
   struct c_goto_bindings *g;
+  bool error_p = false;
 
   FOR_EACH_VEC_SAFE_ELT (label_vars->gotos, ix, g)
     {
@@ -3276,6 +3296,7 @@  check_earlier_gotos (tree label, struct c_label_vars* label_vars)
 
       if (g->goto_bindings.stmt_exprs > 0)
 	{
+	  error_p = true;
 	  error_at (g->loc, "jump into statement expression");
 	  inform (DECL_SOURCE_LOCATION (label), "label %qD defined here",
 		  label);
@@ -3286,6 +3307,7 @@  check_earlier_gotos (tree label, struct c_label_vars* label_vars)
      subsequent gotos to this label when we see them.  */
   vec_safe_truncate (label_vars->gotos, 0);
   label_vars->gotos = NULL;
+  return error_p;
 }
 
 /* Define a label, specifying the location in the source file.
@@ -3323,7 +3345,20 @@  define_label (location_t location, tree name)
 
       /* Issue warnings as required about any goto statements from
 	 earlier in the function.  */
-      check_earlier_gotos (label, label_vars);
+      bool gave_error = check_earlier_gotos (label, label_vars);
+
+      /* If we're defining a label in a statement expression,
+	 check that the declaration wasn't outside of this statement
+	 expression.  */
+      if (STATEMENT_LIST_STMT_EXPR (cur_stmt_list)
+	  && label_vars->seen_outside_stmt_expr
+	  && ! gave_error)
+	{
+	  error_at (location, "taking address of label %qD defined in "
+		    "statement expression", label);
+	  DECL_INITIAL (label) = error_mark_node;
+	  return 0;
+	}
     }
   else
     {
diff --git gcc/doc/extend.texi gcc/doc/extend.texi
index 347a94a..2309840 100644
--- gcc/doc/extend.texi
+++ gcc/doc/extend.texi
@@ -206,6 +206,8 @@  Jumping into a statement expression with @code{goto} or using a
 @code{case} or @code{default} label inside the statement expression is
 not permitted.  Jumping into a statement expression with a computed
 @code{goto} (@pxref{Labels as Values}) has undefined behavior.
+Taking the address of a label defined inside of a statement
+expression from outside of the statement expression is invalid.
 Jumping out of a statement expression is permitted, but if the
 statement expression is part of a larger expression then it is
 unspecified which other subexpressions of that expression have been
diff --git gcc/testsuite/gcc.c-torture/compile/pr17913.c gcc/testsuite/gcc.c-torture/compile/pr17913.c
index 30654a3..5e88fbe 100644
--- gcc/testsuite/gcc.c-torture/compile/pr17913.c
+++ gcc/testsuite/gcc.c-torture/compile/pr17913.c
@@ -2,6 +2,6 @@ 
 void f (void) 
 { 
   void *p = &&a;
-  1 ? 1 : ({ a : 1; }); 
+  1 ? 1 : ({ a : 1; }); /* { dg-error "taking address of label" } */
   goto *p;
 }
diff --git gcc/testsuite/gcc.dg/pr51088.c gcc/testsuite/gcc.dg/pr51088.c
index e69de29..6a5619e 100644
--- gcc/testsuite/gcc.dg/pr51088.c
+++ gcc/testsuite/gcc.dg/pr51088.c
@@ -0,0 +1,104 @@ 
+/* PR c/51088 */
+/* { dg-do compile } */
+/* { dg-options "-std=c99" } */
+
+void
+fn1 (void)
+{
+  void *L = &&A;
+  ({ A:0; }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn2 (void)
+{
+  void *L = &&A;
+  ({ void *p = &&A; A:0; }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn3 (void)
+{
+  ({ void *p = &&A; A:0; });
+}
+
+void
+fn4 (void)
+{
+  {
+    {
+      {
+	void *L = &&A;
+	({ A:0; }); /* { dg-error "taking address of label" } */
+      }
+    }
+  }
+}
+
+void
+fn5 (void)
+{
+  void *L = &&A;
+  ({ ({ ({ ({ A:0; }); }); }); }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn6 (void)
+{
+  void *L = &&A;
+  {
+    A:;
+  }
+}
+
+void
+fn7 (void)
+{
+  void *L = &&A;
+  {
+    ({ A:0; }); /* { dg-error "taking address of label" } */
+  }
+}
+
+void
+fn8 (void)
+{
+  ({ void *p = &&A; ({ A:0; }); });
+}
+
+void
+fn9 (void)
+{
+  &&A;
+  ({ ({ A:0; }); }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn10 (void)
+{
+  &&A;
+  ({ ({ 0; }); });
+  ({ ({ A:0; }); }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn11 (void)
+{
+  goto A;
+  ({ ({ &&A; 0; }); });
+  A:;
+}
+
+void
+fn12 (void)
+{
+  ({ &&A; });
+  ({ A:0; }); /* { dg-error "taking address of label" } */
+}
+
+void
+fn13 (void)
+{
+  ({ ({ ({ &&A; }); }); });
+  ({ ({ ({ A:0; }); }); }); /* { dg-error "taking address of label" } */
+}