This is an archive of the discontinued LLVM Phabricator instance.

[libc++] std::to_address mustn't depend on P::element_type.
ClosedPublic

Authored by ldionne on Apr 30 2021, 8:10 AM.

Details

Summary

This came out of @zoecarver's observation on D101404 that std::to_address wasn't working correctly with a type such as vector<int>::iterator whose element_type was being deduced wrongly. std::to_address actually shouldn't care about element_type at all.

[libc++] std::to_address mustn't depend on P::element_type.

Also, make sure it simulates an `auto` return type correctly.

[libc++] Prefer __to_address(it) over addressof(*it), for UBSAN's benefit.

UBSAN doesn't like `addressof(*it)` when `it` is positioned one-past-the-end
of an array, because that expression binds a reference to a non-object.
On the other hand, we have an overload of `__to_address` for raw pointer types
that Just Works; and we can expect that any user-provided fancy pointer type
will implement `operator->` so that `__to_address` will work just as well
(or better) than `operator*`.

This fixes a UBSAN failure in the previous commit's new test:

    iterator:1343:42: runtime error: reference binding to address 0x[...]
    with insufficient space for an object of type 'const char'
    [...]
      in std::__1::__wrap_iter<char const*>::operator->() const
      in std::__1::__to_address_helper<false>
      in std::__1::__to_address<std::__1::__wrap_iter<char const*> >
      in auto std::__1::to_address<std::__1::__wrap_iter<char const*> >
      in void test_container_iterators<std::__1::span<char const> >

The aforementioned failing test is here: https://buildkite.com/llvm-project/libcxx-ci/builds/2924

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Apr 30 2021, 8:10 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 30 2021, 8:10 AM
libcxx/include/__memory/pointer_traits.h
200–210

I really want to pull this specialization up to right below the primary template, so that all the definitions of __to_address_helper are grouped together. However, that doesn't work, for some reason (probably somehow related to two-phase lookup).

zoecarver accepted this revision as: zoecarver.Apr 30 2021, 11:17 AM

This is great, thanks for working on this (and related)! I think this will fix some of the sfinae issues I was having with contiguous_iterator.

Do you know if __wap_iter plays nice with to_address after this patch? I suspect it will because it won't try to use element_type to determine the return type (it will just use operator->).

libcxx/include/__memory/pointer_traits.h
169

(Non-blocking) Could we get rid of __return_type and just make this auto everywhere? We'd have to change __choose_to_address to something like _IsValidExpansion<decltype(pointer_traits<_Pointer>::to_address(declval<const _Pointer&>()))>. I don't know if that would work but wdyt?

libcxx/test/std/utilities/memory/pointer.conversion/to_address.pass.cpp
58

Not sure if this is substantially different, what about a test where element_type = int*?

Add libcxx-specific tests for _VSTD::__to_address.
Merge my new tests from D101404 — indeed I believe they pass now and D101404 might be obsolete (if still maybe containing some good ideas for tech debt reduction).

Quuxplusone edited the summary of this revision. (Show Details)Apr 30 2021, 11:32 AM
Quuxplusone added inline comments.
libcxx/include/__memory/pointer_traits.h
169

I tried a bunch of simplifications locally and everything I tried eventually ran into some template-metaprogramming issue. Patches welcome (but test your idea locally because I bet it won't work for some obscure reason).

libcxx/test/std/utilities/memory/pointer.conversion/to_address.pass.cpp
58

The point of P5/P6 is just to test that auto to_address in fact returns a decayed type (distinguishing from decltype(auto) to_address which is effectively what we had before this patch). P5 tests the operator-> case; P6 tests the pointer_traits::to_address case.

Still LGTM. Thanks again for working on this!

Merge my new tests from D101404 — indeed I believe they pass now and D101404 might be obsolete (if still maybe containing some good ideas for tech debt reduction).

Great that these are passing now. I know it took a bit of iteration, but I think this is the "right" fix for the problem :) Always satisfying. Anyway, I agree it would probably be good to keep some of the cleanups from the other patch (but as a nfc commit or something).

