On Sat, 15 Jun 2024 at 14:04, François Dumont <frs.dum...@gmail.com> wrote: > > Here is the simplified patch then.
The use of std::__to_address seems wrong. The allocator returns a __buckets_ptr, and that function returns a __buckets_ptr, so it should just be returned unchanged, not by converting to a raw pointer with __to_address. > > libstdc++: Do not use memset in _Hashtable buckets allocation > > Using memset is incorrect if the __bucket_ptr type is non-trivial, or > does not use an all-zero bit pattern for its null value. > > Replace the use of memset with std::__uinitialized_default_n to set the > pointers to nullptr. Doing so and corresponding std::_Destroy_n > when deallocating > buckets. > > libstdc++-v3/ChangeLog: > > * include/bits/hashtable_policy.h > (_Hashtable_alloc::_M_allocate_buckets): Do not use memset > to zero > out bucket pointers. > (_Hashtable_alloc::_M_deallocate_buckets): Add destroy of > buckets. > > Tested under Linux x64, ok to commit ? > > François > > On 13/06/2024 20:58, Jonathan Wakely wrote: > > On Thu, 13 Jun 2024 at 19:57, Jonathan Wakely <jwak...@redhat.com> wrote: > >> On Thu, 13 Jun 2024 at 18:40, François Dumont <frs.dum...@gmail.com> wrote: > >>> Hi > >>> > >>> Following your recent change here: > >>> > >>> https://gcc.gnu.org/pipermail/libstdc++/2024-June/058998.html > >>> > >>> I think we also need to fix the memset at bucket allocation level. > >>> > >>> I did it trying also to be more fancy pointer friendly by running > >>> __uninitialized_default_n_a on the allocator returned pointer rather > >>> than on the __to_address result. I wonder if an __uninitialized_fill_n_a > >>> would have been better ? Doing so I also had to call std::_Destroy on > >>> deallocation. Let me know if it is too early. > >> You don't need the RAII guard. Initializing Alloc::pointer isn't > >> allowed to throw exceptions: > >> > >> "An allocator type X shall meet the Cpp17CopyConstructible > >> requirements (Table 32). The XX::pointer, > >> XX::const_pointer, XX::void_pointer, and XX::const_void_pointer types > >> shall meet the Cpp17Nullable- > >> Pointer requirements (Table 36). No constructor, comparison operator > >> function, copy operation, move > >> operation, or swap operation on these pointer types shall exit via an > >> exception." > >> > >> And you should not pass the allocator to the __uninitialized_xxx call, > >> nor the _Destroy call. We don't want to use the allocator's > >> construct/destroy members for those pointers. They are not container > >> elements. > >> > >> I think either uninitialized_fill_n with nullptr or > >> __uninitialized_default_n is fine. Not the _a forms taking an > >> allocator though. > > And I'd use _Destroy_n(_M_buckets, _M_bucket_count) > > > > > >>> I also wonder if the compiler will be able to optimize it to a memset > >>> call ? I'm interested to work on it if you confirm that it won't. > >> It will do whatever is fastest, which might be memset or might be > >> vectorized code to zero it out (which is probably what libc memset > >> does too). > >> > >>> libstdc++: Do not use memset in _Hashtable buckets allocation > >>> > >>> Using memset is incorrect if the __bucket_ptr type is non-trivial, or > >>> does not use an all-zero bit pattern for its null value. > >>> > >>> Replace the use of memset with std::__uinitialized_default_n_a to set the > >>> pointers to nullptr. Doing so and corresponding std::_Destroy when > >>> deallocating > >>> buckets. > >>> > >>> libstdc++-v3/ChangeLog: > >>> > >>> * include/bits/hashtable_policy.h > >>> (_Hashtable_alloc::_M_allocate_buckets): Do not use memset to zero > >>> out bucket pointers. > >>> (_Hashtable_alloc::_M_deallocate_buckets): Add destroy of buckets. > >>> > >>> > >>> I hope you won't ask for copy rights on the changelog entry :-) > >>> > >>> Tested under Linux x64, ok to commit ? > >>> > >>> François
