RFR: 8178495: Bug in the align_size_up_ macro

[Stefan Karlsson]

On 2017-06-28 22:00, Robbin Ehn wrote:
> Looks good.
>
> Is there a problem with always widen it to ULL ?

Yes, you get problems on 32-bit builds as well as problems with signed 
vs unsigned comparisons.

>
> E.g.
> ~((alignment) - 1ULL)
>
> Your widen_to_type_of is obliviously much cleaner.
>
> Thanks for fixing!

Thanks for reviewing.

StefanK

>
> /Robbin
>
> On 06/28/2017 05:08 PM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to fix a bug in the align_size_up_ macro.
>>
>> http://cr.openjdk.java.net/~stefank/8178495/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8178495
>>
>> The following:
>>   align_size_up_((uintptr_t)0x512345678ULL, (int8_t) 16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (int16_t) 16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (int32_t) 16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (int64_t) 16);
>>
>>   align_size_up_((uintptr_t)0x512345678ULL, (uint8_t) 16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (uint16_t)16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (uint32_t)16);
>>   align_size_up_((uintptr_t)0x512345678ULL, (uint64_t)16);
>>
>> Gives this output:
>>   0x512345680
>>   0x512345680
>>   0x512345680
>>   0x512345680
>>
>>   0x512345680
>>   0x512345680
>>    0x12345680
>>   0x512345680
>>
>> So, align_size_up_((uintptr_t)0x512345678ULL, (uint32_t)16) returns 
>> an unexpected, truncated value.
>>
>> This happens because in this macro:
>> #define align_size_up_(size, alignment) (((size) + ((alignment) - 1)) 
>> & ~((alignment) - 1))
>>
>> ~((alignment) - 1) returns 0x00000000FFFFFFF0 instead of 
>> 0xFFFFFFFFFFFFFFF0
>>
>> This isn't a problem for the 64-bit types, and maybe more non-obvious 
>> is that it doesn't happen for types 8-bit and 16-bit types.
>>
>> For the 8-bit and 16-bit types, the (alignment - 1) is promoted to a 
>> signed int, when it later is used in the & expression it is signed 
>> extended into a signed 64-bit value.
>>
>> When the type is an unsigned 32-bit integer, it isn't promoted to a 
>> signed int, and therefore it is not singed extended to 64 bits, but 
>> instead zero extended to 64 bits.
>>
>> This bug is currently not affecting the code base, since the inline 
>> align functions promote all integers to intptr_t, before passing them 
>> down to the align macros. However, when/if JDK-8178489 
>> <https://bugs.openjdk.java.net/browse/JDK-8178489> is pushed the 
>> macro is actually used with 32 bits unsigned ints.
>>
>> Tested with the unit test and JPRT with and without patches for 
>> JDK-8178489 <https://bugs.openjdk.java.net/browse/JDK-8178489>.
>>
>> Thanks,
>> StefanK