LLVM breakage on seawasp

Hi,

On August 24, 2019 1:08:11 PM PDT, Thomas Munro <thomas.munro@gmail.com> wrote:

Hi,

llvmjit_inline.cpp:177:55: error: ‘make_unique’ is not a member of
‘llvm’
std::unique_ptr<ImportMapTy> globalsToInline =
llvm::make_unique<ImportMapTy>();

That's because they just moved to C++14 and replaced their own
llvm::make_unique<> with std::make_unique<>:

https://github.com/llvm-mirror/llvm/commit/114087caa6f95b526861c3af94b3093d9444c57b

Perhaps we'll need some macrology to select between llvm and std
versions? I am guessing we can't decree that PostgreSQL's minimum C++
level is C++14 and simply change it to std::make_unique.

Perhaps just a
#if new_enough
using std::make_unique
#else
using llvm::mak_eunique

At the start of the file, and then use it unqualified?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Andres Freund <andres@anarazel.de> writes:

On August 24, 2019 1:08:11 PM PDT, Thomas Munro <thomas.munro@gmail.com> wrote:

That's because they just moved to C++14 and replaced their own
llvm::make_unique<> with std::make_unique<>:
https://github.com/llvm-mirror/llvm/commit/114087caa6f95b526861c3af94b3093d9444c57b
Perhaps we'll need some macrology to select between llvm and std
versions? I am guessing we can't decree that PostgreSQL's minimum C++
level is C++14 and simply change it to std::make_unique.

So we're depending on APIs that upstream doesn't think are stable?

regards, tom lane

Hi,

On August 24, 2019 1:57:56 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On August 24, 2019 1:08:11 PM PDT, Thomas Munro

<thomas.munro@gmail.com> wrote:

That's because they just moved to C++14 and replaced their own
llvm::make_unique<> with std::make_unique<>:

https://github.com/llvm-mirror/llvm/commit/114087caa6f95b526861c3af94b3093d9444c57b

Perhaps we'll need some macrology to select between llvm and std
versions? I am guessing we can't decree that PostgreSQL's minimum

C++

level is C++14 and simply change it to std::make_unique.

So we're depending on APIs that upstream doesn't think are stable?

Seawasp iirc builds against the development branch of llvm, which explains why we see failures there. Does that address what you are concerned about? If not, could you expand?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Sun, Aug 25, 2019 at 8:24 AM Andres Freund <andres@anarazel.de> wrote:

On August 24, 2019 1:08:11 PM PDT, Thomas Munro <thomas.munro@gmail.com> wrote:

Perhaps we'll need some macrology to select between llvm and std
versions? I am guessing we can't decree that PostgreSQL's minimum C++
level is C++14 and simply change it to std::make_unique.

Perhaps just a
#if new_enough
using std::make_unique
#else
using llvm::mak_eunique

At the start of the file, and then use it unqualified?

Yeah, it's a pain though, you'd have to say:

#if llvm >= 9
# if cpp >= 14
# using std::make_unique;
# else
# error "postgres needs at least c++ 14 to use llvm 9"
# endif
#else
# using llvm::make_unique;
#endif

Maybe we should just use std::unique_ptr's constructor, ie give it new
ImportMayTy() instead of using make_unique(), even though that's not
cool C++ these days?

--
Thomas Munro
https://enterprisedb.com

Andres Freund <andres@anarazel.de> writes:

On August 24, 2019 1:57:56 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So we're depending on APIs that upstream doesn't think are stable?

Seawasp iirc builds against the development branch of llvm, which explains why we see failures there. Does that address what you are concerned about? If not, could you expand?

I know it's the development branch. The question is whether this
breakage is something *they* ought to be fixing. If not, I'm
worried that we're too much in bed with implementation details
of LLVM that we shouldn't be depending on.

regards, tom lane

Hi,

On August 24, 2019 2:37:55 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

On August 24, 2019 1:57:56 PM PDT, Tom Lane <tgl@sss.pgh.pa.us>

wrote:

So we're depending on APIs that upstream doesn't think are stable?

Seawasp iirc builds against the development branch of llvm, which

explains why we see failures there. Does that address what you are
concerned about? If not, could you expand?

I know it's the development branch. The question is whether this
breakage is something *they* ought to be fixing. If not, I'm
worried that we're too much in bed with implementation details
of LLVM that we shouldn't be depending on.

Don't think so - it's a C++ standard feature in the version of the standard LLVM is based on. So it's pretty reasonable for them to drop their older backwards compatible function.

Access
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On 2019-08-25 09:15:35 +1200, Thomas Munro wrote:

On Sun, Aug 25, 2019 at 8:24 AM Andres Freund <andres@anarazel.de> wrote:

On August 24, 2019 1:08:11 PM PDT, Thomas Munro <thomas.munro@gmail.com> wrote:

Perhaps we'll need some macrology to select between llvm and std
versions? I am guessing we can't decree that PostgreSQL's minimum C++
level is C++14 and simply change it to std::make_unique.

Perhaps just a
#if new_enough
using std::make_unique
#else
using llvm::mak_eunique

At the start of the file, and then use it unqualified?

Yeah, it's a pain though, you'd have to say:

#if llvm >= 9
# if cpp >= 14
# using std::make_unique;
# else
# error "postgres needs at least c++ 14 to use llvm 9"
# endif
#else
# using llvm::make_unique;
#endif

I don't think we'd really need the inner part, because you can't use
llvm 9 without a new enough compiler. There's plenty make_unique use in
inline functions etc.

Maybe we should just use std::unique_ptr's constructor, ie give it new
ImportMayTy() instead of using make_unique(), even though that's not
cool C++ these days?

Yea, wfm. do you want to make it so?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On August 24, 2019 2:37:55 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I know it's the development branch. The question is whether this
breakage is something *they* ought to be fixing. If not, I'm
worried that we're too much in bed with implementation details
of LLVM that we shouldn't be depending on.

Don't think so - it's a C++ standard feature in the version of the standard LLVM is based on. So it's pretty reasonable for them to drop their older backwards compatible function.

Whether it's reasonable or not doesn't really matter to my point.
We shouldn't be in the business of tracking multitudes of small
changes in LLVM, no matter whether they're individually "reasonable".
The more often this happens, the more concerned I am that we chose
the wrong semantic level to interface at.

regards, tom lane

On Sun, Aug 25, 2019 at 9:46 AM Andres Freund <andres@anarazel.de> wrote:

Maybe we should just use std::unique_ptr's constructor, ie give it new
ImportMayTy() instead of using make_unique(), even though that's not
cool C++ these days?

Yea, wfm. do you want to make it so?

Done.

--
Thomas Munro
https://enterprisedb.com