Bug 96416 - [DR 3545] to_address() is broken by static_assert in pointer_traits
Summary: [DR 3545] to_address() is broken by static_assert in pointer_traits
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: 10.5
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-02 04:17 UTC by Zach Laine
Modified: 2022-06-28 10:41 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 12.0
Known to fail: 10.3.0, 11.2.0
Last reconfirmed: 2020-11-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zach Laine 2020-08-02 04:17:13 UTC
pointer_traits contains this static_assert:

      static_assert(!is_same<element_type, __undefined>::value,
	  "pointer type defines element_type or is like SomePointer<T, Args>");

When trying to use address_of() with an iterator that has a usable operator->(), address_of() fails because the static_assert causes a hard error in the trailing decltype here:

  template<typename _Ptr>
    constexpr auto
    __to_address(const _Ptr& __ptr) noexcept
    -> decltype(std::pointer_traits<_Ptr>::to_address(__ptr))
    { return std::pointer_traits<_Ptr>::to_address(__ptr); }

  template<typename _Ptr, typename... _None>
    constexpr auto
    __to_address(const _Ptr& __ptr, _None...) noexcept
    {
      if constexpr (is_base_of_v<__gnu_debug::_Safe_iterator_base, _Ptr>)
	return std::__to_address(__ptr.base().operator->());
      else
	return std::__to_address(__ptr.operator->());
    }

When I comment out the static_assert, to_address() works with the iterator in question.
Comment 1 Jonathan Wakely 2020-08-03 12:58:45 UTC
I think this is the "correct" behaviour.

The specification of pointer_traits<Ptr>::element_type says that if Ptr::element_type isn't valid, and Ptr isn't a template that can be "rebound", then "otherwise, the specialization is ill-formed."

This explicitly says "the specialization" (meaning pointer_traits<Ptr> itself). Compare this with th rebind member which only says "the instantiation of rebind is ill-formed".
Comment 2 Zach Laine 2020-08-03 15:34:53 UTC
Fair enough.  [pointer.conversion] says that to_pointer(const Ptr& p) is "pointer_­traits<Ptr>​::​to_­address(p) if that expression is well-formed (see [pointer.traits.optmem]), otherwise to_­address(p.operator->())".  Do we then have a spec bug?
Comment 3 Jonathan Wakely 2020-08-03 15:38:11 UTC
Ugh, yes, I think we do.
Comment 4 Jonathan Wakely 2020-11-11 17:46:09 UTC
On reading this again, I think the implementation should be required to check whether the specialization pointer_­traits<Ptr>​ would be well-formed as part of checking the expression pointer_­traits<Ptr>​::​to_­address(p).

So it's unnecessarily awkward, but still the implementation's job to do that.
Comment 5 Jonathan Wakely 2020-11-11 18:52:51 UTC
But on third thoughts, I don't know how to implement this reliably.
Comment 6 Glen Joseph Fernandes 2020-11-11 20:05:55 UTC
> Do we then have a spec bug?

The to_address(const P&) overload always assumed a valid pointer_traits<P>. Even before it was std::to_address in C++20, when it was __to_address in libstdc++, or boost::to_address, or __to_raw_pointer in libc++, and was used in C++11 and above, its return type was 'typename std::pointer_traits<P>::element_type*' which requires a valid pointer_traits.

i.e. Our allocator-aware containers would only ever work with fancy pointer P for which pointer_traits<P> is valid.

