Request for review (S): 7102044 G1: VM crashes with assert(old_end != new_end) failed: don't call this otherwise

[Bengt Rutisson]

Hi all,

Can I get a couple of reviews to this fairly small change?
http://cr.openjdk.java.net/~brutisso/7102044/webrev.01/

Sorry for the wide distribution, but the bug is reported on GC, I think 
the actual change (in arrayOop.hpp) is runtime responsibility and the 
method that is being changed is used on a few occasions in C2. So, I 
figured I'd like everyone to know that this is about to go in.

CR:
7102044 G1: VM crashes with assert(old_end != new_end) failed: don't 
call this otherwise
http://monaco.sfbay.sun.com/detail.jsf?cr=7102044

Background
arrayOopDesc::max_array_length() returns the maximum length an array can 
have based on the type of the array elements. This is passed on through 
the allcocation path as a word size. When we need to expand the heap we 
have to convert the word size to a byte size. The problem is that on 32 
bit platforms this conversion may overflow a size_t. Which will result 
in that we call expand() with a much smaller size than required size or 
even with a size of 0.

Here is a small reproducer that will trigger the assert mentioned in the 
CR if we run it with -XX:+UseG1GC:

public class LargeObj {
         public static void main(String[] args) {
                 int[] a = new int[2147483639];
         }
}

It is not only G1 that has this problem. ParallelGC has the same issue, 
it just doesn't have an assert to detect it. It can be argued that the 
GCs should check for overflows, but I think that since 
arrayOopDesc::max_array_length()  will catch the problem earlier and 
solve it for all GCs this is a better place to fix the problem.

I added some unit tests to test the new implementation. To make sure 
that these tests are run I had to update some more files. The actual 
change is in arrayOop.hpp. The rest of the files have just been changed 
to allow the unit tests to run.

A few questions:

(1) In arrayOopDesc::max_array_length() I use MIN2 to make sure that we 
don't overflow a size_t. This is only needed on 32 bit platforms. I 
prefer this general code compared to #ifndef _LP64. Any thoughts?

(2) I made the unit tests run as JPRT make targets. I guess it can also 
be done as jtreg tests, but that way it would only be executed for PIT 
etc. Any thoughts on this?

Thanks,
Bengt