Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If __GNUC__, check if exceptions are enabled before using them #60

Merged
merged 5 commits into from
Jan 23, 2023

Conversation

chriselrod
Copy link
Contributor

@chriselrod chriselrod commented Jan 20, 2023

GCC and Clang both define __EXCEPTIONS when exceptions are enabled, and do not define it otherwise. They should also both define __GNUC__.
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

Thus, this PR allows unordered_dense to compile with clang and gcc when using -fno-exceptions, which isn't currently supported.

It shouldn't change the behavior otherwise.

Not sure what the best way to test this is. Add -fno-excpetions to an arbitrary test that doesn't currently use exceptions and is compiled with clang or gcc?

@martinus
Copy link
Owner

Hi @chriselrod, thanks for the contribution! I had similar functionality in my older map, where I was doing it that way:
https://github.com/martinus/robin-hood-hashing/blob/master/src/include/robin_hood.h#L128-L133

and a generic doThrow() function that otherwise calls abort():
https://github.com/martinus/robin-hood-hashing/blob/master/src/include/robin_hood.h#L331-L344

I think it should be done in a similar way. I don't think assert() is a good solution because it might be disabled, so better call abort(). I'd just rename the function to from doThrow to throwOrAbort().

please clang-format everything :)

@chriselrod
Copy link
Contributor Author

I've copied the check, I'll copy the doThrow later today.
When compiling without NDEBUG being defined, the assert will still print the message.

Only when defining NDEBUG (which cmake does on Release) does the message (and check) go away.
Correctly predicted branches are cheap, so if you want to keep them even in release builds, that's fine.

please clang-format everything :)

Just found a bug in my editor configuration where things going through "c or c++ mode" weren't enabling the language server or getting formatted...

@martinus
Copy link
Owner

My concern with the assert is that when compiling with NDEBUG the program just continues, but it shouldn't or else it runs into undefined behavior. Better to abort() to be sure the program really stops

@chriselrod
Copy link
Contributor Author

Okay, I've copied the doThrow implementation, but I've only added noinline when exceptions are enabled, because abort generates hardly any code.

I also added likely/unlikely to each of these to indicate they're unlikely.
FWIW, c++20 adds likely/unlikely attributes.
If you want to check for c++20, you could let MSVC use them.

@martinus
Copy link
Owner

I think the macos doctest problem can be fixed by upgrading doctest, please run meson wrap update doctest and commit the updated doctest.wrap file

@chriselrod
Copy link
Contributor Author

chriselrod commented Jan 22, 2023

I'm getting this locally

