Thread: Fix compilation failure against LLVM 11
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
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
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
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
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
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
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
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
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
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.
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.