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

[Bengt Rutisson]

Hi David,

On 2011-10-27 04:07, David Holmes wrote:
> On 27/10/2011 1:44 AM, 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?
>
> I was wondering something similar. We have a hard-wired maximum array 
> size (which has fortunately grown larger and larger over time).

Was my explanation to Vladimir good enough?

> BTW: one email to hotspot-dev might be more effective than three to 
> hotspot-*-dev.

I was back and forth beteween hotspot-dev and the three specific lists. 
Didn't really know what would be best. I'll try hotspot-dev next time.

Thanks,
Bengt

>
> David
> -----
>
>> Is SIZE_MAX defined on all platforms?
>>
>> You still have problem with short[] and char[] since
>> 2*max_jint+header_size will overflow. 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;
>>
>>
>> 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;
>> +}
>>
>> Rename check_overflow() to check_max_length_overflow() or something.
>>
>> I don't think we need incorrect old_max_array_length() method and
>> corresponding checks for LP64.
>>
>> 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
>>
>> 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