Round 2 Code Review request for new MallocMaxTestWords option and fixed test (7030610, 7037122, 7123945)

[Daniel D. Daugherty]

On 3/18/13 9:00 PM, David Holmes wrote:
> On 19/03/2013 12:37 PM, Coleen Phillimore wrote:
>>
>> Hi Ron,  I was looking at this again and I like David's suggestion for
>> moving this to a new function, like below.   Can you make this change
>> too?   A lot of us do look at os::malloc() function so having it easier
>> to read would be beneficial.
>> thanks,
>> Coleen
>>
>> On 3/18/2013 9:43 PM, Coleen Phillimore wrote:
>>>>>> Otherwise seems functionally ok but I really hate to see test-only
>>>>>> code being injected into a primary execution path like this. Why
>>>>>> not move it off to the side:
>>>>>>
>>>>>> if (MallocMaxTestWords > 0)
>>>>>>      return testMalloc(alloc_size);
>
> As Dan has pointed out offline we can't actually return here as we 
> need to go through the rest of the debug/accounting/NMT code. So we 
> need to do:
>
>  if (MallocMaxTestWords > 0)
>     ptr = testMalloc(alloc_size);
>  else
>     ptr = (u_char*)::malloc(alloc_size);

Just for the record, this solution results in calls to the
underlying ::malloc() from two places where the VM previously
called ::malloc() from only one place. While I'm not fond of
that change, I won't veto it either.

Dan



>
> Thanks,
> David
>
>>>>>>
>>>>>> and have
>>>>>>
>>>>>> void* testMalloc(size_t alloc_size) {
>>>>>>     if (!check_malloc_max_test_words(alloc_size)) return NULL;
>>>>>>     u_char* ptr = (u_char*)::malloc(alloc_size);
>>>>>>     add_current_malloc_test_words(alloc_size, ptr);
>>>>>>     return ptr;
>>>>>> }
>>>>
>>>> BTW you would of course just inline those other methods.
>>
>