From 0cb4ac79d5238011396377fcd2e50ef3b13fd2e5 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 23 Jun 2023 11:25:20 +0200 Subject: [PATCH 1/5] unit-tests: intialize global test params at runtime Move the intialization of the paramters to runtime by converting the static array to a vector and filling it when critirion calls the test setup. Global variable initialization order across translation units is not defined. Referring to e.g. the memory::mmap global variable in the initialization of the static unit test parameter set may leave an incorrect address in the struct param. Signed-off-by: Philipp Jungkamp --- lib/pool.cpp | 4 +++- tests/unit/pool.cpp | 22 +++++++++++----------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/lib/pool.cpp b/lib/pool.cpp index 011101ea4..5225a40e2 100644 --- a/lib/pool.cpp +++ b/lib/pool.cpp @@ -17,17 +17,19 @@ using namespace villas; int villas::node::pool_init(struct Pool *p, size_t cnt, size_t blocksz, struct memory::Type *m) { int ret; + auto logger = logging.get("pool"); /* Make sure that we use a block size that is aligned to the size of a cache line */ p->alignment = kernel::getCachelineSize(); p->blocksz = p->alignment * CEIL(blocksz, p->alignment); p->len = cnt * p->blocksz; + logger->debug("New memory pool: alignment={}, blocksz={}, len={}, memory={}", p->alignment, p->blocksz, p->len, m->name); + void *buffer = memory::alloc_aligned(p->len, p->alignment, m); if (!buffer) throw MemoryAllocationError(); - auto logger = logging.get("pool"); logger->debug("Allocated {:#x} bytes for memory pool", p->len); p->buffer_off = (char*) buffer - (char*) p; diff --git a/tests/unit/pool.cpp b/tests/unit/pool.cpp index 65dd3eaaa..fdbaecd86 100644 --- a/tests/unit/pool.cpp +++ b/tests/unit/pool.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -29,14 +30,15 @@ struct param { ParameterizedTestParameters(pool, basic) { - static struct param params[] = { - { 1, 4096, 150, &memory::heap }, - { 1, 128, 8, &memory::mmap }, - { 1, 4, 8192, &memory::mmap_hugetlb }, - { 1, 1 << 13, 4, &memory::mmap_hugetlb } - }; + static std::vector params; - return cr_make_param_array(struct param, params, ARRAY_LEN(params)); + params.clear(); + params.push_back({ 1, 4096, 150, &memory::heap }); + params.push_back({ 1, 128, 8, &memory::mmap }); + params.push_back({ 1, 4, 8192, &memory::mmap_hugetlb }); + params.push_back({ 1, 1 << 13, 4, &memory::mmap_hugetlb }); + + return cr_make_param_array(struct param, params.data(), params.size()); } // cppcheck-suppress unknownMacro @@ -44,12 +46,10 @@ ParameterizedTest(struct param *p, pool, basic, .init = init_memory) { int ret; struct Pool pool; - - // some strange LTO stuff is going on here.. - auto *m __attribute__((unused)) = &memory::mmap; - void *ptr, *ptrs[p->pool_size]; + logging.setLevel("trace"); + if (!utils::isPrivileged() && p->mt == &memory::mmap_hugetlb) cr_skip_test("Skipping memory_mmap_hugetlb tests allocatpr because we are running in an unprivileged environment."); From 4c662ff56a4ba7a0e5c0f434733fa1e3fd5f1f19 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 23 Jun 2023 11:34:21 +0200 Subject: [PATCH 2/5] packaging-nix: Add criterion to devShells to allow building unit-tests Signed-off-by: Philipp Jungkamp --- packaging/nix/flake.lock | 6 +++--- packaging/nix/flake.nix | 9 +++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packaging/nix/flake.lock b/packaging/nix/flake.lock index c9bc3fc81..3892ff349 100644 --- a/packaging/nix/flake.lock +++ b/packaging/nix/flake.lock @@ -90,11 +90,11 @@ }, "nixpkgs": { "locked": { - "lastModified": 1686656800, - "narHash": "sha256-duScdQZNeZcde0JwmQ9W4XfqlO/Z24MDhlTq2MokuSM=", + "lastModified": 1687482344, + "narHash": "sha256-ZKsH8Cpb97Zbozaz26XkLv7Swa5uk3G1Vmy34MVt1ro=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "2b273c2351fe1ab490158cf8acc8aafad02592ce", + "rev": "3612945be80f916cde80537433ec0a4a0e6b3d3b", "type": "github" }, "original": { diff --git a/packaging/nix/flake.nix b/packaging/nix/flake.nix index d8e6dd8d1..a4eb07670 100644 --- a/packaging/nix/flake.nix +++ b/packaging/nix/flake.nix @@ -122,19 +122,20 @@ pkgs = pkgsFor system; shellHook = ''[ -z "$PS1" ] || exec "$SHELL"''; hardeningDisable = ["all"]; + packages = with pkgs; [criterion]; in rec { default = full; minimal = pkgs.mkShell { - inherit shellHook hardeningDisable; + inherit shellHook hardeningDisable packages; name = "minimal"; - inputsFrom = [pkgs.villas-minimal]; + inputsFrom = with pkgs; [villas-minimal]; }; full = pkgs.mkShell { - inherit shellHook hardeningDisable; + inherit shellHook hardeningDisable packages; name = "full"; - inputsFrom = [pkgs.villas]; + inputsFrom = with pkgs; [villas]; }; } ); From cd5a99df67ef5e8cf50b6fac5690f9be11ee6061 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 23 Jun 2023 11:50:07 +0200 Subject: [PATCH 3/5] packaging-nix: fix cross-compilation Update nixpkgs for upstream libre cross compilation fix. Use minimal systemd dependency for mosquitto and rdma-core. Signed-off-by: Philipp Jungkamp --- packaging/nix/flake.lock | 8 ++++---- packaging/nix/flake.nix | 23 ++++++++++++++++------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packaging/nix/flake.lock b/packaging/nix/flake.lock index 3892ff349..321ea822f 100644 --- a/packaging/nix/flake.lock +++ b/packaging/nix/flake.lock @@ -90,16 +90,16 @@ }, "nixpkgs": { "locked": { - "lastModified": 1687482344, - "narHash": "sha256-ZKsH8Cpb97Zbozaz26XkLv7Swa5uk3G1Vmy34MVt1ro=", + "lastModified": 1687354544, + "narHash": "sha256-1Xu+QzyA10AiY21i27Zu9bqQAaxXBacNKbGUA9OZy7Y=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "3612945be80f916cde80537433ec0a4a0e6b3d3b", + "rev": "876181e3ae452cc6186486f6f7300a8a6de237cb", "type": "github" }, "original": { "owner": "NixOS", - "ref": "release-22.11", + "ref": "release-23.05", "repo": "nixpkgs", "type": "github" } diff --git a/packaging/nix/flake.nix b/packaging/nix/flake.nix index a4eb07670..91c13f242 100644 --- a/packaging/nix/flake.nix +++ b/packaging/nix/flake.nix @@ -2,7 +2,7 @@ description = "a tool for connecting real-time power grid simulation equipment"; inputs = { - nixpkgs.url = "github:NixOS/nixpkgs/release-22.11"; + nixpkgs.url = "github:NixOS/nixpkgs/release-23.05"; common = { url = "github:VILLASframework/common"; @@ -55,18 +55,23 @@ # generate attributes corresponding to all supported combinations of system and crossSystem forSupportedCrossSystems = f: forSupportedSystems (system: lib.genAttrs supportedCrossSystems (f system)); - # this overlay can be applied to nixpkgs (see `pkgsFor` below for an example) - overlay = final: prev: packagesWith final; - # initialize nixpkgs for the specified `system` pkgsFor = system: import nixpkgs { inherit system; - overlays = [overlay]; + overlays = [self.overlays.default]; }; # initialize nixpkgs for cross-compiling from `system` to `crossSystem` - crossPkgsFor = system: crossSystem: (pkgsFor system).pkgsCross.${crossSystem}; + crossPkgsFor = system: crossSystem: + (import nixpkgs { + inherit system; + overlays = [ + self.overlays.default + self.overlays.minimal + ]; + }) + .pkgsCross.${crossSystem}; # build villas and its dependencies for the specified `pkgs` packagesWith = pkgs: rec { @@ -113,7 +118,11 @@ # standard flake attribute allowing you to add the villas packages to your nixpkgs overlays = { - default = overlay; + default = final: prev: packagesWith final; + minimal = final: prev: { + mosquitto = prev.mosquitto.override {systemd = final.systemdMinimal;}; + rdma-core = prev.rdma-core.override {udev = final.systemdMinimal;}; + }; }; # standard flake attribute for defining developer environments From 4224f72ecbbd30170958aee72cee2ae02aefee9d Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Thu, 22 Jun 2023 11:17:53 +0200 Subject: [PATCH 4/5] cmake: disable warning as error on default release builds A new warning introduced by the compiler on a target system should not break the build on that target. You can override that behaviour using VILLAS_COMPILE_WARNING_AS_ERROR. Signed-off-by: Philipp Jungkamp --- .gitlab-ci.yml | 8 +++++--- CMakeLists.txt | 17 ++++++++++++++++- cmake/config/Release.cmake | 5 +++++ packaging/docker/Dockerfile.debian-multiarch | 1 + packaging/nix/flake.nix | 2 +- 5 files changed, 28 insertions(+), 5 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 7ff363473..df48decba 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -7,7 +7,8 @@ variables: DOCKER_IMAGE: registry.git.rwth-aachen.de/acs/public/villas/node DOCKER_IMAGE_DEV: ${DOCKER_IMAGE}/dev-${DISTRO} DOCKER_CLI_EXPERIMENTAL: enabled - CMAKE_BUILD_OPTS: "--config Release --parallel 16" + CMAKE_BUILD_OPTS: "--parallel 16" + CMAKE_EXTRA_OPTS: "-DCMAKE_BUILD_TYPE=Release -DVILLAS_COMPILE_WARNING_AS_ERROR=ON" stages: - prepare @@ -64,7 +65,8 @@ build:source: matrix: - DISTRO: [ fedora, fedora-minimal, debian, rocky, ubuntu ] - DISTRO: fedora-minimal - CMAKE_EXTRA_OPTS: -DWITH_API=OFF + CMAKE_EXTRA_OPTS: -DVILLAS_COMPILE_WARNING_AS_ERROR=ON + -DWITH_API=OFF -DWITH_CLIENTS=OFF -DWITH_CONFIG=OFF -DWITH_DOC=OFF @@ -183,7 +185,7 @@ pkg:docker: --target ${TARGET} --build-arg ARCH=${ARCH} --build-arg TRIPLET=${TRIPLET} - --build-arg CMAKE_OPTS="${CMAKE_OPTS}" + --build-arg CMAKE_OPTS="${CMAKE_OPTS} ${CMAKE_EXTRA_OPTS}" --platform ${PLATFORM} --file ${DOCKER_FILE} --tag ${DOCKER_IMAGE}:${DOCKER_TAG}-${ARCH} . diff --git a/CMakeLists.txt b/CMakeLists.txt index e9a0f1062..0b90cbfdb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,7 @@ include(FindSymbol) include(CMakeDependentOption) add_definitions(-D_POSIX_C_SOURCE=200809L -D_GNU_SOURCE) -add_compile_options(-Wall -Wno-unknown-pragmas -Werror -fdiagnostics-color=auto) +add_compile_options(-Wall -Wno-unknown-pragmas -fdiagnostics-color=auto) # Check OS check_include_file("sys/eventfd.h" HAS_EVENTFD) @@ -213,12 +213,27 @@ cmake_dependent_option(WITH_NODE_WEBRTC "Build with webrtc node-type" cmake_dependent_option(WITH_NODE_WEBSOCKET "Build with websocket node-type" ON "WITH_WEB; LIBWEBSOCKETS_FOUND" OFF) cmake_dependent_option(WITH_NODE_ZEROMQ "Build with zeromq node-type" ON "LIBZMQ_FOUND;NOT WITHOUT_GPL" OFF) +# set a default for the build type +if("${CMAKE_BUILD_TYPE}" STREQUAL "") + set(CMAKE_BUILD_TYPE "Debug") +endif() + # Add more build configurations include(cmake/config/Debug.cmake) include(cmake/config/Release.cmake) include(cmake/config/Coverage.cmake) include(cmake/config/Profiling.cmake) +if(NOT DEFINED VILLAS_COMPILE_WARNING_AS_ERROR) + set(VILLAS_COMPILE_WARNING_AS_ERROR ON) +endif() + +if(VILLAS_COMPILE_WARNING_AS_ERROR) + add_compile_options(-Werror) +else() + add_compile_options(-Wno-error) +endif() + # Get version info and buildid from Git GetVersion(${PROJECT_SOURCE_DIR} "CMAKE_PROJECT") diff --git a/cmake/config/Release.cmake b/cmake/config/Release.cmake index 37961761c..1cc8307c5 100644 --- a/cmake/config/Release.cmake +++ b/cmake/config/Release.cmake @@ -4,3 +4,8 @@ # @copyright 2014-2022, Institute for Automation of Complex Power Systems, EONERC # @license Apache 2.0 ################################################################################### + +# don't treat warnings as errors on normal release builds +if("${CMAKE_BUILD_TYPE}" STREQUAL "Release" AND NOT DEFINED VILLAS_COMPILE_WARNING_AS_ERROR) + set(VILLAS_COMPILE_WARNING_AS_ERROR OFF) +endif() diff --git a/packaging/docker/Dockerfile.debian-multiarch b/packaging/docker/Dockerfile.debian-multiarch index 304ae76c1..6564bd1c7 100644 --- a/packaging/docker/Dockerfile.debian-multiarch +++ b/packaging/docker/Dockerfile.debian-multiarch @@ -69,6 +69,7 @@ ADD cmake/toolchains/debian-${ARCH}.cmake / ENV PKG_CONFIG_PATH=/usr/lib/${TRIPLET}/pkgconfig:/usr/local/lib/${TRIPLET}/pkgconfig ENV CMAKE_EXTRA_OPTS="-DCMAKE_TOOLCHAIN_FILE=/debian-${ARCH}.cmake \ + -DCMAKE_BUILD_TYPE=Release \ -DCMAKE_PREFIX_PATH=${PREFIX} \ -DCMAKE_INSTALL_PREFIX=${PREFIX} \ -DCMAKE_INSTALL_LIBDIR=${PREFIX}/lib/${TRIPLET} \ diff --git a/packaging/nix/flake.nix b/packaging/nix/flake.nix index 91c13f242..e2ef3f7d6 100644 --- a/packaging/nix/flake.nix +++ b/packaging/nix/flake.nix @@ -131,7 +131,7 @@ pkgs = pkgsFor system; shellHook = ''[ -z "$PS1" ] || exec "$SHELL"''; hardeningDisable = ["all"]; - packages = with pkgs; [criterion]; + packages = with pkgs; [bashInteractive criterion]; in rec { default = full; From b9c8ffce5b9a5a4b2d98458eeff464579e753b12 Mon Sep 17 00:00:00 2001 From: Philipp Jungkamp Date: Fri, 23 Jun 2023 12:00:08 +0200 Subject: [PATCH 5/5] node-iec60870: ignore -Wmaybe-uninitialized false positive Signed-off-by: Philipp Jungkamp --- lib/nodes/iec60870.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/nodes/iec60870.cpp b/lib/nodes/iec60870.cpp index d3683bb76..09b2461d4 100644 --- a/lib/nodes/iec60870.cpp +++ b/lib/nodes/iec60870.cpp @@ -256,8 +256,11 @@ std::optional ASDUData::checkASDU(CS101_ASDU const &asdu) cons bool ASDUData::addSampleToASDU(CS101_ASDU &asdu, ASDUData::Sample sample) const { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" + std::optional timestamp = sample.timestamp.has_value() - ? std::optional { timespec_to_cp56time2a(sample.timestamp.value()) } + ? std::optional { timespec_to_cp56time2a(*sample.timestamp) } : std::nullopt; InformationObject io = nullptr; @@ -341,6 +344,7 @@ bool ASDUData::addSampleToASDU(CS101_ASDU &asdu, ASDUData::Sample sample) const InformationObject_destroy(io); return successfully_added; +#pragma GCC diagnostic pop } ASDUData::ASDUData(ASDUData::Descriptor const *descriptor, int ioa, int ioa_sequence_start) : ioa(ioa), ioa_sequence_start(ioa_sequence_start), descriptor(descriptor)