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

[David Holmes]

Hi Ron,

(fixed subject)

On 19/03/2013 3:25 AM, Ron Durbin wrote:
>> There is a type mismatch between MallocMaxTestWords (uintx) and
>> cur_malloc_words (juint). The former will be 64-bit on 64-bit systems, while the latter is always 32-bit.
>>
>> os.cpp:
>>
>>    580   if (MallocMaxTestWords > 0 &&
>>    581       (cur_malloc_words + (alloc_size / BytesPerWord)) >
>> MallocMaxTestWords) {
>>    582     return false;
>>    583   }
>>
>> this calculation can overflow if MallocMaxWords is close to the limit.
>> Now I doubt we can actually allocate that much but the expression can be rearranged to avoid overflow.
>
> David I do not see the overflow. I see compiler promoting an expression.

Thanks for clarifying. I hadn't picked up that alloc_size was size_t and 
so also potentially 64-bit.

>> 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);
>>
>> 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.
>
>
>
> For the above comments, the code is structured the way Coleen requested and I'm deferring to her judgement.

Paging Coleen :)

Thanks,
David
------