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

[John Coomes]

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 :(.

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

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
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>
>