From c25f22e5653e8075dfd75f2d34c2392b398f3d73 Mon Sep 17 00:00:00 2001 From: Dan Rose Date: Sun, 8 Dec 2019 16:03:30 -0600 Subject: [PATCH] Mark code that should be unreachable (#77) Introduce a new [[noreturn]] unreachable() function that marks code as unreachable and throws a logic error if it is executed. Fix build error due to Windows min/max macros. Fix linker errors from referring to a non-constexpr extern from a constexpr. Fix warnings about narrowing conversions. Signed-off-by: Dan Rose --- .../include/rmw_cyclonedds_cpp/exception.hpp | 16 ++++++++++- rmw_cyclonedds_cpp/src/Serialization.cpp | 27 ++++++++++++++----- rmw_cyclonedds_cpp/src/TypeSupport2.cpp | 12 ++++++--- rmw_cyclonedds_cpp/src/TypeSupport2.hpp | 19 ++++++++----- rmw_cyclonedds_cpp/src/bytewise.hpp | 3 +++ rmw_cyclonedds_cpp/src/rmw_node.cpp | 17 ++++++++++++ rmw_cyclonedds_cpp/src/serdata.cpp | 9 ++++--- rmw_cyclonedds_cpp/src/serdata.hpp | 6 ++--- 8 files changed, 85 insertions(+), 24 deletions(-) diff --git a/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/exception.hpp b/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/exception.hpp index 048684a..d03cd58 100644 --- a/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/exception.hpp +++ b/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/exception.hpp @@ -15,8 +15,8 @@ #ifndef RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_ #define RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_ +#include #include -#include namespace rmw_cyclonedds_cpp { @@ -35,6 +35,20 @@ protected: std::string m_message; }; +/// Stub for code that should never be reachable by design. +/// If it is possible to reach the code due to bad data or other runtime conditions, +/// use a runtime_error instead +[[noreturn]] inline void unreachable() +{ +#if defined(__has_builtin) +#if __has_builtin(__builtin_unreachable) + __builtin_unreachable(); +#endif +#elif (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)) + __builtin_unreachable(); +#endif + throw std::logic_error("This code should be unreachable."); +} } // namespace rmw_cyclonedds_cpp #endif // RMW_CYCLONEDDS_CPP__EXCEPTION_HPP_ diff --git a/rmw_cyclonedds_cpp/src/Serialization.cpp b/rmw_cyclonedds_cpp/src/Serialization.cpp index 559e1fa..8412336 100644 --- a/rmw_cyclonedds_cpp/src/Serialization.cpp +++ b/rmw_cyclonedds_cpp/src/Serialization.cpp @@ -11,16 +11,22 @@ // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. // See the License for the specific language governing permissions and // limitations under the License. + +// suppress definition of min/max macros on Windows. +// TODO(dan@digilabs.io): Move this closer to where Windows.h/Windef.h is included +#ifndef NOMINMAX +#define NOMINMAX +#endif + #include "Serialization.hpp" -#include +#include #include #include #include #include "TypeSupport2.hpp" #include "bytewise.hpp" -#include "rmw_cyclonedds_cpp/TypeSupport_impl.hpp" namespace rmw_cyclonedds_cpp { @@ -51,7 +57,7 @@ struct CDRCursor if (n_bytes == 1 || start_offset % n_bytes == 0) { return; } - advance((-start_offset) % n_bytes); + advance(n_bytes - start_offset % n_bytes); assert(offset() - start_offset < n_bytes); assert(offset() % n_bytes == 0); } @@ -194,6 +200,8 @@ protected: case EncodingVersion::CDR1: eversion_byte = '\1'; break; + default: + unreachable(); } std::array rtps_header{eversion_byte, // encoding format = PLAIN_CDR @@ -304,17 +312,21 @@ protected: case EValueType::SpanSequenceValueType: case EValueType::BoolVectorValueType: result = false; + break; + default: + unreachable(); } trivially_serialized_cache.emplace(key, result); } return result; } - size_t get_cdr_alignof_primitive(ROSIDL_TypeKind vt) + size_t get_cdr_alignof_primitive(ROSIDL_TypeKind tk) { /// return 0 if the value type is not primitive /// else returns the number of bytes it should align to - return std::min(get_cdr_size_of_primitive(vt), max_align); + size_t sizeof_ = get_cdr_size_of_primitive(tk); + return sizeof_ < max_align ? sizeof_ : max_align; } void serialize(CDRCursor * cursor, const void * data, const PrimitiveValueType & value_type) @@ -357,7 +369,8 @@ protected: case ROSIDL_TypeKind::STRING: case ROSIDL_TypeKind::WSTRING: case ROSIDL_TypeKind::MESSAGE: - throw std::logic_error("not a primitive"); + default: + unreachable(); } } @@ -443,7 +456,7 @@ protected: if (auto s = dynamic_cast(value_type)) { return serialize(cursor, data, *s); } - throw std::logic_error("Unhandled case"); + unreachable(); } } diff --git a/rmw_cyclonedds_cpp/src/TypeSupport2.cpp b/rmw_cyclonedds_cpp/src/TypeSupport2.cpp index 3374b0f..053e66e 100644 --- a/rmw_cyclonedds_cpp/src/TypeSupport2.cpp +++ b/rmw_cyclonedds_cpp/src/TypeSupport2.cpp @@ -68,11 +68,13 @@ public: std::unique_ptr make_message_value_type(const rosidl_message_type_support_t * mts) { - if (auto ts_c = mts->func(mts, TypeGeneratorInfo::identifier)) { + if (auto ts_c = mts->func(mts, TypeGeneratorInfo::get_identifier())) { auto members = static_cast *>(ts_c->data); return std::make_unique(members); } - if (auto ts_cpp = mts->func(mts, TypeGeneratorInfo::identifier)) { + if (auto ts_cpp = + mts->func(mts, TypeGeneratorInfo::get_identifier())) + { auto members = static_cast *>(ts_cpp->data); return std::make_unique(members); } @@ -83,7 +85,9 @@ std::unique_ptr make_message_value_type(const rosidl_message_ty std::pair, std::unique_ptr> make_request_response_value_types(const rosidl_service_type_support_t * svc_ts) { - if (auto tsc = svc_ts->func(svc_ts, TypeGeneratorInfo::identifier)) { + if (auto tsc = + svc_ts->func(svc_ts, TypeGeneratorInfo::get_identifier())) + { auto typed = static_cast::MetaService *>(tsc->data); return { @@ -93,7 +97,7 @@ make_request_response_value_types(const rosidl_service_type_support_t * svc_ts) } if (auto tscpp = - svc_ts->func(svc_ts, TypeGeneratorInfo::identifier)) + svc_ts->func(svc_ts, TypeGeneratorInfo::get_identifier())) { auto typed = static_cast::MetaService *>(tscpp->data); diff --git a/rmw_cyclonedds_cpp/src/TypeSupport2.hpp b/rmw_cyclonedds_cpp/src/TypeSupport2.hpp index 030cf8a..0beadc1 100644 --- a/rmw_cyclonedds_cpp/src/TypeSupport2.hpp +++ b/rmw_cyclonedds_cpp/src/TypeSupport2.hpp @@ -15,6 +15,7 @@ #define TYPESUPPORT2_HPP_ #include +#include #include #include #include @@ -22,6 +23,7 @@ #include #include "bytewise.hpp" +#include "rmw_cyclonedds_cpp/exception.hpp" #include "rosidl_generator_c/string_functions.h" #include "rosidl_generator_c/u16string_functions.h" #include "rosidl_typesupport_introspection_c/identifier.h" @@ -79,8 +81,7 @@ template<> struct TypeGeneratorInfo { static constexpr auto enum_value = TypeGenerator::ROSIDL_C; - - static constexpr auto & identifier = rosidl_typesupport_introspection_c__identifier; + static const auto & get_identifier() {return rosidl_typesupport_introspection_c__identifier;} using MetaMessage = rosidl_typesupport_introspection_c__MessageMembers; using MetaMember = rosidl_typesupport_introspection_c__MessageMember; using MetaService = rosidl_typesupport_introspection_c__ServiceMembers; @@ -90,7 +91,10 @@ template<> struct TypeGeneratorInfo { static constexpr auto enum_value = TypeGenerator::ROSIDL_Cpp; - static constexpr auto & identifier = rosidl_typesupport_introspection_cpp::typesupport_identifier; + static const auto & get_identifier() + { + return rosidl_typesupport_introspection_cpp::typesupport_identifier; + } using MetaMessage = rosidl_typesupport_introspection_cpp::MessageMembers; using MetaMember = rosidl_typesupport_introspection_cpp::MessageMember; using MetaService = rosidl_typesupport_introspection_cpp::ServiceMembers; @@ -327,9 +331,8 @@ struct PrimitiveValueType : public AnyValueType case ROSIDL_TypeKind::STRING: case ROSIDL_TypeKind::WSTRING: case ROSIDL_TypeKind::MESSAGE: - throw std::runtime_error( - "not a primitive value type: " + - std::to_string(std::underlying_type_t(m_type_kind))); + default: + unreachable(); } } EValueType e_value_type() const override {return EValueType::PrimitiveValueType;} @@ -482,6 +485,8 @@ auto AnyValueType::apply(UnaryFunction f) const return f(*static_cast(this)); case EValueType::BoolVectorValueType: return f(*static_cast(this)); + default: + unreachable(); } } @@ -503,6 +508,8 @@ auto AnyValueType::apply(UnaryFunction f) return f(*static_cast(this)); case EValueType::BoolVectorValueType: return f(*static_cast(this)); + default: + unreachable(); } } diff --git a/rmw_cyclonedds_cpp/src/bytewise.hpp b/rmw_cyclonedds_cpp/src/bytewise.hpp index fe5def9..00a63d6 100644 --- a/rmw_cyclonedds_cpp/src/bytewise.hpp +++ b/rmw_cyclonedds_cpp/src/bytewise.hpp @@ -18,6 +18,9 @@ #include "dds/ddsrt/endian.h" +using std::size_t; +using std::ptrdiff_t; + enum class endian { little = DDSRT_LITTLE_ENDIAN, diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp index de8ae00..b17c6de 100644 --- a/rmw_cyclonedds_cpp/src/rmw_node.cpp +++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp @@ -1031,6 +1031,8 @@ static dds_qos_t * create_readwrite_qos( case RMW_QOS_POLICY_HISTORY_KEEP_ALL: dds_qset_history(qos, DDS_HISTORY_KEEP_ALL, DDS_LENGTH_UNLIMITED); break; + default: + rmw_cyclonedds_cpp::unreachable(); } switch (qos_policies->reliability) { case RMW_QOS_POLICY_RELIABILITY_SYSTEM_DEFAULT: @@ -1041,6 +1043,8 @@ static dds_qos_t * create_readwrite_qos( case RMW_QOS_POLICY_RELIABILITY_BEST_EFFORT: dds_qset_reliability(qos, DDS_RELIABILITY_BEST_EFFORT, 0); break; + default: + rmw_cyclonedds_cpp::unreachable(); } switch (qos_policies->durability) { case RMW_QOS_POLICY_DURABILITY_SYSTEM_DEFAULT: @@ -1060,6 +1064,8 @@ static dds_qos_t * create_readwrite_qos( DDS_LENGTH_UNLIMITED, DDS_LENGTH_UNLIMITED); break; } + default: + rmw_cyclonedds_cpp::unreachable(); } /* deadline, lifespan, liveliness are not yet supported */ if (ignore_local_publications) { @@ -1092,6 +1098,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic qos_policies->history = RMW_QOS_POLICY_HISTORY_KEEP_ALL; qos_policies->depth = (uint32_t) depth; break; + default: + rmw_cyclonedds_cpp::unreachable(); } } @@ -1109,6 +1117,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic case DDS_RELIABILITY_RELIABLE: qos_policies->reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE; break; + default: + rmw_cyclonedds_cpp::unreachable(); } } @@ -1129,6 +1139,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic case DDS_DURABILITY_PERSISTENT: qos_policies->durability = RMW_QOS_POLICY_DURABILITY_UNKNOWN; break; + default: + rmw_cyclonedds_cpp::unreachable(); } } @@ -1176,6 +1188,8 @@ static bool get_readwrite_qos(dds_entity_t handle, rmw_qos_profile_t * qos_polic case DDS_LIVELINESS_MANUAL_BY_TOPIC: qos_policies->liveliness = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC; break; + default: + rmw_cyclonedds_cpp::unreachable(); } if (lease_duration == DDS_INFINITY) { qos_policies->liveliness_lease_duration.sec = qos_policies->liveliness_lease_duration.nsec = @@ -1823,6 +1837,9 @@ extern "C" rmw_ret_t rmw_take_event( case RMW_EVENT_INVALID: { break; } + + default: + rmw_cyclonedds_cpp::unreachable(); } *taken = false; return RMW_RET_ERROR; diff --git a/rmw_cyclonedds_cpp/src/serdata.cpp b/rmw_cyclonedds_cpp/src/serdata.cpp index 23a85a9..fb7a941 100644 --- a/rmw_cyclonedds_cpp/src/serdata.cpp +++ b/rmw_cyclonedds_cpp/src/serdata.cpp @@ -111,7 +111,10 @@ void * create_response_type_support( static uint32_t serdata_rmw_size(const struct ddsi_serdata * dcmn) { - return static_cast(dcmn)->size(); + size_t size = static_cast(dcmn)->size(); + uint32_t size_u32(size); + assert(size == size_u32); + return size_u32; } static void serdata_rmw_free(struct ddsi_serdata * dcmn) @@ -142,7 +145,7 @@ static struct ddsi_serdata * serdata_rmw_from_ser( memcpy(cursor, src, n_bytes); cursor = byte_offset(cursor, n_bytes); off = fragchain->maxp1; - assert(off < size); + assert(off <= size); } fragchain = fragchain->nextfrag; } @@ -491,7 +494,7 @@ struct sertopic_rmw * create_sertopic( return st; } -void serdata_rmw::resize(uint32_t requested_size) +void serdata_rmw::resize(size_t requested_size) { if (!requested_size) { m_size = 0; diff --git a/rmw_cyclonedds_cpp/src/serdata.hpp b/rmw_cyclonedds_cpp/src/serdata.hpp index 22d6155..d6a62d8 100644 --- a/rmw_cyclonedds_cpp/src/serdata.hpp +++ b/rmw_cyclonedds_cpp/src/serdata.hpp @@ -43,15 +43,15 @@ struct sertopic_rmw : ddsi_sertopic class serdata_rmw : public ddsi_serdata { protected: - uint32_t m_size {0}; + size_t m_size {0}; /* first two bytes of data is CDR encoding second two bytes are encoding options */ std::unique_ptr m_data {nullptr}; public: serdata_rmw(const ddsi_sertopic * topic, ddsi_serdata_kind kind); - void resize(uint32_t requested_size); - uint32_t size() const {return m_size;} + void resize(size_t requested_size); + size_t size() const {return m_size;} void * data() const {return m_data.get();} };