diff --git a/.travis.yml b/.travis.yml index e88b579ef..dda19c1b2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -57,13 +57,13 @@ script: make -j && CTEST_OUTPUT_ON_FAILURE=1 make ExperimentalTest ExperimentalMemCheck ) - ( mkdir build && cd build && - cmake -DCMAKE_PREFIX_PATH=$PREFIX -DCMAKE_INSTALL_PREFIX=../install .. && + cmake -DCMAKE_PREFIX_PATH=$PREFIX -DCMAKE_INSTALL_PREFIX=../install -DCMAKE_BUILD_TYPE=Release .. && make -j && CTEST_OUTPUT_ON_FAILURE=1 make test && make install && cpack --verbose -G DEB && dpkg --info *.deb) - ( cd skeleton-subsystem && - cmake -DCMAKE_INSTALL_PREFIX=../install . && + cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=../install . && make && CTEST_OUTPUT_ON_FAILURE=1 make ExperimentalTest ExperimentalMemCheck && make install ) diff --git a/CMakeLists.txt b/CMakeLists.txt index c44adc31e..b07beae72 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -42,7 +42,7 @@ option(DOXYGEN "Enable doxygen generation (you still have to run 'make doc')" OF option(REQUIREMENTS "Generate the html version of the 'requirements' documentation" OFF) option(PYTHON_BINDINGS "Python library to use the Parameter Framework from python" ON) option(C_BINDINGS "Library to use the Parameter Framework using a C API" ON) - +option(FATAL_WARNINGS "Turn warnings into errors (-Werror flag)" ON) # find and set the Parameter Framework's version execute_process(COMMAND git describe --tags --dirty @@ -73,8 +73,14 @@ if(WIN32) # only public methods instead of the whole class, but they are too many to # do that. A separated plugin interface would fix that. set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4 /FIiso646.h -wd4127 -wd4251") + + # FIXME: Once we have removed all warnings on windows, add the /WX flags if + # FATAL_WARNINGS is enabled else() - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Werror -Wall -Wextra -Wconversion -Wno-sign-conversion") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wextra -Wconversion -Wno-sign-conversion") + if(FATAL_WARNINGS) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror") + endif() endif() # Hide symbols by default, then exposed symbols are the same in linux and windows diff --git a/bindings/python/CMakeLists.txt b/bindings/python/CMakeLists.txt index feaf658b6..580763389 100644 --- a/bindings/python/CMakeLists.txt +++ b/bindings/python/CMakeLists.txt @@ -50,7 +50,10 @@ find_package(PythonLibs ${PYTHON_VERSION_STRING} EXACT REQUIRED) include_directories(${PYTHON_INCLUDE_DIRS}) set_property(SOURCE pfw.i PROPERTY CPLUSPLUS ON) -set_property(SOURCE pfw.i PROPERTY SWIG_FLAGS "-Wall" "-Werror") +set_property(SOURCE pfw.i PROPERTY SWIG_FLAGS "-Wall") +if(FATAL_WARNINGS) + set_property(SOURCE pfw.i APPEND PROPERTY SWIG_FLAGS "-Werror") +endif() swig_add_module(PyPfw python pfw.i) swig_link_libraries(PyPfw parameter ${PYTHON_LIBRARIES}) @@ -70,7 +73,8 @@ set_property(TARGET _PyPfw PROPERTY LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BIN # spurious anyway, as it relates to "ILogger *" which is an abstract # class/interface class and as such cannot be destroyed. # -Wno-error is set to prevent compilation failure in case of the code -# generated by swig generates warnings +# generated by swig generates warnings. We don't apply the FATAL_WARNING policy +# here, since we consider this generated code as external. target_compile_definitions(_PyPfw PRIVATE SWIG_PYTHON_SILENT_MEMLEAK) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error") diff --git a/parameter/FloatingPointParameterType.cpp b/parameter/FloatingPointParameterType.cpp index 503785ab1..64358757a 100644 --- a/parameter/FloatingPointParameterType.cpp +++ b/parameter/FloatingPointParameterType.cpp @@ -36,6 +36,7 @@ #include #include "convert.hpp" #include "Utility.h" +#include "BinaryCopy.hpp" using std::string; @@ -138,7 +139,7 @@ bool CFloatingPointParameterType::toBlackboard( return false; } - float fData = reinterpret_cast(uiValue); + auto fData = utility::binaryCopy(uiValue); // Check against NaN or infinity if (!std::isfinite(fData)) { @@ -172,9 +173,7 @@ bool CFloatingPointParameterType::toBlackboard( } // Move to the "raw memory" value space - // Using an intermediary reference variable to avoid klocwork false positive - const uint32_t &value = reinterpret_cast(fValue); - uiValue = value; + uiValue = utility::binaryCopy(fValue); return true; } } @@ -193,8 +192,8 @@ void CFloatingPointParameterType::setOutOfRangeError( ostrStream << "real range [" << _fMin << ", " << _fMax << "]"; } else { - uint32_t uiMin = reinterpret_cast(_fMin); - uint32_t uiMax = reinterpret_cast(_fMax); + auto uiMin = utility::binaryCopy(_fMin); + auto uiMax = utility::binaryCopy(_fMax); if (utility::isHexadecimal(strValue)) { @@ -228,7 +227,7 @@ bool CFloatingPointParameterType::fromBlackboard( } else { // Move from "raw memory" value space to real space - float fValue = reinterpret_cast(uiValue); + auto fValue = utility::binaryCopy(uiValue); ostrStream << fValue; } @@ -252,9 +251,7 @@ bool CFloatingPointParameterType::toBlackboard( // Cast is fine because dValue has been checked against the value range float fValue = static_cast(dUserValue); - // Using an intermediary reference variable to avoid klocwork false positive - const uint32_t &value = reinterpret_cast(fValue); - uiValue = value; + uiValue = utility::binaryCopy(fValue); return true; } @@ -264,8 +261,7 @@ bool CFloatingPointParameterType::fromBlackboard( CParameterAccessContext& /*ctx*/) const { // Move from "raw memory" value space to real space - // Using an intermediary reference variable to avoid klocwork false positive - const float &fValue = reinterpret_cast(uiValue); + auto fValue = utility::binaryCopy(uiValue); dUserValue = fValue; return true; diff --git a/test/functional-tests/FloatingPoint.cpp b/test/functional-tests/FloatingPoint.cpp index 5f66e78ff..bf6bdb8a4 100644 --- a/test/functional-tests/FloatingPoint.cpp +++ b/test/functional-tests/FloatingPoint.cpp @@ -32,6 +32,7 @@ #include "ParameterFramework.hpp" #include "ElementHandle.hpp" #include "Test.hpp" +#include "BinaryCopy.hpp" #include @@ -114,12 +115,12 @@ SCENARIO_METHOD(FloatsPF, "Floating points", "[floating points]") { REQUIRE_NOTHROW(setRawValueSpace(true)); for (auto &vec : Tests{ { "(too high, as decimal)", - std::to_string(reinterpret_cast(tooHigh)) }, + std::to_string(::utility::binaryCopy(tooHigh)) }, { "(too low, as decimal)", - std::to_string(reinterpret_cast(tooLow)) }, + std::to_string(::utility::binaryCopy(tooLow)) }, { "(meaningless)", "foobar" }, - { "(infinity)", std::to_string(reinterpret_cast(inf))}, - { "(NaN)", std::to_string(reinterpret_cast(nan))}, + { "(infinity)", std::to_string(::utility::binaryCopy(inf))}, + { "(NaN)", std::to_string(::utility::binaryCopy(nan))}, }) { GIVEN("Invalid value " + vec.title) { CHECK_THROWS_AS(setParameter(path, vec.payload), Exception); @@ -130,11 +131,11 @@ SCENARIO_METHOD(FloatsPF, "Floating points", "[floating points]") { const float zero = 0.0f; for (auto &vec : Tests{ { "(upper limit, as decimal)", - std::to_string(reinterpret_cast(upper)) }, + std::to_string(::utility::binaryCopy(upper)) }, { "(lower limit, as decimal)", - std::to_string(reinterpret_cast(lower)) }, + std::to_string(::utility::binaryCopy(lower)) }, { "(inside range, as decimal)", - std::to_string(reinterpret_cast(zero)) }, + std::to_string(::utility::binaryCopy(zero)) }, }) { GIVEN("A valid value " + vec.title) { CHECK_NOTHROW(setParameter(path, vec.payload)); diff --git a/test/functional-tests/Handle.cpp b/test/functional-tests/Handle.cpp index 879829e41..fef03b71a 100644 --- a/test/functional-tests/Handle.cpp +++ b/test/functional-tests/Handle.cpp @@ -162,7 +162,10 @@ struct AllParamsPF : public ParameterFramework utility::TmpFile resultFile(result); utility::TmpFile expectedFile(expected); auto gitCommand = "git --no-pager diff --word-diff-regex='[^ <>]+' --color --no-index "; - system((gitCommand + resultFile.getPath() + ' ' + expectedFile.getPath()).c_str()); + auto diffSuccess = system((gitCommand + resultFile.getPath() + ' ' + expectedFile.getPath()).c_str()); + if (diffSuccess != 0) { + WARN("Failed to pretty-print the difference between actual and expected results."); + } } } diff --git a/utility/BinaryCopy.hpp b/utility/BinaryCopy.hpp new file mode 100644 index 000000000..65ef4bb65 --- /dev/null +++ b/utility/BinaryCopy.hpp @@ -0,0 +1,70 @@ +/* + * Copyright (c) 2015, Intel Corporation + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without modification, + * are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation and/or + * other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR + * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include +#include +#include "Iterator.hpp" +#include + +namespace utility { + +/** + * Raw copy of one variable to another of the same size + * + * This can be regarder as a reinterpret_cast but does a copy and does not + * break strict-aliasing rules. + * + * The source and the destination must have the same storage size (e.g. copying + * a uint8_t into a uint32_t won't compile) + * + * @tparam Source The source type + * @tparam Destination the destination type (even if it is a reference, this + * function returns by copy) + * @param source Source variable + * @returns the source, reinterpreted as the destination type + */ +template +typename std::remove_reference::type binaryCopy(const Source source) +{ + static_assert(sizeof(Source) == sizeof(Destination), + "Source and Destination must have the same size"); + + using Destination_ = decltype(binaryCopy(source)); + + union { + Source source; + Destination_ destination; + } hack; + + hack.source = source; + return hack.destination; +} + +} // namespace utility diff --git a/utility/test/utility.cpp b/utility/test/utility.cpp index fe514302e..62d550095 100644 --- a/utility/test/utility.cpp +++ b/utility/test/utility.cpp @@ -29,6 +29,7 @@ */ #include "Utility.h" +#include "BinaryCopy.hpp" #include #include @@ -176,4 +177,52 @@ SCENARIO("isHexadecimal") { } } +template +void checkBinaryEqual(T1 v1, T2 v2) { + // For some yet-unknown reason, GCC 4.8 complains about + // CHECK(a == b); + // and suggests that parentheses should be added. This is related to catch + // internals but such construcuts have been used without problem in lots of + // other places... + // Besides, GCC 4.9 does not seem to have a problem with that either. + // As a workaround, captures variables and parenthesize the expressions. + + auto v2AsT1 = utility::binaryCopy(v2); + CAPTURE(v1); + CAPTURE(v2AsT1); + CHECK((v1 == v2AsT1)); + + auto v1AsT2 = utility::binaryCopy(v1); + CAPTURE(v2); + CAPTURE(v1AsT2); + CHECK((v2 == v1AsT2)); +} + +SCENARIO("binaryCopy bit exactness") { + GIVEN("Integer representations computed using http://babbage.cs.qc.cuny.edu/IEEE-754/") { + + THEN("Floats should be coded on 32bits and fulfill IEEE-754." + " That assumption is made in the Parameter Framework.") { + REQUIRE(sizeof(float) == sizeof(uint32_t)); + REQUIRE(std::numeric_limits::is_iec559); + } + WHEN("Testing float <=> uint32_t conversion") { + checkBinaryEqual(1.23456f, 0x3f9e0610); + } + + THEN("Doubles should be coded on 64bits and fulfill IEEE-754." + " That assumption is made in the Parameter Framework.") { + REQUIRE(sizeof(double) == sizeof(uint64_t)); + REQUIRE(std::numeric_limits::is_iec559); + } + WHEN("Testing double <=> uint64_t conversion") { + checkBinaryEqual(987.65432109876, 0x408edd3c0cb3420e); + } + } + + WHEN("Testing int8_t <=> uint8_t conversion") { + checkBinaryEqual(-1, 0xff); + } +} + } // namespace utility