Re: remaining sql/json patches

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() using the correct types.  I'm not exactly sure which particular argument is not handled correctly in the above code, but perhaps it'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 as follows:

                    /* 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 to a function that returns char *, but perhaps not.

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