Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Fix compile errors #3318

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix compile errors #3318

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9de42e
Fix compile errors
TobyRoseman Sep 11, 2020
296821b
Code review feedback. Use const references.
TobyRoseman Sep 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/storage/fileio/fs_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ EXPORT std::string make_canonical_path(const std::string& path) {
} else {
return fs::canonical(fs::absolute(p)).string();
}
} catch (fs::filesystem_error e) {
} catch (fs::filesystem_error& e) {
TobyRoseman marked this conversation as resolved.
Show resolved Hide resolved
log_and_throw(std::string("Invalid path: ") + path + ". " + e.what());
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/storage/fileio/general_fstream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ general_ofstream::general_ofstream(std::string filename, bool gzip_compress) try
: general_ofstream_base(filename, gzip_compress),
opened_filename(filename) {
exceptions(std::ios_base::badbit);
} catch (std::exception e) {
} catch (std::exception& e) {
TobyRoseman marked this conversation as resolved.
Show resolved Hide resolved
log_and_throw_io_failure("Cannot open " + sanitize_url(filename) +
" for write. " + e.what());
} catch (std::string e) {
Expand Down
2 changes: 1 addition & 1 deletion src/core/storage/serialization/dir_archive.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ dir_archive_impl::archive_index_information read_index_file(std::string index_fi
boost::property_tree::ptree data;
try {
boost::property_tree::ini_parser::read_ini(fin, data);
} catch(boost::property_tree::ini_parser_error e) {
} catch(boost::property_tree::ini_parser_error& e) {
log_and_throw(std::string("Unable to parse archive index file ") + index_file);
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/storage/serialization/iarchive.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ namespace turi {
template <typename T>
inline void read_into(T& c) {
if (buf) {
memcpy(&c, buf + off, sizeof(T));
memcpy(reinterpret_cast<void*>(&c), buf + off, sizeof(T));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary. A cast to void* is automatic for any trivial types (i.e. no inheritance, no pointers, etc., and this memcpy is valid only for those types. The so the lack of an explicit cast here is a guard against that (though it could be made more explicit with a

static_assert(std::is_trivial::value, "Only trivial types allowed for hash.");

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the error message I get without the reinterpret_cast:

[ 14%] Building CXX object src/model_server/CMakeFiles/unity.dir/server/registration.cpp.o
In file included from /home/toby/Documents/turicreate/src/core/storage/serialization/serialize.hpp:8,
                 from /home/toby/Documents/turicreate/src/core/storage/serialization/serialization_includes.hpp:6,
                 from /home/toby/Documents/turicreate/src/core/parallel/atomic.hpp:12,
                 from /home/toby/Documents/turicreate/src/core/data/flexible_type/flexible_type.hpp:15,
                 from /home/toby/Documents/turicreate/src/model_server/lib/toolkit_class_specification.hpp:10,
                 from /home/toby/Documents/turicreate/src/model_server/lib/toolkit_class_registry.hpp:8,
                 from /home/toby/Documents/turicreate/src/model_server/server/registration.hpp:9,
                 from /home/toby/Documents/turicreate/src/model_server/server/registration.cpp:6:
/home/toby/Documents/turicreate/src/core/storage/serialization/iarchive.hpp: In instantiation of ‘void turi::iarchive::read_into(T&) [with T = boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void> >]’:
/home/toby/Documents/turicreate/src/core/storage/serialization/iarchive.hpp:243:9:   required from ‘static void turi::archive_detail::deserialize_impl<InArcType, T, true>::exec(InArcType&, T&) [with InArcType = turi::iarchive; T = boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void> >]’
/home/toby/Documents/turicreate/src/core/storage/serialization/iarchive.hpp:258:65:   required from ‘turi::iarchive& turi::operator>>(turi::iarchive&, T&) [with T = boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void> >]’
/home/toby/Documents/turicreate/src/core/util/hash_value.hpp:65:39:   required from here
/home/toby/Documents/turicreate/src/core/storage/serialization/iarchive.hpp:121:8: error: ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘class boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void> >’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead [-Werror=class-memaccess]
  121 |  memcpy(&c, buf + off, sizeof(T));
      |  ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/toby/Documents/turicreate/src/external/boost/boost_1_68_0/boost/multiprecision/cpp_int.hpp:12,
                 from /home/toby/Documents/turicreate/src/core/util/int128_types.hpp:44,
                 from /home/toby/Documents/turicreate/src/core/data/flexible_type/flexible_type.hpp:16,
                 from /home/toby/Documents/turicreate/src/model_server/lib/toolkit_class_specification.hpp:10,
                 from /home/toby/Documents/turicreate/src/model_server/lib/toolkit_class_registry.hpp:8,
                 from /home/toby/Documents/turicreate/src/model_server/server/registration.hpp:9,
                 from /home/toby/Documents/turicreate/src/model_server/server/registration.cpp:6:
/home/toby/Documents/turicreate/src/external/boost/boost_1_68_0/boost/multiprecision/number.hpp:41:7: note: ‘class boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void> >’ declared here
   41 | class number
      |       ^~~~~~
cc1plus: error: unrecognized command line option ‘-Wno-unknown-warning-option’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-parentheses-equality’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-constant-logical-operand’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-mismatched-tags’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-deprecated-register’ [-Werror]
cc1plus: error: unrecognized command line option ‘-Wno-unused-command-line-argument’ [-Werror]
cc1plus: all warnings being treated as errors
make[2]: *** [src/model_server/CMakeFiles/unity.dir/build.make:63: src/model_server/CMakeFiles/unity.dir/server/registration.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:3952: src/model_server/CMakeFiles/unity.dir/all] Error 2
make: *** [Makefile:152: all] Error 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following program prints out non-trivial:

#include <iostream>
#include <boost/multiprecision/number.hpp>
#include <boost/multiprecision/cpp_int.hpp>

int main()
{

  const bool is_trivial = std::is_trivial<
    boost::multiprecision::number<boost::multiprecision::backends::cpp_int_backend<128, 128, boost::multiprecision::unsigned_magnitude, boost::multiprecision::unchecked, void>>
    >::value;

  if(is_trivial)
     std::cout<<"trivial"<<std::endl;
  else
    std::cout<<"non-trivial"<<std::endl;
  return 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly, as it should. Only trivial types can be used in that serialization function. Boost multiprecision is the fallback type when no native 128 bit integer types are detected at build time (maybe it should error instead?). These are detected by the Find128BitInteger.cmake routines under cmake/. It appears these routines are failing. That is the root cause of this problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the output of cmake config log after a clean cmake run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the relevant section of the logs:

Testing for 128 bit integer support with __int128_t.
Found: Enabling support for 128 bit integers using __int128_t.
Testing for presence of 128 bit unsigned integer hash function for __int128_t.
std::hash<__int128_t> defined.
Testing for 128 bit unsigned integer support with __uint128_t.

Output for debug and release is the same.

So for both signed and unsigned, it seems to be finding the first things it's looking for.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to drop the ball on this -- let's dig into it tomorrow. There's some typo somewhere.

off += sizeof(T);
} else {
in->read(reinterpret_cast<char*>(&c), sizeof(T));
Expand Down
2 changes: 1 addition & 1 deletion src/core/storage/sframe_data/sframe_index_file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ sframe_index_file_information read_sframe_index_file(std::string index_file) {
boost::property_tree::ptree data;
try {
boost::property_tree::ini_parser::read_ini(fin, data);
} catch(boost::property_tree::ini_parser_error e) {
} catch(boost::property_tree::ini_parser_error& e) {
TobyRoseman marked this conversation as resolved.
Show resolved Hide resolved
log_and_throw(std::string("Unable to parse frame index file ") + index_file);
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/storage/sgraph_data/sgraph_triple_apply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ namespace {
g.get_vertex_fields(),
g.get_edge_fields(),
m_srcid_column, m_dstid_column);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need const in all these. Sorry, thought that was implied.

throw(lambda::reinterpret_comm_failure(e));
}

Expand Down Expand Up @@ -943,7 +943,7 @@ namespace {
try {
mutated_edge_data = m_evaluator->proxy->eval_triple_apply(all_edge_data, m_src_partition,
m_dst_partition, mutated_edge_field_ids);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw(lambda::reinterpret_comm_failure(e));
}

Expand Down
10 changes: 5 additions & 5 deletions src/core/system/lambda/lambda_master.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ static lambda_master* instance_ptr = nullptr;
auto release_lambda_fn = [lambda_hash](std::unique_ptr<lambda_evaluator_proxy>& proxy) {
try {
proxy->release_lambda(lambda_hash);
} catch (std::exception e){
} catch (std::exception& e){
TobyRoseman marked this conversation as resolved.
Show resolved Hide resolved
logstream(LOG_ERROR) << "Error on releasing lambda: " << e.what() << std::endl;
} catch (std::string e) {
logstream(LOG_ERROR) << "Error on releasing lambda: " << e << std::endl;
Expand All @@ -134,7 +134,7 @@ static lambda_master* instance_ptr = nullptr;
// catch and reinterpret comm failure
try {
out = worker->proxy->bulk_eval(lambda_hash, args, skip_undefined, seed);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw reinterpret_comm_failure(e);
}
}
Expand Down Expand Up @@ -236,7 +236,7 @@ static lambda_master* instance_ptr = nullptr;
logstream(LOG_WARNING) << "Unexpected SHMIPC failure. Falling back to CPPIPC" << std::endl;
}
out = worker->proxy->bulk_eval_rows(lambda_hash, args, skip_undefined, seed);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw reinterpret_comm_failure(e);
}
}
Expand All @@ -252,7 +252,7 @@ static lambda_master* instance_ptr = nullptr;
// catch and reinterpret comm failure
try {
out = worker->proxy->bulk_eval_dict(lambda_hash, keys, values, skip_undefined, seed);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw reinterpret_comm_failure(e);
}
}
Expand Down Expand Up @@ -289,7 +289,7 @@ static lambda_master* instance_ptr = nullptr;
logstream(LOG_WARNING) << "Unexpected SHMIPC failure. Falling back to CPPIPC" << std::endl;
}
out = worker->proxy->bulk_eval_dict_rows(lambda_hash, keys, rows, skip_undefined, seed);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw reinterpret_comm_failure(e);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/system/lambda/worker_pool.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ class worker_pool {
parallel_for(0, m_num_workers, [&](size_t i) {
try {
ret[i] = f(temp_workers[i]->proxy);
} catch (cppipc::ipcexception e) {
} catch (cppipc::ipcexception& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const

throw reinterpret_comm_failure(e);
}
});
Expand Down