Horton

Thread

Commits

  1. Track LLVM 14 API changes, up to 2022-01-30.

  2. Track LLVM 14 API changes.

  1. Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2021-08-22T12:54:35Z

    Hi,

After [1], seawasp blew up[2].  I tested the following fix on LLVM 13
and 14 (main branch ~2 days ago).  Better ideas welcome.

-       if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
+#if LLVM_VERSION_MAJOR < 14
+#define hasFnAttr hasFnAttribute
+#endif
+
+       if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline))

[1] https://github.com/llvm/llvm-project/commit/92ce6db9ee7666a347fccf0f72ba3225b199d6d1
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-08-21%2023%3A17%3A30

  2. Re: Spelling change in LLVM 14 API

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-22T13:22:43Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> After [1], seawasp blew up[2].  I tested the following fix on LLVM 13
> and 14 (main branch ~2 days ago).  Better ideas welcome.

> -       if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
> +#if LLVM_VERSION_MAJOR < 14
> +#define hasFnAttr hasFnAttribute
> +#endif
> +
> +       if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline))

Seems like either we should push back on pointless renaming, or else
that we're not really supposed to be accessing this non-stable API.
I have no idea which of those situations applies ... but if the LLVM
guys are randomly renaming methods that *are* supposed to be
user-visible, they need re-education.

			regards, tom lane

  3. Re: Spelling change in LLVM 14 API

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-29T15:40:31Z

    On 2021-Aug-22, Tom Lane wrote:

> Thomas Munro <thomas.munro@gmail.com> writes:
> > After [1], seawasp blew up[2].  I tested the following fix on LLVM 13
> > and 14 (main branch ~2 days ago).  Better ideas welcome.
> 
> > -       if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
> > +#if LLVM_VERSION_MAJOR < 14
> > +#define hasFnAttr hasFnAttribute
> > +#endif
> > +
> > +       if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline))
> 
> Seems like either we should push back on pointless renaming, or else
> that we're not really supposed to be accessing this non-stable API.
> I have no idea which of those situations applies ... but if the LLVM
> guys are randomly renaming methods that *are* supposed to be
> user-visible, they need re-education.

Did anything happen?  Seawasp is still red ...

-- 
Álvaro Herrera

  4. Re: Spelling change in LLVM 14 API

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-29T16:11:09Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-Aug-22, Tom Lane wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>>> After [1], seawasp blew up[2].  I tested the following fix on LLVM 13
>>> and 14 (main branch ~2 days ago).  Better ideas welcome.
>>> 
>>> -       if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
>>> +#if LLVM_VERSION_MAJOR < 14
>>> +#define hasFnAttr hasFnAttribute
>>> +#endif
>>> +
>>> +       if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline))

>> Seems like either we should push back on pointless renaming, or else
>> that we're not really supposed to be accessing this non-stable API.
>> I have no idea which of those situations applies ... but if the LLVM
>> guys are randomly renaming methods that *are* supposed to be
>> user-visible, they need re-education.

> Did anything happen?  Seawasp is still red ...

I stand by my opinion that Thomas' patch is a kluge rather than something
we should accept as a long-term answer.  However, in the interests of
keeping the buildfarm green, maybe we should commit it until we have a
better solution.  It looks like seawasp is only building HEAD, so I think
we could commit this in HEAD only.

			regards, tom lane

  5. Re: Spelling change in LLVM 14 API

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-29T16:47:38Z

    On 2021-Aug-29, Tom Lane wrote:

> I stand by my opinion that Thomas' patch is a kluge rather than something
> we should accept as a long-term answer.  However, in the interests of
> keeping the buildfarm green, maybe we should commit it until we have a
> better solution.  It looks like seawasp is only building HEAD, so I think
> we could commit this in HEAD only.

Well, I definitely agree with your opinion, but perhaps we should
consider what to do in case LLVM developers decide not to care and keep
the rename.  I'm not sure that polluting the tree with kludges for
cross-LLVM-version compatibility is the best way to handle this kind of
thing.  Maybe it'd be better to try and centralize them in a single file
perhaps.  For example, pglogical has files for each PG version it
supports so that the main source code only has to use one wording of
each call:

