Re: Spelling change in LLVM 14 API - Mailing list pgsql-hackers

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