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

[Ron Durbin]

David

Thanks for your detailed code review.


> -----Original Message-----
> From: David Holmes
> Sent: Sunday, March 17, 2013 5:38 PM
> To: Ron Durbin
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: Round 2 Code Review request for new MallocMaxBytes option 
> and fixed test (7030610, 7037122, 7123945)
> 
> Hi Ron,
> 
> 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.

# defines taken from source
static juint cur_malloc_words = 0 (always 32 bits)
diagnostic(uintx, MallocMaxTestWords,     0,;(32 or 64 bits based on platform word size)
size_t alloc_size;;(32 or 64 based on platform word size)



Here is what the compiler sees in terms of data types. Replacing the variables with datatypes looks like this. 

  580   if (uintx(32 or 64)) > 0 &&
  581       (juint(32)) + (size_t(32 or 64) / BytesPerWord( macro const value))) > 
uintx(32 or 64)) ) {
  582     return false;
  583   }

> 
> Not sure I see the point of passing ptr to the add function instead of just doing:
> 
> if (ptr != null)
>     add_current_malloc_test_words(alloc_size);
> 
> 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;
> }
> 


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

> For the testcase I only skimmed it. Not sure of the logic there, but 
> the proof of the test is in the testing so I assume you have managed 
> to tickle all the different exit modes for the test?

Yes

> 
> Cheers,
> David
> 
> On 16/03/2013 4:29 AM, Ron Durbin wrote:
> > Dan
> >
> > I have a round 2 code review ready for:
> >
> > Round 2 addresses:
> >
> > - addressed code review comments from Coleen, Dan and David
> > - note: src/share/vm/runtime/os.hpp is no longer changed.
> >
> > Web URL
> > http://cr.openjdk.java.net/~rdurbin/7030610-webrev-cr1/1-hsx25/
> >
> > Internal URL
> > http://javaweb.us.oracle.com/~rdurbin/7030610-webrev/1-hsx25/
> >
> > 7030610 runtime/6878713/Test6878713.sh fails, Error. failed to clean 
> > up files after test.
> >
> > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7030610
> >
> > https://jbs.oracle.com/bugs/browse/JDK-7030610
> >
> > 7037122 runtime/6878713/Test6878713.sh is written incorrectly
> >
> > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7037122
> >
> > https://jbs.oracle.com/bugs/browse/JDK-7037122
> >
> > (This fix only addresses the issues with test 
> > runtime/6878713/Test6878713.sh;
> >
> > 7037122 names other tests that are not addressed by this fix)
> >
> > 7123945 runtime/6878713/Test6878713.sh require about 2G of
> >
> > _http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123945_
> >
> > https://jbs.oracle.com/bugs/browse/JDK-7123945
> >
> > Summary: Add new diagnostic option -XX:MallocMaxTestWords=NNN and 
> > fix
> >
> > Test6878713.sh. Test6878713.sh is a security regression test with 
> > four different failure modes.Three are documented in the defects 
> > above and the forth
> >
> > was found during bug triage. The root cause of these failures can be
> >
> > traced back to two issues: Inconsistent test condition setup and 
> > error handling.
> >
> > All four defect manifestations are fixed by this set of changes.
> >
> > The testing is not completed because the JPRT run is running slowly.
> >