8178495: Bug in the align_size_up_ macro
authorstefank
Wed, 12 Apr 2017 13:05:59 +0200
changeset 46617 0330c5fc49ce
parent 46616 66d452cca30f
child 46618 d503911aa948
8178495: Bug in the align_size_up_ macro Reviewed-by: kbarrett, tschatzl, rehn
hotspot/src/share/vm/utilities/globalDefinitions.hpp
hotspot/test/native/utilities/test_align.cpp
--- a/hotspot/src/share/vm/utilities/globalDefinitions.hpp	Tue Apr 11 23:45:39 2017 +0200
+++ b/hotspot/src/share/vm/utilities/globalDefinitions.hpp	Wed Apr 12 13:05:59 2017 +0200
@@ -505,7 +505,18 @@
 // for use in places like enum definitions that require compile-time constant
 // expressions and a function for all other places so as to get type checking.
 
-#define align_size_up_(size, alignment) (((size) + ((alignment) - 1)) & ~((alignment) - 1))
+// Using '(what) & ~align_mask(alignment)' to align 'what' down is broken when
+// 'alignment' is an unsigned int and 'what' is a wider type. The & operation
+// will widen the inverted mask, and not sign extend it, leading to a mask with
+// zeros in the most significant bits. The use of align_mask_widened() solves
+// this problem.
+#define align_mask(alignment) ((alignment) - 1)
+#define widen_to_type_of(what, type_carrier) (true ? (what) : (type_carrier))
+#define align_mask_widened(alignment, type_carrier) widen_to_type_of(align_mask(alignment), (type_carrier))
+
+#define align_size_down_(size, alignment) ((size) & ~align_mask_widened((alignment), (size)))
+
+#define align_size_up_(size, alignment) (align_size_down_((size) + align_mask(alignment), (alignment)))
 
 inline bool is_size_aligned(size_t size, size_t alignment) {
   return align_size_up_(size, alignment) == size;
@@ -519,8 +530,6 @@
   return align_size_up_(size, alignment);
 }
 
