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

[David Holmes]

On 19/03/2013 12:39 PM, Daniel D. Daugherty wrote:
> Greetings,
>
> Now that it looks like the dust is settling on round 2 of
> this code review, I have a question for the code reviewers.
>
> This if-statement came up ina couple of reviews:
>
> 580   if (MallocMaxTestWords > 0 &&
> 581       (cur_malloc_words + (alloc_size / BytesPerWord)) >
> MallocMaxTestWords) {
>
> Nobody was worried about the part on line 580and I had
> a very simple way of looking at line 581:
>
> (expr1 > expr2)
>
> but it occurs to me that maybe I'm wrong. 'expr2' is
> pretty simple: MallocMaxTestWords which is of type
> uintx. As David H pointed out, uintx is 32-bits or
> 64-bits depending on the VM (and the OS). To me the
> important partwas not explicitly stated and that is
> the factthat uintx is unsigned.
>
> Since 'expr2' is unsigned, 'expr1' will be "promoted"
> by the compiler to unsigned for the comparison. In
> that case, "overflow"or "rollover" doesn't matter
> because we're comparing two unsigned values.
>
> So the question: Am I misunderstanding how 'unsigned'
> works in comparison expressions in C++/C?

alloc_size is size_t which is also unsigned. So cur_malloc_words will be 
treated as unsigned, the arithmetic performed and an unsigned comparison 
done. At that point we are relying on rollover to indicate overflow. 
This is no different to the logic just above it, but maybe both of them 
are wrong and the arithmetic should be reorganized so that it doesn't 
rely on rollover?

David

> Perhaps I was looking at this way too simplistically.
>
> Dan
>
> P.S.
> Another way that I looked at these changesis risk:
>
> If Ron's early malloc() failure logic doesn'tcatch
> when the (artificial) limit is reached, thenwhat
> happens? We don't return NULL early and the system
> works the way it did before. Little to no risk.
>
>
> On 3/15/13 12:29 PM, Ron Durbin wrote:
>> 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/
>> <http://cr.openjdk.java.net/%7Erdurbin/7030610-webrev-cr1/1-hsx25/>
>>
>> Internal URL
>> http://javaweb.us.oracle.com/~rdurbin/7030610-webrev/1-hsx25/
>> <http://javaweb.us.oracle.com/%7Erdurbin/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.
>>
>