Patchwork fix memory corruption bug in tm_region_init

login
register
mail settings
Submitter Aldy Hernandez
Date March 5, 2012, 4:54 p.m.
Message ID <4F54EFC3.8060203@redhat.com>
Download mbox | patch
Permalink /patch/144714/
State New
Headers show

Comments

Aldy Hernandez - March 5, 2012, 4:54 p.m.
Hi folks.

Torvald has a testcase from the STAMP benchmark that is showing a memory 
corruption error after my fix to publication safety problems.

The problem is we're allocating a chunk of worklist memory of size 
n_basic_blocks which changes with tail merge optimization and such.  We 
end up with a smaller n_basic_blocks than some of the BB's left in the 
region.  I believe what we want is last_basic_block.

This fixes the memory corruption bug.  I couldn't minimize a sensible 
testcase.

OK?
* trans-mem.c (tm_region_init): Use last_basic_block.
Rainer Orth - March 5, 2012, 5:08 p.m.
Aldy Hernandez <aldyh@redhat.com> writes:

> Torvald has a testcase from the STAMP benchmark that is showing a memory
> corruption error after my fix to publication safety problems.
>
> The problem is we're allocating a chunk of worklist memory of size
> n_basic_blocks which changes with tail merge optimization and such.  We end
> up with a smaller n_basic_blocks than some of the BB's left in the region.
> I believe what we want is last_basic_block.
>
> This fixes the memory corruption bug.  I couldn't minimize a sensible
> testcase.

Could this be PR middle-end/52463 libitm.c/memcpy-1.c FAILs?

	Rainer
Richard Henderson - March 5, 2012, 5:16 p.m.
On 03/05/2012 08:54 AM, Aldy Hernandez wrote:
>    region_worklist =
>      (struct tm_region **) xcalloc (sizeof (struct tm_region *),
> -				  n_basic_blocks + NUM_FIXED_BLOCKS + 2);
> +				  last_basic_block + NUM_FIXED_BLOCKS);

This is ok.

I was confused for a moment by the "worklist" variable name, which
suggests a queue.  I'd also suggest that you change to use a vec,
rather than callocing yourself, and would have caught the memory
error earlier.


r~
Aldy Hernandez - March 5, 2012, 6:37 p.m.
On 03/05/12 11:16, Richard Henderson wrote:
> On 03/05/2012 08:54 AM, Aldy Hernandez wrote:
>>     region_worklist =
>>       (struct tm_region **) xcalloc (sizeof (struct tm_region *),
>> -				  n_basic_blocks + NUM_FIXED_BLOCKS + 2);
>> +				  last_basic_block + NUM_FIXED_BLOCKS);
>
> This is ok.
>
> I was confused for a moment by the "worklist" variable name, which
> suggests a queue.  I'd also suggest that you change to use a vec,
> rather than callocing yourself, and would have caught the memory
> error earlier.
>
>
> r~

I thought there'd be a lot less overhead by callocing the value myself. 
  Is the overhead 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.

Aldy
Aldy Hernandez - March 5, 2012, 6:39 p.m.
On 03/05/12 11:08, Rainer Orth wrote:
> Aldy Hernandez<aldyh@redhat.com>  writes:
>
>> Torvald has a testcase from the STAMP benchmark that is showing a memory
>> corruption error after my fix to publication safety problems.
>>
>> The problem is we're allocating a chunk of worklist memory of size
>> n_basic_blocks which changes with tail merge optimization and such.  We end
>> up with a smaller n_basic_blocks than some of the BB's left in the region.
>> I believe what we want is last_basic_block.
>>
>> This fixes the memory corruption bug.  I couldn't minimize a sensible
>> testcase.
>
> Could this be PR middle-end/52463 libitm.c/memcpy-1.c FAILs?
>
> 	Rainer
>

Yes, fixed and closed.
Richard Henderson - March 5, 2012, 6:47 p.m.
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~

Patch

Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 184935)
+++ trans-mem.c	(working copy)
@@ -1868,7 +1868,7 @@  tm_region_init (struct tm_region *region
      using bb->aux.  */
   region_worklist =
     (struct tm_region **) xcalloc (sizeof (struct tm_region *),
-				  n_basic_blocks + NUM_FIXED_BLOCKS + 2);
+				  last_basic_block + NUM_FIXED_BLOCKS);
 
   VEC_safe_push (basic_block, heap, queue, bb);
   region_worklist[bb->index] = region;