This is an archive of the discontinued LLVM Phabricator instance.

Use the allocator's pointers for deallocation in `std::list`
AbandonedPublic

Authored by Quuxplusone on Jul 25 2017, 3:56 PM.

Details

Reviewers
EricWF
Summary

pointer_traits<P>::to_pointer(r) is not required to return a deallocatable pointer; indeed generally it *cannot* determine a deallocatable representation without help from the allocator. Therefore, we must not rely on representations derived from to_pointer when deallocating; we must pass to deallocate the exact same pointer (or at least a pointer linearly derived from the pointer) that we originally received from allocate.

I have an example fancy-pointer allocator here which might be convertible into a test case:
https://github.com/Quuxplusone/from-scratch/blob/master/include/scratch/bits/containers/segmented-allocator.h

Diff Detail

Event Timeline

EricWF requested changes to this revision.Aug 2 2017, 4:38 PM

First, this patch would need tests before continuing, Could you please them? The list tests live under test/std/containers/sequences/list. Let me know if you have any questions or need any help writing tests.

That being said. I'm not sure this patch is viable (or currently correct). First, LWG 2260 was resolved at the Toronto meeting by requiring ::pointer_to(r) to be valid on allocator pointers, so I'm not sure your point about "not returning a deallocatable pointer" is valid anymore. In fact this seems to suggest that implementations should store raw pointers (since fancy-pointers don't require base <--> derived conversions), and then use pointer_to to generate the fancy-pointer for deallocation.

I understand the above behavior is quite limiting for fancy-pointers, since as you say, fancy-pointers often depend on information stored in the allocator in order to be constructed. However this point would have to be addressed to the committee in order for anything to be done about it.

Even if your point were valid, I'm not sure it's possible to correctly remove our dependency on pointer_to (though you're free to try). Leaving even one call to pointer_to within list seems like the manifestation of a bug.

Another problem to addressing this issue is that the pointer types used in list, map, unordered_map, ect are different in ABI v1 and ABI v2, and correctly converting between them is overly complicated and easy to get wrong.

This revision now requires changes to proceed.Aug 2 2017, 4:38 PM
Quuxplusone edited edge metadata.

I've updated D35863 to be actually correct AFAICT from my local testing; but I'm not sure what's the most appropriate way to get "fancy allocator" tests into libcxx's test suite. The way I did it locally is here:
https://github.com/Quuxplusone/libcxx/pull/1/files
Basically, I conditionally replace "test_allocator.h" with "test_fancy_allocator.h", and then re-run all the existing tests. "test_fancy_allocator" uses fancy pointers that carry with them a "payload" of the allocated size n, and then in a.deallocate(p,n) we assert that p.payload()==n. A bunch of list tests fail this assertion before this patch, and none fail after this patch.

@EricWF re our Slack conversation yesterday afternoon: Am I correct that changing the fancy pointer to a raw pointer in iterator::operator->() like this would be an ABI-breaking change and therefore a non-starter? https://github.com/Quuxplusone/libcxx/commit/c80f4ffad2ad04e8749b162629255b191c896b4f

Even if *that* change were friendly, would I be correct that changing the fancy pointer to a raw pointer in the data member iterator::__ptr_ would be an ABI-breaking change and therefore a non-starter? (That would also require more drastic surgery in list::erase(p), because we could no longer trust p.__ptr_ to be deallocatable; we'd have to compute q = p.__ptr_->__prev_->__next_ and deallocate *that*. I have not attempted to write that patch, because I assume it'd be ABI-breaking anyway.)

Quuxplusone abandoned this revision.Dec 19 2021, 7:44 AM