On Thu, 9 May 2019 at 15:43, Szabolcs Nagy <szabolcs.n...@arm.com> wrote:
>
> On 07/05/2019 13:21, Christophe Lyon wrote:
> > On Tue, 7 May 2019 at 12:07, Jonathan Wakely <jwak...@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 <jwak...@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 <jwak...@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.krueg...@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.
