Header checking failures on LLVM-less machines

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