Thread: Fix compilation failure against LLVM 11

Fix compilation failure against LLVM 11

From
Jesse Zhang
Date:
Hi hackers,

My local build of master started failing last night with this error:

llvmjit_inline.cpp:59:10: fatal error: 'llvm/IR/CallSite.h' file not found
#include <llvm/IR/CallSite.h>
         ^~~~~~~~~~~~~~~~~~~~

I searched my inbox and the archive, strange that nobody else is seeing
this.

Turns out that LLVM has recently removed "llvm/IR/CallSite.h" in
(unreleased) version 11 [1][2]. To fix the build I tried conditionally
(on LLVM_VERSION_MAJOR < 11) including CallSite.h, but that looks yuck.
Then I poked at llvmjit_inline.cpp a bit and found that CallSite.h
doesn't seem to be really necessary. PFA a patch that simply removes
this #include.

In addition, I've done the due dilligence of trying to build against
LLVM versions 8, 9, 10.

Cheers,
Jesse

[1] LLVM Differential Revision: https://reviews.llvm.org/D78794
[2] LLVM commit https://github.com/llvm/llvm-project/commit/2c24051bacd2
"[CallSite removal] Rename CallSite.h to AbstractCallSite.h. NFC"

Attachment

Re: Fix compilation failure against LLVM 11

From
Michael Paquier
Date:
On Sat, Apr 25, 2020 at 09:41:20PM -0700, Jesse Zhang wrote:
> I searched my inbox and the archive, strange that nobody else is seeing
> this.
>
> Turns out that LLVM has recently removed "llvm/IR/CallSite.h" in
> (unreleased) version 11 [1][2]. To fix the build I tried conditionally
> (on LLVM_VERSION_MAJOR < 11) including CallSite.h, but that looks yuck.
> Then I poked at llvmjit_inline.cpp a bit and found that CallSite.h
> doesn't seem to be really necessary. PFA a patch that simply removes
> this #include.
>
> In addition, I've done the due dilligence of trying to build against
> LLVM versions 8, 9, 10.

LLVM 11 has not been released yet.  Do you think that this part or
even more are subject to change before the 11 release?  My take would
be to wait more before fixing this issue and make sure that our code
is fixed when their code is GA'd.
--
Michael

Attachment

Re: Fix compilation failure against LLVM 11

From
Jesse Zhang
Date:
Hi Michael,

On Sun, Apr 26, 2020 at 11:21 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Apr 25, 2020 at 09:41:20PM -0700, Jesse Zhang wrote:
> > I searched my inbox and the archive, strange that nobody else is seeing
> > this.
> >
> > Turns out that LLVM has recently removed "llvm/IR/CallSite.h" in
> > (unreleased) version 11 [1][2]. To fix the build I tried conditionally
> > (on LLVM_VERSION_MAJOR < 11) including CallSite.h, but that looks yuck.
> > Then I poked at llvmjit_inline.cpp a bit and found that CallSite.h
> > doesn't seem to be really necessary. PFA a patch that simply removes
> > this #include.
> >
> > In addition, I've done the due dilligence of trying to build against
> > LLVM versions 8, 9, 10.
>
> LLVM 11 has not been released yet.  Do you think that this part or
> even more are subject to change before the 11 release?  My take would
> be to wait more before fixing this issue and make sure that our code
> is fixed when their code is GA'd.
> --
> Michael

Are you expressing a concern against "churning" this part of the code in
reaction to upstream LLVM changes? I'd agree with you in general. But
then the question we need to ask is "will we need to revert this 3 weeks
from now if upstream reverts their changes?", or "we change X to Y now,
will we need to instead change X to Z 3 weeks later?". In that frame of
mind, the answer is simply "no" w.r.t this patch: it's removing an
#include that simply has been dead: the upstream change merely exposed
it.

OTOH, is your concern more around "how many more dead #include will LLVM
11 reveal before September?", I'm open to suggestions. I personally have
a bias to keep things working.

Cheers,
Jesse



Re: Fix compilation failure against LLVM 11