libcxx/test/libcxx/utilities/memory/pointer.conversion/to_address.pass.cpp
2

Is this testing something substantially different than libcxx/test/std/utilities/memory/pointer.conversion/to_address.pass.cpp?

libcxx/test/libcxx/utilities/memory/pointer.conversion/to_address.pass.cpp
2

This one tests _VSTD::__to_address in all modes; that one tests std::to_address in C++20 only.
I think it's important to have some relatively explicit coverage of __to_address pre-C++20 (now that its innermost guts are so significantly different in C++20-and-later), and I didn't see any existing coverage for __to_address anywhere.
This file is intentionally a cut-and-paste of the other one with only the underscores changed (and constexpr changed to TEST_CONSTEXPR and so on).

Quuxplusone edited the summary of this revision. (Show Details)
[libc++] Prefer __to_address(it) over addressof(*it), for UBSAN's benefit.

UBSAN doesn't like `addressof(*it)` when `it` is positioned one-past-the-end
of an array, because that expression binds a reference to a non-object.
On the other hand, we have an overload of `__to_address` for raw pointer types
that Just Works; and we can expect that any user-provided fancy pointer type
will implement `operator->` so that `__to_address` will work just as well
(or better) than `operator*`.

This fixes a UBSAN failure in the previous commit's new test:

    iterator:1343:42: runtime error: reference binding to address 0x[...]
    with insufficient space for an object of type 'const char'
    [...]
      in std::__1::__wrap_iter<char const*>::operator->() const
      in std::__1::__to_address_helper<false>
      in std::__1::__to_address<std::__1::__wrap_iter<char const*> >
      in auto std::__1::to_address<std::__1::__wrap_iter<char const*> >
      in void test_container_iterators<std::__1::span<char const> >
Quuxplusone marked 3 inline comments as done.May 2 2021, 11:10 AM
Quuxplusone planned changes to this revision.May 3 2021, 8:28 AM

I'm stumped as to how to make my new to_address tests pass with both UBSan and _LIBCPP_DEBUG=1 (individually). I can get one or the other via different techniques, but haven't hit on the technique that will make them both (individually) work.
Actually I think I could do it with a partial specialization of pointer_traits<__wrap_iter<T>>; but I really resist that because that's just awful.

I would be willing to commit just the include/ diff, without the new tests, if that is ever needed in order to unblock @zoecarver or anyone. Just ping here, I guess.

zoecarver added inline comments.May 3 2021, 12:54 PM
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

@Quuxplusone this is the reason the test is failing in debug mode, we can't dereference end. Do we ever really want to be dereferencing end anyway, isn't that UB? I don't think we need to test this, at least not in debug mode.

zoecarver added inline comments.
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

Actually on second thought: we're not actually dereferencing anything here. I guess when the assertion was added to operator-> they didn't expect the caller of the operator to be using the pointer itself. So I feel like there's a good argument to remove the assertion all together. @EricWF, thoughts?

Quuxplusone added inline comments.May 3 2021, 3:49 PM
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

FYI @zoecarver, I believe you've got it now. But I don't think "remove the assertion" is correct. I think, (well, as you know I think) pointer_traits was just a dumb place to put this customization point (it should have been ADL like swap), but, I think the whole intention of letting the user customize to_address was precisely to give them a way to solve this exact problem. They have an operator-> that is unsafe to call on non-dereferenceable pointers, so they should customize to_address.

"They/the user" here means __wrap_iter, of course, which means us.

IOW, the answer is clearly to have libc++ provide a partial specialization of pointer_traits<__wrap_iter<U>> and maintain it forever. That's clearly the right thing to do. I resist it only because it's so, so dumb.

zoecarver added inline comments.May 4 2021, 11:33 AM
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

FYI @zoecarver, I believe you've got it now. But I don't think "remove the assertion" is correct. I think, (well, as you know I think) pointer_traits was just a dumb place to put this customization point (it should have been ADL like swap), but, I think the whole intention of letting the user customize to_address was precisely to give them a way to solve this exact problem. They have an operator-> that is unsafe to call on non-dereferenceable pointers, so they should customize to_address.

