Обсуждение: Spelling change in LLVM 14 API
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
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
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
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
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)
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
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.
Вложения
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.
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
Вложения
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
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.
Вложения
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...