Patchwork Do not allow non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs

login
register
mail settings
Submitter Martin Jambor
Date May 22, 2013, 3:11 p.m.
Message ID <20130522151105.GF23266@virgil.suse>
Download mbox | patch
Permalink /patch/245643/
State New
Headers show

Comments

Martin Jambor - May 22, 2013, 3:11 p.m.
Hi,

earlier this week I asked on IRC whether we could have non-top-level
BIT_FIELD_REFs and Richi said that we could.  However, when I later
looked at SRA code, quite apparently it is not designed to handle
non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs.  So in
order to test whether that assumption is OK, I added the following
into the gimple verifier and ran bootstrap and testsuite of all
languages including Ada and ObjC++ on x86_64.  It survived, which
makes me wondering whether we do not want it in trunk.

What do you think?

Martin


2013-05-22  Martin Jambor  <mjambor@suse.cz>

	* tree-cfg.c (verify_types_in_gimple_reference): Do not allow
	non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs.
Eric Botcazou - May 23, 2013, 9:20 a.m.
> earlier this week I asked on IRC whether we could have non-top-level
> BIT_FIELD_REFs and Richi said that we could.  However, when I later
> looked at SRA code, quite apparently it is not designed to handle
> non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs.  So in
> order to test whether that assumption is OK, I added the following
> into the gimple verifier and ran bootstrap and testsuite of all
> languages including Ada and ObjC++ on x86_64.  It survived, which
> makes me wondering whether we do not want it in trunk.

This looks plausible to me, but I think that you ought to verify the real 
assumption instead, which is that the type of the 3 nodes is always scalar.
The non-toplevelness of the nodes is merely a consequence of this property.
Richard Guenther - May 23, 2013, 9:38 a.m.
On Thu, 23 May 2013, Eric Botcazou wrote:

> > earlier this week I asked on IRC whether we could have non-top-level
> > BIT_FIELD_REFs and Richi said that we could.  However, when I later
> > looked at SRA code, quite apparently it is not designed to handle
> > non-top-level BIT_FIELD_REFs, IMAGPART_EXPRs or REALPART_EXPRs.  So in
> > order to test whether that assumption is OK, I added the following
> > into the gimple verifier and ran bootstrap and testsuite of all
> > languages including Ada and ObjC++ on x86_64.  It survived, which
> > makes me wondering whether we do not want it in trunk.
> 
> This looks plausible to me, but I think that you ought to verify the real 
> assumption instead, which is that the type of the 3 nodes is always scalar.
> The non-toplevelness of the nodes is merely a consequence of this property.

Yeah.  But please put the verification into tree-cfg.c:verify_expr
instead.

Thanks,
Richard.

Patch

Index: src/gcc/tree-cfg.c
===================================================================
--- src.orig/gcc/tree-cfg.c
+++ src/gcc/tree-cfg.c
@@ -2858,6 +2858,8 @@  verify_types_in_gimple_min_lval (tree ex
 static bool
 verify_types_in_gimple_reference (tree expr, bool require_lvalue)
 {
+  bool top_level = true;
+
   while (handled_component_p (expr))
     {
       tree op = TREE_OPERAND (expr, 0);
@@ -2944,6 +2946,17 @@  verify_types_in_gimple_reference (tree e
 	    return false;
 	}
 
+      if (!top_level
+	  && (TREE_CODE (expr) == BIT_FIELD_REF
+	      || TREE_CODE (expr) == IMAGPART_EXPR
+	      || TREE_CODE (expr) == REALPART_EXPR))
+	{
+	  error ("non-top-level BIT_FIELD_REF, IMAGPART_EXPR or REALPART_EXPR");
+	  debug_generic_stmt (expr);
+	  return true;
+	}
+
+      top_level = false;
       expr = op;
     }