Re: Don't clean up LLVM state when exiting in a bad way

On Tue, Sep 07, 2021 at 12:27:27PM -0700, Andres Freund wrote:
> Hi,
> 
> On 2021-08-18 15:00:59 +0000, Jelte Fennema wrote:
> > I ran into some segfaults when using Postgres that was compiled with LLVM
> > 7. According to the backtraces these crashes happened during the call to
> > llvm_shutdown, during cleanup after another out of memory condition. It
> > seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7)
> > when LLVM is left in bad state. I attached the relevant part of the
> > stacktrace to this email.
> 
> > With the attached patch these segfaults went away. The patch turns
> > llvm_shutdown into a no-op whenever the backend is exiting with an
> > error. Based on my understanding of the code this should be totally fine. No
> > memory should be leaked, since all memory will be cleaned up anyway once the
> > backend exits shortly after. The only reason this cleanup code even seems to
> > exist at all is to get useful LLVM profiling data. To me it seems be
> > acceptable if the profiling data is incorrect/missing when the backend exits
> > with an error.
> 
> I think this is a tad too strong. We should continue to clean up on exit as
> long as the error didn't happen while we're already inside llvm
> code. Otherwise we loose some ability to find leaks. How about checking in the
> error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
> that case? Because that's precisely when it should be unsafe to reenter LLVM.

This avoids a crash when compiled with llvm7+clang7 on RH7 on master:

python3 -c "import pg; db=pg.DB('dbname=postgres host=/tmp port=5678'); db.query('SET jit_above_cost=0; SET jit_inline_above_cost=-1; SET jit=on; SET client_min_messages=debug'); db.query('begin'); db.query_formatted('SELECT 1 FROM generate_series(1,99)a WHERE a=%s', [1], inline=False);"

diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index df691cbf1c..a3869ee700 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -885,6 +885,12 @@ llvm_session_initialize(void)
 static void
 llvm_shutdown(int code, Datum arg)
 {
+	extern int fatal_new_handler_depth;
+
+	// if (code!=0)
+	if (fatal_new_handler_depth > 0)
+		return;
+
 #if LLVM_VERSION_MAJOR > 11
 	{
 		if (llvm_opt3_orc)
diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp
index 26bc828875..802dc1b058 100644
--- a/src/backend/jit/llvm/llvmjit_error.cpp
+++ b/src/backend/jit/llvm/llvmjit_error.cpp
@@ -24,7 +24,7 @@ extern "C"
 #include "jit/llvmjit.h"
 
 
-static int fatal_new_handler_depth = 0;
+int fatal_new_handler_depth = 0;
 static std::new_handler old_new_handler = NULL;
 
 static void fatal_system_new_handler(void);