Thread: LLVM compile failing in seawasp
Seawasp (using experimental clang 9.0) has been complaining of late:
/home/fabien/clgtk/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 -D__STDC_LIMIT_MACROS
-D__STDC_FORMAT_MACROS-D__STDC_CONSTANT_MACROS -D_DEBUG -D_GNU_SOURCE -I/home/fabien/clgtk/include
-I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -flto=thin -emit-llvm -c -o llvmjit_types.bc
llvmjit_types.c
In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
from llvmjit_inline.cpp:45:
/home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:12: error: macro "Min" requires 2 arguments, but only 1
given
: Min(Min), Scalable(Scalable) {}
^
In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
from llvmjit_inline.cpp:45:
/home/fabien/clgtk/include/llvm/Support/ScalableSize.h: In constructor
\xe2\x80\x98llvm::ElementCount::ElementCount(unsignedint, bool)\xe2\x80\x99:
/home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:13: error: expected \xe2\x80\x98(\xe2\x80\x99 before
\xe2\x80\x98,\xe2\x80\x99token
: Min(Min), Scalable(Scalable) {}
^
<builtin>: recipe for target 'llvmjit_inline.o' failed
This was working earlier, and as far as I can tell the cpluspluscheck
fixes are not the cause (because those happened earlier than the first
failure). Apparently clang got upgraded from "trunk 361691" to "trunk
362290" ... is the new clang broken?
--
Álvaro Herrera 39°50'S 73°21'W
Hi,
On 2019-06-06 13:32:16 -0400, Alvaro Herrera wrote:
> Seawasp (using experimental clang 9.0) has been complaining of late:
>
> /home/fabien/clgtk/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 -D__STDC_LIMIT_MACROS
-D__STDC_FORMAT_MACROS-D__STDC_CONSTANT_MACROS -D_DEBUG -D_GNU_SOURCE -I/home/fabien/clgtk/include
-I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -flto=thin -emit-llvm -c -o llvmjit_types.bc
llvmjit_types.c
> In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
> from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
> from llvmjit_inline.cpp:45:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:12: error: macro "Min" requires 2 arguments, but only 1
given
> : Min(Min), Scalable(Scalable) {}
> ^
> In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
> from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
> from llvmjit_inline.cpp:45:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h: In constructor
\xe2\x80\x98llvm::ElementCount::ElementCount(unsignedint, bool)\xe2\x80\x99:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:13: error: expected \xe2\x80\x98(\xe2\x80\x99 before
\xe2\x80\x98,\xe2\x80\x99token
> : Min(Min), Scalable(Scalable) {}
> ^
> <builtin>: recipe for target 'llvmjit_inline.o' failed
>
> This was working earlier, and as far as I can tell the cpluspluscheck
> fixes are not the cause (because those happened earlier than the first
> failure). Apparently clang got upgraded from "trunk 361691" to "trunk
> 362290" ... is the new clang broken?
I think that machine might also update llvm to a trunk checkout. Is that
right Fabien? If so that's possible "just" a minor API break.
Greetings,
Andres Freund
c.h defines a C Min macro conflicting with llvm new class
llvm:ElementCount Min member
On Thu, Jun 6, 2019 at 7:32 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> Seawasp (using experimental clang 9.0) has been complaining of late:
>
> /home/fabien/clgtk/bin/clang -Wno-ignored-attributes -fno-strict-aliasing -fwrapv -O2 -D__STDC_LIMIT_MACROS
-D__STDC_FORMAT_MACROS-D__STDC_CONSTANT_MACROS -D_DEBUG -D_GNU_SOURCE -I/home/fabien/clgtk/include
-I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -flto=thin -emit-llvm -c -o llvmjit_types.bc
llvmjit_types.c
> In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
> from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
> from llvmjit_inline.cpp:45:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:12: error: macro "Min" requires 2 arguments, but only 1
given
> : Min(Min), Scalable(Scalable) {}
> ^
> In file included from /home/fabien/clgtk/include/llvm/ADT/DenseMapInfo.h:20:0,
> from /home/fabien/clgtk/include/llvm/ADT/DenseMap.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/DenseSet.h:16,
> from /home/fabien/clgtk/include/llvm/ADT/SetVector.h:23,
> from llvmjit_inline.cpp:45:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h: In constructor
\xe2\x80\x98llvm::ElementCount::ElementCount(unsignedint, bool)\xe2\x80\x99:
> /home/fabien/clgtk/include/llvm/Support/ScalableSize.h:27:13: error: expected \xe2\x80\x98(\xe2\x80\x99 before
\xe2\x80\x98,\xe2\x80\x99token
> : Min(Min), Scalable(Scalable) {}
> ^
> <builtin>: recipe for target 'llvmjit_inline.o' failed
>
> This was working earlier, and as far as I can tell the cpluspluscheck
> fixes are not the cause (because those happened earlier than the first
> failure). Apparently clang got upgraded from "trunk 361691" to "trunk
> 362290" ... is the new clang broken?
>
> --
> Álvaro Herrera 39°50'S 73°21'W
>
>
>> failure). Apparently clang got upgraded from "trunk 361691" to "trunk >> 362290" ... is the new clang broken? > > I think that machine might also update llvm to a trunk checkout. Is that > right Fabien? Yes, the version is recompiled from sources on every Saturday. -- Fabien.
didier <did447@gmail.com> writes:
> c.h defines a C Min macro conflicting with llvm new class
> llvm:ElementCount Min member
Really? Well, we will hardly be the only code they broke with that.
I think we can just wait for them to reconsider.
regards, tom lane
On Fri, Jun 7, 2019 at 12:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > didier <did447@gmail.com> writes: > > c.h defines a C Min macro conflicting with llvm new class > > llvm:ElementCount Min member > > Really? Well, we will hardly be the only code they broke with that. > I think we can just wait for them to reconsider. FYI This is now on LLVM's release_90 branch, due out on August 28. -- Thomas Munro https://enterprisedb.com
>>> c.h defines a C Min macro conflicting with llvm new class >>> llvm:ElementCount Min member >> >> Really? Well, we will hardly be the only code they broke with that. >> I think we can just wait for them to reconsider. > > FYI This is now on LLVM's release_90 branch, due out on August 28. Maybe we should consider doing an explicit bug report, but I would not bet that they are going to fold… or fixing the issue pg side, eg "pg_Min", less than 400 hundred instances, and backpatch to all supported versions:-( -- Fabien.
On Sat, Jul 27, 2019 at 7:06 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > >>> c.h defines a C Min macro conflicting with llvm new class > >>> llvm:ElementCount Min member > >> > >> Really? Well, we will hardly be the only code they broke with that. > >> I think we can just wait for them to reconsider. > > > > FYI This is now on LLVM's release_90 branch, due out on August 28. > > Maybe we should consider doing an explicit bug report, but I would not bet > that they are going to fold… or fixing the issue pg side, eg "pg_Min", > less than 400 hundred instances, and backpatch to all supported > versions:-( I would just #undef Min for our small number of .cpp files that include LLVM headers. It's not as though you need it in C++, which has std::min() from <algorithm>. -- Thomas Munro https://enterprisedb.com
On Sat, Jul 27, 2019 at 7:12 PM Thomas Munro <thomas.munro@gmail.com> wrote: > On Sat, Jul 27, 2019 at 7:06 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > > Maybe we should consider doing an explicit bug report, but I would not bet > > that they are going to fold… or fixing the issue pg side, eg "pg_Min", > > less than 400 hundred instances, and backpatch to all supported > > versions:-( > > I would just #undef Min for our small number of .cpp files that > include LLVM headers. It's not as though you need it in C++, which > has std::min() from <algorithm>. Like so. Fixes the problem for me (llvm-devel-9.0.d20190712). -- Thomas Munro https://enterprisedb.com
Attachment
Hello Thomas, >> I would just #undef Min for our small number of .cpp files that >> include LLVM headers. It's not as though you need it in C++, which >> has std::min() from <algorithm>. > > Like so. Fixes the problem for me (llvm-devel-9.0.d20190712). Hmmm. Not so nice, but if it works, why not, at least the impact is much smaller than renaming. Note that the Min macro is used in several pg headers (ginblock.h, ginxlog.h, hash.h, simplehash.h, spgist_private.h), so you might really need it depending on what is being done later. Otherwise, why not simply move llvm C++ includes *before* postgres includes? They should be fully independent anyway, so the order should not matter? -- Fabien.
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Otherwise, why not simply move llvm C++ includes *before* postgres
> includes?
We've been burnt in the past by putting other headers before postgres.h.
(A typical issue is that the interpretation of <stdio.h> varies depending
on _LARGE_FILES or a similar macro, so you get problems if something
causes that to be included before pg_config.h has set that macro.)
Maybe none of the platforms where that's an issue have C++, but that
doesn't seem like a great assumption.
> They should be fully independent anyway, so the order should
> not matter?
On what grounds do you claim that's true anywhere, let alone
everywhere?
regards, tom lane
Hello Tom, >> They should be fully independent anyway, so the order should >> not matter? > > On what grounds do you claim that's true anywhere, let alone > everywhere? I mean that the intersection of Postgres realm, a database written in C, and LLVM realm, a compiler written in C++, should not interfere much one with the other, bar the jit compilation stuff which mixes both, so having one set of realm-specific includes before/after the other *should* not matter. Obviously the Min macro is a counter example of that, but that is indeed the problem to solve, and it is really accidental. It would be very unlucky if there was an issue the other way around. But maybe not. Anyway, I'm just trying to suggest a minimum fuss solution. One point of "seawasp" and "jellyfish" is to have early warning of compilation issues with future compilers, and it is serving this purpose beautifully. Another point is to detect compiler bugs early when compiling a significant project, and I have reported issues about both clang & gcc in the past, so it works there too. If reordering includes is not an option, too bad. Then "#undef Min" which I find disputable, allthough I've done much worse... it might or might not work depending on what is done afterwards. Or rename the macro, as I suggested first, but there are many instances. Or convince LLVM people that they should change their stuff. Or document that pg jit will cannot use the latest LLVM, as a feature. Or find another solution:-) -- Fabien.
On Mon, Jul 29, 2019 at 8:03 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote: > If reordering includes is not an option, too bad. Then "#undef Min" which > I find disputable, allthough I've done much worse... it might or might not > work depending on what is done afterwards. Or rename the macro, as I > suggested first, but there are many instances. Or convince LLVM people > that they should change their stuff. Or document that pg jit will cannot > use the latest LLVM, as a feature. Or find another solution:-) Let's just commit the #undef so that seawasp is green and back to being ready to tell us if something else breaks. Personally, I don't see any reason why <random other project> should entertain a request to change their variable names to avoid our short common word macros that aren't even all-caps, but if someone asks them and they agree to do that before the final 9.0 release we can just revert. -- Thomas Munro https://enterprisedb.com
Thomas Munro <thomas.munro@gmail.com> writes:
> Let's just commit the #undef so that seawasp is green and back to
> being ready to tell us if something else breaks.
+1. I was afraid that working around this would be impossibly
painful ... but if it just takes one judiciously placed #undef,
let's do that and not argue about it.
regards, tom lane
On Mon, Jul 29, 2019 at 9:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@gmail.com> writes: > > Let's just commit the #undef so that seawasp is green and back to > > being ready to tell us if something else breaks. > > +1. I was afraid that working around this would be impossibly > painful ... but if it just takes one judiciously placed #undef, > let's do that and not argue about it. Done. -- Thomas Munro https://enterprisedb.com
Hi, On 2019-07-29 10:27:54 +1200, Thomas Munro wrote: > On Mon, Jul 29, 2019 at 9:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Thomas Munro <thomas.munro@gmail.com> writes: > > > Let's just commit the #undef so that seawasp is green and back to > > > being ready to tell us if something else breaks. > > > > +1. I was afraid that working around this would be impossibly > > painful ... but if it just takes one judiciously placed #undef, > > let's do that and not argue about it. > > Done. cool, thanks. Greetings, Andres Freund
>>> Let's just commit the #undef so that seawasp is green and back to >>> being ready to tell us if something else breaks. >> >> +1. I was afraid that working around this would be impossibly >> painful ... but if it just takes one judiciously placed #undef, >> let's do that and not argue about it. > > Done. Seawasp is back to green. -- Fabien.