"They/the user" here means __wrap_iter, of course, which means us.

IOW, the answer is clearly to have libc++ provide a partial specialization of pointer_traits<__wrap_iter<U>> and maintain it forever. That's clearly the right thing to do. I resist it only because it's so, so dumb.

Okay... I think you're right. Even though, as you said, this is super dumb.

However, we should still take this change, because it does fix something that was broken. We just also need to keep the specialization of to_address. I think we also need to add an element_type to the specialization of pointer_traits, otherwise it's ill formed.

Thanks for fixing this. This LGTM, but we need to figure out how to write the tests correctly.

libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

It's actually not clear to me that this test is correct. I don't see why we're allowed to call to_address on a one-past-the-end iterator of a container. Imagine another implementation uses a different type of their own to implement e.g. std::vector<T>::iterator, then there would be no guarantee that we can use std::to_address on that at all, since vector<T>::iterator is only required to be a LegacyRandomAccessIterator (which doesn't even include to_address). Am I mistaken here?

ldionne added inline comments.May 4 2021, 12:49 PM
libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp
32

Nevermind this, spoke with Zoe offline and we do indeed want to specialize pointer_traits for __wrap_iter.

ldionne commandeered this revision.May 4 2021, 1:11 PM
ldionne edited reviewers, added: Quuxplusone; removed: ldionne.

Commandeering since this has become pretty complicated. We have two issues:

  1. std::to_address is broken and uses T::element_type when it shouldn't.
  2. std::vector<T>::iterator is not a contiguous_iterator because it doesn't support std::to_address.

This patch fixes (1), and not (2).

ldionne updated this revision to Diff 342840.May 4 2021, 1:13 PM

Mark failing test as UNSUPPORTED in debug mode, to be fixed once we fix std::vector::iterator.

ldionne updated this revision to Diff 342850.May 4 2021, 1:25 PM

Poke BuildKite after trying to fix it

ldionne accepted this revision.May 4 2021, 1:58 PM

The Debug Iterator job has passed, shipping. Thanks for your work on this Arthur (and others), let's fix __wrap_iter in D101404 after rebasing onto this patch.

This revision is now accepted and ready to land.May 4 2021, 1:58 PM
This revision was landed with ongoing or failed builds.May 4 2021, 1:59 PM
This revision was automatically updated to reflect the committed changes.
ldionne reopened this revision.May 5 2021, 9:46 AM
This revision is now accepted and ready to land.May 5 2021, 9:46 AM
ldionne updated this revision to Diff 343092.May 5 2021, 9:46 AM

Fix Clang failure and add test case. Basically the issue was a subtle
case where we'd instantiate SFINAE checks too early even when the raw
pointer should have been selected right away.

That is not an issue when writing to_address to the spec, because it uses
auto-deduced return type instead, which doesn't require instantiating the
fancy pointer checking helper class during template argument deduction.

ldionne added inline comments.May 5 2021, 9:48 AM
libcxx/include/__memory/pointer_traits.h
227–228

Does anybody see an issue with doing this? I don't think this is detectable, so we should be conforming?

libcxx/test/std/utilities/memory/pointer.conversion/to_address.pass.cpp
68–78

This is the reduction of what triggered a build breakage in Clang yesterday.

Discussed offline and gave Louis a couple more test cases to merge in here.

libcxx/include/__memory/pointer_traits.h
227–228

Is "this" the perfect-forwarding? I think it's reasonably undetectable, but also unneeded. I just changed this back to

auto
to_address(const _Pointer& __p) _NOEXCEPT
{
    return _VSTD::__to_address(__p);
}

locally, and it still passed all of our new tests with flying colors. So I strongly recommend either changing it back, or (if I'm missing something) adding a test case that "explains" the purpose of the forwarding reference.

ldionne updated this revision to Diff 343397.May 6 2021, 7:14 AM
ldionne marked 4 inline comments as done.

Apply Arthur's comments

This revision was landed with ongoing or failed builds.May 6 2021, 7:15 AM
This revision was automatically updated to reflect the committed changes.