| Message ID | 20170323174905.GG4425@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/05/19 23:42 +0100, Jonathan Wakely wrote: >On 23/03/17 17:49 +0000, Jonathan Wakely wrote: >>On 12/03/17 13:16 +0100, Daniel Krügler wrote: >>>The following is an *untested* patch suggestion, please verify. >>> >>>Notes: My interpretation is that hash<error_condition> should be >>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please >>>double-check that course of action. >> >>That's right. >> >>>I noticed that the preexisting hash<error_code> did directly refer to >>>the private members of error_code albeit those have public access >>>functions. For consistency I mimicked that existing style when >>>implementing hash<error_condition>. >> >>I see no reason for that, so I've removed the friend declaration and >>used the public member functions. > >I'm going to do the same for hash<error_code> too. It can also use the >public members instead of being a friend. > > >>Although this is a DR, I'm treating it as a new C++17 feature, so I've >>adjusted the patch to only add the new specialization for C++17 mode. >>We're too close to the GCC 7 release to be adding new things to the >>default mode, even minor things like this. After GCC 7 is released we >>can revisit it and decide if we want to enable it for all modes. > >We never revisited that, and it's still only enabled for C++17 and up. >I guess that's OK, but we could enabled it for C++11 and 14 on trunk >if we want. Anybody care enough to argue for that? > >>Here's what I've tested and will be committing. >> >> > >>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 >>Author: Jonathan Wakely <jwakely@redhat.com> >>Date: Thu Mar 23 11:47:39 2017 +0000 >> >> Implement LWG 2686, std::hash<error_condition>, for C++17 >> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> >> Implement LWG 2686, Why is std::hash specialized for error_code, >> but not error_condition? >> * include/std/system_error (hash<error_condition>): Define for C++17. >> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): >> Instantiate test for error_condition. >> * testsuite/20_util/hash/requirements/explicit_instantiation.cc >> (hash<error_condition>): Instantiate hash<error_condition>. >> >>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error >>index 6775a6e..ec7d25f 100644 >>--- a/libstdc++-v3/include/std/system_error >>+++ b/libstdc++-v3/include/std/system_error >>@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>_GLIBCXX_END_NAMESPACE_VERSION >>} // namespace >> >>-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>- >>#include <bits/functional_hash.h> >> >>namespace std _GLIBCXX_VISIBILITY(default) >>{ >>_GLIBCXX_BEGIN_NAMESPACE_VERSION >> >>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >> // DR 1182. >> /// std::hash specialization for error_code. >> template<> >>@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); >> } >> }; >>+#endif // _GLIBCXX_COMPATIBILITY_CXX0X >>+ >>+#if __cplusplus > 201402L >>+ // DR 2686. >>+ /// std::hash specialization for error_condition. >>+ template<> >>+ struct hash<error_condition> >>+ : public __hash_base<size_t, error_condition> >>+ { >>+ size_t >>+ operator()(const error_condition& __e) const noexcept >>+ { >>+ const size_t __tmp = std::_Hash_impl::hash(__e.value()); >>+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp); > >When I changed this from using __e._M_cat (as in Daniel's patch) to >__e.category() I introduced a bug, because the former is a pointer to >the error_category (and error_category objects are unique and so can >be identified by their address) and the latter is the object itself, >so we hash the bytes of an abstract base class instead of hashing the >pointer to it. Oops. > >Patch coming up to fix that. Here's the fix. Tested powerpc64le-linux, committed to trunk. I'll backport this to 7, 8 and 9 as well.
On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: > > On 03/05/19 23:42 +0100, Jonathan Wakely wrote: > >On 23/03/17 17:49 +0000, Jonathan Wakely wrote: > >>On 12/03/17 13:16 +0100, Daniel Krügler wrote: > >>>The following is an *untested* patch suggestion, please verify. > >>> > >>>Notes: My interpretation is that hash<error_condition> should be > >>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please > >>>double-check that course of action. > >> > >>That's right. > >> > >>>I noticed that the preexisting hash<error_code> did directly refer to > >>>the private members of error_code albeit those have public access > >>>functions. For consistency I mimicked that existing style when > >>>implementing hash<error_condition>. > >> > >>I see no reason for that, so I've removed the friend declaration and > >>used the public member functions. > > > >I'm going to do the same for hash<error_code> too. It can also use the > >public members instead of being a friend. > > > > > >>Although this is a DR, I'm treating it as a new C++17 feature, so I've > >>adjusted the patch to only add the new specialization for C++17 mode. > >>We're too close to the GCC 7 release to be adding new things to the > >>default mode, even minor things like this. After GCC 7 is released we > >>can revisit it and decide if we want to enable it for all modes. > > > >We never revisited that, and it's still only enabled for C++17 and up. > >I guess that's OK, but we could enabled it for C++11 and 14 on trunk > >if we want. Anybody care enough to argue for that? > > > >>Here's what I've tested and will be committing. > >> > >> > > > >>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 > >>Author: Jonathan Wakely <jwakely@redhat.com> > >>Date: Thu Mar 23 11:47:39 2017 +0000 > >> > >> Implement LWG 2686, std::hash<error_condition>, for C++17 > >> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> > >> Implement LWG 2686, Why is std::hash specialized for error_code, > >> but not error_condition? > >> * include/std/system_error (hash<error_condition>): Define for C++17. > >> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): > >> Instantiate test for error_condition. > >> * testsuite/20_util/hash/requirements/explicit_instantiation.cc > >> (hash<error_condition>): Instantiate hash<error_condition>. > >> > >>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error > >>index 6775a6e..ec7d25f 100644 > >>--- a/libstdc++-v3/include/std/system_error > >>+++ b/libstdc++-v3/include/std/system_error > >>@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>_GLIBCXX_END_NAMESPACE_VERSION > >>} // namespace > >> > >>-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >>- > >>#include <bits/functional_hash.h> > >> > >>namespace std _GLIBCXX_VISIBILITY(default) > >>{ > >>_GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > >>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >> // DR 1182. > >> /// std::hash specialization for error_code. > >> template<> > >>@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); > >> } > >> }; > >>+#endif // _GLIBCXX_COMPATIBILITY_CXX0X > >>+ > >>+#if __cplusplus > 201402L > >>+ // DR 2686. > >>+ /// std::hash specialization for error_condition. > >>+ template<> > >>+ struct hash<error_condition> > >>+ : public __hash_base<size_t, error_condition> > >>+ { > >>+ size_t > >>+ operator()(const error_condition& __e) const noexcept > >>+ { > >>+ const size_t __tmp = std::_Hash_impl::hash(__e.value()); > >>+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp); > > > >When I changed this from using __e._M_cat (as in Daniel's patch) to > >__e.category() I introduced a bug, because the former is a pointer to > >the error_category (and error_category objects are unique and so can > >be identified by their address) and the latter is the object itself, > >so we hash the bytes of an abstract base class instead of hashing the > >pointer to it. Oops. > > > >Patch coming up to fix that. > > Here's the fix. Tested powerpc64le-linux, committed to trunk. > > I'll backport this to 7, 8 and 9 as well. > Hi Jonathan, Does the new test lack dg-require-filesystem-ts ? I'm seeing link failures on arm-eabi (using newlib): Excess errors: /libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' /libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' /libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' /libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined reference to `chmod' /libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' /libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' Christophe
On 07/05/19 11:05 +0200, Christophe Lyon wrote: >On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> On 03/05/19 23:42 +0100, Jonathan Wakely wrote: >> >On 23/03/17 17:49 +0000, Jonathan Wakely wrote: >> >>On 12/03/17 13:16 +0100, Daniel Krügler wrote: >> >>>The following is an *untested* patch suggestion, please verify. >> >>> >> >>>Notes: My interpretation is that hash<error_condition> should be >> >>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please >> >>>double-check that course of action. >> >> >> >>That's right. >> >> >> >>>I noticed that the preexisting hash<error_code> did directly refer to >> >>>the private members of error_code albeit those have public access >> >>>functions. For consistency I mimicked that existing style when >> >>>implementing hash<error_condition>. >> >> >> >>I see no reason for that, so I've removed the friend declaration and >> >>used the public member functions. >> > >> >I'm going to do the same for hash<error_code> too. It can also use the >> >public members instead of being a friend. >> > >> > >> >>Although this is a DR, I'm treating it as a new C++17 feature, so I've >> >>adjusted the patch to only add the new specialization for C++17 mode. >> >>We're too close to the GCC 7 release to be adding new things to the >> >>default mode, even minor things like this. After GCC 7 is released we >> >>can revisit it and decide if we want to enable it for all modes. >> > >> >We never revisited that, and it's still only enabled for C++17 and up. >> >I guess that's OK, but we could enabled it for C++11 and 14 on trunk >> >if we want. Anybody care enough to argue for that? >> > >> >>Here's what I've tested and will be committing. >> >> >> >> >> > >> >>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 >> >>Author: Jonathan Wakely <jwakely@redhat.com> >> >>Date: Thu Mar 23 11:47:39 2017 +0000 >> >> >> >> Implement LWG 2686, std::hash<error_condition>, for C++17 >> >> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> >> >> Implement LWG 2686, Why is std::hash specialized for error_code, >> >> but not error_condition? >> >> * include/std/system_error (hash<error_condition>): Define for C++17. >> >> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): >> >> Instantiate test for error_condition. >> >> * testsuite/20_util/hash/requirements/explicit_instantiation.cc >> >> (hash<error_condition>): Instantiate hash<error_condition>. >> >> >> >>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error >> >>index 6775a6e..ec7d25f 100644 >> >>--- a/libstdc++-v3/include/std/system_error >> >>+++ b/libstdc++-v3/include/std/system_error >> >>@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> >>_GLIBCXX_END_NAMESPACE_VERSION >> >>} // namespace >> >> >> >>-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >> >>- >> >>#include <bits/functional_hash.h> >> >> >> >>namespace std _GLIBCXX_VISIBILITY(default) >> >>{ >> >>_GLIBCXX_BEGIN_NAMESPACE_VERSION >> >> >> >>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >> >> // DR 1182. >> >> /// std::hash specialization for error_code. >> >> template<> >> >>@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> >> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); >> >> } >> >> }; >> >>+#endif // _GLIBCXX_COMPATIBILITY_CXX0X >> >>+ >> >>+#if __cplusplus > 201402L >> >>+ // DR 2686. >> >>+ /// std::hash specialization for error_condition. >> >>+ template<> >> >>+ struct hash<error_condition> >> >>+ : public __hash_base<size_t, error_condition> >> >>+ { >> >>+ size_t >> >>+ operator()(const error_condition& __e) const noexcept >> >>+ { >> >>+ const size_t __tmp = std::_Hash_impl::hash(__e.value()); >> >>+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp); >> > >> >When I changed this from using __e._M_cat (as in Daniel's patch) to >> >__e.category() I introduced a bug, because the former is a pointer to >> >the error_category (and error_category objects are unique and so can >> >be identified by their address) and the latter is the object itself, >> >so we hash the bytes of an abstract base class instead of hashing the >> >pointer to it. Oops. >> > >> >Patch coming up to fix that. >> >> Here's the fix. Tested powerpc64le-linux, committed to trunk. >> >> I'll backport this to 7, 8 and 9 as well. >> > >Hi Jonathan, > >Does the new test lack dg-require-filesystem-ts ? It lacks it, because it doesn't use the filesystem library at all. >I'm seeing link failures on arm-eabi (using newlib): >Excess errors: >/libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' >/libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' >/libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' >/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined >reference to `chmod' >/libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' >/libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' > >Christophe
On 07/05/19 10:37 +0100, Jonathan Wakely wrote: >On 07/05/19 11:05 +0200, Christophe Lyon wrote: >>On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: >>> >>>On 03/05/19 23:42 +0100, Jonathan Wakely wrote: >>>>On 23/03/17 17:49 +0000, Jonathan Wakely wrote: >>>>>On 12/03/17 13:16 +0100, Daniel Krügler wrote: >>>>>>The following is an *untested* patch suggestion, please verify. >>>>>> >>>>>>Notes: My interpretation is that hash<error_condition> should be >>>>>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please >>>>>>double-check that course of action. >>>>> >>>>>That's right. >>>>> >>>>>>I noticed that the preexisting hash<error_code> did directly refer to >>>>>>the private members of error_code albeit those have public access >>>>>>functions. For consistency I mimicked that existing style when >>>>>>implementing hash<error_condition>. >>>>> >>>>>I see no reason for that, so I've removed the friend declaration and >>>>>used the public member functions. >>>> >>>>I'm going to do the same for hash<error_code> too. It can also use the >>>>public members instead of being a friend. >>>> >>>> >>>>>Although this is a DR, I'm treating it as a new C++17 feature, so I've >>>>>adjusted the patch to only add the new specialization for C++17 mode. >>>>>We're too close to the GCC 7 release to be adding new things to the >>>>>default mode, even minor things like this. After GCC 7 is released we >>>>>can revisit it and decide if we want to enable it for all modes. >>>> >>>>We never revisited that, and it's still only enabled for C++17 and up. >>>>I guess that's OK, but we could enabled it for C++11 and 14 on trunk >>>>if we want. Anybody care enough to argue for that? >>>> >>>>>Here's what I've tested and will be committing. >>>>> >>>>> >>>> >>>>>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 >>>>>Author: Jonathan Wakely <jwakely@redhat.com> >>>>>Date: Thu Mar 23 11:47:39 2017 +0000 >>>>> >>>>> Implement LWG 2686, std::hash<error_condition>, for C++17 >>>>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> >>>>> Implement LWG 2686, Why is std::hash specialized for error_code, >>>>> but not error_condition? >>>>> * include/std/system_error (hash<error_condition>): Define for C++17. >>>>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): >>>>> Instantiate test for error_condition. >>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc >>>>> (hash<error_condition>): Instantiate hash<error_condition>. >>>>> >>>>>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error >>>>>index 6775a6e..ec7d25f 100644 >>>>>--- a/libstdc++-v3/include/std/system_error >>>>>+++ b/libstdc++-v3/include/std/system_error >>>>>@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>>_GLIBCXX_END_NAMESPACE_VERSION >>>>>} // namespace >>>>> >>>>>-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>>>>- >>>>>#include <bits/functional_hash.h> >>>>> >>>>>namespace std _GLIBCXX_VISIBILITY(default) >>>>>{ >>>>>_GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>> >>>>>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>>>> // DR 1182. >>>>> /// std::hash specialization for error_code. >>>>> template<> >>>>>@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); >>>>> } >>>>> }; >>>>>+#endif // _GLIBCXX_COMPATIBILITY_CXX0X >>>>>+ >>>>>+#if __cplusplus > 201402L >>>>>+ // DR 2686. >>>>>+ /// std::hash specialization for error_condition. >>>>>+ template<> >>>>>+ struct hash<error_condition> >>>>>+ : public __hash_base<size_t, error_condition> >>>>>+ { >>>>>+ size_t >>>>>+ operator()(const error_condition& __e) const noexcept >>>>>+ { >>>>>+ const size_t __tmp = std::_Hash_impl::hash(__e.value()); >>>>>+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp); >>>> >>>>When I changed this from using __e._M_cat (as in Daniel's patch) to >>>>__e.category() I introduced a bug, because the former is a pointer to >>>>the error_category (and error_category objects are unique and so can >>>>be identified by their address) and the latter is the object itself, >>>>so we hash the bytes of an abstract base class instead of hashing the >>>>pointer to it. Oops. >>>> >>>>Patch coming up to fix that. >>> >>>Here's the fix. Tested powerpc64le-linux, committed to trunk. >>> >>>I'll backport this to 7, 8 and 9 as well. >>> >> >>Hi Jonathan, >> >>Does the new test lack dg-require-filesystem-ts ? > >It lacks it, because it doesn't use the filesystem library at all. > >>I'm seeing link failures on arm-eabi (using newlib): >>Excess errors: >>/libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' >>/libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' >>/libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' >>/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined >>reference to `chmod' >>/libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' >>/libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' >> >>Christophe Is it definitely the new 19_diagnostics/error_condition/hash.cc test that's giving this error? I adjusted the pre-existing 27_io/filesystem/operations/absolute.cc test in r270874, which seems a more likely culprit, but that already has dg-require-filesystem-ts.
On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote: > > On 07/05/19 10:37 +0100, Jonathan Wakely wrote: > >On 07/05/19 11:05 +0200, Christophe Lyon wrote: > >>On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: > >>> > >>>On 03/05/19 23:42 +0100, Jonathan Wakely wrote: > >>>>On 23/03/17 17:49 +0000, Jonathan Wakely wrote: > >>>>>On 12/03/17 13:16 +0100, Daniel Krügler wrote: > >>>>>>The following is an *untested* patch suggestion, please verify. > >>>>>> > >>>>>>Notes: My interpretation is that hash<error_condition> should be > >>>>>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please > >>>>>>double-check that course of action. > >>>>> > >>>>>That's right. > >>>>> > >>>>>>I noticed that the preexisting hash<error_code> did directly refer to > >>>>>>the private members of error_code albeit those have public access > >>>>>>functions. For consistency I mimicked that existing style when > >>>>>>implementing hash<error_condition>. > >>>>> > >>>>>I see no reason for that, so I've removed the friend declaration and > >>>>>used the public member functions. > >>>> > >>>>I'm going to do the same for hash<error_code> too. It can also use the > >>>>public members instead of being a friend. > >>>> > >>>> > >>>>>Although this is a DR, I'm treating it as a new C++17 feature, so I've > >>>>>adjusted the patch to only add the new specialization for C++17 mode. > >>>>>We're too close to the GCC 7 release to be adding new things to the > >>>>>default mode, even minor things like this. After GCC 7 is released we > >>>>>can revisit it and decide if we want to enable it for all modes. > >>>> > >>>>We never revisited that, and it's still only enabled for C++17 and up. > >>>>I guess that's OK, but we could enabled it for C++11 and 14 on trunk > >>>>if we want. Anybody care enough to argue for that? > >>>> > >>>>>Here's what I've tested and will be committing. > >>>>> > >>>>> > >>>> > >>>>>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 > >>>>>Author: Jonathan Wakely <jwakely@redhat.com> > >>>>>Date: Thu Mar 23 11:47:39 2017 +0000 > >>>>> > >>>>> Implement LWG 2686, std::hash<error_condition>, for C++17 > >>>>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> > >>>>> Implement LWG 2686, Why is std::hash specialized for error_code, > >>>>> but not error_condition? > >>>>> * include/std/system_error (hash<error_condition>): Define for C++17. > >>>>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): > >>>>> Instantiate test for error_condition. > >>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc > >>>>> (hash<error_condition>): Instantiate hash<error_condition>. > >>>>> > >>>>>diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error > >>>>>index 6775a6e..ec7d25f 100644 > >>>>>--- a/libstdc++-v3/include/std/system_error > >>>>>+++ b/libstdc++-v3/include/std/system_error > >>>>>@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>>_GLIBCXX_END_NAMESPACE_VERSION > >>>>>} // namespace > >>>>> > >>>>>-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >>>>>- > >>>>>#include <bits/functional_hash.h> > >>>>> > >>>>>namespace std _GLIBCXX_VISIBILITY(default) > >>>>>{ > >>>>>_GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>> > >>>>>+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >>>>> // DR 1182. > >>>>> /// std::hash specialization for error_code. > >>>>> template<> > >>>>>@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); > >>>>> } > >>>>> }; > >>>>>+#endif // _GLIBCXX_COMPATIBILITY_CXX0X > >>>>>+ > >>>>>+#if __cplusplus > 201402L > >>>>>+ // DR 2686. > >>>>>+ /// std::hash specialization for error_condition. > >>>>>+ template<> > >>>>>+ struct hash<error_condition> > >>>>>+ : public __hash_base<size_t, error_condition> > >>>>>+ { > >>>>>+ size_t > >>>>>+ operator()(const error_condition& __e) const noexcept > >>>>>+ { > >>>>>+ const size_t __tmp = std::_Hash_impl::hash(__e.value()); > >>>>>+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp); > >>>> > >>>>When I changed this from using __e._M_cat (as in Daniel's patch) to > >>>>__e.category() I introduced a bug, because the former is a pointer to > >>>>the error_category (and error_category objects are unique and so can > >>>>be identified by their address) and the latter is the object itself, > >>>>so we hash the bytes of an abstract base class instead of hashing the > >>>>pointer to it. Oops. > >>>> > >>>>Patch coming up to fix that. > >>> > >>>Here's the fix. Tested powerpc64le-linux, committed to trunk. > >>> > >>>I'll backport this to 7, 8 and 9 as well. > >>> > >> > >>Hi Jonathan, > >> > >>Does the new test lack dg-require-filesystem-ts ? > > > >It lacks it, because it doesn't use the filesystem library at all. > > > >>I'm seeing link failures on arm-eabi (using newlib): > >>Excess errors: > >>/libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' > >>/libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' > >>/libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' > >>/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined > >>reference to `chmod' > >>/libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' > >>/libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' > >> > >>Christophe > > Is it definitely the new 19_diagnostics/error_condition/hash.cc test > that's giving this error? > > I adjusted the pre-existing 27_io/filesystem/operations/absolute.cc > test in r270874, which seems a more likely culprit, but that already > has dg-require-filesystem-ts. > > Yes, and there full errors are: /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): In function `std::filesystem::current_path(std::filesystem::__cxx11::path const&, std::error_code&)': /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): In function `(anonymous namespace)::create_dir(std::filesystem::__cxx11::path const&, std::filesystem::perms, std::error_code&)': /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): In function `std::filesystem::permissions(std::filesystem::__cxx11::path const&, std::filesystem::perms, std::filesystem::perm_options, std::error_code&)': /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): In function `std::filesystem::do_copy_file(char const*, char const*, std::filesystem::copy_options_existing_file, stat*, stat*, std::error_code&)': /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined reference to `chmod' /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): In function `std::filesystem::current_path[abi:cxx11](std::error_code&)': /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd'
On 04/05/19 15:36 +0100, Jonathan Wakely wrote: >On 03/05/19 23:42 +0100, Jonathan Wakely wrote: >>On 23/03/17 17:49 +0000, Jonathan Wakely wrote: >>>On 12/03/17 13:16 +0100, Daniel Krügler wrote: >>>>The following is an *untested* patch suggestion, please verify. >>>> >>>>Notes: My interpretation is that hash<error_condition> should be >>>>defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please >>>>double-check that course of action. >>> >>>That's right. >>> >>>>I noticed that the preexisting hash<error_code> did directly refer to >>>>the private members of error_code albeit those have public access >>>>functions. For consistency I mimicked that existing style when >>>>implementing hash<error_condition>. >>> >>>I see no reason for that, so I've removed the friend declaration and >>>used the public member functions. >> >>I'm going to do the same for hash<error_code> too. It can also use the >>public members instead of being a friend. >> >> >>>Although this is a DR, I'm treating it as a new C++17 feature, so I've >>>adjusted the patch to only add the new specialization for C++17 mode. >>>We're too close to the GCC 7 release to be adding new things to the >>>default mode, even minor things like this. After GCC 7 is released we >>>can revisit it and decide if we want to enable it for all modes. >> >>We never revisited that, and it's still only enabled for C++17 and up. >>I guess that's OK, but we could enabled it for C++11 and 14 on trunk >>if we want. Anybody care enough to argue for that? >> >>>Here's what I've tested and will be committing. >>> >>> >> >>>commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 >>>Author: Jonathan Wakely <jwakely@redhat.com> >>>Date: Thu Mar 23 11:47:39 2017 +0000 >>> >>> Implement LWG 2686, std::hash<error_condition>, for C++17 >>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> >>> Implement LWG 2686, Why is std::hash specialized for error_code, >>> but not error_condition? >>> * include/std/system_error (hash<error_condition>): Define for C++17. >>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): >>> Instantiate test for error_condition. >>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc >>> (hash<error_condition>): Instantiate hash<error_condition>. I'm adding a similar test for hash<error_code> too. Tested powerpc64le-linux, committing to trunk shortly.
On 07/05/2019 13:21, Christophe Lyon wrote: > On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> On 07/05/19 10:37 +0100, Jonathan Wakely wrote: >>> On 07/05/19 11:05 +0200, Christophe Lyon wrote: >>>> On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: >>>>> >>>>> On 03/05/19 23:42 +0100, Jonathan Wakely wrote: >>>>>> On 23/03/17 17:49 +0000, Jonathan Wakely wrote: >>>>>>> On 12/03/17 13:16 +0100, Daniel Krügler wrote: >>>>>>>> The following is an *untested* patch suggestion, please verify. >>>>>>>> >>>>>>>> Notes: My interpretation is that hash<error_condition> should be >>>>>>>> defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please >>>>>>>> double-check that course of action. >>>>>>> >>>>>>> That's right. >>>>>>> >>>>>>>> I noticed that the preexisting hash<error_code> did directly refer to >>>>>>>> the private members of error_code albeit those have public access >>>>>>>> functions. For consistency I mimicked that existing style when >>>>>>>> implementing hash<error_condition>. >>>>>>> >>>>>>> I see no reason for that, so I've removed the friend declaration and >>>>>>> used the public member functions. >>>>>> >>>>>> I'm going to do the same for hash<error_code> too. It can also use the >>>>>> public members instead of being a friend. >>>>>> >>>>>> >>>>>>> Although this is a DR, I'm treating it as a new C++17 feature, so I've >>>>>>> adjusted the patch to only add the new specialization for C++17 mode. >>>>>>> We're too close to the GCC 7 release to be adding new things to the >>>>>>> default mode, even minor things like this. After GCC 7 is released we >>>>>>> can revisit it and decide if we want to enable it for all modes. >>>>>> >>>>>> We never revisited that, and it's still only enabled for C++17 and up. >>>>>> I guess that's OK, but we could enabled it for C++11 and 14 on trunk >>>>>> if we want. Anybody care enough to argue for that? >>>>>> >>>>>>> Here's what I've tested and will be committing. >>>>>>> >>>>>>> >>>>>> >>>>>>> commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 >>>>>>> Author: Jonathan Wakely <jwakely@redhat.com> >>>>>>> Date: Thu Mar 23 11:47:39 2017 +0000 >>>>>>> >>>>>>> Implement LWG 2686, std::hash<error_condition>, for C++17 >>>>>>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> >>>>>>> Implement LWG 2686, Why is std::hash specialized for error_code, >>>>>>> but not error_condition? >>>>>>> * include/std/system_error (hash<error_condition>): Define for C++17. >>>>>>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): >>>>>>> Instantiate test for error_condition. >>>>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc >>>>>>> (hash<error_condition>): Instantiate hash<error_condition>. >>>>>>> >>>>>>> diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error >>>>>>> index 6775a6e..ec7d25f 100644 >>>>>>> --- a/libstdc++-v3/include/std/system_error >>>>>>> +++ b/libstdc++-v3/include/std/system_error >>>>>>> @@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>>>> _GLIBCXX_END_NAMESPACE_VERSION >>>>>>> } // namespace >>>>>>> >>>>>>> -#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>>>>>> - >>>>>>> #include <bits/functional_hash.h> >>>>>>> >>>>>>> namespace std _GLIBCXX_VISIBILITY(default) >>>>>>> { >>>>>>> _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>>>> >>>>>>> +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X >>>>>>> // DR 1182. >>>>>>> /// std::hash specialization for error_code. >>>>>>> template<> >>>>>>> @@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >>>>>>> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); >>>>>>> } >>>>>>> }; >>>>>>> +#endif // _GLIBCXX_COMPATIBILITY_CXX0X >>>>>>> + >>>>>>> +#if __cplusplus > 201402L >>>>>>> + // DR 2686. >>>>>>> + /// std::hash specialization for error_condition. >>>>>>> + template<> >>>>>>> + struct hash<error_condition> >>>>>>> + : public __hash_base<size_t, error_condition> >>>>>>> + { >>>>>>> + size_t >>>>>>> + operator()(const error_condition& __e) const noexcept >>>>>>> + { >>>>>>> + const size_t __tmp = std::_Hash_impl::hash(__e.value()); >>>>>>> + return std::_Hash_impl::__hash_combine(__e.category(), __tmp); >>>>>> >>>>>> When I changed this from using __e._M_cat (as in Daniel's patch) to >>>>>> __e.category() I introduced a bug, because the former is a pointer to >>>>>> the error_category (and error_category objects are unique and so can >>>>>> be identified by their address) and the latter is the object itself, >>>>>> so we hash the bytes of an abstract base class instead of hashing the >>>>>> pointer to it. Oops. >>>>>> >>>>>> Patch coming up to fix that. >>>>> >>>>> Here's the fix. Tested powerpc64le-linux, committed to trunk. >>>>> >>>>> I'll backport this to 7, 8 and 9 as well. >>>>> >>>> >>>> Hi Jonathan, >>>> >>>> Does the new test lack dg-require-filesystem-ts ? >>> >>> It lacks it, because it doesn't use the filesystem library at all. >>> >>>> I'm seeing link failures on arm-eabi (using newlib): >>>> Excess errors: >>>> /libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' >>>> /libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' >>>> /libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' >>>> /libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined >>>> reference to `chmod' >>>> /libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' >>>> /libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' >>>> >>>> Christophe >> >> Is it definitely the new 19_diagnostics/error_condition/hash.cc test >> that's giving this error? i looked at this and ld -M reports /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) hash.o (std::_V2::error_category::default_error_condition(int) const) /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o) /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) (void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)) ... i.e. hash.o pulls system_error.o in because of std::_V2::error_category::default_error_condition(int) const and system_error.o pulls fs_ops.o in because of std::__cxx11::basic_string... symbol reference. it seems fs_ops.o is the first object in libstdc++.a that provides a (weak) definition for _ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE12_M_constructIPKcEEvT_S8_St20forward_iterator_tag >> >> I adjusted the pre-existing 27_io/filesystem/operations/absolute.cc >> test in r270874, which seems a more likely culprit, but that already >> has dg-require-filesystem-ts. >> >> > > Yes, and there full errors are: > /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): > In function `std::filesystem::current_path(std::filesystem::__cxx11::path > const&, std::error_code&)': > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:806: > undefined reference to `chdir' > /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): > In function `(anonymous > namespace)::create_dir(std::filesystem::__cxx11::path const&, > std::filesystem::perms, std::error_code&)': > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:583: > undefined reference to `mkdir' > /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): > In function `std::filesystem::permissions(std::filesystem::__cxx11::path > const&, std::filesystem::perms, std::filesystem::perm_options, > std::error_code&)': > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:1134: > undefined reference to `chmod' > /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): > In function `std::filesystem::do_copy_file(char const*, char const*, > std::filesystem::copy_options_existing_file, stat*, stat*, > std::error_code&)': > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: > undefined reference to `chmod' > /home/christophe.lyon/src/GCC/builds/gcc-fsf-trunk/obj-arm-none-eabi/gcc3/arm-none-eabi/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o): > In function `std::filesystem::current_path[abi:cxx11](std::error_code&)': > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:750: > undefined reference to `pathconf' > /home/christophe.lyon/src/GCC/sources/gcc-fsf/trunk/libstdc++-v3/src/c++17/fs_ops.cc:769: > undefined reference to `getcwd' >
On Thu, 9 May 2019 at 15:43, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote: > > On 07/05/2019 13:21, Christophe Lyon wrote: > > On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote: > >> > >> On 07/05/19 10:37 +0100, Jonathan Wakely wrote: > >>> On 07/05/19 11:05 +0200, Christophe Lyon wrote: > >>>> On Sat, 4 May 2019 at 16:36, Jonathan Wakely <jwakely@redhat.com> wrote: > >>>>> > >>>>> On 03/05/19 23:42 +0100, Jonathan Wakely wrote: > >>>>>> On 23/03/17 17:49 +0000, Jonathan Wakely wrote: > >>>>>>> On 12/03/17 13:16 +0100, Daniel Krügler wrote: > >>>>>>>> The following is an *untested* patch suggestion, please verify. > >>>>>>>> > >>>>>>>> Notes: My interpretation is that hash<error_condition> should be > >>>>>>>> defined outside of the _GLIBCXX_COMPATIBILITY_CXX0X block, please > >>>>>>>> double-check that course of action. > >>>>>>> > >>>>>>> That's right. > >>>>>>> > >>>>>>>> I noticed that the preexisting hash<error_code> did directly refer to > >>>>>>>> the private members of error_code albeit those have public access > >>>>>>>> functions. For consistency I mimicked that existing style when > >>>>>>>> implementing hash<error_condition>. > >>>>>>> > >>>>>>> I see no reason for that, so I've removed the friend declaration and > >>>>>>> used the public member functions. > >>>>>> > >>>>>> I'm going to do the same for hash<error_code> too. It can also use the > >>>>>> public members instead of being a friend. > >>>>>> > >>>>>> > >>>>>>> Although this is a DR, I'm treating it as a new C++17 feature, so I've > >>>>>>> adjusted the patch to only add the new specialization for C++17 mode. > >>>>>>> We're too close to the GCC 7 release to be adding new things to the > >>>>>>> default mode, even minor things like this. After GCC 7 is released we > >>>>>>> can revisit it and decide if we want to enable it for all modes. > >>>>>> > >>>>>> We never revisited that, and it's still only enabled for C++17 and up. > >>>>>> I guess that's OK, but we could enabled it for C++11 and 14 on trunk > >>>>>> if we want. Anybody care enough to argue for that? > >>>>>> > >>>>>>> Here's what I've tested and will be committing. > >>>>>>> > >>>>>>> > >>>>>> > >>>>>>> commit 90ca0fd91f5c65af370beb20af06bdca257aaf63 > >>>>>>> Author: Jonathan Wakely <jwakely@redhat.com> > >>>>>>> Date: Thu Mar 23 11:47:39 2017 +0000 > >>>>>>> > >>>>>>> Implement LWG 2686, std::hash<error_condition>, for C++17 > >>>>>>> 2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com> > >>>>>>> Implement LWG 2686, Why is std::hash specialized for error_code, > >>>>>>> but not error_condition? > >>>>>>> * include/std/system_error (hash<error_condition>): Define for C++17. > >>>>>>> * testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>): > >>>>>>> Instantiate test for error_condition. > >>>>>>> * testsuite/20_util/hash/requirements/explicit_instantiation.cc > >>>>>>> (hash<error_condition>): Instantiate hash<error_condition>. > >>>>>>> > >>>>>>> diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error > >>>>>>> index 6775a6e..ec7d25f 100644 > >>>>>>> --- a/libstdc++-v3/include/std/system_error > >>>>>>> +++ b/libstdc++-v3/include/std/system_error > >>>>>>> @@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>>>> _GLIBCXX_END_NAMESPACE_VERSION > >>>>>>> } // namespace > >>>>>>> > >>>>>>> -#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >>>>>>> - > >>>>>>> #include <bits/functional_hash.h> > >>>>>>> > >>>>>>> namespace std _GLIBCXX_VISIBILITY(default) > >>>>>>> { > >>>>>>> _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>>>> > >>>>>>> +#ifndef _GLIBCXX_COMPATIBILITY_CXX0X > >>>>>>> // DR 1182. > >>>>>>> /// std::hash specialization for error_code. > >>>>>>> template<> > >>>>>>> @@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >>>>>>> return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp); > >>>>>>> } > >>>>>>> }; > >>>>>>> +#endif // _GLIBCXX_COMPATIBILITY_CXX0X > >>>>>>> + > >>>>>>> +#if __cplusplus > 201402L > >>>>>>> + // DR 2686. > >>>>>>> + /// std::hash specialization for error_condition. > >>>>>>> + template<> > >>>>>>> + struct hash<error_condition> > >>>>>>> + : public __hash_base<size_t, error_condition> > >>>>>>> + { > >>>>>>> + size_t > >>>>>>> + operator()(const error_condition& __e) const noexcept > >>>>>>> + { > >>>>>>> + const size_t __tmp = std::_Hash_impl::hash(__e.value()); > >>>>>>> + return std::_Hash_impl::__hash_combine(__e.category(), __tmp); > >>>>>> > >>>>>> When I changed this from using __e._M_cat (as in Daniel's patch) to > >>>>>> __e.category() I introduced a bug, because the former is a pointer to > >>>>>> the error_category (and error_category objects are unique and so can > >>>>>> be identified by their address) and the latter is the object itself, > >>>>>> so we hash the bytes of an abstract base class instead of hashing the > >>>>>> pointer to it. Oops. > >>>>>> > >>>>>> Patch coming up to fix that. > >>>>> > >>>>> Here's the fix. Tested powerpc64le-linux, committed to trunk. > >>>>> > >>>>> I'll backport this to 7, 8 and 9 as well. > >>>>> > >>>> > >>>> Hi Jonathan, > >>>> > >>>> Does the new test lack dg-require-filesystem-ts ? > >>> > >>> It lacks it, because it doesn't use the filesystem library at all. > >>> > >>>> I'm seeing link failures on arm-eabi (using newlib): > >>>> Excess errors: > >>>> /libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' > >>>> /libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' > >>>> /libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' > >>>> /libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined > >>>> reference to `chmod' > >>>> /libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' > >>>> /libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' > >>>> > >>>> Christophe > >> > >> Is it definitely the new 19_diagnostics/error_condition/hash.cc test > >> that's giving this error? > > i looked at this and ld -M reports > > /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) > hash.o (std::_V2::error_category::default_error_condition(int) const) > /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o) > /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) (void std::__cxx11::basic_string<char, std::char_traits<char>, > std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)) > ... > > i.e. hash.o pulls system_error.o in because of > > std::_V2::error_category::default_error_condition(int) const > > and system_error.o pulls fs_ops.o in because of > > std::__cxx11::basic_string... > > symbol reference. > > it seems fs_ops.o is the first object in libstdc++.a > that provides a (weak) definition for > > _ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE12_M_constructIPKcEEvT_S8_St20forward_iterator_tag Ah, so maybe we need an explicit instantiation elsewhere. Or completely disable all the stuff using chdir, mkdir etc for these newlib targets, which is probably a good idea anyway.
On 09/05/2019 16:16, Jonathan Wakely wrote: > On Thu, 9 May 2019 at 15:43, Szabolcs Nagy <Szabolcs.Nagy@arm.com> wrote: >> On 07/05/2019 13:21, Christophe Lyon wrote: >>> On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwakely@redhat.com> wrote: >>>> On 07/05/19 10:37 +0100, Jonathan Wakely wrote: >>>>> On 07/05/19 11:05 +0200, Christophe Lyon wrote: >>>>>> I'm seeing link failures on arm-eabi (using newlib): >>>>>> Excess errors: >>>>>> /libstdc++-v3/src/c++17/fs_ops.cc:806: undefined reference to `chdir' >>>>>> /libstdc++-v3/src/c++17/fs_ops.cc:583: undefined reference to `mkdir' >>>>>> /libstdc++-v3/src/c++17/fs_ops.cc:1134: undefined reference to `chmod' >>>>>> /libstdc++-v3/src/c++17/../filesystem/ops-common.h:439: undefined >>>>>> reference to `chmod' >>>>>> /libstdc++-v3/src/c++17/fs_ops.cc:750: undefined reference to `pathconf' >>>>>> /libstdc++-v3/src/c++17/fs_ops.cc:769: undefined reference to `getcwd' >>>>>> >>>>>> Christophe >>>> >>>> Is it definitely the new 19_diagnostics/error_condition/hash.cc test >>>> that's giving this error? >> >> i looked at this and ld -M reports >> >> /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) >> hash.o (std::_V2::error_category::default_error_condition(int) const) >> /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(fs_ops.o) >> /B/aarch64-none-elf/libstdc++-v3/src/.libs/libstdc++.a(system_error.o) (void std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::_M_construct<char const*>(char const*, char const*, std::forward_iterator_tag)) >> ... >> >> i.e. hash.o pulls system_error.o in because of >> >> std::_V2::error_category::default_error_condition(int) const >> >> and system_error.o pulls fs_ops.o in because of >> >> std::__cxx11::basic_string... >> >> symbol reference. >> >> it seems fs_ops.o is the first object in libstdc++.a >> that provides a (weak) definition for >> >> _ZNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE12_M_constructIPKcEEvT_S8_St20forward_iterator_tag > > Ah, so maybe we need an explicit instantiation elsewhere. > Or completely disable all the stuff using chdir, mkdir etc for these > newlib targets, which is probably a good idea anyway. disabling fs things for baremetal makes sense. but i would not expect system_error.o to depend on fs_ops.o even on non-baremetal targets. so whatever causes the dependency should be fixed as well i think. in this case the base_string_whatever should have a definition either in system_error.o or in a .o with minimal deps that is guaranteed to come before fs_ops.o in libstdc++.a
commit 90ca0fd91f5c65af370beb20af06bdca257aaf63
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Thu Mar 23 11:47:39 2017 +0000
Implement LWG 2686, std::hash<error_condition>, for C++17
2017-03-23 Daniel Kruegler <daniel.kruegler@gmail.com>
Implement LWG 2686, Why is std::hash specialized for error_code,
but not error_condition?
* include/std/system_error (hash<error_condition>): Define for C++17.
* testsuite/20_util/hash/operators/size_t.cc (hash<error_condition>):
Instantiate test for error_condition.
* testsuite/20_util/hash/requirements/explicit_instantiation.cc
(hash<error_condition>): Instantiate hash<error_condition>.
diff --git a/libstdc++-v3/include/std/system_error b/libstdc++-v3/include/std/system_error
index 6775a6e..ec7d25f 100644
--- a/libstdc++-v3/include/std/system_error
+++ b/libstdc++-v3/include/std/system_error
@@ -373,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
_GLIBCXX_END_NAMESPACE_VERSION
} // namespace
-#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
-
#include <bits/functional_hash.h>
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_VERSION
+#ifndef _GLIBCXX_COMPATIBILITY_CXX0X
// DR 1182.
/// std::hash specialization for error_code.
template<>
@@ -394,12 +393,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
return std::_Hash_impl::__hash_combine(__e._M_cat, __tmp);
}
};
+#endif // _GLIBCXX_COMPATIBILITY_CXX0X
+
+#if __cplusplus > 201402L
+ // DR 2686.
+ /// std::hash specialization for error_condition.
+ template<>
+ struct hash<error_condition>
+ : public __hash_base<size_t, error_condition>
+ {
+ size_t
+ operator()(const error_condition& __e) const noexcept
+ {
+ const size_t __tmp = std::_Hash_impl::hash(__e.value());
+ return std::_Hash_impl::__hash_combine(__e.category(), __tmp);
+ }
+ };
+#endif
_GLIBCXX_END_NAMESPACE_VERSION
} // namespace
-#endif // _GLIBCXX_COMPATIBILITY_CXX0X
-
#endif // C++11
#endif // _GLIBCXX_SYSTEM_ERROR
diff --git a/libstdc++-v3/testsuite/20_util/hash/operators/size_t.cc b/libstdc++-v3/testsuite/20_util/hash/operators/size_t.cc
index ad02843..cc191d6 100644
--- a/libstdc++-v3/testsuite/20_util/hash/operators/size_t.cc
+++ b/libstdc++-v3/testsuite/20_util/hash/operators/size_t.cc
@@ -43,6 +43,9 @@ template<typename T>
void test01()
{
do_test<std::error_code>();
+#if __cplusplus > 201402L
+ do_test<std::error_condition>();
+#endif
}
int main()
diff --git a/libstdc++-v3/testsuite/20_util/hash/requirements/explicit_instantiation.cc b/libstdc++-v3/testsuite/20_util/hash/requirements/explicit_instantiation.cc
index e9e5ea1..d01829a 100644
--- a/libstdc++-v3/testsuite/20_util/hash/requirements/explicit_instantiation.cc
+++ b/libstdc++-v3/testsuite/20_util/hash/requirements/explicit_instantiation.cc
@@ -40,6 +40,9 @@ template class std::hash<long double>;
template class std::hash<void*>;
template class std::hash<std::string>;
template class std::hash<std::error_code>;
+#if __cplusplus > 201402L
+template class std::hash<std::error_condition>;
+#endif
#ifdef _GLIBCXX_USE_WCHAR_T
template class std::hash<wchar_t>;