[libcxx-commits] [PATCH] D132149: [String] Allow fancy pointer as pointer type of basic_string allocator

[Brendan Emery via Phabricator via libcxx-commits]

bemeryesr added a comment.

Hi @philnik. When you get a chance, could you please have a look at my PR? I made a few changes as mentioned in my comment a while ago. I also updated all the unit tests to also use a fancy pointer allocator. Thanks!

In D132149#3799799 <https://reviews.llvm.org/D132149#3799799>, @bemeryesr wrote:

> Thanks for the feedback! Regarding your OffsetPtr example:
>
> 1. My initial code was broken as it was calling the copy assignment operator of `__long` even if `__rep` was a short string. For an offset pointer, the copy assignment operator recalculates the offset to the pointed-to address and so the copied value will be different to the original. Hence, it failed for short strings. So now I perform the copy depending on whether it's a short or a long string.
> 2. As I think you alluded to, it seems that some of the pointer arithmetic is optimized-away by the compiler, leading to incorrect behaviour. In boost::interprocess::offset_ptr they mentioned this: "Note: using the address of a local variable to point to another address is not standard conforming and this can be optimized-away by the compiler. Non-inlining is a method to remain illegal but correct." To deal with this, we used a couple of tricks that prevent the functions from being inlined and the test passes. Would it make sense to add this code as a unit test as is? Or perhaps we should find a better way to deal with the UB? This is the code that works for me:
>
>
>
>   // main.cpp
>   //===----------------------------------------------------------------------===//
>   //
>   // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
>   // See https://llvm.org/LICENSE.txt for license information.
>   // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
>   //
>   //===----------------------------------------------------------------------===//
>   
>   #include <cassert>
>   #include <string>
>   
>   #include "OffsetPtr.h"
>   
>   int main()
>   {
>       using string = std::basic_string<char, std::char_traits<char>, OffsetPtrAllocator<char>>;
>       {
>           string str3 = "almost long string";
>           string str4 = std::move(str3);
>           assert(str4 == "almost long string");
>       }
>   }
>
>
>
>   // OffsetPtr.h
>   #ifndef OFFSETPTR_H
>   #define OFFSETPTR_H
>   
>   #include <cassert>
>   #include <cstdint>
>   #include <iostream>
>   #include <iterator>
>   #include <type_traits>
>   #include <utility>
>   
>   namespace detail
>   {
>   using local_difference_type = std::ptrdiff_t;
>   
>   local_difference_type SubtractPointersBytesImpl(const volatile void* ptr1, const volatile void* ptr2);
>   
>   // Alias for checking if template types are of the same type after stripping const/volatile qualifiers.
>   template <typename T1, typename T2>
>   using cv_stripped_types_equal =
>       typename std::is_same<typename std::remove_cv<T1>::type, typename std::remove_cv<T2>::type>;
>   
>   template <typename T1, typename T2, typename = std::enable_if_t<cv_stripped_types_equal<T1, T2>::value>>
>   constexpr local_difference_type SubtractPointersBytes(T1* const ptr1, T2* const ptr2)
>   {
>       return SubtractPointersBytesImpl(ptr1, ptr2);
>   }
>   
>   template <typename T>
>   constexpr const T* __attribute__((noinline)) AddOffsetToPointer(const T* const ptr, local_difference_type offset)
>   {
>       return reinterpret_cast<const T*>(reinterpret_cast<const std::uint8_t*>(ptr) + offset);
>   }
>   
>   template <typename T>
>   constexpr const T* __attribute__((noinline)) AddOffsetToPointer(T* const ptr, local_difference_type offset)
>   {
>       return reinterpret_cast<T*>(reinterpret_cast<std::uint8_t*>(ptr) + offset);
>   }
>   
>   }  // namespace detail
>   
>   template <class T>
>   class alignas(std::max(alignof(T), alignof(std::ptrdiff_t))) OffsetPtr
>   {
>       std::ptrdiff_t offset;
>   
>     public:
>       using value_type = T;
>       using iterator_category = std::random_access_iterator_tag;
>       using difference_type = detail::local_difference_type;
>       using pointer = OffsetPtr;
>       using reference = T&;
>   
>       constexpr OffsetPtr() = default;
>       constexpr OffsetPtr(const OffsetPtr& other)
>           : offset(detail::SubtractPointersBytes(other.operator->(), reinterpret_cast<T*>(this)))
>       {
>       }
>   
>       constexpr operator OffsetPtr<const T>() const
>       {
>           return OffsetPtr<const T>{detail::AddOffsetToPointer(reinterpret_cast<const T*>(this), offset)};
>       }
>   
>       constexpr OffsetPtr& operator=(const OffsetPtr& other)
>       {
>           T* ptr = other.operator->();
>           offset = detail::SubtractPointersBytes(ptr, reinterpret_cast<T*>(this));
>           return *this;
>       }
>   
>       constexpr explicit OffsetPtr(T* ptr) : offset(detail::SubtractPointersBytes(ptr, reinterpret_cast<T*>(this))) {}
>   
>       constexpr T* operator->() const
>       {
>           return const_cast<T*>(detail::AddOffsetToPointer(reinterpret_cast<T*>(const_cast<OffsetPtr*>(this)), offset));
>       }
>   
>       constexpr T& operator[](size_t i) { return operator->()[i]; }
>   
>       static OffsetPtr pointer_to(T& e) { return OffsetPtr(&e); }
>   
>       friend OffsetPtr operator+(const OffsetPtr& lhs, size_t rhs)
>       {
>           return OffsetPtr{detail::AddOffsetToPointer(lhs.operator->(), rhs)};
>       }
>   };
>   
>   static_assert(sizeof(OffsetPtr<int>) == 8, "");
>   
>   template <class T>
>   class OffsetPtrAllocator
>   {
>     public:
>       using value_type = T;
>       using pointer = OffsetPtr<T>;
>   
>       pointer allocate(size_t n) { return pointer(std::allocator<T>{}.allocate(n)); }
>   
>       void deallocate(pointer ptr, size_t n) { std::allocator<T>{}.deallocate(ptr.operator->(), n); }
>   };
>   
>   #endif  // OFFSETPTR_H
>
>   // OffsetPtr.cpp
>   #include "OffsetPtr.h"
>   
>   namespace detail
>   {
>   
>   local_difference_type SubtractPointersBytesImpl(const volatile void* const ptr1, const volatile void* const ptr2)
>   {
>       return local_difference_type(reinterpret_cast<local_difference_type>(ptr1) -
>                                    reinterpret_cast<local_difference_type>(ptr2));
>   }
>   
>   }  // namespace detail




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132149/new/

https://reviews.llvm.org/D132149