ODR violation in ranges

ODR violation in ranges

[Nathan Sidwell]

Jonathan,
the ranges header contains code like:
     inline constexpr __adaptor::_RangeAdaptorClosure all
       = [] <viewable_range _Range> (_Range&& __r)
       {
  if constexpr (view<decay_t<_Range>>)
    return std::forward<_Range>(__r);
  else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
    return ref_view{std::forward<_Range>(__r)};
  else
    return subrange{std::forward<_Range>(__r)};
       };

(line 1236)

When you strip away all the templateyness, you have:


inline constexpr auto all = [] () {};


That's an ODR violation -- the initializers in different TUs are not the 
same!

As you can guess, I can't turn this into a header unit (well, I can, but 
merging duplicates complains at you)

nathan
-- 
Nathan Sidwell

Re: ODR violation in ranges

[Jonathan Wakely]

On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
>Jonathan,
>the ranges header contains code like:
>    inline constexpr __adaptor::_RangeAdaptorClosure all
>      = [] <viewable_range _Range> (_Range&& __r)
>      {
> if constexpr (view<decay_t<_Range>>)
>   return std::forward<_Range>(__r);
> else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
>   return ref_view{std::forward<_Range>(__r)};
> else
>   return subrange{std::forward<_Range>(__r)};
>      };
>
>(line 1236)
>
>When you strip away all the templateyness, you have:
>
>
>inline constexpr auto all = [] () {};
>
>
>That's an ODR violation -- the initializers in different TUs are not 
>the same!
>
>As you can guess, I can't turn this into a header unit (well, I can, 
>but merging duplicates complains at you)

CC libstdc++@ and Patrick.

I did wonder if using lambdas for those global variables would be OK.

I think we'll need a new class template for each view adaptor, rather
than using the _RangeAdaptorClosure to hold a closure.

Patrick, can you look into that please?

Re: ODR violation in ranges

[Tam S. B.]

IIUC using lambda in inline variable initializer is not ODR violation. This is covered in CWG 2300 ( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300 ).

________________________________________
From: Libstdc++ <libstdc++-bounces@gcc.gnu.org> on behalf of Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org>
Sent: Wednesday, March 11, 2020 10:26
To: Nathan Sidwell
Cc: libstdc++@gcc.gnu.org; GCC Patches
Subject: Re: ODR violation in ranges

On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
>Jonathan,
>the ranges header contains code like:
>    inline constexpr __adaptor::_RangeAdaptorClosure all
>      = [] <viewable_range _Range> (_Range&& __r)
>      {
> if constexpr (view<decay_t<_Range>>)
>   return std::forward<_Range>(__r);
> else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
>   return ref_view{std::forward<_Range>(__r)};
> else
>   return subrange{std::forward<_Range>(__r)};
>      };
>
>(line 1236)
>
>When you strip away all the templateyness, you have:
>
>
>inline constexpr auto all = [] () {};
>
>
>That's an ODR violation -- the initializers in different TUs are not
>the same!
>
>As you can guess, I can't turn this into a header unit (well, I can,
>but merging duplicates complains at you)

CC libstdc++@ and Patrick.

I did wonder if using lambdas for those global variables would be OK.

I think we'll need a new class template for each view adaptor, rather
than using the _RangeAdaptorClosure to hold a closure.

Patrick, can you look into that please?

Re: ODR violation in ranges

[Jonathan Wakely]

On 11/03/20 10:56 +0000, Tam S. B. via Libstdc++ wrote:
>IIUC using lambda in inline variable initializer is not ODR violation. This is covered in CWG 2300 ( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300 ).

Ah yes, I think somebody (probably Patrick) has pointed out that
wording before, and I'd forgotten about it.

So that would make it an unsupported G++ feature then?

Re: ODR violation in ranges

[Patrick Palka]

On Wed, 11 Mar 2020, Tam S. B. via Gcc-patches wrote:

> IIUC using lambda in inline variable initializer is not ODR violation. This is covered in CWG 2300 ( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300 ).
> 
> ________________________________________
> From: Libstdc++ <libstdc++-bounces@gcc.gnu.org> on behalf of Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org>
> Sent: Wednesday, March 11, 2020 10:26
> To: Nathan Sidwell
> Cc: libstdc++@gcc.gnu.org; GCC Patches
> Subject: Re: ODR violation in ranges
> 
> On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
> >Jonathan,
> >the ranges header contains code like:
> >    inline constexpr __adaptor::_RangeAdaptorClosure all
> >      = [] <viewable_range _Range> (_Range&& __r)
> >      {
> > if constexpr (view<decay_t<_Range>>)
> >   return std::forward<_Range>(__r);
> > else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
> >   return ref_view{std::forward<_Range>(__r)};
> > else
> >   return subrange{std::forward<_Range>(__r)};
> >      };
> >
> >(line 1236)
> >
> >When you strip away all the templateyness, you have:
> >
> >
> >inline constexpr auto all = [] () {};
> >
> >
> >That's an ODR violation -- the initializers in different TUs are not
> >the same!
> >
> >As you can guess, I can't turn this into a header unit (well, I can,
> >but merging duplicates complains at you)
> 
> CC libstdc++@ and Patrick.
> 
> I did wonder if using lambdas for those global variables would be OK.
> 
> I think we'll need a new class template for each view adaptor, rather
> than using the _RangeAdaptorClosure to hold a closure.
> 
> Patrick, can you look into that please?

