RFR: 8178495: Bug in the align_size_up_ macro

[Kim Barrett]

> 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.

------------------------------------------------------------------------------ 
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;

------------------------------------------------------------------------------ 
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?

------------------------------------------------------------------------------ 
test/native/utilities/test_align.cpp 
  56 template <typename T, typename A>
  57 static void test_alignments() {
  ...
and
 117 TEST(Align, functions_and_macros) {
 ... calls to test_alignments for various types.

I keep meaning to find out if we support Google Test's "Typed Tests" [1],
and if not, try to figure out how hard it would be to change that.

[1] https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md
Search for "Typed Tests".

------------------------------------------------------------------------------
test/native/utilities/test_align.cpp 
 79         ASSERT_EQ(align_size_up(value, alignment), (intptr_t)up);
and elsewhere

The Google Test docs suggest that the expected value be first, and the
value being tested be second, and that the failure reporting assumes
that when printing a failure message. Hm, I see the Google Test docs
have been changed since I read that, and now say

  Historical note: Before February 2016 *_EQ had a convention of
  calling it as ASSERT_EQ(expected, actual), so lots of existing code
  uses this order. Now *_EQ treats both parameters in the same way.

I wonder how old our Google Test code snapshot might be.

------------------------------------------------------------------------------