From
Michael Paquier
Date:
On Mon, Apr 27, 2020 at 07:48:54AM -0700, Jesse Zhang wrote:
> Are you expressing a concern against "churning" this part of the code in
> reaction to upstream LLVM changes? I'd agree with you in general. But
> then the question we need to ask is "will we need to revert this 3 weeks
> from now if upstream reverts their changes?", or "we change X to Y now,
> will we need to instead change X to Z 3 weeks later?".

My concerns are a mix of all that, because we may finish by doing the
same verification work multiple times instead of fixing all existing
issues at once.  A good thing is that we may be able to conclude
rather soon, it looks like LLVM releases a new major version every 3
months or so.

> In that frame of
> mind, the answer is simply "no" w.r.t this patch: it's removing an
> #include that simply has been dead: the upstream change merely exposed
> it.

The docs claim support for LLVM down to 3.9.  Are versions older than
8 fine with your proposed change?

> OTOH, is your concern more around "how many more dead #include will LLVM
> 11 reveal before September?", I'm open to suggestions. I personally have
> a bias to keep things working.

This position can have advantages, though it seems to me that we
should still wait to see if there are more issues popping up.
--
Michael

Attachment

Re: Fix compilation failure against LLVM 11

From
Andres Freund
Date:
Hi,

On 2020-04-28 13:56:23 +0900, Michael Paquier wrote:
> On Mon, Apr 27, 2020 at 07:48:54AM -0700, Jesse Zhang wrote:
> > Are you expressing a concern against "churning" this part of the code in
> > reaction to upstream LLVM changes? I'd agree with you in general. But
> > then the question we need to ask is "will we need to revert this 3 weeks
> > from now if upstream reverts their changes?", or "we change X to Y now,
> > will we need to instead change X to Z 3 weeks later?".
> 
> My concerns are a mix of all that, because we may finish by doing the
> same verification work multiple times instead of fixing all existing
> issues at once.  A good thing is that we may be able to conclude
> rather soon, it looks like LLVM releases a new major version every 3
> months or so.

Given the low cost of a change like this, and the fact that we have a
buildfarm animal building recent trunk versions of llvm, I think it's
better to just backpatch now.

> > In that frame of
> > mind, the answer is simply "no" w.r.t this patch: it's removing an
> > #include that simply has been dead: the upstream change merely exposed
> > it.
> 
> The docs claim support for LLVM down to 3.9.  Are versions older than
> 8 fine with your proposed change?

I'll check.


> > OTOH, is your concern more around "how many more dead #include will LLVM
> > 11 reveal before September?", I'm open to suggestions. I personally have
> > a bias to keep things working.
> 
> This position can have advantages, though it seems to me that we
> should still wait to see if there are more issues popping up.

What's the benefit? The cost of checking this will be not meaningfully
lower if there's other things to check as well, given their backward
compat story presumably is different.

Greetings,

Andres Freund



Re: Fix compilation failure against LLVM 11

From
Jesse Zhang
Date:
Hi Michael,

On Mon, Apr 27, 2020 at 9:56 PM Michael Paquier wrote:
> rather soon, it looks like LLVM releases a new major version every 3
> months or so.

