RFR: 8178495: Bug in the align_size_up_ macro

[Stefan Karlsson]

Fixing the logging with SCOPED_TRACE was only tested locally, but failed 
on OSX. Here's a fix for that problem:
  http://cr.openjdk.java.net/~stefank/8178495/webrev.02
  http://cr.openjdk.java.net/~stefank/8178495/webrev.02.delta

Passes JPRT now.

StefanK

On 2017-06-30 10:06, Stefan Karlsson wrote:
> Hi Kim,
>
> Updated webrevs:
>  http://cr.openjdk.java.net/~stefank/8178495/webrev.01
>  http://cr.openjdk.java.net/~stefank/8178495/webrev.01.delta
>
> Inlined:
>
> On 2017-06-30 00:04, Kim Barrett wrote:
>>> On Jun 28, 2017, at 11:08 AM, Stefan Karlsson 
>>> <stefan.karlsson at oracle.com> 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
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/utilities/globalDefinitions.hpp
>>   514 #define widen_to_type_of(what, type_carrier) ((what) | 
>> ((type_carrier) & 0))
>>
>> I think a better form of widen_to_type_of is the following:
>>
>> #define widen_to_type_of(what, type_carrier) (true ? (what) : 
>> (type_carrier))
>>
>> The difference is that this just promotes as needed, and will never
>> execute any part of type_carrier. The definition in the webrev may not
>> be able to completely optimize away the carrier expression under some
>> conditions.
>
> Thanks for the suggestion.
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> test/native/utilities/test_align.cpp
>>    35 struct TypeInfo : AllStatic {
>>    36   static const bool is_unsigned = T(-1) > T(0);
>>    37   static const T    max = is_unsigned
>>
>> We recently (JDK-8181318) made it possible to #include <limits> and
>> use std::numeric_limits<T>, so you could get is_signed and max() from
>> there.  max_alignment could then just be a static helper function
>> template.
>>
>> Also here:
>>   54 static const intptr_t max_intptr = (intptr_t)max_intx;
>
> Done.
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> test/native/utilities/test_align.cpp
>>    25 #include "logging/log.hpp"
>> and
>>    44 const static bool logging_enabled = false;
>>    45 #define log(...)            \
>>    ...
>>
>> I'm guessing the logging macro is because logging/log stuff isn't
>> initialized yet when executing a TEST (as opposed to a TEST_VM)?  So
>> the #include is superfluous?
>
> You are right.
>
> I changed the implementation to use SCOPED_TRACE, as you suggested in 
> another mail.
>
> Thanks,
> StefanK
>