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

[Daniel D. Daugherty]

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?

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.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130318/ba3719b8/attachment-0001.html