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

[Bengt Rutisson]

Hi Vladimir,

Thanks for looking at this change!

On 2011-10-26 17:44, Vladimir Kozlov wrote:
> Hi Bengt,
>
> I thought VM should throw OOM exception in such cases (as your 
> example) since you can't allocate such arrays in 32-bit VM. Does the 
> size check for OOM happened after heap resize calculation?

You are right. The VM will throw an OOME both before and after my 
change. But for different reasons.

With my change we will detect already in 
typeArrayKlass::allocate_common() that  length <= max_length() and throw 
an OOME. Before this change we could pass that test and go on in the 
allocation path. This would result in a size_t overflow when we want to 
expand the heap. We would do a lot of extra work to expand to the wrong 
size. Then the allocation would fail and we would throw OOME.

I think it is better to detect this early and avoid the extra work. This 
way the assert that G1 does (which I think makes sense) will hold. We 
already have the length check so I assume that this was the intention 
from the beginning.

> Is SIZE_MAX defined on all platforms?

I did try to look around the hotspot code for any uses of max size_t but 
I could not find it. So, I reverted to looking at standard ways of doing 
it. SIZE_MAX seems to be defined on all our platforms, since my code 
passes JPRT. And it seems to be part of the C99 standard:

http://en.wikipedia.org/wiki/Size_t

I thought it would be better to rely on this than for example 
"(size_t)-1". But I am happy to use any other define.

> You still have problem with short[] and char[] since 
> 2*max_jint+header_size will overflow. 

Right. I did add some unit tests to catch this (but as you point out 
below those tests did not detect this overflow). Thanks for catching it. 
I will fix it and send out an updated webrev.

> Also max_array_length() puts unneeded limitation on double[] and 
> long[] max length in 64 but VM, it should be max_jint. I think we can 
> do something like next:
>
>     const size_t max_words_per_size_t = 
> align_size_down((SIZE_MAX/HeapWordSize - header_size(type)), 
> MinObjAlignment);
>     const size_t max_elements_per_size_t = HeapWordSize * 
> max_words_per_size_t / type2aelembytes(type);
>     if ((size_t)max_jint < max_elements_per_size_t);
>       return max_jint ;
>     return (int32_t)max_elements_per_size_t;

I did not intend for my change to limit anything for 64 bits. That's why 
I added the MIN2((size_t)max_jint, max_words_per_size_t) statement and 
included the old calculation in the unit tests to verify that nothing 
has changed on 64 bits.

I can change the code as you suggest, but I would like to understand 
what the problem is with my code first. I think it handles 64 bits 
correctly.

> check_overflow() should use julong type for expressions instead of 
> size_t to catch overflow:
>
> +bool arrayOopDesc::check_overflow(BasicType type) {
> +  julong length = (julong)max_array_length(type);
> +  julong bytes_per_element = (julong)type2aelembytes(type);
> +  julong bytes = length * bytes_per_element + 
> (julong)header_size_in_bytes();
> +  return (julong)(size_t)bytes == bytes;
> +}

Right. This is why I didn't catch the problem above. Thanks. I'll fix.

> Rename check_overflow() to check_max_length_overflow() or something.

Done.

> I don't think we need incorrect old_max_array_length() method and 
> corresponding checks for LP64.

Ok. I'll keep it for some more testing to make sure that I don't 
unintentionally change anything for 64 bit. But I'll remove it before I 
push.

> Initialization and execution of additional JPRT tests will increase 
> jobs time. I think you could add InternalVMTests to existing client 
> and server tests:
>
> <    $(PRODUCT_HOME)/bin/java $(JAVA_OPTIONS) -version
> ---
> >    $(PRODUCT_HOME)/bin/java $(JAVA_OPTIONS) 
> -XX:+IgnoreUnrecognizedVMOptions -XX:+ExecuteInternalVMTests -version
>

I see your point regarding the increased job times. But I think it is 
much clearer to have the internal unit tests as a separate target. That 
way they can be included and excluded with a JPRT command line.

Also, I don't really like the IgnoreUnrecognizedVMOptions solution. I 
would like to be more explicit about that this is only a (fast)debug 
build flag.

Thanks,
Bengt

> regards,
> Vladimir
>
> On 10/26/11 6:18 AM, Bengt Rutisson wrote:
>>
>> 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