mvhooren/JitCat

Segfault on Windows/MSVC in release builds when using a recent checkout of LLVM

mvhooren opened this issue · 8 comments

This is due to an incompatibility between LLVM compiled with C++14 and JitCat compiled with C++17. I have filed a bugreport with LLVM: https://bugs.llvm.org/show_bug.cgi?id=44131

Meanwhile a workaround is to find Compiler.h in the llvm repo and commenting out or delete the #ifdef __cpp_aligned_new blocks within the allocate_buffer and deallocate_buffer functions.
Like this:

inline void *allocate_buffer(size_t Size, size_t Alignment) {
  return ::operator new(Size
/*#ifdef __cpp_aligned_new
                        ,
                        std::align_val_t(Alignment)
#endif*/
  );
}

and

inline void deallocate_buffer(void *Ptr, size_t Size, size_t Alignment) {
  ::operator delete(Ptr
#ifdef __cpp_sized_deallocation
                    ,
                    Size
#endif
/*#ifdef __cpp_aligned_new
                    ,
                    std::align_val_t(Alignment)
#endif*/
  );
}
Zueuk commented

LLVM compiled with C++14 and JitCat compiled with C++17

Weird, I have them compiled this way too, and I'm not getting any problems.

LLVM compiled with C++14 and JitCat compiled with C++17

Weird, I have them compiled this way too, and I'm not getting any problems.

Interesting. For me this started happening when I switched the LLVM repository from their svn repo to their github repo. They have recently migrated to github. I still need to update the JitCat documentation on building LLVM because that refers to the svn repository. However, I think the issue that causes the segfault was introduced before they migrated so it is still strange that you don't see it happening. It only happends in release builds, not debug builds, are you running a release build?

Building LLVM with C++17 enabled fixes this as well. I have updated the LLVM build instructions and will now close this issue.

Zueuk commented

I got LLVM from github, and only changed HasCOFFComdatConstants to false, as your instruction (used to) say.
The release build seems to be working fine.

Zueuk commented

So I tried to rebuild LLVM without that fix.
Indeed it crashed with unpatched LLVM built using C++14, and unit tests worked with the C++17 LLVM build.

However, the BasicExample still crashes:

myConstantExpression: 42
aFloatingPointExpression: 42.0031
JIT session error: Duplicate definition of symbol '__real@40000000'
anotherFloatingPointExpression: 656.63

with "Frame not in module" error...

So I tried to rebuild LLVM without that fix.
Indeed it crashed with unpatched LLVM built using C++14, and unit tests worked with the C++17 LLVM build.

However, the BasicExample still crashes:

myConstantExpression: 42
aFloatingPointExpression: 42.0031
JIT session error: Duplicate definition of symbol '__real@40000000'
anotherFloatingPointExpression: 656.63

with "Frame not in module" error...

Thanks for reproducing the crash, good to know it's not just happening on my end.
I thought the "Duplicate definition of symbol" problem was fixed by now because I have not encountered it myself recently. This is another open issue with LLVM. See https://bugs.llvm.org/show_bug.cgi?id=40074

Today I removed the instructions for the workaround for this problem from the LLVM build instructions. I have now restored it. (Find MCAsmInfoCOFF.cpp and change HasCOFFComdatConstants to false.)

Does your BasicExample still crash when you re-apply that change? Or did that duplicate definition of symbol error appear despite HasCOFFComdatConstants being false?

Hmm I see what you mean, I have now reproduced that duplicate definition of symbol error. HasCoffComdatConstats is set to false. I will investigate. Made a mistake, it was set to true. Setting it back to false fixes the Duplicate definition of symbol issue for me.

Zueuk commented

Yes, patched C++17 build seems to be working fine.

Strangely enough, my release build crashes again if I rebuild LLVM with C++14 - but I'm pretty sure the same configuration worked fine yesterday, when I didn't know that it's not supposed to work.