fix memory corruption bug in tm_region_init

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.

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

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~

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

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.

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~