https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/compat14/pglogical_compat.h
https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/compat96/pglogical_compat.c

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La rebeldía es la virtud original del hombre" (Arthur Schopenhauer)

  6. Re: Spelling change in LLVM 14 API

    Andres Freund <andres@anarazel.de> — 2021-08-29T17:40:59Z

    Hi,

On 2021-08-22 09:22:43 -0400, Tom Lane wrote:
> Seems like either we should push back on pointless renaming, or else
> that we're not really supposed to be accessing this non-stable API.

Unfortunately LLVM only considers the C API (and even there only subsets) as
stable. Most of our code uses the stable C API, but there are parts where that
wasn't / isn't sufficient.

The weirdest part about this change [1] is that it's doing such a halfway
job. There's plenty other variants of the hasFnAttribute() spelling around,
e.g. in Function.h ([2]). I kinda get wanting to clean up that half the
functions are named like hasFnAttr() and the other like hasFnAttribute(), but
right now it seems to make things worse rather than better.

Right now the easiest change would be to just use the
Function::hasFnAttribute() helper function. But I'm a bit loathe to do so
because in a way one would hope that that gets changed too :(.

The next best thing seems to be to use the slightly longer form spelling, like
this:

diff --git i/src/backend/jit/llvm/llvmjit_inline.cpp w/src/backend/jit/llvm/llvmjit_inline.cpp
index 6f03595db5a..eb350003935 100644
--- i/src/backend/jit/llvm/llvmjit_inline.cpp
+++ w/src/backend/jit/llvm/llvmjit_inline.cpp
@@ -594,7 +594,7 @@ function_inlinable(llvm::Function &F,
 	if (F.materialize())
 		elog(FATAL, "failed to materialize metadata");
 
-	if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
+	if (F.getAttributes().hasAttribute(llvm::Attribute::FunctionIndex, llvm::Attribute::NoInline))
 	{
 		ilog(DEBUG1, "ineligibile to import %s due to noinline",
 			 F.getName().data());

which seems not likely to fall under the same "cleanup" spree.


On 2021-08-29 12:47:38 -0400, Alvaro Herrera wrote:
> On 2021-Aug-29, Tom Lane wrote:
> > I stand by my opinion that Thomas' patch is a kluge rather than something
> > we should accept as a long-term answer.  However, in the interests of
> > keeping the buildfarm green, maybe we should commit it until we have a
> > better solution.  It looks like seawasp is only building HEAD, so I think
> > we could commit this in HEAD only.
> 
> Well, I definitely agree with your opinion, but perhaps we should
> consider what to do in case LLVM developers decide not to care and keep
> the rename.

Yea :(.

I just pinged them, but I don't have much hope that this will be backed out at
this point. There's probably more llvm users that already adapted their code
than "us".


> I'm not sure that polluting the tree with kludges for
> cross-LLVM-version compatibility is the best way to handle this kind of
> thing.  Maybe it'd be better to try and centralize them in a single file
> perhaps.

I think that makes sense for things that are in more than one place, but if
there's just a single case of the ifdef it doesn't seem that obvious to me. In
particular because there's a C / C++ boundary involved here...

Greetings,

Andres Freund


[1] https://github.com/llvm/llvm-project/commit/92ce6db9ee7666a347fccf0f72ba3225b199d6d1
[2] https://github.com/llvm/llvm-project/blob/96d329455501dd9f7b38c6f4c5d54c7e13247dd1/llvm/include/llvm/IR/Function.h#L390

  7. Re: Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2021-09-24T00:52:09Z

    On Mon, Aug 30, 2021 at 5:41 AM Andres Freund <andres@anarazel.de> wrote:
> -       if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
> +       if (F.getAttributes().hasAttribute(llvm::Attribute::FunctionIndex, llvm::Attribute::NoInline))

Yeah, that's already stopped working since you wrote it a few weeks
ago... There's also been a second breakage site in our code due to
LLVM commit 85b732b5.  New fix attached.  Tested on LLVM 7, 9, 13, 14
(= LLVM main branch commit 945df8bc4cf3 built 2021-09-15).

Even though the macro approach is ugly, we're already handling every
other API change that way, and at least it's at least very clear which
lines to delete in a few years when older LLVMs fall off the conveyor
belt of time.  Admittedly renaming an identifiers is a new kludge, but
I didn't want to duplicate the whole if block or confuse pgindent.

  8. Re: Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2021-09-26T21:54:27Z

    On Fri, Sep 24, 2021 at 12:52 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Yeah, that's already stopped working since you wrote it a few weeks
> ago... There's also been a second breakage site in our code due to
> LLVM commit 85b732b5.  New fix attached.  Tested on LLVM 7, 9, 13, 14
> (= LLVM main branch commit 945df8bc4cf3 built 2021-09-15).

And pushed.  Probably won't be the last change and seawasp only tests
master, so no back-patch for now.

  9. Re: Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2021-10-26T00:39:53Z

    On Mon, Sep 27, 2021 at 10:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> And pushed.  Probably won't be the last change and seawasp only tests
> master, so no back-patch for now.

According to my crystal ball, seawasp will shortly break again[1][2]
and the attached patch will fix it.

[1] https://github.com/llvm/llvm-project/commit/f6fa95b77f33c3690e4201e505cb8dce1433abd9
[2] https://github.com/llvm/llvm-project/commit/e463b69736da8b0a950ecd937cf990401bdfcdeb

  10. Re: Spelling change in LLVM 14 API

    Andres Freund <andres@anarazel.de> — 2021-10-26T00:51:57Z

    Hi,

On 2021-10-26 13:39:53 +1300, Thomas Munro wrote:
> On Mon, Sep 27, 2021 at 10:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > And pushed.  Probably won't be the last change and seawasp only tests
> > master, so no back-patch for now.
> 
> According to my crystal ball, seawasp will shortly break again[1][2]
> and the attached patch will fix it.

That feels lot more convincing though. Based on past experience it's not hard
to believe that the compile-time impact of the include the use of std::string
forces into a lot of places is pretty significant...

Could we make old case a wrapper around the new case that just passes on the
error as a const char *? That should be more maintainable long-term, because
there's only one copy of the body?

Greetings,

Andres Freund

  11. Re: Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2021-10-26T01:21:40Z

    On Tue, Oct 26, 2021 at 1:52 PM Andres Freund <andres@anarazel.de> wrote:
> On 2021-10-26 13:39:53 +1300, Thomas Munro wrote:
> > According to my crystal ball, seawasp will shortly break again[1][2]
> > and the attached patch will fix it.
>
> That feels lot more convincing though. Based on past experience it's not hard
> to believe that the compile-time impact of the include the use of std::string
> forces into a lot of places is pretty significant...
>
> Could we make old case a wrapper around the new case that just passes on the
> error as a const char *? That should be more maintainable long-term, because
> there's only one copy of the body?

Here's one like that.  The previous message "Track LLVM 14 API
changes" didn't seem too scalable so I added date and commit ID.

  12. Re: Spelling change in LLVM 14 API

    Thomas Munro <thomas.munro@gmail.com> — 2022-01-31T05:34:16Z

    On Tue, Oct 26, 2021 at 2:21 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> Here's one like that.  The previous message "Track LLVM 14 API
> changes" didn't seem too scalable so I added date and commit ID.

seawasp finally caught up with these LLVM changes and turned red.  I
retested the patch against this week's LLVM locally.  New version also
adds #include <new>, for the definition of std::new_handler, which g++
is now complaining about in llvmjit_error.cpp.

Since then, the LLVM 14 headers have started spewing deprecation
notices about LLVMBuildStructGEP, LLVMBuildLoad, LLVMBuildCall.  The
warnings say things like "Use LLVMBuildStructGEP2 instead to support
opaque pointers", and the -2 variants need a new argument that takes
an extra LLVMTypeRef argument, but I didn't look further...