Spelling change in LLVM 14 API
Hi,
After [1]https://github.com/llvm/llvm-project/commit/92ce6db9ee7666a347fccf0f72ba3225b199d6d1, seawasp blew up[2]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=seawasp&dt=2021-08-21%2023%3A17%3A30. 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]https://github.com/llvm/llvm-project/commit/92ce6db9ee7666a347fccf0f72ba3225b199d6d1 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]https://github.com/llvm/llvm-project/blob/96d329455501dd9f7b38c6f4c5d54c7e13247dd1/llvm/include/llvm/IR/Function.h#L390). 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.
Attachments:
0001-Track-LLVM-14-API-changes.patchtext/x-patch; charset=US-ASCII; name=0001-Track-LLVM-14-API-changes.patchDownload
From 8678db35ef27b7f197bebcd5da1f91f55f583f8c Mon Sep 17 00:00:00 2001
From: Thomas Munro <tmunro@postgresql.org>
Date: Fri, 24 Sep 2021 12:45:22 +1200
Subject: [PATCH] Track LLVM 14 API changes.
Discussion: https://postgr.es/m/CA%2BhUKGL%3Dyg6qqgg6W6SAuvRQejditeoDNy-X3b9H_6Fnw8j5Wg%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit_inline.cpp | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/backend/jit/llvm/llvmjit_inline.cpp b/src/backend/jit/llvm/llvmjit_inline.cpp
index 6f03595db5..9bb4b672a7 100644
--- a/src/backend/jit/llvm/llvmjit_inline.cpp
+++ b/src/backend/jit/llvm/llvmjit_inline.cpp
@@ -594,7 +594,11 @@ function_inlinable(llvm::Function &F,
if (F.materialize())
elog(FATAL, "failed to materialize metadata");
- if (F.getAttributes().hasFnAttribute(llvm::Attribute::NoInline))
+#if LLVM_VERSION_MAJOR < 14
+#define hasFnAttr hasFnAttribute
+#endif
+
+ if (F.getAttributes().hasFnAttr(llvm::Attribute::NoInline))
{
ilog(DEBUG1, "ineligibile to import %s due to noinline",
F.getName().data());
@@ -871,7 +875,9 @@ create_redirection_function(std::unique_ptr<llvm::Module> &importMod,
llvm::Function *AF;
llvm::BasicBlock *BB;
llvm::CallInst *fwdcall;
+#if LLVM_VERSION_MAJOR < 14
llvm::Attribute inlineAttribute;
+#endif
AF = llvm::Function::Create(F->getFunctionType(),
LinkageTypes::AvailableExternallyLinkage,
@@ -880,9 +886,13 @@ create_redirection_function(std::unique_ptr<llvm::Module> &importMod,
Builder.SetInsertPoint(BB);
fwdcall = Builder.CreateCall(F, &*AF->arg_begin());
+#if LLVM_VERSION_MAJOR < 14
inlineAttribute = llvm::Attribute::get(Context,
llvm::Attribute::AlwaysInline);
fwdcall->addAttribute(~0U, inlineAttribute);
+#else
+ fwdcall->addFnAttr(llvm::Attribute::AlwaysInline);
+#endif
Builder.CreateRet(fwdcall);
return AF;
--
2.30.2
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]https://github.com/llvm/llvm-project/commit/f6fa95b77f33c3690e4201e505cb8dce1433abd9[2]https://github.com/llvm/llvm-project/commit/e463b69736da8b0a950ecd937cf990401bdfcdeb
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
Attachments:
llvm-14.difftext/x-patch; charset=US-ASCII; name=llvm-14.diffDownload
diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp
index daefb3e1fd..ee9ff5e571 100644
--- a/src/backend/jit/llvm/llvmjit_error.cpp
+++ b/src/backend/jit/llvm/llvmjit_error.cpp
@@ -28,10 +28,15 @@ static int fatal_new_handler_depth = 0;
static std::new_handler old_new_handler = NULL;
static void fatal_system_new_handler(void);
+#if LLVM_VERSION_MAJOR >= 14
+static void fatal_llvm_new_handler(void *user_data, const char *reason, bool gen_crash_diag);
+static void fatal_llvm_error_handler(void *user_data, const char *reason, bool gen_crash_diag);
+#else
#if LLVM_VERSION_MAJOR > 4
static void fatal_llvm_new_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
#endif
static void fatal_llvm_error_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
+#endif
/*
@@ -126,7 +131,18 @@ fatal_system_new_handler(void)
errdetail("while in LLVM")));
}
-#if LLVM_VERSION_MAJOR > 4
+#if LLVM_VERSION_MAJOR >= 14
+static void
+fatal_llvm_new_handler(void *user_data,
+ const char *reason,
+ bool gen_crash_diag)
+{
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("While in LLVM: %s", reason)));
+}
+#elif LLVM_VERSION_MAJOR > 4
static void
fatal_llvm_new_handler(void *user_data,
const std::string& reason,
@@ -139,6 +155,18 @@ fatal_llvm_new_handler(void *user_data,
}
#endif
+#if LLVM_VERSION_MAJOR >= 14
+static void
+fatal_llvm_error_handler(void *user_data,
+ const char *reason,
+ bool gen_crash_diag)
+{
+ ereport(FATAL,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("fatal llvm error: %s",
+ reason)));
+}
+#else
static void
fatal_llvm_error_handler(void *user_data,
const std::string& reason,
@@ -149,3 +177,4 @@ fatal_llvm_error_handler(void *user_data,
errmsg("fatal llvm error: %s",
reason.c_str())));
}
+#endif
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.
Attachments:
0001-Track-LLVM-14-API-changes-2021-10-25.patchtext/x-patch; charset=US-ASCII; name=0001-Track-LLVM-14-API-changes-2021-10-25.patchDownload
From 575ea753e29424cb1b8ee4fc399284e5b6930a4a Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 26 Oct 2021 14:03:59 +1300
Subject: [PATCH] Track LLVM 14 API changes, 2021-10-25.
Tested with LLVM 13 and LLVM 14 at commit dbab339ea44e.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA%2BhUKG%2B3Ac3He9_SpJcxeiiVknbcES1tbZEkH9sRBdJFGj8K5Q%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit_error.cpp | 34 ++++++++++++++++++++++----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp
index daefb3e1fd..bfa19468a6 100644
--- a/src/backend/jit/llvm/llvmjit_error.cpp
+++ b/src/backend/jit/llvm/llvmjit_error.cpp
@@ -29,9 +29,15 @@ static std::new_handler old_new_handler = NULL;
static void fatal_system_new_handler(void);
#if LLVM_VERSION_MAJOR > 4
+static void fatal_llvm_new_handler(void *user_data, const char *reason, bool gen_crash_diag);
+#if LLVM_VERSION_MAJOR < 14
static void fatal_llvm_new_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
#endif
+#endif
+static void fatal_llvm_error_handler(void *user_data, const char *reason, bool gen_crash_diag);
+#if LLVM_VERSION_MAJOR < 14
static void fatal_llvm_error_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
+#endif
/*
@@ -129,23 +135,41 @@ fatal_system_new_handler(void)
#if LLVM_VERSION_MAJOR > 4
static void
fatal_llvm_new_handler(void *user_data,
- const std::string& reason,
+ const char *reason,
bool gen_crash_diag)
{
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
- errdetail("While in LLVM: %s", reason.c_str())));
+ errdetail("While in LLVM: %s", reason)));
+}
+#if LLVM_VERSION_MAJOR < 14
+static void
+fatal_llvm_new_handler(void *user_data,
+ const std::string& reason,
+ bool gen_crash_diag)
+{
+ fatal_llvm_new_handler(user_data, reason.c_str(), gen_crash_diag);
}
#endif
+#endif
static void
fatal_llvm_error_handler(void *user_data,
- const std::string& reason,
+ const char *reason,
bool gen_crash_diag)
{
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("fatal llvm error: %s",
- reason.c_str())));
+ errmsg("fatal llvm error: %s", reason)));
}
+
+#if LLVM_VERSION_MAJOR < 14
+static void
+fatal_llvm_error_handler(void *user_data,
+ const std::string& reason,
+ bool gen_crash_diag)
+{
+ fatal_llvm_error_handler(user_data, reason.c_str(), gen_crash_diag);
+}
+#endif
--
2.30.2
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...
Attachments:
v2-0001-Track-LLVM-14-API-changes-2022-01-30.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Track-LLVM-14-API-changes-2022-01-30.patchDownload
From 961d9a150e38396142d1adab310147e3f172e8a9 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Tue, 26 Oct 2021 14:03:59 +1300
Subject: [PATCH v2] Track LLVM 14 API changes, 2022-01-30
Tested with LLVM 13 and LLVM 14 at commit 8d8fce87bbd5. Also add
missing #include <new>.
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CA%2BhUKG%2B3Ac3He9_SpJcxeiiVknbcES1tbZEkH9sRBdJFGj8K5Q%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit_error.cpp | 35 ++++++++++++++++++++++----
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp
index 3ea7dcd3d1..542425dc62 100644
--- a/src/backend/jit/llvm/llvmjit_error.cpp
+++ b/src/backend/jit/llvm/llvmjit_error.cpp
@@ -23,15 +23,22 @@ extern "C"
#include "jit/llvmjit.h"
+#include <new>
static int fatal_new_handler_depth = 0;
static std::new_handler old_new_handler = NULL;
static void fatal_system_new_handler(void);
#if LLVM_VERSION_MAJOR > 4
+static void fatal_llvm_new_handler(void *user_data, const char *reason, bool gen_crash_diag);
+#if LLVM_VERSION_MAJOR < 14
static void fatal_llvm_new_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
#endif
+#endif
+static void fatal_llvm_error_handler(void *user_data, const char *reason, bool gen_crash_diag);
+#if LLVM_VERSION_MAJOR < 14
static void fatal_llvm_error_handler(void *user_data, const std::string& reason, bool gen_crash_diag);
+#endif
/*
@@ -129,23 +136,41 @@ fatal_system_new_handler(void)
#if LLVM_VERSION_MAJOR > 4
static void
fatal_llvm_new_handler(void *user_data,
- const std::string& reason,
+ const char *reason,
bool gen_crash_diag)
{
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
- errdetail("While in LLVM: %s", reason.c_str())));
+ errdetail("While in LLVM: %s", reason)));
+}
+#if LLVM_VERSION_MAJOR < 14
+static void
+fatal_llvm_new_handler(void *user_data,
+ const std::string& reason,
+ bool gen_crash_diag)
+{
+ fatal_llvm_new_handler(user_data, reason.c_str(), gen_crash_diag);
}
#endif
+#endif
static void
fatal_llvm_error_handler(void *user_data,
- const std::string& reason,
+ const char *reason,
bool gen_crash_diag)
{
ereport(FATAL,
(errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("fatal llvm error: %s",
- reason.c_str())));
+ errmsg("fatal llvm error: %s", reason)));
}
+
+#if LLVM_VERSION_MAJOR < 14
+static void
+fatal_llvm_error_handler(void *user_data,
+ const std::string& reason,
+ bool gen_crash_diag)
+{
+ fatal_llvm_error_handler(user_data, reason.c_str(), gen_crash_diag);
+}
+#endif
--
2.30.2