-#define align_size_down_(size, alignment) ((size) & ~((alignment) - 1))
-
 inline intptr_t align_size_down(intptr_t size, intptr_t alignment) {
   return align_size_down_(size, alignment);
 }
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/native/utilities/test_align.cpp	Wed Apr 12 13:05:59 2017 +0200
@@ -0,0 +1,145 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include "precompiled.hpp"
+#include "utilities/formatBuffer.hpp"
+#include "utilities/globalDefinitions.hpp"
+#include "unittest.hpp"
+
+#include <limits>
+
+// A few arbitrarily chosen values to test the align functions on.
+static uint64_t values[] = {1, 3, 10, 345, 1023, 1024, 1025, 23909034, INT_MAX, uint64_t(-1) / 2, uint64_t(-1) / 2 + 100, -1 };
+
+template <typename T>
+static T max_alignment() {
+  T max = std::numeric_limits<T>::max();
+  return max ^ (max >> 1);
+}
+
+#define log(...) SCOPED_TRACE(err_msg(__VA_ARGS__).buffer())
+
+template <typename T, typename A>
+static void test_alignments() {
+  log("### Test: %c" SIZE_FORMAT " " UINT64_FORMAT " : %c" SIZE_FORMAT " " UINT64_FORMAT " ###\n",
+      std::numeric_limits<T>::is_signed ? 's' : 'u', sizeof(T), (uint64_t)std::numeric_limits<T>::max(),
+      std::numeric_limits<A>::is_signed ? 's' : 'u', sizeof(A), (uint64_t)std::numeric_limits<A>::max());
+
+  ASSERT_LE((uint64_t)std::numeric_limits<T>::max(), (uint64_t)std::numeric_limits<intptr_t>::max()) << "The test assumes that casting to intptr_t will not truncate bits";
+
+  // Test all possible alignment values that fit in type A.
+  for (A alignment = max_alignment<A>(); alignment > 0; alignment >>= 1) {
+    log("=== Alignment: " UINT64_FORMAT " ===\n", (uint64_t)alignment);
+
+    for (size_t i = 0; i < ARRAY_SIZE(values); i++) {
+      log("--- Value: " UINT64_FORMAT "\n", values[i]);
+
+      // Test align up
+      const uint64_t up = align_size_up_(values[i], (uint64_t)alignment);
+      if (0 < up && up <= (uint64_t)std::numeric_limits<T>::max()) {
+        log("Testing align_up:   alignment: 0x" UINT64_FORMAT_X " value: 0x" UINT64_FORMAT_X " expected: 0x" UINT64_FORMAT_X "\n", (uint64_t)alignment, values[i], up);
+
+        T value = T(values[i]);
+
+        // Check against uint64_t version
+        ASSERT_EQ(align_size_up(value, alignment), (intptr_t)up);
+        // Check inline function vs macro
+        ASSERT_EQ(align_size_up(value, alignment), (intptr_t)align_size_up_(value, alignment));
+        // Sanity check
+        ASSERT_GE(align_size_up(value, alignment), (intptr_t)value);
+      }
+
+      // Test align down
+      const uint64_t down = align_size_down_(values[i], (uint64_t)alignment);
+      if (down <= (uint64_t)std::numeric_limits<T>::max()) {
+        log("Testing align_size_down: alignment: 0x" UINT64_FORMAT_X " value: 0x" UINT64_FORMAT_X " expected: 0x" UINT64_FORMAT_X "\n", (uint64_t)alignment, values[i], down);
+
+        T value = T(values[i]);
+
+        // Check against uint64_t version
+        ASSERT_EQ(align_size_down(value, alignment), (intptr_t)down);
+        // Check inline function vs macro
+        ASSERT_EQ(align_size_down(value, alignment), (intptr_t)align_size_down_(value, alignment));
+        // Sanity check
+        ASSERT_LE(align_size_down(value, alignment), (intptr_t)value);
+      }
+
+      // Test is aligned
+      const bool is = is_size_aligned_(values[i], (uint64_t)alignment);
+      if (values[i] <= (uint64_t)std::numeric_limits<T>::max()) {
+        log("Testing is_aligned: alignment: 0x" UINT64_FORMAT_X " value: 0x" UINT64_FORMAT_X " expected: %s\n", (uint64_t)alignment, values[i], is ? "true" : "false");
+
+        T value = T(values[i]);
+
+        // Check against uint64_t version
+        ASSERT_EQ(is_size_aligned(value, alignment), is);
+        // Check inline function vs macro
+        ASSERT_EQ(is_size_aligned(value, alignment), is_size_aligned_(value, alignment));
+      }
+    }
+  }
+}
+
+TEST(Align, functions_and_macros) {
+  // Test the alignment functions with different type combinations.
+
+  // The current implementation of the alignment functions use intptr_t
+  // as return and input parameter type. Therefore, we restrict the tested
+  // types on 32-bit platforms.
+#ifdef _LP64
+  test_alignments<int64_t, uint8_t>();
+  test_alignments<int64_t, uint16_t>();
+  test_alignments<int64_t, uint32_t>();
+  test_alignments<int64_t, int8_t>();
+  test_alignments<int64_t, int16_t>();
+  test_alignments<int64_t, int32_t>();
+  test_alignments<int64_t, int64_t>();
+
+  test_alignments<uint32_t, uint8_t>();
+  test_alignments<uint32_t, uint16_t>();
+  test_alignments<uint32_t, uint32_t>();
+  test_alignments<uint32_t, int8_t>();
+  test_alignments<uint32_t, int16_t>();
+  test_alignments<uint32_t, int32_t>();
+#endif
+
+  test_alignments<int32_t, uint8_t>();
+  test_alignments<int32_t, uint16_t>();
+  test_alignments<int32_t, int8_t>();
+  test_alignments<int32_t, int16_t>();
+  test_alignments<int32_t, int32_t>();
+
+  test_alignments<uint16_t, uint8_t>();
+  test_alignments<uint16_t, uint16_t>();
+  test_alignments<uint16_t, int8_t>();
+  test_alignments<uint16_t, int16_t>();
+
+  test_alignments<int16_t, uint8_t>();
+  test_alignments<int16_t, int8_t>();
+  test_alignments<int16_t, int16_t>();
+
+  test_alignments<uint8_t, int8_t>();
+  test_alignments<uint8_t, uint8_t>();
+
+  test_alignments<int8_t, int8_t>();
+}