Thread: Header checking failures on LLVM-less machines
Since the LLVM stuff went in, src/tools/pginclude/cpluspluscheck fails on my main devel machine, because In file included from /tmp/cpluspluscheck.jobsM6/test.cpp:3: ./src/include/jit/llvmjit_emit.h:13:25: error: llvm-c/Core.h: No such file or directory and then there's a bunch of ensuing spewage due to missing typedefs etc. llvmjit.h has the same problem plus this additional pointless aggravation: In file included from /tmp/cpluspluscheck.jobsM6/test.cpp:3: ./src/include/jit/llvmjit.h:15:2: error: #error "llvmjit.h should only be included by code dealing with llvm" I propose that we should fix this by making the whole bodies of those two headers be #ifdef USE_LLVM. regards, tom lane
I wrote: > Since the LLVM stuff went in, src/tools/pginclude/cpluspluscheck > fails on my main devel machine, because Actually, it also fails on another machine that does have LLVM installed: In file included from /tmp/cpluspluscheck.LqnoZj/test.cpp:3: ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_ptr_const(void*, LLVMTypeRef)': ./src/include/jit/llvmjit_emit.h:22:32: error: 'TypeSizeT' was not declared in this scope LLVMValueRef c = LLVMConstInt(TypeSizeT, (uintptr_t) ptr, false); ^~~~~~~~~ ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_sizet_const(size_t)': ./src/include/jit/llvmjit_emit.h:78:22: error: 'TypeSizeT' was not declared in this scope return LLVMConstInt(TypeSizeT, i, false); ^~~~~~~~~ ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_sbool_const(bool)': ./src/include/jit/llvmjit_emit.h:87:22: error: 'TypeStorageBool' was not declared in this scope return LLVMConstInt(TypeStorageBool, (int) i, false); ^~~~~~~~~~~~~~~ ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_pbool_const(bool)': ./src/include/jit/llvmjit_emit.h:96:22: error: 'TypeParamBool' was not declared in this scope return LLVMConstInt(TypeParamBool, (int) i, false); ^~~~~~~~~~~~~ ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_mcxt_switch(LLVMModuleRef, LLVMBuilderRef, LLVMValueRef)': ./src/include/jit/llvmjit_emit.h:205:34: error: 'StructMemoryContextData' was not declared in this scope cur = LLVMAddGlobal(mod, l_ptr(StructMemoryContextData), cmc); ^~~~~~~~~~~~~~~~~~~~~~~ ./src/include/jit/llvmjit_emit.h:205:34: note: suggested alternative: 'MemoryContextData' cur = LLVMAddGlobal(mod, l_ptr(StructMemoryContextData), cmc); ^~~~~~~~~~~~~~~~~~~~~~~ MemoryContextData ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_funcnullp(LLVMBuilderRef, LLVMValueRef, size_t)': ./src/include/jit/llvmjit_emit.h:223:9: error: 'FIELDNO_FUNCTIONCALLINFODATA_ARGS' was not declared in this scope FIELDNO_FUNCTIONCALLINFODATA_ARGS, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_funcvaluep(LLVMBuilderRef, LLVMValueRef, size_t)': ./src/include/jit/llvmjit_emit.h:241:9: error: 'FIELDNO_FUNCTIONCALLINFODATA_ARGS' was not declared in this scope FIELDNO_FUNCTIONCALLINFODATA_ARGS, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Evidently, llvmjit_emit.h doesn't meet the project standard that says it should be includable standalone (to ensure that header inclusion order isn't important in .c files). It looks like it needs to #include llvmjit.h and fmgr.h to satisfy these references. Is there a good reason why this wasn't done? regards, tom lane
On 2019-01-28 19:51:22 -0500, Tom Lane wrote: > On 2019-01-28 11:19:21 -0500, Tom Lane wrote: > > Since the LLVM stuff went in, src/tools/pginclude/cpluspluscheck > > fails on my main devel machine, because > > > > In file included from /tmp/cpluspluscheck.jobsM6/test.cpp:3: > > ./src/include/jit/llvmjit_emit.h:13:25: error: llvm-c/Core.h: No such file or directory > > > > and then there's a bunch of ensuing spewage due to missing typedefs > > etc. llvmjit.h has the same problem plus this additional pointless > > aggravation: > > > > In file included from /tmp/cpluspluscheck.jobsM6/test.cpp:3: > > ./src/include/jit/llvmjit.h:15:2: error: #error "llvmjit.h should only be included by code dealing with llvm" > > > > I propose that we should fix this by making the whole bodies of those > > two headers be #ifdef USE_LLVM. Hm, I think it's sufficient that we error out if llvm was configured, we've sufficient buildfarm machines running with it enabled. > Actually, it also fails on another machine that does have LLVM installed: > > In file included from /tmp/cpluspluscheck.LqnoZj/test.cpp:3: > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_ptr_const(void*, LLVMTypeRef)': > ./src/include/jit/llvmjit_emit.h:22:32: error: 'TypeSizeT' was not declared in this scope > LLVMValueRef c = LLVMConstInt(TypeSizeT, (uintptr_t) ptr, false); > ^~~~~~~~~ > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_sizet_const(size_t)': > ./src/include/jit/llvmjit_emit.h:78:22: error: 'TypeSizeT' was not declared in this scope > return LLVMConstInt(TypeSizeT, i, false); > ^~~~~~~~~ > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_sbool_const(bool)': > ./src/include/jit/llvmjit_emit.h:87:22: error: 'TypeStorageBool' was not declared in this scope > return LLVMConstInt(TypeStorageBool, (int) i, false); > ^~~~~~~~~~~~~~~ > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_pbool_const(bool)': > ./src/include/jit/llvmjit_emit.h:96:22: error: 'TypeParamBool' was not declared in this scope > return LLVMConstInt(TypeParamBool, (int) i, false); > ^~~~~~~~~~~~~ > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_mcxt_switch(LLVMModuleRef, LLVMBuilderRef, LLVMValueRef)': > ./src/include/jit/llvmjit_emit.h:205:34: error: 'StructMemoryContextData' was not declared in this scope > cur = LLVMAddGlobal(mod, l_ptr(StructMemoryContextData), cmc); > ^~~~~~~~~~~~~~~~~~~~~~~ > ./src/include/jit/llvmjit_emit.h:205:34: note: suggested alternative: 'MemoryContextData' > cur = LLVMAddGlobal(mod, l_ptr(StructMemoryContextData), cmc); > ^~~~~~~~~~~~~~~~~~~~~~~ > MemoryContextData > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_funcnullp(LLVMBuilderRef, LLVMValueRef, size_t)': > ./src/include/jit/llvmjit_emit.h:223:9: error: 'FIELDNO_FUNCTIONCALLINFODATA_ARGS' was not declared in this scope > FIELDNO_FUNCTIONCALLINFODATA_ARGS, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ./src/include/jit/llvmjit_emit.h: In function 'LLVMOpaqueValue* l_funcvaluep(LLVMBuilderRef, LLVMValueRef, size_t)': > ./src/include/jit/llvmjit_emit.h:241:9: error: 'FIELDNO_FUNCTIONCALLINFODATA_ARGS' was not declared in this scope > FIELDNO_FUNCTIONCALLINFODATA_ARGS, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Evidently, llvmjit_emit.h doesn't meet the project standard that says > it should be includable standalone (to ensure that header inclusion > order isn't important in .c files). It looks like it needs to #include > llvmjit.h and fmgr.h to satisfy these references. Is there a good > reason why this wasn't done? Not really a good one - it's not really meant as an API just a collection of mini helpers for codegen, but there's no reason not to make it self sufficient. Will make them so. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2019-01-28 19:51:22 -0500, Tom Lane wrote: >> I propose that we should fix this by making the whole bodies of those >> two headers be #ifdef USE_LLVM. > Hm, I think it's sufficient that we error out if llvm was configured, > we've sufficient buildfarm machines running with it enabled. That is not the point. The point is that you've broken a developer tool. I don't really care whether cpluspluscheck would work on some subset of buildfarm machines --- what I need is for it to work on *my* machine. It is not any more okay for the llvm headers to fail like this than it would be for libxml- or openssl- or Windows-dependent headers to fail on machines lacking respective infrastructure. (And yes, I realize that that means that cpluspluscheck can only check a subset of the header declarations on any particular machine. But there's no way around that.) regards, tom lane
Hi, On 2019-01-28 20:21:49 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-01-28 19:51:22 -0500, Tom Lane wrote: > >> I propose that we should fix this by making the whole bodies of those > >> two headers be #ifdef USE_LLVM. > > > Hm, I think it's sufficient that we error out if llvm was configured, > > we've sufficient buildfarm machines running with it enabled. > > That is not the point. The point is that you've broken a developer > tool. I don't really care whether cpluspluscheck would work on > some subset of buildfarm machines --- what I need is for it to work > on *my* machine. It is not any more okay for the llvm headers to fail > like this than it would be for libxml- or openssl- or Windows-dependent > headers to fail on machines lacking respective infrastructure. > > (And yes, I realize that that means that cpluspluscheck can only > check a subset of the header declarations on any particular machine. > But there's no way around that.) I don't think we are actually contradicting each other. The aim of that error was to prevent pieces of code that aren't conditional on --with-llvm from including llvmjit.h. I was, for a moment and wrongly, thinking that we could style the header in a way that'd make it both for safe for cpluspluscheck and still error in that case, but that was a brainfart. But even leaving that brainfart aside, I'm not arguing against adding those include guards, so ...? I think the raison d'etre for that error has shrunk considerably anyway - it was introduced before the LLVM including/linking code was separated into it's own .so / directory. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > I don't think we are actually contradicting each other. The aim of that > error was to prevent pieces of code that aren't conditional on > --with-llvm from including llvmjit.h. I was, for a moment and wrongly, > thinking that we could style the header in a way that'd make it both for > safe for cpluspluscheck and still error in that case, but that was a > brainfart. But even leaving that brainfart aside, I'm not arguing > against adding those include guards, so ...? Works for me now. Thanks for fixing. regards, tom lane