IIUC, it should suffice to replace the lambda in the initializer with a
function object, maybe something like:

  struct _All
  {
    constexpr auto
    operator()(...)
    {
    };
  };

  inline constexpr __adaptor::_RangeAdaptorClosure<_All> all;

Do the lambdas in the bodies of _RangeAdaptor::operator() and
_RangeAdaptorClosure::operator|() pose a problem too?

Re: ODR violation in ranges

[Nathan Sidwell]

On 3/11/20 6:56 AM, Tam S. B. wrote:
> IIUC using lambda in inline variable initializer is not ODR violation. This is covered in CWG 2300 ( http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300 ).

ah, thanks for the pointer.  For lambdas in function scope we do achieve 
this (the mangling of the lambda uses the function as scope and a 
function-specific index).

We do not do so for lambdas in the initializer of namespace-scope inline 
variables, which is I think the missing bit.  That's an ABI thing, I'll 
ask over there.

The ranges header we'll die horribly if we ever emit non-inline code to 
evaluate the lambda at runtime.  We'll give it a name that depends on 
the order of (namespace-scope?) lambdas. :(

inline constexpr auto foo = [] (int i) {return i * 2;};
int bob (int i)
{ return foo (i); }

compile with -fno-inline and observe '_ZNKUliE_clEi'

sigh,

nathan
> 
> ________________________________________
> From: Libstdc++ <libstdc++-bounces@gcc.gnu.org> on behalf of Jonathan Wakely via Libstdc++ <libstdc++@gcc.gnu.org>
> Sent: Wednesday, March 11, 2020 10:26
> To: Nathan Sidwell
> Cc: libstdc++@gcc.gnu.org; GCC Patches
> Subject: Re: ODR violation in ranges
> 
> On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
>> Jonathan,
>> the ranges header contains code like:
>>     inline constexpr __adaptor::_RangeAdaptorClosure all
>>       = [] <viewable_range _Range> (_Range&& __r)
>>       {
>> if constexpr (view<decay_t<_Range>>)
>>    return std::forward<_Range>(__r);
>> else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
>>    return ref_view{std::forward<_Range>(__r)};
>> else
>>    return subrange{std::forward<_Range>(__r)};
>>       };
>>
>> (line 1236)
>>
>> When you strip away all the templateyness, you have:
>>
>>
>> inline constexpr auto all = [] () {};
>>
>>
>> That's an ODR violation -- the initializers in different TUs are not
>> the same!
>>
>> As you can guess, I can't turn this into a header unit (well, I can,
>> but merging duplicates complains at you)
> 
> CC libstdc++@ and Patrick.
> 
> I did wonder if using lambdas for those global variables would be OK.
> 
> I think we'll need a new class template for each view adaptor, rather
> than using the _RangeAdaptorClosure to hold a closure.
> 
> Patrick, can you look into that please?
> 


-- 
Nathan Sidwell

Re: ODR violation in ranges

[Jason Merrill]

On Wed, Mar 11, 2020 at 11:05 AM Nathan Sidwell <nathan@acm.org> wrote:

> On 3/11/20 6:56 AM, Tam S. B. wrote:
> > IIUC using lambda in inline variable initializer is not ODR violation.
> This is covered in CWG 2300 (
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300
> ).
>
> ah, thanks for the pointer.  For lambdas in function scope we do achieve
> this (the mangling of the lambda uses the function as scope and a
> function-specific index).
>
> We do not do so for lambdas in the initializer of namespace-scope inline
> variables, which is I think the missing bit.  That's an ABI thing, I'll
> ask over there.
>

Looks like the condition on the call to start_lambda_scope in
cp_parser_init_declarator just needs to be adjusted to handle non-template
inline variables.


