Thread

Re: Minor LLVM cleanups

Matheus Alcantara <matheusssilv97@gmail.com> — 2025-12-02T18:53:55Z

Hi, I did a quick look at the patches and here are my comments.

On Fri Nov 28, 2025 at 12:41 AM -03, Thomas Munro wrote:
> 0001:  These days we handle LLVM API evolution with LLVM_VERSION_MAJOR
> guards.  These GDB and Perf support probes escaped recent garbage
> collection cycles by not being phrased like that.  Function probes are
> generally better for cross-platform variations and library build
> options that are exposed by function visibility, but in this case all
> supported versions have the functions, even when the relevant feature
> isn't enabled in LLVM.
>
LGTM

> 0002:  On my FreeBSD box (and presumably any non-Linux system), if I
> set jit_profiling_support=1 then LLVMCreatePerfJITEventListener() is a
> dummy function that returns NULL and we crash.
>
Just confirming that I tested this on MacOS and it also crashes.

> The attached just silently skips in that case.  If we raised an error
> instead I suppose it would have to be FATAL given the call site in a
> callback invoked by LLVM/C++.  We could work harder and teach the GUC
> to probe LLVM when you try to turn it on, but apparently no one tried
> to turn on perf on a system without perf in all these years...  Should
> the manual say that it's only available on Linux?  Would it be
> reasonable to additionally assume that __linux__ implies LLVM_USE_PERF
> and disable the GUC otherwise?
>
The patch looks good, it fix the crash and IMHO the documentation change
would be enough. On guc_parameter.dat we have the following comment that
I agree and make my point about why just the documentation change would
be enough:
    # This is not guaranteed to be available, but given it's a developer
    # oriented option, it doesn't seem worth adding code checking
    # availability.

> (There are more kinds of profiling support available, which I might
> learn more about as part of the JITLink work.)
>
You are referring to this patch [1]? I've noticed that this patch didn't
get any review yet, I'm still learning about this area of the code but I
can try to give a review and test it.

> 0003:  While contemplating how close we are to an empty
> llvmjit_wrap.cpp file, I considered whether the two wrappers added by
> commit 37d5babb should be upstreamed, and then realised that this one
> is not needed if you jump though one extra hoop.
>
LGTM

> 0004:  I *think* the second one is redundant too: all the functions in
> question are either global or we have a template function of the same
> type that is.  From a spartan trail of bread crumbs[1][2] I realised
> that we should be able to use LLVMGlobalGetValueType() instead.  make
> check with passes with TEMP_CONFIG set to define jit_above_cost=0
> against bleeding-edge LLVM built with
> -DLLVM_USE_SANITIZER="Address;Undefined" and
> -DLLVM_ENABLE_ASSERTIONS=ON.
>
I think that these includes can be removed 
#include "jit/llvmjit.h"
#include "jit/llvmjit_backport.h"

I also did some tests and I didn't find any issue with this change.

[1] https://www.postgresql.org/message-id/CA%2BhUKGJBJx4fDGLv8zUtmsmg16Swry7DJbMr2_GNZcd6sgE0rg%40mail.gmail.com

--
Matheus Alcantara
EDB: http://www.enterprisedb.com