[PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Andrew Pinski]

The problem here is that we were getting rid of some bb as they were 
being marked as unreachable (which is true in the code flow sense) but 
should not
be removed.  I fixed this by adding an edge from the first bb to the 
label's
bb which has its address taken.  I think this is most correct solution 
as
this does not happen much we should not have to worry about losing some
optimization possibility from doing it this way.

OK? Bootstrapped and tested on powerpc-apple-darwin.

Thanks,
Andrew Pinski


Testcase:
void
f (void)
{
   static __SIZE_TYPE__ u = &&a-&&b;
   a : b :  return;
}


ChangeLog:
	* tre-cfg.c (make_edges): After making the edges but before
	cleaning up the cfg, make an edge from the first BB to ever
	BB which we take the address of the label.

Patch:
Index: tree-cfg.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-cfg.c,v
retrieving revision 2.64
diff -u -p -r2.64 tree-cfg.c
--- tree-cfg.c	2 Oct 2004 12:47:10 -0000	2.64
+++ tree-cfg.c	4 Oct 2004 14:44:19 -0000
@@ -417,6 +417,7 @@ static void
  make_edges (void)
  {
    basic_block bb;
+  basic_block target_bb;

    /* Create an edge from entry to the first block with executable
       statements in it.  */
@@ -448,6 +449,30 @@ make_edges (void)
    /* We do not care about fake edges, so remove any that the CFG
       builder inserted for completeness.  */
    remove_fake_exit_edges ();
+
+  /* Look for the block starting with the destination label.  In the
+     case of a computed label, make an edge from the entry block to the
+     computed lablel.  */
+  FOR_EACH_BB (target_bb)
+    {
+      block_stmt_iterator bsi;
+
+      for (bsi = bsi_start (target_bb); !bsi_end_p (bsi); bsi_next 
(&bsi))
+	{
+	  tree target = bsi_stmt (bsi);
+
+	  if (TREE_CODE (target) != LABEL_EXPR)
+	    break;
+
+	  if (FORCED_LABEL (LABEL_EXPR_LABEL (target))
+	      || DECL_NONLOCAL (LABEL_EXPR_LABEL (target)))
+	    {
+	      make_edge (EDGE_SUCC (ENTRY_BLOCK_PTR, 0)->dest,
+	                 target_bb, EDGE_ABNORMAL);
+	      break;
+	    }
+	}
+    }

    /* Clean up the graph and warn for unreachable code.  */
    cleanup_tree_cfg ();

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Jeffrey A Law]

On Mon, 2004-10-04 at 08:46, Andrew Pinski wrote:
> The problem here is that we were getting rid of some bb as they were 
> being marked as unreachable (which is true in the code flow sense) but 
> should not
> be removed.  I fixed this by adding an edge from the first bb to the 
> label's
> bb which has its address taken.  I think this is most correct solution 
> as
> this does not happen much we should not have to worry about losing some
> optimization possibility from doing it this way.
> 
> OK? Bootstrapped and tested on powerpc-apple-darwin.
> 
> Thanks,
> Andrew Pinski
> 
> 
> Testcase:
> void
> f (void)
> {
>    static __SIZE_TYPE__ u = &&a-&&b;
>    a : b :  return;
> }
> 
> 
> ChangeLog:
> 	* tre-cfg.c (make_edges): After making the edges but before
> 	cleaning up the cfg, make an edge from the first BB to ever
> 	BB which we take the address of the label.
Didn't we decide to disallow taking the addresses of user labels
for use by anything but control flow statements?  That would
make this code ill-formed.

If we're going to allow this, then I would strongly recommend
against this approach to the problem.  Instead just move
these labels to the start of their nearest following block that
is reachable.

Inserting arbitrary edges into the CFG is generally bad.

Jeff

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Joseph S. Myers]

On Tue, 5 Oct 2004, Jeffrey A Law wrote:

> Didn't we decide to disallow taking the addresses of user labels
> for use by anything but control flow statements?  That would
> make this code ill-formed.

The stated use of differences of local labels 
<http://gcc.gnu.org/ml/gcc-patches/2003-02/msg01143.html> (and thread) is 
for static tables of such (e.g. glibc/stdio-common/vfprintf.c) - which 
rather suggests cases such as this are valid even though we do not care 
what value the difference has since there are no computed gotos present 
that might use it.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    joseph@codesourcery.com (CodeSourcery mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Andrew Pinski]

