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

Thu Nov 10 17:28:01 UTC 2011

Thanks, Vladimir. I'll shorten the lines for the comments and file a CR 
for the size_t cleanup.

Bengt

On 2011-11-10 18:24, Vladimir Kozlov wrote:
> (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
>>>>>>>>
>>>>>>>
>>>>>
>>