RFR: 8178495: Bug in the align_size_up_ macro

[Stefan Karlsson]

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