diff mbox

fix memory corruption bug in tm_region_init

Message ID 4F563361.3060005@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez March 6, 2012, 3:55 p.m. UTC 
On 03/05/12 12:47, Richard Henderson wrote:
> On 03/05/2012 10:37 AM, Aldy Hernandez wrote:
>> I thought there'd be a lot less overhead by callocing the value myself.  Is the overhead negligible?
>
> Yes, it's negligible.
>
>> I can certainly make it a VEC in a follow up patch if you want, though I'll commit this now so I can at get Rainer and Torvald happy while I do so.
>
> Certainly.
>
>
> r~


Conversion to vectors.

I reduced the count to just last_basic_blocks, as I don't think we need 
to add NUM_FIXED_BLOCKS.  The original NUM_FIXED_BLOCKS was probably a 
kludge because I was using the wrong size and things didn't match.

I also changed the name so we don't get confused any more :).

OK for trunk and 4.7 branch?
* trans-mem.c: New typedef for tm_region_p.
	Define vector types for tm_region_p.
	(tm_region_init): Replace region_worklist to a vector called
	bb_regions.

Comments

Richard Henderson March 6, 2012, 4:04 p.m. UTC | #1 
On 03/06/12 07:55, Aldy Hernandez wrote:
> +  bb_regions = VEC_alloc (tm_region_p, heap, last_basic_block);
> +  VEC_reserve (tm_region_p, heap, bb_regions, last_basic_block);
> +  for (i = 0; i < last_basic_block; ++i)
> +    VEC_quick_insert (tm_region_p, bb_regions, i, NULL);

The reserve is redundant, since you already did that in the alloc.
You're looking for VEC_safe_grow_cleared here instead of that loop.

Otherwise ok.


r~
Jakub Jelinek March 6, 2012, 4:20 p.m. UTC | #2 
On Tue, Mar 06, 2012 at 08:04:12AM -0800, Richard Henderson wrote:
> On 03/06/12 07:55, Aldy Hernandez wrote:
> > +  bb_regions = VEC_alloc (tm_region_p, heap, last_basic_block);
> > +  VEC_reserve (tm_region_p, heap, bb_regions, last_basic_block);
> > +  for (i = 0; i < last_basic_block; ++i)
> > +    VEC_quick_insert (tm_region_p, bb_regions, i, NULL);
> 
> The reserve is redundant, since you already did that in the alloc.
> You're looking for VEC_safe_grow_cleared here instead of that loop.
> 
> Otherwise ok.

And VEC_safe_grow_cleared will do even the VEC_alloc if bb_regions is
NULL before this or you just set it to NULL before VEC_safe_grow_cleared.

	Jakub
diff mbox

Patch

Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 184949)
+++ trans-mem.c	(working copy)
@@ -1757,6 +1757,10 @@  struct tm_region
   bitmap irr_blocks;
 };
 
+typedef struct tm_region *tm_region_p;
+DEF_VEC_P (tm_region_p);
+DEF_VEC_ALLOC_P (tm_region_p, heap);
+
 /* True if there are pending edge statements to be committed for the
    current function being scanned in the tmmark pass.  */
 bool pending_edge_inserts_p;
@@ -1858,7 +1862,8 @@  tm_region_init (struct tm_region *region
   VEC(basic_block, heap) *queue = NULL;
   bitmap visited_blocks = BITMAP_ALLOC (NULL);
   struct tm_region *old_region;
-  struct tm_region **region_worklist;
+  VEC(tm_region_p, heap) *bb_regions;
+  int i;
 
   all_tm_regions = region;
   bb = single_succ (ENTRY_BLOCK_PTR);
@@ -1866,17 +1871,18 @@  tm_region_init (struct tm_region *region
   /* We could store this information in bb->aux, but we may get called
      through get_all_tm_blocks() from another pass that may be already
      using bb->aux.  */
-  region_worklist =
-    (struct tm_region **) xcalloc (sizeof (struct tm_region *),
-				  last_basic_block + NUM_FIXED_BLOCKS);
+  bb_regions = VEC_alloc (tm_region_p, heap, last_basic_block);
+  VEC_reserve (tm_region_p, heap, bb_regions, last_basic_block);
+  for (i = 0; i < last_basic_block; ++i)
+    VEC_quick_insert (tm_region_p, bb_regions, i, NULL);
 
   VEC_safe_push (basic_block, heap, queue, bb);
-  region_worklist[bb->index] = region;
+  VEC_replace (tm_region_p, bb_regions, bb->index, region);
   do
     {
       bb = VEC_pop (basic_block, queue);
-      region = region_worklist[bb->index];
-      region_worklist[bb->index] = NULL;
+      region = VEC_index (tm_region_p, bb_regions, bb->index);
+      VEC_replace (tm_region_p, bb_regions, bb->index, NULL);
 
       /* Record exit and irrevocable blocks.  */
       region = tm_region_init_1 (region, bb);
@@ -1898,15 +1904,15 @@  tm_region_init (struct tm_region *region
 	       the entry block of the new region is associated with this region.
 	       Other successors are still part of the old region.  */
 	    if (old_region != region && e->dest != region->entry_block)
-	      region_worklist[e->dest->index] = old_region;
+	      VEC_replace (tm_region_p, bb_regions, e->dest->index, old_region);
 	    else
-	      region_worklist[e->dest->index] = region;
+	      VEC_replace (tm_region_p, bb_regions, e->dest->index, region);
 	  }
     }
   while (!VEC_empty (basic_block, queue));
   VEC_free (basic_block, heap, queue);
   BITMAP_FREE (visited_blocks);
-  free (region_worklist);
+  VEC_free (tm_region_p, heap, bb_regions);
 }
 
 /* The "gate" function for all transactional memory expansion and optimization