std::to_address being used for more than just raw-or-fancy-pointers came later (since Casey's P1474 which chose to use it for contiguous iterators). My guess is they didn't realize pointer_traits<I> wouldn't be valid for those iterator types.
Comment 7 Jonathan Wakely 2020-11-11 20:40:22 UTC
Thanks, Glen. So it seems I was right the first time, and using std::to_address does require a type that can be used with std::pointer_traits.

Not a bug then. Sorry, Zach!
Comment 8 Jonathan Wakely 2020-11-11 20:41:15 UTC
I suppose I could change the static_assert message to also suggest defining your own specialization of std::pointer_traits, which is another way to make it work.
Comment 9 Giuseppe D'Angelo 2021-03-26 14:19:28 UTC
Hi,

Stumbled across this indeed when trying to use contiguous iterators.

Sure enough, pointer_traits for them is ill-formed. But then I don't understand why the "otherwise to_­address(p.operator->())" part is in the Standard at all, if it can never be used.

Has this been raised as a library defect?

And are you recommending that everyone who defines their custom contiguous iterators specializes pointer_traits for them? Call it _quite_ annoying...
Comment 10 Giuseppe D'Angelo 2021-03-26 14:45:17 UTC
(By the way, finding this bug is quite hard. Could "address_of" be changed to "to_address" , in the bug description? I think that's the intended meaning. And, "to_pointer", mentioned a few times, doesn't exist.)
Comment 11 Glen Joseph Fernandes 2021-03-26 23:44:13 UTC
> if it can never be used.

You're misunderstanding.   to_address(p) requires that pointer_traits<P> is valid. It just doesn't need to have a to_address member function.

Example 1. You have a pointer-like type Ptr1. You haven't specialized pointer_traits<Ptr1>, but pointer_traits<Ptr1> is valid. Here it will call to_address(p1.operator->()).

Example 2. You have a pointer-like type Ptr2. You have specialized pointer_traits<Ptr2> with a to_address function. Here it will call pointer_traits<Ptr2>::to_address(p2).

to_address() was intended for used with pointers and pointer-like types (and pointer_traits<Ptr> was always required to be valid).

It was intended for use with allocator pointers, and the original standard library implementations had a return type of: typename pointer_traits<Ptr>::element_type*

If (for contiguous iterators, which came later) you want pointer_traits<X> to be valid even when X does not have element_type, that is a design change to pointer_traits.
Comment 12 Jonathan Wakely 2021-03-27 00:48:55 UTC
(In reply to Giuseppe D'Angelo from comment #10)
> (By the way, finding this bug is quite hard. Could "address_of" be changed
> to "to_address" , in the bug description?

Done.
Comment 13 Arthur O'Dwyer 2021-03-27 01:41:29 UTC
> And are you recommending that everyone who defines their custom contiguous
> iterators specializes pointer_traits for them? Call it _quite_ annoying...

Definitely not! When you define a contiguous iterator type, you should just give it a sixth nested typedef alongside the other five (or three in C++20): `using element_type = value_type;`. This enables contiguous-iterator machinery.
See https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-contiguous-iterator/66050521#66050521

You should never specialize std::pointer_traits for your own type.
("Can" you? Yes. "Should" you? No.)
Comment 14 Giuseppe D'Angelo 2021-03-27 16:53:05 UTC
Hello,

(In reply to Glen Joseph Fernandes from comment #11)
> > if it can never be used.
> 
> You're misunderstanding.   to_address(p) requires that pointer_traits<P> is
> valid. It just doesn't need to have a to_address member function.

Thank you for clarifying this. I think the wording in the standard is very unfortunate, but combined with the realization that pointer_traits isn't SFINAE-friendly, then it's the only intended meaning.



> If (for contiguous iterators, which came later) you want pointer_traits<X>
> to be valid even when X does not have element_type, that is a design change
> to pointer_traits.

One might claim that pointer_traits should become SFINAE-friendly (like C++17's iterator_traits), but sure, that's a different design question and not necessarily needed here.


(In reply to Jonathan Wakely from comment #12)
> (In reply to Giuseppe D'Angelo from comment #10)
> > (By the way, finding this bug is quite hard. Could "address_of" be changed
> > to "to_address" , in the bug description?
> 
> Done.

Thank you!



(In reply to Arthur O'Dwyer from comment #13)
> > And are you recommending that everyone who defines their custom contiguous
> > iterators specializes pointer_traits for them? Call it _quite_ annoying...
> 
> Definitely not! When you define a contiguous iterator type, you should just
> give it a sixth nested typedef alongside the other five (or three in C++20):
> `using element_type = value_type;`. This enables contiguous-iterator
> machinery.
> See
> https://stackoverflow.com/questions/65712091/in-c20-how-do-i-write-a-
> contiguous-iterator/66050521#66050521


This gets evil really quick: the presence of both value_type and element_type in an contiguous iterator will make you smash face-first against LWG3446, which isn't implemented in GCC 10 AFAICS.

https://cplusplus.github.io/LWG/issue3446


What's more, the accepted resolution wording for it appears to be wrong:


  template<classhas-member-value-type T>
    requires has-member-element-type<T> &&
             same_as<remove_cv_t<typename T::element_type>, remove_cv_t<typename T::value_type>>
  struct indirectly_readable_traits<T>
    : cond-value-type<typename T::value_type> { };


For const iterators, value_type is actually different from element_type (!). Thankfully libstdc++ seems to have considered this as a non-standard extension, https://github.com/gcc-mirror/gcc/commit/186aa6304570e15065f31482e9c27326a3a6679f 


To summarize:

* should a wording defect be raised against std::to_address(Ptr), to state that pointer_traits<Ptr> being well-formed is actually a prerequisite?

* should LWG3446's resolution be amended?

* if there's going to be a GCC 10.3, is the commit above solving LWG3446 going to be cherry-picked into it? Otherwise, either one blacklists GCC 10, or has to specialize pointer_traits there as a workaround (?).


Thank you all for the insightful comments.
Comment 15 Jonathan Wakely 2021-03-27 19:46:43 UTC
(In reply to Giuseppe D'Angelo from comment #14)
> This gets evil really quick: the presence of both value_type and
> element_type in an contiguous iterator will make you smash face-first
> against LWG3446, which isn't implemented in GCC 10 AFAICS.

That's right, but it's on my list to backport to the gcc-10 branch, probably in the next 48 hours.
Comment 16 Glen Joseph Fernandes 2021-03-29 03:08:26 UTC
> should a wording defect be raised against std::to_address(Ptr), to state that 
> pointer_traits<Ptr> being well-formed is actually a prerequisite?

That's not an omission in the specification of to_address. The function is intended for pointers, and specified in terms of checking for a pointer_traits<P> member, and this means pointer_traits<P> must be well-formed. 

Adding an additional text to the specification saying this explicitly is unlikely to help anyone. The real change that users of iterators[1] would want is to make pointer_traits SFINAE friendly.

[1] Users of pointers don't care much, since all the pointer types people are using with to_address(p) already have a well-formed pointer_traits<P>.
Comment 17 Jonathan Wakely 2021-04-20 20:11:24 UTC
(In reply to Giuseppe D'Angelo from comment #14)
> To summarize:
> 
> * should a wording defect be raised against std::to_address(Ptr), to state
> that pointer_traits<Ptr> being well-formed is actually a prerequisite?

I'd prefer if pointer_traits was just SFINAE friendly.

> * should LWG3446's resolution be amended?

See https://cplusplus.github.io/LWG/issue3541

> * if there's going to be a GCC 10.3, is the commit above solving LWG3446
> going to be cherry-picked into it? Otherwise, either one blacklists GCC 10,
> or has to specialize pointer_traits there as a workaround (?).

It missed the 10.3 release, but it's on the gcc-10 branch as r10-9698, which will be in GCC 10.4:
https://gcc.gnu.org/g:32a859531e854382c18abf0b14a306d83f793eb5
That also includes the fix for LWG 3541.
Comment 18 Giuseppe D'Angelo 2021-04-21 08:45:51 UTC
Hello,

(In reply to Jonathan Wakely from comment #17)
> (In reply to Giuseppe D'Angelo from comment #14)
> > To summarize:
> > 
> > * should a wording defect be raised against std::to_address(Ptr), to state
> > that pointer_traits<Ptr> being well-formed is actually a prerequisite?
> 
> I'd prefer if pointer_traits was just SFINAE friendly.

I guess that's a reasonable thing to wish for, given I'm not the first falling for it; I hope I'll be the last :)

> > * should LWG3446's resolution be amended?
> 
> See https://cplusplus.github.io/LWG/issue3541
> 
> > * if there's going to be a GCC 10.3, is the commit above solving LWG3446
> > going to be cherry-picked into it? Otherwise, either one blacklists GCC 10,
> > or has to specialize pointer_traits there as a workaround (?).
> 
> It missed the 10.3 release, but it's on the gcc-10 branch as r10-9698, which
> will be in GCC 10.4:
> https://gcc.gnu.org/g:32a859531e854382c18abf0b14a306d83f793eb5
> That also includes the fix for LWG 3541.

Thank you very much for the new issue and the cherry-pick of the fix.
Comment 19 gcc-bugs 2021-08-05 22:55:56 UTC
> I guess that's a reasonable thing to wish for, given I'm not the first
> falling for it; I hope I'll be the last :)
> 

Unfortunately not, I ran into the same issue :(

But thanks to the thread, I got some insight into the question: Should every type with an "operator->()" really be considered a pointer?
Comment 20 Jonathan Wakely 2021-09-28 14:00:35 UTC
Reopening and suspending, until https://wg21.link/lwg3545 is resolved.
Comment 21 CVS Commits 2021-11-25 23:12:28 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:b8018e5c5ec0e9b6948182f13fba47c67b758d8a

commit r12-5532-gb8018e5c5ec0e9b6948182f13fba47c67b758d8a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 25 16:49:45 2021 +0000

    libstdc++: Make std::pointer_traits SFINAE-friendly [PR96416]
    
    This implements the resolution I'm proposing for LWG 3545, to avoid hard
    errors when using std::to_address for types that make pointer_traits
    ill-formed.
    
    Consistent with std::iterator_traits, instantiating std::pointer_traits
    for a non-pointer type will be well-formed, but give an empty type with
    no member types. This avoids the problematic cases for std::to_address.
    Additionally, the pointer_to member is now only declared when the
    element type is not cv void (and for C++20, when the function body would
    be well-formed). The rebind member was already SFINAE-friendly in our
    implementation.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/96416
            * include/bits/ptr_traits.h (pointer_traits): Reimplement to be
            SFINAE-friendly (LWG 3545).
            * testsuite/20_util/pointer_traits/lwg3545.cc: New test.
            * testsuite/20_util/to_address/1_neg.cc: Adjust dg-error line.
            * testsuite/20_util/to_address/lwg3545.cc: New test.
Comment 22 CVS Commits 2021-11-26 17:46:00 UTC
The releases/gcc-11 branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:8d3391d64799d490117ad48432a9ad2cf38b0091

commit r11-9317-g8d3391d64799d490117ad48432a9ad2cf38b0091
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 25 23:29:08 2021 +0000

    libstdc++: Make std::pointer_traits SFINAE-friendly [PR96416]
    
    This is a simplified version of r12-5532 for the release branches.  It
    still removes the problematic static_assert, but rather than making
    std::pointer_traits completely empty when the element_type can't be
    deduced, it just disables element_type and pointer_to. Additionally, the
    pointer_to member is not completely absent when element_type is cv void,
    it just has an unusable signature.
    
    This is sufficient to avoid errors outside the immediate context when
    trying to use std::to_address.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/96416
            * include/bits/ptr_traits.h (pointer_traits): Remove
            static_assert checking for valid element_type.
            (pointer_traits::element_type, pointer_traits::pointer_to):
            Do not define when element type cannot be deduced.
            * testsuite/20_util/pointer_traits/lwg3545.cc: New test.
            * testsuite/20_util/to_address/1_neg.cc: Adjust dg-error line.
            * testsuite/20_util/to_address/lwg3545.cc: New test.
    
    (cherry picked from commit b8018e5c5ec0e9b6948182f13fba47c67b758d8a)
Comment 23 Jonathan Wakely 2021-11-26 17:52:25 UTC
Fixed for 11.3 now.

If any of you who hit this bug could test GCC trunk or the gcc-11 branch and confirm it works for your cases now, that would be much appreciated (there are no actual testcases here in this bug).
Comment 24 Zach Laine 2021-12-09 01:24:55 UTC
Sorry for the delay.  I confirmed that this makes my case well-formed with releases/gcc-11, and that it's ill-formed with GCC 11.2 and GCC 10.x.
Comment 25 Jakub Jelinek 2022-06-28 10:41:28 UTC
GCC 10.4 is being released, retargeting bugs to GCC 10.5.