On Oct 5, 2004, at 6:35 PM, Jeffrey A Law wrote:
> If we're going to allow this, then I would strongly recommend
> against this approach to the problem.  Instead just move
> these labels to the start of their nearest following block that
> is reachable.
Well then you have to fix cfgcleanup which is done using non-tree
or non-RTL code.  This is much harder than it should be for the
problem at hand which is why I picked the method I did.

> Inserting arbitrary edges into the CFG is generally bad.

Usually it is but this case does not show up too much so really
it should not matter that much as I said before.

-- Pinski

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Jeffrey A Law]

On Tue, 2004-10-05 at 20:13, Andrew Pinski wrote:
> On Oct 5, 2004, at 6:35 PM, Jeffrey A Law wrote:
> > If we're going to allow this, then I would strongly recommend
> > against this approach to the problem.  Instead just move
> > these labels to the start of their nearest following block that
> > is reachable.
> Well then you have to fix cfgcleanup which is done using non-tree
> or non-RTL code.  This is much harder than it should be for the
> problem at hand which is why I picked the method I did.
Harder or not, it's a much better solution.



> 
> > Inserting arbitrary edges into the CFG is generally bad.
> 
> Usually it is but this case does not show up too much so really
> it should not matter that much as I said before.
We've already been down this road before.  Adding bogus edges to the
CFG is bad bad bad.  

Jeff

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]


On Oct 5, 2004, at 6:35 PM, Jeffrey A Law wrote:
>> ChangeLog:
>> 	* tre-cfg.c (make_edges): After making the edges but before
>> 	cleaning up the cfg, make an edge from the first BB to ever
>> 	BB which we take the address of the label.
>
> If we're going to allow this, then I would strongly recommend
> against this approach to the problem.  Instead just move
> these labels to the start of their nearest following block that
> is reachable.

Actually this was easier than I thought.
And here is the patch which fixes this problem, the way you recommended.
I had forgot that remove_bb was actually doing the removal of 
basic_block.

OK? Bootstrapped and tested on PPC-darwin.

Thanks,
Andrew Pinski

ChangeLog:

	* tree-cfg.c (remove_bb): If we have a label expression in the
	basic block and the label we have taken the address, move the
	label expression to the basic block which is previous in the
	linked list.
	(tree_verify_flow_info): Fix printing out the label name of the
	problematic label expression.

Patch:


[-- Attachment #2: temp.diff.txt --]
[-- Type: text/plain, Size: 1940 bytes --]

Index: tree-cfg.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-cfg.c,v
retrieving revision 2.73
diff -u -p -r2.73 tree-cfg.c
--- tree-cfg.c	8 Oct 2004 17:11:16 -0000	2.73
+++ tree-cfg.c	10 Oct 2004 17:48:13 -0000
@@ -1810,12 +1810,25 @@ remove_bb (basic_block bb)
     }
 
   /* Remove all the instructions in the block.  */
