From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id C322B386F453 for ; Wed, 26 Aug 2020 16:45:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C322B386F453 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-91-Jpd9Zqf8MWObXEwWcDj1_A-1; Wed, 26 Aug 2020 12:45:16 -0400 X-MC-Unique: Jpd9Zqf8MWObXEwWcDj1_A-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 08F8310ABDBF; Wed, 26 Aug 2020 16:45:15 +0000 (UTC) Received: from localhost (unknown [10.33.36.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6A89319C58; Wed, 26 Aug 2020 16:45:14 +0000 (UTC) Date: Wed, 26 Aug 2020 17:45:13 +0100 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: [PATCH][Hashtable 5/6] Remove H1/H2 template parameters Message-ID: <20200826164513.GM3400@redhat.com> References: <93d1a4b7-bbea-2766-9a71-6ba1a789d0da@gmail.com> <20200717113538.GM4137376@redhat.com> <8280a7d0-975d-c2e8-b6c2-0149314d9ca2@gmail.com> <20200806092739.GS3400@redhat.com> <554ef124-94aa-f590-f7f6-056c409910e1@gmail.com> <20200825143042.GF3400@redhat.com> <20200826153023.GH3400@redhat.com> <20200826155534.GI3400@redhat.com> <20200826155803.GJ3400@redhat.com> MIME-Version: 1.0 In-Reply-To: <20200826155803.GJ3400@redhat.com> X-Clacks-Overhead: GNU Terry Pratchett X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0.001 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="aSM3KCOUSI0G0tph" Content-Disposition: inline X-Spam-Status: No, score=-14.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Aug 2020 16:45:20 -0000 --aSM3KCOUSI0G0tph Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline On 26/08/20 16:58 +0100, Jonathan Wakely wrote: >On 26/08/20 16:55 +0100, Jonathan Wakely wrote: >>On 26/08/20 16:30 +0100, Jonathan Wakely wrote: >>>I'm seeing new FAILures with this: >>> >>>FAIL: 20_util/function_objects/searchers.cc (test for excess errors) >>>UNRESOLVED: 20_util/function_objects/searchers.cc compilation failed to produce executable >>>FAIL: experimental/functional/searchers.cc (test for excess errors) >>>UNRESOLVED: experimental/functional/searchers.cc compilation failed to produce executable >>> >>>It looks like what you committed is not what you sent for review. The >>>patch sent for review has: >>> >>> /// Specialization: hash function and range-hashing function, no >>> /// caching of hash codes. >>> /// Provides typedef and accessor required by C++ 11. >>> template>>- typename _H1, typename _H2> >>>- struct _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2, >>>- _Default_ranged_hash, false> >>>+ typename _Hash, typename _RangeHash, typename _Unused> >>>+ struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash, >>>+ _Unused, false> >>> : private _Hashtable_ebo_helper<0, _ExtractKey>, >>>- private _Hashtable_ebo_helper<1, _H1>, >>>- private _Hashtable_ebo_helper<2, _H2> >>>+ private _Hashtable_ebo_helper<1, _Hash> >>> { >>> >>> >>>But what you committed has: >>> >>> /// Specialization: hash function and range-hashing function, no >>> /// caching of hash codes. >>> /// Provides typedef and accessor required by C++ 11. >>> template>>- typename _H1, typename _H2> >>>- struct _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2, >>>- _Default_ranged_hash, false> >>>- : private _Hashtable_ebo_helper<0, _ExtractKey>, >>>- private _Hashtable_ebo_helper<1, _H1>, >>>- private _Hashtable_ebo_helper<2, _H2> >>>+ typename _Hash, typename _RangeHash, typename _Unused> >>>+ struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash, >>>+ _Unused, false> >>>+ : private _Hashtable_ebo_helper<0, _Hash> >>> { >>> >>> >>>Note that you've changed the type of the base class from: >>> >>>+ private _Hashtable_ebo_helper<1, _Hash> >>> >>>to >>> >>>+ private _Hashtable_ebo_helper<0, _Hash> >>> >>>This causes an ambiguity: >>> >>>/home/jwakely/src/gcc/build/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/hashtable_policy.h:1706: error: 'std::__detail::_Hashtable_ebo_helper<0, test03()::, true>' is an ambiguous base of 'std::__detail::_Hashtable_base, std::__detail::_Select1st, test03()::, test03()::, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Hashtable_traits >' >>> >>>However, what I don't understand is why we are storing that _Hash type >>>more than once as a base class. That seems wrong (but not something we >>>can change without ABI impact). >> >>Ah, we're not storing it more than once. >> >>The problem is: >> >> template> typename _ExtractKey, typename _Equal, >> typename _H1, typename _H2, typename _Hash, typename _Traits> >> struct _Hashtable_base >> : public _Hash_code_base<_Key, _Value, _ExtractKey, _H1, _H2, _Hash, >> _Traits::__hash_cached::value>, >> private _Hashtable_ebo_helper<0, _Equal> >> >>This has a base of _Hashtable_ebo_helper<0, _Equal> so it used to >>have these bases: >> >>_Hashtable_ebo_helper<0, _ExtractKey> >>_Hashtable_ebo_helper<1, _Hash> >>_Hashtable_ebo_helper<2, _RangeHash> >>_Hashtable_ebo_helper<0, _Equal> >> >>but after your change it has these bases: >> >>_Hashtable_ebo_helper<0, _Hash> >>_Hashtable_ebo_helper<0, _Equal> >> >>In the case >>where _Equal and _Hash are the same type (which is what I was testing >>in the test that fail, because I'm sneaky) that means: >> >>_Hashtable_ebo_helper<0, T> >>_Hashtable_ebo_helper<0, T> >> >>which is obviously ambiguous. >> >>I think the _hash_code_base should still use the index 1 for its base >>class, i.e. _Hashtable_ebo_helper<1, _Hash>. That way we have these: >> >>_Hashtable_ebo_helper<1, _Hash> >>_Hashtable_ebo_helper<0, _Equal> >> >>which works even if they're the same types. > >Here's a minimal test: > >#include > >struct Evil : std::hash, std::equal_to >{ >}; > >int main() >{ > std::unordered_map h; > h.key_eq(); >} > >This fails on current trunk. Fixed by the attached patch. Tested powerpc64le-linux, committed to trunk. --aSM3KCOUSI0G0tph Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" commit 9f9c0549dd42e85e2500ca67cef89dddb142c0c7 Author: Jonathan Wakely Date: Wed Aug 26 17:30:31 2020 libstdc++: Fix regression in hash containers A recent change altered the layout of EBO-helper base classes, resulting in an ambiguity when the hash function and equality predicate are the same type. This modifies the type of one of the base classes, so that we don't get two base classes of the same type. libstdc++-v3/ChangeLog: * include/bits/hashtable_policy.h (_Hash_code_base): Change index of _Hashtable_ebo_helper base class. * testsuite/23_containers/unordered_map/dup_types.cc: New test. diff --git a/libstdc++-v3/include/bits/hashtable_policy.h b/libstdc++-v3/include/bits/hashtable_policy.h index 38bd5ae4e81..0109ef86a7b 100644 --- a/libstdc++-v3/include/bits/hashtable_policy.h +++ b/libstdc++-v3/include/bits/hashtable_policy.h @@ -1190,10 +1190,10 @@ namespace __detail typename _Hash, typename _RangeHash, typename _Unused> struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash, _Unused, false> - : private _Hashtable_ebo_helper<0, _Hash> + : private _Hashtable_ebo_helper<1, _Hash> { private: - using __ebo_hash = _Hashtable_ebo_helper<0, _Hash>; + using __ebo_hash = _Hashtable_ebo_helper<1, _Hash>; // Gives the local iterator implementation access to _M_bucket_index(). friend struct _Local_iterator_base<_Key, _Value, _ExtractKey, @@ -1260,10 +1260,10 @@ namespace __detail typename _Hash, typename _RangeHash, typename _Unused> struct _Hash_code_base<_Key, _Value, _ExtractKey, _Hash, _RangeHash, _Unused, true> - : private _Hashtable_ebo_helper<0, _Hash> + : private _Hashtable_ebo_helper<1, _Hash> { private: - using __ebo_hash = _Hashtable_ebo_helper<0, _Hash>; + using __ebo_hash = _Hashtable_ebo_helper<1, _Hash>; public: typedef _Hash hasher; diff --git a/libstdc++-v3/testsuite/23_containers/unordered_map/dup_types.cc b/libstdc++-v3/testsuite/23_containers/unordered_map/dup_types.cc new file mode 100644 index 00000000000..0a6053eca87 --- /dev/null +++ b/libstdc++-v3/testsuite/23_containers/unordered_map/dup_types.cc @@ -0,0 +1,29 @@ +// Copyright (C) 2020 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-do compile { target c++11 } } + +#include + +struct Evil : std::hash, std::equal_to +{ }; + +void test01() +{ + std::unordered_map h; + (void) h.key_eq(); +} --aSM3KCOUSI0G0tph--