Обсуждение: Minor LLVM cleanups

Hi,

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.

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.  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?

(There are more kinds of profiling support available, which I might
learn more about as part of the JITLink work.)

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.

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.

[1]
https://github.com/llvm/llvm-project/blob/06c8ee61ab80305be88380e6aa2f1b2fe32f859d/llvm/include/llvm-c/Core.h#L2672
[2]
https://github.com/llvm/llvm-project/blob/06c8ee61ab80305be88380e6aa2f1b2fe32f859d/llvm/include/llvm/IR/Function.h#L210

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

Hi,


On 2025-11-28 16:41:46 +1300, 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.

WFM.

> 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.  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?

> (There are more kinds of profiling support available, which I might
> learn more about as part of the JITLink work.)

LGTM.


> 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.


> 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.

Hm, I guess this reduces the sanity checking a tiny bit, because presumably
LLVMGlobalGetValueType() will also return non-function types?

I am not sure this buys us all that much?

Greetings,

Andres Freund