Thread: LLVM breakage on seawasp

LLVM breakage on seawasp

From
Thomas Munro
Date:
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.

--
Thomas Munro
https://enterprisedb.com



Re: LLVM breakage on seawasp

From
Andres Freund
Date:
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.



Re: LLVM breakage on seawasp

From
Tom Lane
Date:
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



Re: LLVM breakage on seawasp

From
Andres Freund
Date:
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
addresswhat you are concerned about? If not, could you expand? 

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



Re: LLVM breakage on seawasp

From
Thomas Munro
Date:
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



Re: LLVM breakage on seawasp

From
Tom Lane
Date:
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
addresswhat 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



Re: LLVM breakage on seawasp

From
Andres Freund
Date:
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
forthem to drop their older backwards compatible function. 

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



Re: LLVM breakage on seawasp

From
Andres Freund
Date:
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



Re: LLVM breakage on seawasp

From
Tom Lane
Date:
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
reasonablefor 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



Re: LLVM breakage on seawasp

From
Thomas Munro
Date:
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