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

[Bengt Rutisson]

Vladimir, David and Tony,

Thanks for the reviews!

Bengt

On 2011-11-14 02:54, David Holmes wrote:
> Hi Bengt,
>
> On 14/11/2011 12:14 AM, Bengt Rutisson wrote:
>> I already sent this out for review on the GC alias. It started out as a
>> pure GC change, but after some updates it has become a change in
>> src/share/vm/oops/arrayOop.hpp, which deserves a broader alias (thanks
>> David Holmes for pointing this out). So, I am resending this review
>> request to the hotspot-dev alias.
>>
>> http://cr.openjdk.java.net/~brutisso/7110152/webrev.04/
>>
>> Vladimir and David Holmes have already looked at it. But more comments
>> are of course welcome.
>>
>> The background is that when I pushed this:
>> 7102044: G1: VM crashes with assert(old_end != new_end) failed: don't
>> call this otherwise
>> http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot/rev/6fd81579526f
>>
>> I changed arrayOopDesc::max_array_length() to always return max_jint in
>> 64 bit VMs. This was intentional and is correct as far as I can tell.
>> But it turns out that a large part of the GC code passes object sizes
>> around as int and not size_t. Thus, we might overflow the int when we
>> add the object header.
>>
>> My proposed change now reverts back to reducing the maximum array length
>> to be slightly less than max_jint.
>
> Okay, so the original code would only return max_jint for the smaller 
> element types (up to ints on 64-bit, and short/char on 32-bit). And 
> otherwise calculated a size in words that allowed for adding back the 
> header. The previous fix allowed max_jint to be returned for all types 
> but didn't allow for adding back the header - which is now fixed (and 
> only necessary because int is used in some places as you stated).
>
> Anyway this fix seems okay to me as long as nothing else in the code 
> tries to pass around an object size in bytes using an int.
>
>> CR:
>>
>> 7110152 assert(size_in_words <= (julong)max_jint) failed: no overflow
>> http://monaco.sfbay.sun.com/detail.jsf?cr=7110152
>
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7110152
>
> Cheers,
> David
> -----
>
>> Here is a reproducer that triggers the assert before my change but
>> passes after:
>>
>> 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 + "]");
>> }
>> }
>>
>> Thanks,
>> Bengt