[PATCH] Fix build with LLVM 15 or above

On Mon, Oct 3, 2022 at 4:56 PM Po-Chuan Hsieh <sunpoet@sunpoet.net> wrote:

While building PostgreSQL 15 RC 1 with LLVM 15, I got a build failure as follows:

cc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -O2 -pipe -O3 -funroll-loops -fstack-protector-strong -fno-strict-aliasing -Wno-deprecated-declarations -fPIC -DPIC -D__STDC_LIMIT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_CONSTANT_MACROS -I/usr/local/llvm15/include -I../../../../src/include -I/usr/local/include -I/usr/local/include -I/usr/local/include/libxml2 -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -c -o llvmjit.o llvmjit.c
llvmjit.c:1115:50: error: use of undeclared identifier 'LLVMJITCSymbolMapPair'
LLVMOrcCSymbolMapPairs symbols = palloc0(sizeof(LLVMJITCSymbolMapPair) * LookupSetSize);
^
llvmjit.c:1233:81: error: too few arguments to function call, expected 3, have 2
ref_gen = LLVMOrcCreateCustomCAPIDefinitionGenerator(llvm_resolve_symbols, NULL);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
/usr/local/llvm15/include/llvm-c/Orc.h:997:31: note: 'LLVMOrcCreateCustomCAPIDefinitionGenerator' declared here
LLVMOrcDefinitionGeneratorRef LLVMOrcCreateCustomCAPIDefinitionGenerator(
^
2 errors generated.
gmake: *** [<builtin>: llvmjit.o] Error 1
*** Error code 2

I've prepared a patch (attached) to fix the build issue with LLVM 15 or above. It is also available at https://people.FreeBSD.org/~sunpoet/patch/postgres/0001-Fix-build-with-LLVM-15-or-above.patch

Hi,

Unfortunately that is only the tip of a mini iceberg. While that
change makes it compile, there are other API changes that are required
to make our use of LLVM ORC actually work. We can't get through 'make
check', because various code paths in LLVM 15 abort, because we're
using a bunch of APIs from before the big change to "opaque pointers"
https://llvm.org/docs/OpaquePointers.html. I've been trying to get to
a patch to fix that -- basically a few simple-looking changes like
LLVMBuildLoad() to LLVMBuildLoad2() as described there -- that gain an
argument where you have to tell it the type of the pointer (whereas
before it knew the type of pointers automatically). Unfortunately I
had to work on other problems that came up recently and it's probably
going to be at least a week before I can get back to this and post a
patch.

One option I thought about as a stopgap measure is to use
LLVMContextSetOpaquePointers(context, false) to turn the new code
paths off, but it doesn't seem to work for me and I couldn't figure
out why yet (it still aborts -- probably there are more 'contexts'
around that I didn't handle, something like that). That option is
available for LLVM 15 but will be taken out in LLVM 16, so that's
supposed to be the last chance to stop using pre-opaque pointers; see
the bottom of the page I linked above for that, where they call it
setOpaquePointers(false) (the C++ version of
LLVMContextSetOpaquePointers()). I don't really want to go with that
if we can avoid it, though, because it says "Opaque pointers are
enabled by default. Typed pointers are still available, but only
supported on a best-effort basis and may be untested" so I expect it
to be blighted with problems.

Here's my attempt at that minimal change, which is apparently still
missing something (if you can get this to build and pass all tests
against LLVM 15 then it might still be interesting to know about):

https://github.com/macdice/postgres/tree/llvm15-min

Here's my WIP unfinished branch where I'm trying to get the real code
change done. It needs more work on function pointer types, which are
a bit tedious to deal with and I haven't got it all right in here yet
as you can see from failures if you build against 15:

https://github.com/macdice/postgres/tree/llvm15

Hopefully more next week...

Hi,

On 2022-10-03 18:34:18 +1300, Thomas Munro wrote:

One option I thought about as a stopgap measure is to use
LLVMContextSetOpaquePointers(context, false) to turn the new code
paths off, but it doesn't seem to work for me and I couldn't figure
out why yet (it still aborts -- probably there are more 'contexts'
around that I didn't handle, something like that).

I think that's just because of this hunk:

@@ -992,7 +1000,12 @@ llvm_create_types(void)
}

     /* eagerly load contents, going to need it all */
+#if LLVM_VERSION_MAJOR > 14
+    if (LLVMParseBitcodeInContext2(LLVMOrcThreadSafeContextGetContext(llvm_ts_context),
+                                   buf, &llvm_types_module))
+#else
     if (LLVMParseBitcode2(buf, &llvm_types_module))
+#endif
     {
         elog(ERROR, "LLVMParseBitcode2 of %s failed", path);
     }

This is the wrong context to use here. Because of that we end up with types
from two different contexts being used, which leads to this assertion to fail:

#5 0x00007f945a036ab2 in __GI___assert_fail (
assertion=0x7f93cf5a4a1b "getOperand(0)->getType() == getOperand(1)->getType() && \"Both operands to ICmp instruction are not of the same type!\"",
file=0x7f93cf66062a "/home/andres/src/llvm-project/llvm/include/llvm/IR/Instructions.h", line=1191,
function=0x7f93cf5f2db6 "void llvm::ICmpInst::AssertOK()") at ./assert/assert.c:101
#6 0x00007f93cf9e3a3c in llvm::ICmpInst::AssertOK (this=0x56482c3b4b50) at /home/andres/src/llvm-project/llvm/include/llvm/IR/Instructions.h:1190
#7 0x00007f93cf9e38ca in llvm::ICmpInst::ICmpInst (this=0x56482c3b4b50, pred=llvm::CmpInst::ICMP_UGE, LHS=0x56482c3b98d0, RHS=0x56482c3b9920, NameStr="")
at /home/andres/src/llvm-project/llvm/include/llvm/IR/Instructions.h:1245
#8 0x00007f93cf9dc6f9 in llvm::IRBuilderBase::CreateICmp (this=0x56482c3b4650, P=llvm::CmpInst::ICMP_UGE, LHS=0x56482c3b98d0, RHS=0x56482c3b9920, Name="")
at /home/andres/src/llvm-project/llvm/include/llvm/IR/IRBuilder.h:2212
#9 0x00007f93cfa650cd in LLVMBuildICmp (B=0x56482c3b4650, Op=LLVMIntUGE, LHS=0x56482c3b98d0, RHS=0x56482c3b9920, Name=0x7f9459722cf2 "")
at /home/andres/src/llvm-project/llvm/lib/IR/Core.cpp:3883
#10 0x00007f945971b4d7 in llvm_compile_expr (state=0x56482c31f878) at /home/andres/src/postgresql/src/backend/jit/llvm/llvmjit_expr.c:302
#11 0x000056482a28f76b in jit_compile_expr (state=state@entry=0x56482c31f878) at /home/andres/src/postgresql/src/backend/jit/jit.c:177
#12 0x0000564829f44e62 in ExecReadyExpr (state=state@entry=0x56482c31f878) at /home/andres/src/postgresql/src/backend/executor/execExpr.c:885

because types (compared by pointer value) are only unique within a context.

I think all that is needed for this aspect would be:

#if LLVM_VERSION_MAJOR > 14
LLVMContextSetOpaquePointers(LLVMGetGlobalContext(), false);
#endif

I haven't yet run through the whole regression test with an assert enabled
llvm because an assert-enabled llvm is *SLOW*, but it got through the first
few parallel groups ok. Using an optimized llvm 15, all tests pass with
PGOPTIONS=-cjit_above_cost=0.

That option is available for LLVM 15 but will be taken out in LLVM 16, so
that's supposed to be the last chance to stop using pre-opaque pointers; see
the bottom of the page I linked above for that, where they call it
setOpaquePointers(false) (the C++ version of
LLVMContextSetOpaquePointers()). I don't really want to go with that if we
can avoid it, though, because it says "Opaque pointers are enabled by
default. Typed pointers are still available, but only supported on a
best-effort basis and may be untested" so I expect it to be blighted with
problems.

I think it'd be ok for the back branches, while we figure out the opaque stuff
in HEAD.

Greetings,

Andres Freund

Hi,

On 2022-10-03 12:16:12 -0700, Andres Freund wrote:

I haven't yet run through the whole regression test with an assert enabled
llvm because an assert-enabled llvm is *SLOW*, but it got through the first
few parallel groups ok. Using an optimized llvm 15, all tests pass with
PGOPTIONS=-cjit_above_cost=0.

That did pass. But to be able to use clang >= 15 one more piece is
needed. Updated patch attached.

Greetings,

Andres Freund

On Mon, Oct 3, 2022 at 2:41 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2022-10-03 12:16:12 -0700, Andres Freund wrote:

I haven't yet run through the whole regression test with an assert

enabled

llvm because an assert-enabled llvm is *SLOW*, but it got through the

first

few parallel groups ok. Using an optimized llvm 15, all tests pass with
PGOPTIONS=-cjit_above_cost=0.

That did pass. But to be able to use clang >= 15 one more piece is
needed. Updated patch attached.

Greetings,

Andres Freund

Hi,

+ * When targetting an llvm version with opaque pointers enabled by

I think `targetting` should be spelled as targeting

Cheers

On Tue, Oct 4, 2022 at 10:45 AM Zhihong Yu <zyu@yugabyte.com> wrote:

On Mon, Oct 3, 2022 at 2:41 PM Andres Freund <andres@anarazel.de> wrote:

On 2022-10-03 12:16:12 -0700, Andres Freund wrote:

I haven't yet run through the whole regression test with an assert enabled
llvm because an assert-enabled llvm is *SLOW*, but it got through the first
few parallel groups ok. Using an optimized llvm 15, all tests pass with
PGOPTIONS=-cjit_above_cost=0.

+    /*
+     * When targetting an llvm version with opaque pointers enabled by
+     * default, turn them off for the context we build our code in. Don't need
+     * to do so for other contexts (e.g. llvm_ts_context) - once the IR is
+     * generated, it carries the necessary information.
+     */
+#if LLVM_VERSION_MAJOR > 14
+    LLVMContextSetOpaquePointers(LLVMGetGlobalContext(), false);
+#endif

Ahh, right, thanks!

That did pass. But to be able to use clang >= 15 one more piece is
needed. Updated patch attached.

+ bitcode_cflags += ['-Xclang', '-no-opaque-pointers']

Oh, right. That makes sense.

I think `targetting` should be spelled as targeting

Yeah.

OK, I'll wait for the dust to settle on our 15 release and then
back-patch this. Then I'll keep working on the opaque pointer support
for master, which LLVM 16 will need (I expect we'll eventually want to
back-patch that eventually, but first things first...).

Hi Thomas,

On Tue, 2022-10-11 at 11:07 +1300, Thomas Munro wrote:

OK, I'll wait for the dust to settle on our 15 release and then
back-patch this.  Then I'll keep working on the opaque pointer
support for master, which LLVM 16 will need (I expect we'll
eventually want to back-patch that eventually, but first things
first...).

Fedora 37 is out very very soon, and ships with CLANG/LLVM 15. What is
the timeline for backpatching llvm15 support?

Thanks!

Cheers,
--
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

On Tue, Oct 18, 2022 at 10:01 PM Devrim Gündüz <devrim@gunduz.org> wrote:

On Tue, 2022-10-11 at 11:07 +1300, Thomas Munro wrote:

OK, I'll wait for the dust to settle on our 15 release and then
back-patch this. Then I'll keep working on the opaque pointer
support for master, which LLVM 16 will need (I expect we'll
eventually want to back-patch that eventually, but first things
first...).

Fedora 37 is out very very soon, and ships with CLANG/LLVM 15. What is
the timeline for backpatching llvm15 support?

Hi Devrim,

Will do first thing tomorrow.

Hi,

On Tue, 2022-10-18 at 22:06 +1300, Thomas Munro wrote:

Will do first thing tomorrow.

Just wanted to confirm that I pushed Fedora RPMs built against LLVM 15
by adding these patches.

Thanks Thomas.

Regards,
--
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

On Wed, Oct 26, 2022 at 4:28 AM Devrim Gündüz <devrim@gunduz.org> wrote:

On Tue, 2022-10-18 at 22:06 +1300, Thomas Munro wrote:

Will do first thing tomorrow.

Just wanted to confirm that I pushed Fedora RPMs built against LLVM 15
by adding these patches.

Thanks Thomas.

Cool.

FTR I still have to finish the 'real' fixes for LLVM 16. Their
cadence is one major release every 6 months, putting it at about April
'23, but I'll try to get it ready quite soon on our master branch. BF
animal seawasp is green again for now, but I expect it will turn back
to red pretty soon when they start ripping out the deprecated stuff on
their master branch...