Edges, predictions, and GC crashes ...

Edges, predictions, and GC crashes ...

[Ulrich Weigand]

Hello,

I'm seeing compiler crashes during garbage collection when using mudflap.

The problem appears to be that some basic_block_def structures point to
edge_prediction structures which point to edge_def structures that have
already been ggc_free()'d.

Now, looking at remove_edge (cfg.c) is does indeed appear that it is
possible for edges to get deleted without the corresponding prediction
structure being removed as well ...

How is this supposed to work?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: Edges, predictions, and GC crashes ...

[Daniel Berlin]

On Thu, 2 Jun 2005, Ulrich Weigand wrote:

> Hello,
>
> I'm seeing compiler crashes during garbage collection when using mudflap.

> The problem appears to be that some basic_block_def structures point to
> edge_prediction structures which point to edge_def structures that have
> already been ggc_free()'d.

I keep hitting this problem on my darwin machine with my "rename exit and 
entry blocks to block 0 and 1" patch.
At first i thought it was something in the patch, but it's not.
It's the exact problem you describe.

>
> Now, looking at remove_edge (cfg.c) is does indeed appear that it is
> possible for edges to get deleted without the corresponding prediction
> structure being removed as well ...

Re: Edges, predictions, and GC crashes ...

[Jan Hubicka]

> Hello,
> 
> I'm seeing compiler crashes during garbage collection when using mudflap.
> 
> The problem appears to be that some basic_block_def structures point to
> edge_prediction structures which point to edge_def structures that have
> already been ggc_free()'d.
> 
> Now, looking at remove_edge (cfg.c) is does indeed appear that it is
> possible for edges to get deleted without the corresponding prediction
> structure being removed as well ...
> 
> How is this supposed to work?

I didn't have any cleanup_cfg in between earliest place putting
predictions and the profiling pass consuming them, so this scenario
didn't happen.  This has however changed a long time ago.  I guess just
teaching remove_edge to walk prediction list if it is present and kill
now dead predictions would not be perfomrance overkill as the
predictions are rare, I can implement that if you pass me some testcase.

Honza
> 
> Bye,
> Ulrich
> 
> -- 
>   Dr. Ulrich Weigand
>   Linux on zSeries Development
>   Ulrich.Weigand@de.ibm.com

Re: Edges, predictions, and GC crashes ...

[Ulrich Weigand]

Jan Hubicka wrote:

> I didn't have any cleanup_cfg in between earliest place putting
> predictions and the profiling pass consuming them, so this scenario
> didn't happen.  This has however changed a long time ago.  I guess just
> teaching remove_edge to walk prediction list if it is present and kill
> now dead predictions would not be perfomrance overkill as the
> predictions are rare, I can implement that if you pass me some testcase.

FAIL: libmudflap.c++/pass57-frag.cxx (test for excess errors)
WARNING: libmudflap.c++/pass57-frag.cxx compilation failed to produce executable
FAIL: libmudflap.c++/pass57-frag.cxx (-static) (test for excess errors)
WARNING: libmudflap.c++/pass57-frag.cxx (-static) compilation failed to produce executable
FAIL: libmudflap.c++/pass57-frag.cxx (-O2) (test for excess errors)
WARNING: libmudflap.c++/pass57-frag.cxx (-O2) compilation failed to produce executable
FAIL: libmudflap.c++/pass57-frag.cxx (-O3) (test for excess errors)
WARNING: libmudflap.c++/pass57-frag.cxx (-O3) compilation failed to produce executable

with current mainline on s390-ibm-linux.

Thanks for looking into this issue!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: Edges, predictions, and GC crashes ...

[Jan Hubicka]

> Jan Hubicka wrote:
> 
> > I didn't have any cleanup_cfg in between earliest place putting
> > predictions and the profiling pass consuming them, so this scenario
> > didn't happen.  This has however changed a long time ago.  I guess just
> > teaching remove_edge to walk prediction list if it is present and kill
> > now dead predictions would not be perfomrance overkill as the
> > predictions are rare, I can implement that if you pass me some testcase.
> 
> FAIL: libmudflap.c++/pass57-frag.cxx (test for excess errors)
> WARNING: libmudflap.c++/pass57-frag.cxx compilation failed to produce executable
> FAIL: libmudflap.c++/pass57-frag.cxx (-static) (test for excess errors)
> WARNING: libmudflap.c++/pass57-frag.cxx (-static) compilation failed to produce executable
> FAIL: libmudflap.c++/pass57-frag.cxx (-O2) (test for excess errors)
> WARNING: libmudflap.c++/pass57-frag.cxx (-O2) compilation failed to produce executable
> FAIL: libmudflap.c++/pass57-frag.cxx (-O3) (test for excess errors)
> WARNING: libmudflap.c++/pass57-frag.cxx (-O3) compilation failed to produce executable
> 
> with current mainline on s390-ibm-linux.
> 
> Thanks for looking into this issue!

