MaybeUninit is not aligned, and mmtk_get_next_mutator should not use global variable

[wks]

In openjdk/mmtkUpcalls.cpp, there is a struct MaybeUninit<T> which is used to prevent the constructor to be called on a global variable. It is implemented with char _data[sizeof(T)] as its member.

However, char[sizeof(T)] does not necessarily have the same alignment requirement as T.

I added a static assertion and a run-time assertion:

diff --git a/openjdk/mmtkUpcalls.cpp b/openjdk/mmtkUpcalls.cpp
index a11e186..aaebfb1 100644
--- a/openjdk/mmtkUpcalls.cpp
+++ b/openjdk/mmtkUpcalls.cpp
@@ -118,7 +118,18 @@ private:
 static MaybeUninit<JavaThreadIteratorWithHandle> jtiwh;
 static bool mutator_iteration_start = true;
 
+static_assert(alignof(jtiwh) == alignof(JavaThreadIteratorWithHandle),
+        "jtiwh and JavaThreadIteratorWithHandle does not have the same alignment");
+
 static void* mmtk_get_next_mutator() {
+    if ((uintptr_t)&jtiwh % alignof(JavaThreadIteratorWithHandle) != 0) {
+        fprintf(stderr, "[mmtk_get_next_mutator] error: jtiwh is not aligned. jtiwh: %p, align: %zd\n",
+                &jtiwh, alignof(JavaThreadIteratorWithHandle));
+        exit(1);
+    } else {
+        fprintf(stderr, "[mmtk_get_next_mutator] OK: jtiwh is aligned. jtiwh: %p, align: %zd\n",
+                &jtiwh, alignof(JavaThreadIteratorWithHandle));
+    }
     if (mutator_iteration_start) {
         *jtiwh = JavaThreadIteratorWithHandle();
         mutator_iteration_start = false;

The static assertion fails. But if I comment out the static_assert, the run-time assertion always succeeds on my x86_64 machine running Linux. I did not find any ABI documentation that requires static variables (jtiwh) to have any particular alignment (such as word-aligned, 8-byte aligned, 16-byte aligned, ...). So there is a chance that jtiwh may be mis-aligned on some platform-compiler combinations, but not manifested yet. The proper way to avoid initializing a global variable is to use std::opitonal<T>, or manually implement it using a union if OpenJDK does not support C++17.

However, from a higher level, it is almost always bad to use global variables. mmtk_get_next_mutator and mmtk_reset_mutator_iterator depends on the global jtiwh variable to maintain the iterator between successive calls. In a reentrant API, they should take a parameter which points to an allocated JavaThreadIteratorWithHandle instance. It shouldn't be a problem to just new JavaThreadIteratorWithHandle and allocate it on the heap, I think. See opendir and readdir_r for example.

[qinsoon]

The type MaybeUninit is a bit strange. I thought it was mimicking the Rust MaybeUninit and we may pass a Rust MaybeUninit value to the c++ code. But it seems not the case. The alignment could lead to bugs.

For the global to store a iterator state, it is bad code, but it is not necessarily a bug for now. The mmtk-core does not require get_next_mutator() to be thread safe. https://github.com/mmtk/mmtk-core/blob/3d9861da1836ed7dd40778687aa732a121263491/src/vm/active_plan.rs#L57. But anyway, I think we should refactor the trait about mutator iterator. We should actually expose an iterator trait, let the bindings to implement an iterator and let the iterator to carry the state rather than using a global variable.

@javadamiri @wenyuzhao