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

[Vladimir Kozlov]

(I replied to wrong mail so I am repeating my review here)

I agree. Changes looks good but could you limit length of comment lines (some of us use emacs in terminal).

Also, please, file RFE to clean up this mess with size of object, it should be size_t type.

Thanks,
Vladimir

On 11/10/11 7:24 AM, Bengt Rutisson wrote:
>
> Vladimir,
>
> This change turns out to propagate to all kinds of places. In fact, Klass:: oop_oop_iterate() returns an int for the
> object size. This seems wrong to me, but it will become a very large change to correct it.
>
> How about just limiting the maximum array length to be "max_jint - headersize"? That will be a very small change and
> hopefully it will not have such a huge impact on "real" applications. The difference between the allowed maximum array
> length and the theoretical maximum will only be a few elements.
>
> Here is a webrev of what I think that would look like:
> http://cr.openjdk.java.net/~brutisso/7110152/webrev.03/
>
> Thanks,
> Bengt
>
> On 2011-11-10 01:30, Vladimir Kozlov wrote:
>> Sorry, my last comment is incorrect. Yes, max_array_length() returns max_jint in 64 bit VM.
>>
>> Removing assert from object_size() is not enough. You need to change returned type to size_t. Otherwise the size will
>> be cut incorrectly. Yes, it become messy.
>>
>> I all cases it would be nice to keep the assert (maybe changed) to catch incorrect size in 32 bit VM. And move the
>> assert in object_size() after size alignment.
>>
>> Thanks,
>> Vladimir
>>
>> Vladimir Kozlov wrote:
>>> 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
>>>>>>>
>>>>>>
>>>>
>