Horton

Thread

Commits

  1. Don't rely on llvm::make_unique.

  1. LLVM breakage on seawasp

    Thomas Munro <thomas.munro@gmail.com> — 2019-08-24T20:08:11Z

    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

  2. Re: LLVM breakage on seawasp

    Andres Freund <andres@anarazel.de> — 2019-08-24T20:24:09Z

    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.

  3. Re: LLVM breakage on seawasp

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-24T20:57:56Z

    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

  4. Re: LLVM breakage on seawasp

    Andres Freund <andres@anarazel.de> — 2019-08-24T21:11:00Z

    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.

  5. Re: LLVM breakage on seawasp

    Thomas Munro <thomas.munro@gmail.com> — 2019-08-24T21:15:35Z

    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

  6. Re: LLVM breakage on seawasp

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-24T21:37:55Z

    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

  7. Re: LLVM breakage on seawasp

    Andres Freund <andres@anarazel.de> — 2019-08-24T21:39:32Z

    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.

  8. Re: LLVM breakage on seawasp

    Andres Freund <andres@anarazel.de> — 2019-08-24T21:46:54Z

    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

  9. Re: LLVM breakage on seawasp

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-24T21:54:50Z

    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

  10. Re: LLVM breakage on seawasp

    Thomas Munro <thomas.munro@gmail.com> — 2019-08-25T02:56:55Z

    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