FYI LLVM has a six-month release cadence [1], the next release is
expected coming September (I can't tell whether you were joking).

Cheers,
Jesse

[1] https://llvm.org/docs/HowToReleaseLLVM.html#release-timeline



Re: Fix compilation failure against LLVM 11

From
Jesse Zhang
Date:
Hi Andres,
On the mensiversary of the last response, what can I do to help move
this along (aside from heeding the advice "don't use LLVM HEAD")?

Michael Paquier expressed concerns over flippant churn upthread:

On Mon, Apr 27, 2020 at 10:19 PM Andres Freund wrote:
AF> On 2020-04-28 13:56:23 +0900, Michael Paquier wrote:
MP> > On Mon, Apr 27, 2020 at 07:48:54AM -0700, Jesse Zhang wrote:
JZ> > > Are you expressing a concern against "churning" this part of the
JZ> > > code in reaction to upstream LLVM changes? I'd agree with you in
JZ> > > general. But then the question we need to ask is "will we need
JZ> > > to revert this 3 weeks from now if upstream reverts their
JZ> > > changes?", or "we change X to Y now, will we need to instead
JZ> > > change X to Z 3 weeks later?".
> >
MP> > My concerns are a mix of all that, because we may finish by doing
MP> > the same verification work multiple times instead of fixing all
MP> > existing issues at once.  A good thing is that we may be able to
MP> > conclude rather soon, it looks like LLVM releases a new major
MP> > version every 3 months or so.
>
AF> Given the low cost of a change like this, and the fact that we have
AF> a buildfarm animal building recent trunk versions of llvm, I think
AF> it's better to just backpatch now.

For bystanders: Andres and I seemed to agree that this is unlikely to be
flippant (in addition to other benefits mentioned below). We haven't
discussed more on this, though I'm uncertain we had a consensus.

>
JZ> > > In that frame of mind, the answer is simply "no" w.r.t this
JZ> > > patch: it's removing an #include that simply has been dead: the
JZ> > > upstream change merely exposed it.
> >
MP> > The docs claim support for LLVM down to 3.9.  Are versions older
MP> > than 8 fine with your proposed change?
>
AF> I'll check.
>

How goes the checking? I was 99% certain it'd work but that might have
been my excuse to be lazy. Do you need help on this?

>
JZ> > > OTOH, is your concern more around "how many more dead #include
JZ> > > will LLVM 11 reveal before September?", I'm open to suggestions.
JZ> > > I personally have a bias to keep things working.
> >
MP> > This position can have advantages, though it seems to me that we
MP> > should still wait to see if there are more issues popping up.
>
AF> What's the benefit? The cost of checking this will be not
AF> meaningfully lower if there's other things to check as well, given
AF> their backward compat story presumably is different.

For bystanders: Andres and I argued for "fixing this sooner and
backpatch" and Michael suggested "wait longer and whack all moles". We
have waited, and there seems to be only one mole (finding all dead
unbroken "include"s was left as an exercise for the reader). Have we
come to an agreement on this?

Cheers,
Jesse



Re: Fix compilation failure against LLVM 11

From
Michael Paquier
Date:
On Wed, May 27, 2020 at 07:49:45AM -0700, Jesse Zhang wrote:
> For bystanders: Andres and I argued for "fixing this sooner and
> backpatch" and Michael suggested "wait longer and whack all moles". We
> have waited, and there seems to be only one mole (finding all dead
> unbroken "include"s was left as an exercise for the reader). Have we
> come to an agreement on this?

If Andres could take care of this issue as he feels is suited, that's
OK for me.  I could look at that and play with llvm builds. Now I am
not really familiar with it, so it would take me some time but we are
all here to learn :)

Please note that I would still wait for their next GA release to plug
in any extra holes at the same time.  @Jesse: or is this change
actually part of the upcoming 10.0.1?
--
Michael

Attachment

Re: Fix compilation failure against LLVM 11

From
Jesse Zhang
Date:
On Thu, May 28, 2020 at 1:07 AM Michael Paquier <michael@paquier.xyz> wrote:
> Please note that I would still wait for their next GA release to plug
> in any extra holes at the same time.  @Jesse: or is this change
> actually part of the upcoming 10.0.1?

No a refactoring like this was not in the back branches (nor is it
expected).

Thanks,
Jesse



Re: Fix compilation failure against LLVM 11

From
Andres Freund
Date:
On 2020-05-28 17:07:46 +0900, Michael Paquier wrote:
> Please note that I would still wait for their next GA release to plug
> in any extra holes at the same time.  @Jesse: or is this change
> actually part of the upcoming 10.0.1?

Why? I plan to just commit this change now.



Re: Fix compilation failure against LLVM 11

From
Andres Freund
Date:
Hi,

On 2020-05-27 07:49:45 -0700, Jesse Zhang wrote:
> On the mensiversary of the last response, what can I do to help move
> this along (aside from heeding the advice "don't use LLVM HEAD")?

Sorry, I had looked at it at point with the intent to commit it, and hit
some stupid small snags (*). And then I forgot about it. Pushed.

Thanks for the patch!

Andres Freund


(*) first I didn't see the problem, because I accidentally had an old
version of the header around. Then I couldn't immediately build old
versions of pg + LLVM due to my existing installation needing to be
rebuilt. Then there were compiler errors, due to a too new
compiler. Etc.