FAILED: test/udm.p/unit_copy_and_assign_maps.cpp.o 
ccache c++ -Itest/udm.p -Itest -I../test -I../include -I../subprojects/doctest-2.4.9/doctest -I../subprojects/fmt-9.0.0/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Wpedantic -Werror -std=c++17 -O0 -g -DANKERL_UNORDERED_DENSE_HAS_BOOST=0 -march=native -mtune=native -O3 -pipe -DNDEBUG -DFMT_SHARED -pthread -Wno-stringop-overflow -Warith-conversion -Wshadow=global -Wconversion -Wextra -Wunreachable-code -Wuninitialized -pedantic-errors -Wold-style-cast -MD -MQ test/udm.p/unit_copy_and_assign_maps.cpp.o -MF test/udm.p/unit_copy_and_assign_maps.cpp.o.d -o test/udm.p/unit_copy_and_assign_maps.cpp.o -c ../test/unit/copy_and_assign_maps.cpp
In file included from ../test/unit/copy_and_assign_maps.cpp:1:
In member function ‘void ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>::copy_buckets(const ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>&) [with Key = int; T = int; Hash = ankerl::unordered_dense::v3_0_2::hash<int>; KeyEqual = std::equal_to<int>; AllocatorOrContainer = std::allocator<std::pair<int, int> >; Bucket = ankerl::unordered_dense::v3_0_2::bucket_type::standard]’,
    inlined from ‘void ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>::copy_buckets(const ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>&) [with Key = int; T = int; Hash = ankerl::unordered_dense::v3_0_2::hash<int>; KeyEqual = std::equal_to<int>; AllocatorOrContainer = std::allocator<std::pair<int, int> >; Bucket = ankerl::unordered_dense::v3_0_2::bucket_type::standard]’ at ../include/ankerl/unordered_dense.h:578:10,
    inlined from ‘ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>::table(const ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>&, const allocator_type&) [with Key = int; T = int; Hash = ankerl::unordered_dense::v3_0_2::hash<int>; KeyEqual = std::equal_to<int>; AllocatorOrContainer = std::allocator<std::pair<int, int> >; Bucket = ankerl::unordered_dense::v3_0_2::bucket_type::standard]’ at ../include/ankerl/unordered_dense.h:853:21,
    inlined from ‘ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>::table(const ankerl::unordered_dense::v3_0_2::detail::table<Key, T, Hash, KeyEqual, AllocatorOrContainer, Bucket>&) [with Key = int; T = int; Hash = ankerl::unordered_dense::v3_0_2::hash<int>; KeyEqual = std::equal_to<int>; AllocatorOrContainer = std::allocator<std::pair<int, int> >; Bucket = ankerl::unordered_dense::v3_0_2::bucket_type::standard]’ at ../include/ankerl/unordered_dense.h:846:54,
    inlined from ‘void std::_Construct(_Tp*, _Args&& ...) [with _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>; _Args = {const ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int, void>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>&}]’ at /usr/include/c++/12/bits/stl_construct.h:119:7,
    inlined from ‘_ForwardIterator std::__do_uninit_fill_n(_ForwardIterator, _Size, const _Tp&) [with _ForwardIterator = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>*; _Size = long unsigned int; _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>]’ at /usr/include/c++/12/bits/stl_uninitialized.h:267:21,
    inlined from ‘static _ForwardIterator std::__uninitialized_fill_n<_TrivialValueType>::__uninit_fill_n(_ForwardIterator, _Size, const _Tp&) [with _ForwardIterator = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>*; _Size = long unsigned int; _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>; bool _TrivialValueType = false]’ at /usr/include/c++/12/bits/stl_uninitialized.h:284:34,
    inlined from ‘_ForwardIterator std::uninitialized_fill_n(_ForwardIterator, _Size, const _Tp&) [with _ForwardIterator = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>*; _Size = long unsigned int; _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>]’ at /usr/include/c++/12/bits/stl_uninitialized.h:327:17,
    inlined from ‘_ForwardIterator std::__uninitialized_fill_n_a(_ForwardIterator, _Size, const _Tp&, allocator<_Tp2>&) [with _ForwardIterator = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>*; _Size = long unsigned int; _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>; _Tp2 = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, equal_to<int>, allocator<pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>]’ at /usr/include/c++/12/bits/stl_uninitialized.h:467:39,
    inlined from ‘void std::vector<_Tp, _Alloc>::_M_fill_initialize(size_type, const value_type&) [with _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>; _Alloc = std::allocator<ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard> >]’ at /usr/include/c++/12/bits/stl_vector.h:1702:33,
    inlined from ‘std::vector<_Tp, _Alloc>::vector(size_type, const value_type&, const allocator_type&) [with _Tp = ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard>; _Alloc = std::allocator<ankerl::unordered_dense::v3_0_2::detail::table<int, int, ankerl::unordered_dense::v3_0_2::hash<int>, std::equal_to<int>, std::allocator<std::pair<int, int> >, ankerl::unordered_dense::v3_0_2::bucket_type::standard> >]’ at /usr/include/c++/12/bits/stl_vector.h:567:27,
    inlined from ‘void DOCTEST_ANON_FUNC_2()’ at ../test/unit/copy_and_assign_maps.cpp:73:38:
../include/ankerl/unordered_dense.h:582:24: error: ‘void* memcpy(void*, const void*, size_t)’ forming offset [128, 34359738367] is out of the bounds [0, 128] [-Werror=array-bounds]
  582 |             std::memcpy(m_buckets, other.m_buckets, sizeof(Bucket) * bucket_count());
      |             ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

However, after clearing my local CXXFLAGS and rerunning meson setup builddir, tests pass.

@chriselrod
Copy link
Contributor Author

All checks passed, good to merge @martinus?

@martinus martinus merged commit e4401db into martinus:main Jan 23, 2023
@martinus
Copy link
Owner

Merged it, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants