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

Thu Nov 10 22:49:44 UTC 2011

John,

On 2011-11-10 20:06, John Coomes wrote:
> Bengt Rutisson (bengt.rutisson at oracle.com) wrote:
>> Thanks, Vladimir. I'll shorten the lines for the comments and file a CR
>> for the size_t cleanup.
> As you discovered, that cleanup is pervasive :(.

Indeed.

> There's an existing bug for this - 4718400 (unfortunately, it's not
> available externally).

Thanks for pointing this out. Just read through 4718400. I had already 
filed a new CR (7110613) for this. I am tempted to close my bug as a dup 
of 4718400, but since that one is not open it might be better to do it 
the other way around. Why is 4718400 confidential in the first case? Any 
suggestion on which bug to close?

Bengt

>
> Excerpts from the evaluation:
>
> HotSpot methods such as oopDesc::size(), oop_size() and
> object_size() return an int, which represents a size in words.
> ...
> using int as the return type of oop_size(), object_size(), etc.,
> is a limitation only in the 64-bit VM where the maximum length for an
> array of longs or doubles in hotspot is 2G - 4 instead of 2G - 1.
> Changing the return type to unsigned int would be enough to remove
> this limitation.
>
> -John
>
>> 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