> The ranges header we'll die horribly if we ever emit non-inline code to
> evaluate the lambda at runtime.  We'll give it a name that depends on
> the order of (namespace-scope?) lambdas. :(
>
> inline constexpr auto foo = [] (int i) {return i * 2;};
> int bob (int i)
> { return foo (i); }
>
> compile with -fno-inline and observe '_ZNKUliE_clEi'
>
> sigh,
>
> nathan
> >
> > ________________________________________
> > From: Libstdc++ <libstdc++-bounces@gcc.gnu.org> on behalf of Jonathan
> Wakely via Libstdc++ <libstdc++@gcc.gnu.org>
> > Sent: Wednesday, March 11, 2020 10:26
> > To: Nathan Sidwell
> > Cc: libstdc++@gcc.gnu.org; GCC Patches
> > Subject: Re: ODR violation in ranges
> >
> > On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
> >> Jonathan,
> >> the ranges header contains code like:
> >>     inline constexpr __adaptor::_RangeAdaptorClosure all
> >>       = [] <viewable_range _Range> (_Range&& __r)
> >>       {
> >> if constexpr (view<decay_t<_Range>>)
> >>    return std::forward<_Range>(__r);
> >> else if constexpr (requires { ref_view{std::forward<_Range>(__r)}; })
> >>    return ref_view{std::forward<_Range>(__r)};
> >> else
> >>    return subrange{std::forward<_Range>(__r)};
> >>       };
> >>
> >> (line 1236)
> >>
> >> When you strip away all the templateyness, you have:
> >>
> >>
> >> inline constexpr auto all = [] () {};
> >>
> >>
> >> That's an ODR violation -- the initializers in different TUs are not
> >> the same!
> >>
> >> As you can guess, I can't turn this into a header unit (well, I can,
> >> but merging duplicates complains at you)
> >
> > CC libstdc++@ and Patrick.
> >
> > I did wonder if using lambdas for those global variables would be OK.
> >
> > I think we'll need a new class template for each view adaptor, rather
> > than using the _RangeAdaptorClosure to hold a closure.
> >
> > Patrick, can you look into that please?
> >
>
>
> --
> Nathan Sidwell
>
>

Re: ODR violation in ranges

[Nathan Sidwell]

On 3/11/20 11:23 AM, Jason Merrill wrote:
> On Wed, Mar 11, 2020 at 11:05 AM Nathan Sidwell <nathan@acm.org 
> <mailto:nathan@acm.org>> wrote:
> 
>     On 3/11/20 6:56 AM, Tam S. B. wrote:
>      > IIUC using lambda in inline variable initializer is not ODR
>     violation. This is covered in CWG 2300 (
>     http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1510r0.html#2300
>     ).
> 
>     ah, thanks for the pointer.  For lambdas in function scope we do
>     achieve
>     this (the mangling of the lambda uses the function as scope and a
>     function-specific index).
> 
>     We do not do so for lambdas in the initializer of namespace-scope
>     inline
>     variables, which is I think the missing bit.  That's an ABI thing, I'll
>     ask over there.
> 
> 
> Looks like the condition on the call to start_lambda_scope in 
> cp_parser_init_declarator just needs to be adjusted to handle 
> non-template inline variables.

thanks!

The ABI does describe how to mangle these (it focusses on data-members, 
but AFAICT works for namespace-scope vars too).  We don't do any of that 
though :(

> 
>     The ranges header we'll die horribly if we ever emit non-inline code to
>     evaluate the lambda at runtime.  We'll give it a name that depends on
>     the order of (namespace-scope?) lambdas. :(
> 
>     inline constexpr auto foo = [] (int i) {return i * 2;};
>     int bob (int i)
>     { return foo (i); }
> 
>     compile with -fno-inline and observe '_ZNKUliE_clEi'
> 
>     sigh,
> 
>     nathan
>      >
>      > ________________________________________
>      > From: Libstdc++ <libstdc++-bounces@gcc.gnu.org
>     <mailto:libstdc%2B%2B-bounces@gcc.gnu.org>> on behalf of Jonathan
>     Wakely via Libstdc++ <libstdc++@gcc.gnu.org
>     <mailto:libstdc%2B%2B@gcc.gnu.org>>
>      > Sent: Wednesday, March 11, 2020 10:26
>      > To: Nathan Sidwell
>      > Cc: libstdc++@gcc.gnu.org <mailto:libstdc%2B%2B@gcc.gnu.org>; GCC
>     Patches
>      > Subject: Re: ODR violation in ranges
>      >
>      > On 11/03/20 06:08 -0400, Nathan Sidwell wrote:
>      >> Jonathan,
>      >> the ranges header contains code like:
>      >>     inline constexpr __adaptor::_RangeAdaptorClosure all
>      >>       = [] <viewable_range _Range> (_Range&& __r)
>      >>       {
>      >> if constexpr (view<decay_t<_Range>>)
>      >>    return std::forward<_Range>(__r);
>      >> else if constexpr (requires {
>     ref_view{std::forward<_Range>(__r)}; })
>      >>    return ref_view{std::forward<_Range>(__r)};
>      >> else
>      >>    return subrange{std::forward<_Range>(__r)};
>      >>       };
>      >>
>      >> (line 1236)
>      >>
>      >> When you strip away all the templateyness, you have:
>      >>
>      >>
>      >> inline constexpr auto all = [] () {};
>      >>
>      >>
>      >> That's an ODR violation -- the initializers in different TUs are not
>      >> the same!
>      >>
>      >> As you can guess, I can't turn this into a header unit (well, I can,
>      >> but merging duplicates complains at you)
>      >
>      > CC libstdc++@ and Patrick.
>      >
>      > I did wonder if using lambdas for those global variables would be OK.
>      >
>      > I think we'll need a new class template for each view adaptor, rather
>      > than using the _RangeAdaptorClosure to hold a closure.
>      >
>      > Patrick, can you look into that please?
>      >
> 
> 
>     -- 
>     Nathan Sidwell
> 


-- 
Nathan Sidwell