Hi,
I've comitted the attached patch.  I didn't suceed to reproduce your
failures, but Danny reported it fixes his and it bootstrap/regtests
i686-pc-gnu-linux.

Honza

2005-06-03  Jan Hubicka  <jh@suse.cz>
	* basic-block.h (remove_predictions_associated_with_edge): Declare.
	* cfg.c (remove_edge): Use it.
	* predict.c (remove_predictions_associated_with_edge): New function.
Index: basic-block.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/basic-block.h,v
retrieving revision 1.261
diff -c -3 -p -r1.261 basic-block.h
*** basic-block.h	1 Jun 2005 12:07:42 -0000	1.261
--- basic-block.h	2 Jun 2005 21:45:58 -0000
*************** extern void tree_predict_edge (edge, enu
*** 869,874 ****
--- 869,875 ----
  extern void rtl_predict_edge (edge, enum br_predictor, int);
  extern void predict_edge_def (edge, enum br_predictor, enum prediction);
  extern void guess_outgoing_edge_probabilities (basic_block);
+ extern void remove_predictions_associated_with_edge (edge);
  
  /* In flow.c */
  extern void init_flow (void);
Index: cfg.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cfg.c,v
retrieving revision 1.92
diff -c -3 -p -r1.92 cfg.c
*** cfg.c	12 May 2005 22:32:08 -0000	1.92
--- cfg.c	2 Jun 2005 21:45:58 -0000
*************** make_single_succ_edge (basic_block src, 
*** 349,354 ****
--- 349,355 ----
  void
  remove_edge (edge e)
  {
+   remove_predictions_associated_with_edge (e);
    execute_on_shrinking_pred (e);
  
    disconnect_src (e);
Index: predict.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/predict.c,v
retrieving revision 1.146
diff -c -3 -p -r1.146 predict.c
*** predict.c	27 May 2005 22:06:33 -0000	1.146
--- predict.c	2 Jun 2005 21:45:58 -0000
*************** tree_predict_edge (edge e, enum br_predi
*** 240,245 ****
--- 240,263 ----
    i->edge = e;
  }
  
+ /* Remove all predictions on given basic block that are attached
+    to edge E.  */
+ void
+ remove_predictions_associated_with_edge (edge e)
+ {
+   if (e->src->predictions)
+     {
+       struct edge_prediction **prediction = &e->src->predictions;
+       while (*prediction)
+ 	{
+ 	  if ((*prediction)->edge == e)
+ 	    *prediction = (*prediction)->next;
+ 	  else
+ 	    prediction = &((*prediction)->next);
+ 	}
+     }
+ }
+ 
  /* Return true when we can store prediction on insn INSN.
     At the moment we represent predictions only on conditional
     jumps, not at computed jump or other complicated cases.  */

Re: Edges, predictions, and GC crashes ...

[Ulrich Weigand]

Jan Hubicka wrote:

> I've comitted the attached patch.  I didn't suceed to reproduce your
> failures, but Danny reported it fixes his and it bootstrap/regtests
> i686-pc-gnu-linux.

Thanks; this does fix one crash on s390x, but doesn't fix the
pass57-frag crashes on s390.

What happens is that after the predictions are created, but before 
remove_edge is called, the edge is modified in rtl_split_block
(called from tree_expand_cfg):

  /* Redirect the outgoing edges.  */
  new_bb->succs = bb->succs;
  bb->succs = NULL;
  FOR_EACH_EDGE (e, ei, new_bb->succs)
    e->src = new_bb;

Now the 'src' link points to a different basic block, but the old
basic block still has the prediction pointing to the edge.

When remove_edge is finally called, your new code tries to find
and remove the prediction from the *new* basic block's prediction
list -- but it still remains on the old one's list ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com

Re: Edges, predictions, and GC crashes ...

[Jan Hubicka]

> Jan Hubicka wrote:
> 
> > I've comitted the attached patch.  I didn't suceed to reproduce your
> > failures, but Danny reported it fixes his and it bootstrap/regtests
> > i686-pc-gnu-linux.
> 
> Thanks; this does fix one crash on s390x, but doesn't fix the
> pass57-frag crashes on s390.
> 
> What happens is that after the predictions are created, but before 
> remove_edge is called, the edge is modified in rtl_split_block
> (called from tree_expand_cfg):
> 
>   /* Redirect the outgoing edges.  */
>   new_bb->succs = bb->succs;
>   bb->succs = NULL;
>   FOR_EACH_EDGE (e, ei, new_bb->succs)
>     e->src = new_bb;
> 
> Now the 'src' link points to a different basic block, but the old
> basic block still has the prediction pointing to the edge.
> 
> When remove_edge is finally called, your new code tries to find
> and remove the prediction from the *new* basic block's prediction
> list -- but it still remains on the old one's list ...

Uhm, I will test fix for this too.  Thanks!

Honza
> 
> Bye,
> Ulrich
> 
> -- 
>   Dr. Ulrich Weigand
>   Linux on zSeries Development
>   Ulrich.Weigand@de.ibm.com