Subject: Re: remaining sql/json patches

From: Amit Langote <[email-masked]>
Date: Fri, 6 Oct 2023 18:23:37 +0900

On Wed, Oct 4, 2023 at 10:26 PM Amit Langote <[email-masked]> wrote:
> On Tue, Oct 3, 2023 at 10:11 PM Amit Langote <[email-masked]> wrote:
> > On Mon, Oct 2, 2023 at 2:26 PM Amit Langote <[email-masked]> wrote:
> > > On Mon, Oct 2, 2023 at 1:24 PM Amit Langote <[email-masked]> wrote:
> > > > Pushed this 30 min ago (no email on -committers yet!) and am looking
> > > > at the following llvm crash reported by buildfarm animal pogona [1]:
> > > > This seems to me to be complaining about the following addition:
> > > >
> > > > +                   {
> > > > +                       Oid         ioparam = op->d.iocoerce.typioparam;
> > > > +                       LLVMValueRef v_params[6];
> > > > +                       LLVMValueRef v_success;
> > > > +
> > > > +                       v_params[0] = l_ptr_const(op->d.iocoerce.finfo_in,
> > > > +                                                 l_ptr(StructFmgrInfo));
> > > > +                       v_params[1] = v_output;
> > > > +                       v_params[2] = l_oid_const(lc, ioparam);
> > > > +                       v_params[3] = l_int32_const(lc, -1);
> > > > +                       v_params[4] = l_ptr_const(op->d.iocoerce.escontext,
> > > > +
> > > > l_ptr(StructErrorSaveContext));
> > > >
> > > > -                   LLVMBuildStore(b, v_retval, v_resvaluep);
> > > > +                       /*
> > > > +                        * InputFunctionCallSafe() will write directly into
> > > > +                        * *op->resvalue.
> > > > +                        */
> > > > +                       v_params[5] = v_resvaluep;
> > > > +
> > > > +                       v_success = LLVMBuildCall(b, llvm_pg_func(mod,
> > > > "InputFunctionCallSafe"),
> > > > +                                                 v_params, lengthof(v_params),
> > > > +                                                 "funccall_iocoerce_in_safe");
> > > > +
> > > > +                       /*
> > > > +                        * Return null if InputFunctionCallSafe() encountered
> > > > +                        * an error.
> > > > +                        */
> > > > +                       v_resnullp = LLVMBuildICmp(b, LLVMIntEQ, v_success,
> > > > +                                                  l_sbool_const(0), "");
> > > > +                   }
> > >
> ...I haven't yet pinpointed down
> which of the LLVM's asserts it is, nor have I been able to walk
> through LLVM source code using gdb to figure what the new code is
> doing wrong.  Maybe I'm still missing a trick or two...

I finally managed to analyze the crash by getting the correct LLVM build.

So the following bits are the culprits:

1. v_output needed to be converted from being reference to a Datum to
be reference to char * as follows before passing to
InputFunctionCallSafe():

-                       v_params[1] = v_output;
+                       v_params[1] = LLVMBuildIntToPtr(b, v_output,
+
l_ptr(LLVMInt8TypeInContext(lc)),
+                                                       "");

2. Assignment of op->d.iocoerce.escontext needed to be changed like this:

                        v_params[4] = l_ptr_const(op->d.iocoerce.escontext,
-
l_ptr(StructErrorSaveContext));
+                                                 l_ptr(StructNode));

3. v_success needed to be "zero-extended" to match in type with
whatever s_bool_const() produces, as follows:

+                       v_success = LLVMBuildZExt(b, v_success,
TypeStorageBool, "");
                        v_resnullp = LLVMBuildICmp(b, LLVMIntEQ, v_success,
                                                   l_sbool_const(0), "");

No more crashes with the above fixes.

Attached shows the delta against the patch I reverted.  I'll push the
fixed up version on Monday.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Alvaro Herrera <[email-masked]>
Date: Fri, 6 Oct 2023 12:01:05 +0200

> l_ptr(StructErrorSaveContext));

Oh, so you had to go back to using StructNode in order to get this
fixed?  That's weird.  Is it just because InputFunctionCallSafe is
defined to take fmNodePtr?  (I still fail to see that a pointer to
ErrorSaveContext would differ in any material way from a pointer to
Node).

Another think I thought was weird is that it would only crash in LLVM5
debug and not the other LLVM-enabled animals, but looking closer at the
buildfarm results, I think that may have been only because you reverted
too quickly, and phycodorus and petalura didn't actually run with
7fbc75b26ed8 before you reverted it.  Dragonet did make a run with it,
but it's marked as "LLVM optimized" instead of "LLVM debug".  I suppose
that must be making a difference.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"World domination is proceeding according to plan"        (Andrew Morton)

---

From: Amit Langote <[email-masked]>
Date: Fri, 6 Oct 2023 20:27:12 +0900

> ErrorSaveContext would differ in any material way from a pointer to

The difference matters to LLVM’s type system, which considers Node to be a
type with 1 sub-type (struct member) and ErrorSaveContext with 4 sub-types.
It doesn’t seem to understand that both share the first member.

> that must be making a difference.

AFAICS, only assert-enabled LLVM builds crash.