Re: remaining sql/json patches

On Mon, Oct 2, 2023 at 2:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Mon, Oct 2, 2023 at 1:24 PM Amit Langote <amitlangote09@gmail.com> 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]:
> >
> > #4  0x00007f5bceb673d5 in __assert_fail_base (fmt=0x7f5bcecdbdc8
> > "%s%s%s:%u: %s%sAssertion `%s' failed.\\n%n",
> > assertion=assertion@entry=0x7f5bc1336419 "(i >= FTy->getNumParams() ||
> > FTy->getParamType(i) == Args[i]->getType()) && \\"Calling a function
> > with a bad signature!\\"", file=file@entry=0x7f5bc1336051
> > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp",
> > line=line@entry=299, function=function@entry=0x7f5bc13362af "void
> > llvm::CallInst::init(llvm::FunctionType *, llvm::Value *,
> > ArrayRef<llvm::Value *>, ArrayRef<llvm::OperandBundleDef>, const
> > llvm::Twine &)") at ./assert/assert.c:92
> > #5  0x00007f5bceb763a2 in __assert_fail (assertion=0x7f5bc1336419 "(i
> > >= FTy->getNumParams() || FTy->getParamType(i) == Args[i]->getType())
> > && \\"Calling a function with a bad signature!\\"",
> > file=0x7f5bc1336051
> > "/home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp", line=299,
> > function=0x7f5bc13362af "void llvm::CallInst::init(llvm::FunctionType
> > *, llvm::Value *, ArrayRef<llvm::Value *>,
> > ArrayRef<llvm::OperandBundleDef>, const llvm::Twine &)") at
> > ./assert/assert.c:101
> > #6  0x00007f5bc110f138 in llvm::CallInst::init (this=0x557a91f3e508,
> > FTy=0x557a91ed9ae0, Func=0x557a91f8be88, Args=..., Bundles=...,
> > NameStr=...) at
> > /home/bf/src/llvm-project-5/llvm/lib/IR/Instructions.cpp:297
> > #7  0x00007f5bc0fa579d in llvm::CallInst::CallInst
> > (this=0x557a91f3e508, Ty=0x557a91ed9ae0, Func=0x557a91f8be88,
> > Args=..., Bundles=..., NameStr=..., InsertBefore=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1934
> > #8  0x00007f5bc0fa538c in llvm::CallInst::Create (Ty=0x557a91ed9ae0,
> > Func=0x557a91f8be88, Args=..., Bundles=..., NameStr=...,
> > InsertBefore=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/Instructions.h:1444
> > #9  0x00007f5bc0fa51f9 in llvm::IRBuilder<llvm::ConstantFolder,
> > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > FTy=0x557a91ed9ae0, Callee=0x557a91f8be88, Args=..., Name=...,
> > FPMathTag=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1669
> > #10 0x00007f5bc100edda in llvm::IRBuilder<llvm::ConstantFolder,
> > llvm::IRBuilderDefaultInserter>::CreateCall (this=0x557a91f9c6a0,
> > Callee=0x557a91f8be88, Args=..., Name=..., FPMathTag=0x0) at
> > /home/bf/src/llvm-project-5/llvm/include/llvm/IR/IRBuilder.h:1663
> > #11 0x00007f5bc100714e in LLVMBuildCall (B=0x557a91f9c6a0,
> > Fn=0x557a91f8be88, Args=0x7ffde6fa0b50, NumArgs=6, Name=0x7f5bc30b648c
> > "funccall_iocoerce_in_safe") at
> > /home/bf/src/llvm-project-5/llvm/lib/IR/Core.cpp:2964
> > #12 0x00007f5bc30af861 in llvm_compile_expr (state=0x557a91fbeac0) at
> > /home/bf/bf-build/pogona/HEAD/pgsql.build/../pgsql/src/backend/jit/llvm/llvmjit_expr.c:1373
> >
> > 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), "");
> > +                   }
>
> Although most animals except pogona looked fine, I've decided to revert the patch for now.
>
> IIUC, LLVM is complaining that the code in the above block is not passing the arguments of InputFunctionCallSafe()
usingthe correct types.  I'm not exactly sure which particular argument is not handled correctly in the above code, but
perhapsit's: 
>
>
> +                       v_params[1] = v_output;
>
> which maps to char *str argument of InputFunctionCallSafe().  v_output is set in the code preceding the above block
asfollows: 
>
>                     /* and call output function (can never return NULL) */
>                     v_output = LLVMBuildCall(b, v_fn_out, &v_fcinfo_out,
>                                              1, "funccall_coerce_out");
>
> I thought that it would be fine to pass it as-is to the call of InputFunctionCallSafe() given that v_fn_out is a call
toa function that returns char *, but perhaps not. 

OK, I think I could use some help from LLVM experts here.

So, the LLVM code involving setting up a call to
InputFunctionCallSafe() seems to *work*, but BF animal pogona's debug
build (?) is complaining that the parameter types don't match up.
Parameters are set up as follows:

+                   {
+                       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));
 +                       /*
+                        * 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), "");
+                   }

And here's InputFunctionCallSafe's signature:

bool
InputFunctionCallSafe(FmgrInfo *flinfo, Datum d,
                      Oid typioparam, int32 typmod,
                      fmNodePtr escontext,
                      Datum *result)

I suspected that assignment to either param[1] or param[5] might be wrong.

param[1] in InputFunctionCallSafe's signature is char *, but the code
assigns it v_output, which is an LLVMValueRef for the output
function's output, a Datum, so I thought LLVM's type checker is
complaining that I'm trying to pass the Datum to char * without
appropriate conversion.

param[5] in InputFunctionCallSafe's signature is Node *, but the above
code is assigning it an LLVMValueRef for iocoerce's escontext whose
type is ErrorSaveContext.

Maybe some other param is wrong.

I tried various ways to fix both, but with no success.  My way of
checking for failure is to disassemble the IR code in .bc files
(generated with jit_dump_bitcode) with llvm-dis and finding that it
gives me errors such as:

$ llvm-dis 58536.0.bc
llvm-dis: error: Invalid record (Producer: 'LLVM7.0.1' Reader: 'LLVM 7.0.1')

$ llvm-dis 58536.0.bc
llvm-dis: error: Invalid cast (Producer: 'LLVM7.0.1' Reader: 'LLVM 7.0.1')


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