Fix UB in MCJIT test cases that relied on union type punning

[aaron.ballman]

Thank you for taking care of this! Generally, LGTM, but I do wonder if it might make sense to make something like (totally untested):

template <typename InTy, typename OutTy>
static inline OutTy safe_cast_fn_ptr(InTy fn) {

return reinterpret_cast<OutTy>(reinterpret_cast<uintptr_t>(fn));

}

And use it in place of the extra type casting, since there's a fair amount of it in this test file?

~Aaron

[dblaikie]

Yeah, I'd be OK with that, for sure. (probably add some checks to ensure
the source/dest types are pointers, probably, somehow, maybe)

• msg-21295-403.txt162 BDownload

[lhames]

Apologies for the late reply - I've out on vacation.

This looks great. Thanks for the fix Dave!

unittests/ExecutionEngine/MCJIT/MCJITCAPITest.cpp

 TEST_F(MCJITCAPITest, simple_function) {
   SKIP_UNSUPPORTED_PLATFORM;
   
   buildSimpleFunction();
   buildMCJITOptions();
   buildMCJITEngine();
   buildAndRunPasses();
   
-  union {
-    void *raw;
-    int (*usable)();
-  } functionPointer;
-  functionPointer.raw = LLVMGetPointerToGlobal(Engine, Function);
-  
-  EXPECT_EQ(42, functionPointer.usable());
+  auto *functionPointer = reinterpret_cast<int (*)()>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function)));
+  EXPECT_EQ(42, functionPointer());
 }
 
 TEST_F(MCJITCAPITest, gva) {
   SKIP_UNSUPPORTED_PLATFORM;
 
   Module = LLVMModuleCreateWithName("simple_module");
   LLVMSetTarget(Module, HostTriple.c_str());
   LLVMValueRef GlobalVar = LLVMAddGlobal(Module, LLVMInt32Type(), "simple_value");

 TEST_F(MCJITCAPITest, custom_memory_manager) {
   SKIP_UNSUPPORTED_PLATFORM;
   
   buildSimpleFunction();
   buildMCJITOptions();
   useRoundTripSectionMemoryManager();
   buildMCJITEngine();
   buildAndRunPasses();
-  
-  union {
-    void *raw;
-    int (*usable)();
-  } functionPointer;
-  functionPointer.raw = LLVMGetPointerToGlobal(Engine, Function);
-  
-  EXPECT_EQ(42, functionPointer.usable());
+  auto *functionPointer = reinterpret_cast<int (*)()>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function)));
+  EXPECT_EQ(42, functionPointer());
   EXPECT_TRUE(didCallAllocateCodeSection);
 }
 
 TEST_F(MCJITCAPITest, stackmap_creates_compact_unwind_on_darwin) {
   SKIP_UNSUPPORTED_PLATFORM;
   
   // This test is also not supported on non-x86 platforms.
   if (Triple(HostTriple).getArch() != Triple::x86_64)
     return;
   
   buildFunctionThatUsesStackmap();
   buildMCJITOptions();
   useRoundTripSectionMemoryManager();
   buildMCJITEngine();
   buildAndRunOptPasses();
-  
-  union {
-    void *raw;
-    int (*usable)();
-  } functionPointer;
-  functionPointer.raw = LLVMGetPointerToGlobal(Engine, Function);
-  
-  EXPECT_EQ(42, functionPointer.usable());
+  auto *functionPointer = reinterpret_cast<int (*)()>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function)));
+  EXPECT_EQ(42, functionPointer());
   EXPECT_TRUE(didCallAllocateCodeSection);
   
   // Up to this point, the test is specific only to X86-64. But this next
   // expectation is only valid on Darwin because it assumes that unwind
   // data is made available only through compact_unwind. It would be
   // worthwhile to extend this to handle non-Darwin platforms, in which
   // case you'd want to look for an eh_frame or something.
   //
TEST_F(MCJITCAPITest, reserve_allocation_space) {
   
   TestReserveAllocationSpaceMemoryManager* MM = new TestReserveAllocationSpaceMemoryManager();
   
   buildModuleWithCodeAndData();
   buildMCJITOptions();
   Options.MCJMM = wrap(MM);
   buildMCJITEngine();
   buildAndRunPasses();
-  
-  union {
-    void *raw;
-    int (*usable)();
-  } GetGlobalFct;
-  GetGlobalFct.raw = LLVMGetPointerToGlobal(Engine, Function);
-  
-  union {
-    void *raw;
-    void (*usable)(int);
-  } SetGlobalFct;
-  SetGlobalFct.raw = LLVMGetPointerToGlobal(Engine, Function2);
-  
-  SetGlobalFct.usable(789);
-  EXPECT_EQ(789, GetGlobalFct.usable());
+  auto GetGlobalFct = reinterpret_cast<int (*)()>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function)));
+  auto SetGlobalFct = reinterpret_cast<void (*)(int)>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function2)));
+
+  SetGlobalFct(789);
+  EXPECT_EQ(789, GetGlobalFct());
   EXPECT_LE(MM->UsedCodeSize, MM->ReservedCodeSize);
   EXPECT_LE(MM->UsedDataSizeRO, MM->ReservedDataSizeRO);
   EXPECT_LE(MM->UsedDataSizeRW, MM->ReservedDataSizeRW);
   EXPECT_TRUE(MM->UsedCodeSize > 0); 
   EXPECT_TRUE(MM->UsedDataSizeRW > 0);
 }
 
 TEST_F(MCJITCAPITest, yield) {
   SKIP_UNSUPPORTED_PLATFORM;
 
   buildSimpleFunction();
   buildMCJITOptions();
   buildMCJITEngine();
   LLVMContextRef C = LLVMGetGlobalContext();
   LLVMContextSetYieldCallback(C, yield, nullptr);
   buildAndRunPasses();
 
-  union {
-    void *raw;
-    int (*usable)();
-  } functionPointer;
-  functionPointer.raw = LLVMGetPointerToGlobal(Engine, Function);
+  auto *functionPointer = reinterpret_cast<int (*)()>(
+      reinterpret_cast<uintptr_t>(LLVMGetPointerToGlobal(Engine, Function)));
 
-  EXPECT_EQ(42, functionPointer.usable());
+  EXPECT_EQ(42, functionPointer());
   EXPECT_TRUE(didCallYield);
 }
 
 static int localTestFunc() {
   return 42;
 }
 
 TEST_F(MCJITCAPITest, addGlobalMapping) {
TEST_F(MCJITCAPITest, addGlobalMapping) {
   LLVMDisposeBuilder(Builder);
 
   LLVMVerifyModule(Module, LLVMAbortProcessAction, &Error);
   LLVMDisposeMessage(Error);
 
   buildMCJITOptions();
   buildMCJITEngine();
 
-  union {
-    int (*raw)();
-    void *usable;
-  } functionPointer;
-  functionPointer.raw = &localTestFunc;
-
-  LLVMAddGlobalMapping(Engine, MappedFn, functionPointer.usable);
+  LLVMAddGlobalMapping(
+      Engine, MappedFn,
+      reinterpret_cast<void *>(reinterpret_cast<uintptr_t>(&localTestFunc)));
 
   buildAndRunPasses();
 
   uint64_t raw = LLVMGetFunctionAddress(Engine, "test_fn");
   int (*usable)() = (int (*)()) raw;
 
   EXPECT_EQ(42, usable());
 }