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

Regressed compile support for clang-3.5 #2459

Closed
springmeyer opened this issue May 25, 2016 · 14 comments
Closed

Regressed compile support for clang-3.5 #2459

springmeyer opened this issue May 25, 2016 · 14 comments

Comments

@springmeyer
Copy link
Contributor

The changes in #2440 triggered a new compile error when building with clang-3.5 on linux. This compiler is an important one to maintain support for because it is the only clang version (at least at the moment) that is easily installable on travis and has working sanitizers. In the process of trying to enable a travis job with the sanitizers this broke:

/usr/bin/clang++-3.5   -DBOOST_FILESYSTEM_NO_DEPRECATED -DBOOST_RESULT_OF_USE_DECLTYPE -DBOOST_SPIRIT_USE_PHOENIX_V3 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wall -Wextra -pedantic -Wuninitialized -Wunreachable-code -Wstrict-overflow=2 -D_FORTIFY_SOURCE=2 -fPIC -fcolor-diagnostics -ffunction-sections -fdata-sections -std=c++11  -O3 -DNDEBUG -I/home/penny/osrm-backend/include -I/home/penny/osrm-backend/build/include -isystem /home/penny/osrm-backend/third_party -isystem /home/penny/osrm-backend/third_party/libosmium/include -isystem /home/penny/osrm-backend/build/mason_packages/.link/include -isystem /home/penny/osrm-backend/build/mason_packages/.link/include/luabind    -o CMakeFiles/CONTRACTOR.dir/src/contractor/contractor.cpp.o -c /home/penny/osrm-backend/src/contractor/contractor.cpp
In file included from /home/penny/osrm-backend/src/contractor/contractor.cpp:1:
In file included from /home/penny/osrm-backend/include/contractor/contractor.hpp:31:
In file included from /home/penny/osrm-backend/include/contractor/contractor_config.hpp:31:
In file included from /home/penny/osrm-backend/build/mason_packages/.link/include/boost/filesystem/path.hpp:25:
In file included from /home/penny/osrm-backend/build/mason_packages/.link/include/boost/filesystem/path_traits.hpp:23:
In file included from /home/penny/osrm-backend/build/mason_packages/.link/include/boost/system/error_code.hpp:19:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ostream:38:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ios:40:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/char_traits.h:39:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:739:11: error: overload resolution selected deleted operator '='
        *__first = __value;
        ~~~~~~~~ ^ ~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:786:23: note: in instantiation of function template specialization
      'std::__fill_n_a<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
      return _OI(std::__fill_n_a(std::__niter_base(__first), __n, __value));
                      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:515:9: note: in instantiation of function template specialization
      'std::fill_n<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
          std::fill_n(__first, __n, _ValueType());
               ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:544:2: note: in instantiation of function template specialization
      'std::__uninitialized_default_n_1<true>::__uninit_default_n<std::atomic<unsigned long> *, unsigned long>' requested here
        __uninit_default_n(__first, __n);
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:605:12: note: in instantiation of function template specialization
      'std::__uninitialized_default_n<std::atomic<unsigned long> *, unsigned long>' requested here
    { std::__uninitialized_default_n(__first, __n); }
           ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:1224:7: note: in instantiation of function template specialization
      'std::__uninitialized_default_n_a<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
        std::__uninitialized_default_n_a(this->_M_impl._M_start, __n, 
             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:271:9: note: in instantiation of member function
      'std::vector<std::atomic<unsigned long>, std::allocator<std::atomic<unsigned long> > >::_M_default_initialize' requested here
      { _M_default_initialize(__n); }
        ^
/home/penny/osrm-backend/src/contractor/contractor.cpp:498:49: note: in instantiation of member function 'std::vector<std::atomic<unsigned long>,
      std::allocator<std::atomic<unsigned long> > >::vector' requested here
        std::vector<std::atomic<std::uint64_t>> segment_speeds_counters(
                                                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/atomic:659:15: note: candidate function has been explicitly deleted
      atomic& operator=(const atomic&) = delete;
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:360:22: note: candidate function has been explicitly deleted
      __atomic_base& operator=(const __atomic_base&) = delete;
                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/atomic:660:15: note: candidate function has been explicitly deleted
      atomic& operator=(const atomic&) volatile = delete;
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:361:22: note: candidate function has been explicitly deleted
      __atomic_base& operator=(const __atomic_base&) volatile = delete;
                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:373:7: note: candidate function
      operator=(__int_type __i) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:380:7: note: candidate function
      operator=(__int_type __i) volatile noexcept
      ^
1 error generated.

It looks like the problem is in the constructor, since this works around the problem:

diff --git a/src/contractor/contractor.cpp b/src/contractor/contractor.cpp
index 8dd8d25..f5ffd1c 100644
--- a/src/contractor/contractor.cpp
+++ b/src/contractor/contractor.cpp
@@ -495,8 +495,7 @@ std::size_t Contractor::LoadEdgeExpandedGraph(

         // vector to count used speeds for logging
         // size offset by one since index 0 is used for speeds not from external file
-        std::vector<std::atomic<std::uint64_t>> segment_speeds_counters(
-            segment_speed_filenames.size() + 1);
+        std::vector<std::atomic<std::uint64_t>> segment_speeds_counters;
         for (auto &each : segment_speeds_counters)
             each.store(0);
         const constexpr auto LUA_SOURCE = 0;

/cc @daniel-j-h @freenerd

@springmeyer
Copy link
Contributor Author

springmeyer pushed a commit that referenced this issue May 25, 2016
springmeyer pushed a commit that referenced this issue May 25, 2016
This reverts commit 8b92e2a.
@freenerd
Copy link
Member

Is there a downside to not initializing the vector with the correct size? It should grow automatically and we only expect a maximum of 255 only, so performance-wise there shouldn't be problems.

@danpat
Copy link
Member

danpat commented May 25, 2016

@freenerd your assessment is correct. This is not in a hot loop, so the default growing behaviour will work fine.

@springmeyer
Copy link
Contributor Author

I worked with @daniel-j-h to come up with #2462 - will merge that and close this once I confirm all tests still pass.

@daniel-j-h
Copy link
Member

For the record, it's not about growing behavior. We initialize n counters (depending on the size of data sources) and increment them based on the data source later on in parallel.

We had two problems with the code:

  • gcc48's stdlib did not define atomic_init we worked around that one using .store
  • clang35's stdlib not supporting the default construction with moveable types, Fix compile with clang-3.5 #2462 fixes this

@freenerd welcome to the world of pain and suffering :)

@springmeyer
Copy link
Contributor Author

@daniel-j-h now hitting this with clang-3.8 on circleci: https://circleci.com/gh/Project-OSRM/osrm-backend/47. @daniel-j-h can you think of a way to re-write this code to avoid this type of usage of atomic?

@springmeyer
Copy link
Contributor Author

@daniel-j-h asking about re-writing because overall:

@springmeyer
Copy link
Contributor Author

springmeyer commented May 27, 2016

clang-3.8 on circleci

Forcing libstdc++6 upgrade fixed the issue for clang-3.8 Spoke too soon, did not.

@springmeyer
Copy link
Contributor Author

Nevermind, spoke too soon. Even after upgrading libstdc++6 clang-3.8 still errors with:

In file included from /home/ubuntu/osrm-backend/src/contractor/contractor.cpp:1:
In file included from /home/ubuntu/osrm-backend/include/contractor/contractor.hpp:31:
In file included from /home/ubuntu/osrm-backend/include/contractor/contractor_config.hpp:31:
In file included from /home/ubuntu/osrm-backend/mason_packages/.link/include/boost/filesystem/path.hpp:25:
In file included from /home/ubuntu/osrm-backend/mason_packages/.link/include/boost/filesystem/path_traits.hpp:23:
In file included from /home/ubuntu/osrm-backend/mason_packages/.link/include/boost/system/error_code.hpp:19:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ostream:38:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/ios:40:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/char_traits.h:39:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:739:11: error: overload resolution selected deleted operator '='
 *__first = __value;
 ~~~~~~~~ ^ ~~~~~~~
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_algobase.h:786:23: note: in instantiation of function template specialization 'std::__fill_n_a<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
      return _OI(std::__fill_n_a(std::__niter_base(__first), __n, __value));
                      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:515:9: note: in instantiation of function template specialization 'std::fill_n<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
   std::fill_n(__first, __n, _ValueType());
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:544:2: note: in instantiation of function template specialization 'std::__uninitialized_default_n_1<true>::__uninit_default_n<std::atomic<unsigned long> *, unsigned long>' requested here
 __uninit_default_n(__first, __n);
 ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_uninitialized.h:605:12: note: in instantiation of function template specialization 'std::__uninitialized_default_n<std::atomic<unsigned long> *, unsigned long>' requested here
    { std::__uninitialized_default_n(__first, __n); }
           ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:1224:7: note: in instantiation of function template specialization 'std::__uninitialized_default_n_a<std::atomic<unsigned long> *, unsigned long, std::atomic<unsigned long> >' requested here
 std::__uninitialized_default_n_a(this->_M_impl._M_start, __n,
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/stl_vector.h:271:9: note: in instantiation of member function 'std::vector<std::atomic<unsigned long>, std::allocator<std::atomic<unsigned long> > >::_M_default_initialize' requested here
      { _M_default_initialize(__n); }
        ^
/home/ubuntu/osrm-backend/src/contractor/contractor.cpp:498:49: note: in instantiation of member function 'std::vector<std::atomic<unsigned long>, std::allocator<std::atomic<unsigned long> > >::vector' requested here
        std::vector<std::atomic<std::uint64_t>> segment_speeds_counters(
                                                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/atomic:659:15: note: candidate function has been explicitly deleted
      atomic& operator=(const atomic&) = delete;
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:360:22: note: candidate function has been explicitly deleted
      __atomic_base& operator=(const __atomic_base&) = delete;
                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/atomic:660:15: note: candidate function has been explicitly deleted
      atomic& operator=(const atomic&) volatile = delete;
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:361:22: note: candidate function has been explicitly deleted
      __atomic_base& operator=(const __atomic_base&) volatile = delete;
                     ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:373:7: note: candidate function
      operator=(__int_type __i) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/atomic_base.h:380:7: note: candidate function
      operator=(__int_type __i) volatile noexcept
      ^
1 error generated.

@daniel-j-h
Copy link
Member

This small self-contained example works on gcc 4.9, gcc 6.3, llvm 3.8, llvm 3.6:

#include <vector>
#include <atomic>

int main() {
  std::vector<std::atomic<int>> vais(10);

  for (auto& each : vais)
    each.store(0);
}

It's not using std::atomic_init on purpose since it would break on older libstdc++ versions.

If clang 3.5 breaks on this but works with one of the other workarounds above, you have two options: 1/ ifdef the clang 3.5 workaround 2/ use a std::mutex in combination with a std::lock_guard instead.

@springmeyer
Copy link
Contributor Author

solved by cd30f37

@springmeyer
Copy link
Contributor Author

Interestingly as part of #2496 I tried enabling clang-3.5 builds against, but via mason provided 3.5.2 binary. The compile now worked, but I hit the below errors in tests. I am going to ignore these as clang-3.8 works (and the sanitizers work with 3.8) so I'm not longer personally interested in 3.5 support. But here are the errors for the record:

The command "pushd build" exited with 0.
0.02s$ ./unit_tests/library-tests ../test/data/monaco.osrm
Running 21 test cases...
unknown location(0): fatal error in "test_table_three_coords_one_source_one_dest_matrix": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(22): last checkpoint
unknown location(0): fatal error in "test_table_three_coords_one_source_matrix": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(72): last checkpoint
unknown location(0): fatal error in "test_table_three_coordinates_matrix": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_route_same_coordinates_fixture": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_route_same_coordinates": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_route_response_for_locations_in_small_component": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_route_response_for_locations_in_big_component": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_route_response_for_locations_across_components": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_nearest_response": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_nearest_response_no_coordinates": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_nearest_response_multiple_coordinates": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_nearest_response_for_location_in_small_component": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/table.cpp(122): last checkpoint
unknown location(0): fatal error in "test_match": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/match.cpp(22): last checkpoint
unknown location(0): fatal error in "test_trip_response_for_locations_in_small_component": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/match.cpp(22): last checkpoint
unknown location(0): fatal error in "test_trip_response_for_locations_in_big_component": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/match.cpp(22): last checkpoint
unknown location(0): fatal error in "test_trip_response_for_locations_across_components": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/match.cpp(22): last checkpoint
unknown location(0): fatal error in "test_trip_limits": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/limits.cpp(22): last checkpoint
unknown location(0): fatal error in "test_route_limits": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/limits.cpp(52): last checkpoint
unknown location(0): fatal error in "test_table_limits": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/limits.cpp(82): last checkpoint
unknown location(0): fatal error in "test_match_limits": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/limits.cpp(112): last checkpoint
unknown location(0): fatal error in "test_tile": std::exception: Invalid file paths given!
/home/travis/build/Project-OSRM/osrm-backend/unit_tests/library/limits.cpp(112): last checkpoint
*** 21 failures detected in test suite "library tests"

@daniel-j-h
Copy link
Member

This looks to me like you forgot to run make -C test/data (which generates the monaco.osrm file) before trying to run the library tests on this osrm file. Can you confirm please? If not, this is a serious issue and we should look into it.

@daniel-j-h daniel-j-h reopened this Jun 3, 2016
@daniel-j-h
Copy link
Member

Closing as not actionable for us. Feel free to re-open if this still happens.

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

No branches or pull requests

4 participants