Missing llvm_leave_fatal_on_oom() call

Started by Heikki Linnakangasalmost 3 years ago3 messages
#1Heikki Linnakangas
hlinnaka@iki.fi
1 attachment(s)

llvm_release_context() calls llvm_enter_fatal_on_oom(), but it never
calls llvm_leave_fatal_on_oom(). Isn't that a clear leak?

(spotted this while investigating
/messages/by-id/a53cacb0-8835-57d6-31e4-4c5ef196de1a@deepbluecap.com,
but it seems unrelated)

- Heikki

Attachments:

fix-fatal-on-oom-leak.patchtext/x-patch; charset=UTF-8; name=fix-fatal-on-oom-leak.patchDownload
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 312612115c..94c1d1276d 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -221,6 +221,8 @@ llvm_release_context(JitContext *context)
 	}
 	list_free(llvm_context->handles);
 	llvm_context->handles = NIL;
+
+	llvm_leave_fatal_on_oom();
 }
 
 /*
#2Daniel Gustafsson
daniel@yesql.se
In reply to: Heikki Linnakangas (#1)
Re: Missing llvm_leave_fatal_on_oom() call

On 21 Feb 2023, at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

llvm_release_context() calls llvm_enter_fatal_on_oom(), but it never calls llvm_leave_fatal_on_oom(). Isn't that a clear leak?

Not sure how much of a leak it is since IIUC LLVM just stores a function
pointer to our error handler, but I can't see a reason not clean it up here.
The attached fix LGTM and passes make check with jit_above_cost set to zero.

--
Daniel Gustafsson

#3Heikki Linnakangas
hlinnaka@iki.fi
In reply to: Daniel Gustafsson (#2)
Re: Missing llvm_leave_fatal_on_oom() call

On 04/07/2023 19:33, Daniel Gustafsson wrote:

On 21 Feb 2023, at 15:50, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

llvm_release_context() calls llvm_enter_fatal_on_oom(), but it never calls llvm_leave_fatal_on_oom(). Isn't that a clear leak?

Not sure how much of a leak it is since IIUC LLVM just stores a function
pointer to our error handler, but I can't see a reason not clean it up here.
The attached fix LGTM and passes make check with jit_above_cost set to zero.

Pushed to all live branches, thanks for the review!

--
Heikki Linnakangas
Neon (https://neon.tech)