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

[Coleen Phillimore]

Hi,  Sorry for the delay, see below.

On 3/18/2013 9:15 PM, David Holmes wrote:
> 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.

The compiler would inline those methods.  I sort of like the ::malloc() 
in the main os::malloc() call only so it's easy to see which malloc 
we're using.   We could change which malloc this calls (probably won't) 
so it's nice that there's only one place (here).

>>
>>
>>
>> For the above comments, the code is structured the way Coleen 
>> requested and I'm deferring to her judgement.
>
> Paging Coleen :)

I think this looks good to me.  I did request this be in words so that 
the Atomic::add(int) could be used to avoid the #ifdef LP64 code.   I 
normally hate words, so Ioi is right I would usually complain!

He could do this change for the add_current_malloc_words() to test for 
the ptr != NULL outside the call.

I too only cursorily looked at the test, but I think Dan's review is all 
you need for that.

Thanks,
Coleen

>
> Thanks,
> David
> ------