-  for (i = bsi_start (bb); !bsi_end_p (i); bsi_remove (&i))
+  for (i = bsi_start (bb); !bsi_end_p (i);)
     {
       tree stmt = bsi_stmt (i);
-      release_defs (stmt);
+      if (TREE_CODE (stmt) == LABEL_EXPR
+          && FORCED_LABEL (LABEL_EXPR_LABEL (stmt)))
+	{
+	  basic_block new_bb = bb->prev_bb;
+	  block_stmt_iterator new_bsi = bsi_after_labels (new_bb);
+	  	  
+	  bsi_remove (&i);
+	  bsi_insert_after (&new_bsi, stmt, BSI_NEW_STMT);
+	}
+      else
+        {
+	  release_defs (stmt);
 
-      set_bb_for_stmt (stmt, NULL);
+	  set_bb_for_stmt (stmt, NULL);
+	  bsi_remove (&i);
+	}
 
       /* Don't warn for removed gotos.  Gotos are often removed due to
 	 jump threading, thus resulting in bogus warnings.  Not great,
@@ -3404,8 +3417,9 @@ tree_verify_flow_info (void)
 
 	  if (label_to_block (LABEL_EXPR_LABEL (bsi_stmt (bsi))) != bb)
 	    {
+	      tree stmt = bsi_stmt (bsi);
 	      error ("Label %s to block does not match in bb %d\n",
-		     IDENTIFIER_POINTER (DECL_NAME (bsi_stmt (bsi))),
+		     IDENTIFIER_POINTER (DECL_NAME (LABEL_EXPR_LABEL (stmt))),
 		     bb->index);
 	      err = 1;
 	    }
@@ -3413,8 +3427,9 @@ tree_verify_flow_info (void)
 	  if (decl_function_context (LABEL_EXPR_LABEL (bsi_stmt (bsi)))
 	      != current_function_decl)
 	    {
+	      tree stmt = bsi_stmt (bsi);
 	      error ("Label %s has incorrect context in bb %d\n",
-		     IDENTIFIER_POINTER (DECL_NAME (bsi_stmt (bsi))),
+		     IDENTIFIER_POINTER (DECL_NAME (LABEL_EXPR_LABEL (stmt))),
 		     bb->index);
 	      err = 1;
 	    }

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Jeffrey A Law]

On Sun, 2004-10-10 at 11:52, Andrew Pinski wrote:
> On Oct 5, 2004, at 6:35 PM, Jeffrey A Law wrote:
> >> ChangeLog:
> >> 	* tre-cfg.c (make_edges): After making the edges but before
> >> 	cleaning up the cfg, make an edge from the first BB to ever
> >> 	BB which we take the address of the label.
> >
> > If we're going to allow this, then I would strongly recommend
> > against this approach to the problem.  Instead just move
> > these labels to the start of their nearest following block that
> > is reachable.
> 
> Actually this was easier than I thought.
> And here is the patch which fixes this problem, the way you recommended.
> I had forgot that remove_bb was actually doing the removal of 
> basic_block.
> 
> OK? Bootstrapped and tested on PPC-darwin.
> 
> Thanks,
> Andrew Pinski
> 
> ChangeLog:
> 
> 	* tree-cfg.c (remove_bb): If we have a label expression in the
> 	basic block and the label we have taken the address, move the
> 	label expression to the basic block which is previous in the
> 	linked list.
> 	(tree_verify_flow_info): Fix printing out the label name of the
> 	problematic label expression.
It appears that you move the label to the start of the previous
block, which is probably OK.  I would have moved the label to
the start of the next block which would probably have been less
disruptive in terms of the location of the label.  However, given
the label is not reachable in the flow sense that's probably OK.

Approved.

jeff

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Andrew Pinski]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]


On Oct 18, 2004, at 3:26 PM, Jeffrey A Law wrote:
> It appears that you move the label to the start of the previous
> block, which is probably OK.  I would have moved the label to
> the start of the next block which would probably have been less
> disruptive in terms of the location of the label.  However, given
> the label is not reachable in the flow sense that's probably OK.
>
> Approved.

Thanks.

I also applied the following testcase for PR 16973.

Thanks,
Andrew Pinski


[-- Attachment #2: temp.diff.txt --]
[-- Type: text/plain, Size: 492 bytes --]

Index: testsuite/gcc.dg/pr16973.c
===================================================================
RCS file: testsuite/gcc.dg/pr16973.c
diff -N testsuite/gcc.dg/pr16973.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gcc.dg/pr16973.c	18 Oct 2004 19:35:26 -0000
@@ -0,0 +1,10 @@
+/* We were removing the label "a" because
+   we were removing the BB for it but forgot
+   to add back the label.   */
+
+void
+f (void)
+{
+  static __SIZE_TYPE__ x = &&a - &&b;
+  a : b : return;
+}

Re: [PATCH] Fix PRs middle-end/15014 and middle-end/16973, removing labels with their address taken

[Ulrich Weigand]

Andrew Pinski wrote:

>I also applied the following testcase for PR 16973.

This one fails with:
/home/weigand/fsf/gcc-head/gcc/testsuite/gcc.dg/pr16973.c: In function 'f':
/home/weigand/fsf/gcc-head/gcc/testsuite/gcc.dg/pr16973.c:8: error: taking the address of a label is non-standard
/home/weigand/fsf/gcc-head/gcc/testsuite/gcc.dg/pr16973.c:8: error: taking the address of a label is non-standard
/home/weigand/fsf/gcc-head/gcc/testsuite/gcc.dg/pr16973.c:8: error: pointer of type 'void *' used in subtraction

I guess you should reset dg-options to remove the "-ansi -pedantic-errors".

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de