RFR: 8178495: Bug in the align_size_up_ macro

[Robbin Ehn]

Looks good.

Is there a problem with always widen it to ULL ?

E.g.
~((alignment) - 1ULL)

Your widen_to_type_of is obliviously much cleaner.

Thanks for fixing!

/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