Request for review: 7110152 assert(size_in_words <= (julong)max_jint) failed: no overflow

[Vladimir Kozlov]

Bengt Rutisson wrote:
> On 2011-11-10 00:14, Vladimir Kozlov wrote:
>> Bengt Rutisson wrote:
>>>
>>> ...except that I just saw that CollectedHeap::array_allocate_nozero() 
>>> uses int.
>>
>> Yes, this is what I tried to tell you :)
> 
> Right. Thanks. :-)
> 
>> You also missed code in typeArrayKlass::allocate_permanent() and 
>> nozero code in typeArrayKlass::allocate_common(). May be also other 
>> places where CollectedHeap::array_allocate is called.
> 
> Yes, unfortunately this propagates to typeArrayOopDesc::object_size() 
> that returns an int. So, this really propagates.

I surprise that c++ compilers does not complain about returning int from 
typeArrayOopDesc::object_size() in 64 bit VM when align_object_size() returns 
intptr_t value.

> 
> I'll dig in to it a bit, but if the change is getting too large, I guess 
> an alternative is to go back to limiting 
> arrayOopDesc::max_array_length() to return max_jint - headersize. Seems 
> like there is a lot of code that relize on that an object size will fit 
> into an int.

Your new code does substruct headersize in max_array_length(). And it rounds 
size down to MinObjAlignment. So we should not overflow int in object_size().
One note, in object_size() we need to move assert after align_object_size():

     assert(size_in_words <= (julong)max_jint, "no overflow");

     return align_object_size((intptr_t)size_in_words);
---
     size_t aligned_size = (size_t)align_object_size((intptr_t)size_in_words);
     assert(aligned_size <= (size_t)max_jint, "no overflow");
     return aligned_size;

Thanks,
Vladimir

> 
>> Could you also remove 'size' argument in 
>> post_allocation_install_obj_klass() which does not use it?
> 
> Sure. Done.
> 
> Bengt
> 
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> I'll fix that and update the webrev.
>>>
>>> Bengt
>>>
>>> On 2011-11-09 23:47, Bengt Rutisson wrote:
>>>>
>>>> Vladimir,
>>>>
>>>> Thanks for looking at this!
>>>>
>>>> On 2011-11-09 23:06, Vladimir Kozlov wrote:
>>>>> Why not change it in all CollectedHeap methods?
>>>>
>>>> The other methods in CollectedHeap that use int for size are 
>>>> handling objects. Those should not have been affected by my other 
>>>> change (since it only changed the maximum array length).
>>>>
>>>> I have not investigated whether or not it is possible to create an 
>>>> object (that is not an array) that will have a size larger than int. 
>>>> I assume that there is some limit to how many fields an object can 
>>>> have. But I don't know what that limit is and whether it will 
>>>> guarantee that the largest object fits into an int. It also seems 
>>>> much more unlikely that anyone would actually create such a large 
>>>> object.
>>>>
>>>> I'd like to only change this for arrays now since we have failures 
>>>> in the nightlies. If you think it is ok I could create a separate CR 
>>>> to investigate whether or not the int object sizes in CollectedHeap 
>>>> are safe.
>>>>
>>>> Bengt
>>>>
>>>>>
>>>>> Vladimir
>>>>>
>>>>> Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Could I have a couple of reviews for this small change?
>>>>>> http://cr.openjdk.java.net/~brutisso/7110152/webrev.01/
>>>>>>
>>>>>> The problem is that CollectedHeap::array_allocate() takes its size 
>>>>>> as an int instead of a size_t. This was exposed by the fix I made 
>>>>>> to 7102044 recently.
>>>>>>
>>>>>> That change allows arrays to be max_jint long on 64 bit systems. 
>>>>>> This is correct since we can handle this size in a 64 bit size_t 
>>>>>> even when we add the object header. However, it will overflow an int.
>>>>>>
>>>>>> Here is a small reproducer that will trigger the assert mentioned 
>>>>>> in 7110152 when it is run on a 64 bit VM:
>>>>>>
>>>>>> public class MaxJIntArray {
>>>>>>     public static void main(String[] args) {
>>>>>>         final int MAX_JINT = 2147483647;
>>>>>>         double[] a = new double[MAX_JINT];
>>>>>>         System.out.println("Allocated a[" + a.length + "]");
>>>>>>     }
>>>>>> }
>>>>>>
>>>>>> The fix is to remove the assert (since it is too strict) and 
>>>>>> changing CollectedHeap::array_allocate() to take a size_t instead 
>>>>>> of an int.
>>>>>>
>>>>>> CR:
>>>>>> 7110152 assert(size_in_words <= (julong)max_jint) failed: no overflow
>>>>>> http://monaco.sfbay.sun.com/detail.jsf?cr=7110152
>>>>>>
>>>>>> Bengt
>>>>
>>>
>