From 9c7188547cd53dddd635c86c8ef5655290541966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 20 Feb 2023 19:57:30 +0100 Subject: [PATCH 1/5] tmpfiles.d: drop misleading comment I'm not sure what "suffix" was meant by this comment, but the file has the usual suffix. The file was added with the current name back in c4708f132381e4bbc864d5241381b5cde4f54878. Maybe an earlier version of the patch did something different. --- tmpfiles.d/systemd-nologin.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/tmpfiles.d/systemd-nologin.conf b/tmpfiles.d/systemd-nologin.conf index 39cfd06e8b..69a212a8db 100644 --- a/tmpfiles.d/systemd-nologin.conf +++ b/tmpfiles.d/systemd-nologin.conf @@ -6,6 +6,5 @@ # (at your option) any later version. # See tmpfiles.d(5), systemd-user-sessions.service(8) and pam_nologin(8). -# This file has special suffix so it is not run by mistake. F! /run/nologin 0644 - - - "System is booting up. Unprivileged users are not permitted to log in yet. Please come back later. For technical details, see pam_nologin(8)." From 3f275dcb841bac7a29bba31c3b8b32981f6281fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 22 Feb 2023 11:13:48 +0100 Subject: [PATCH 2/5] test-set: drop left-over valgrind check In b01f31954f1c7c4601925173ae2638b572224e9a mempool_use_allowed was dropped, but apparently it was forgotten here. --- src/test/test-set.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/test-set.c b/src/test/test-set.c index 0fc9dffe23..cba07c207d 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -5,8 +5,6 @@ #include "strv.h" #include "tests.h" -const bool mempool_use_allowed = VALGRIND; - TEST(set_steal_first) { _cleanup_set_free_ Set *m = NULL; int seen[3] = {}; From 50b35193ec6f8f342364742a69a607e967b39b7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 21 Feb 2023 19:59:57 +0100 Subject: [PATCH 3/5] meson: merge our two valgrind configuration conditions into one Most of the support for valgrind was under HAVE_VALGRIND_VALGRIND_H, i.e. we would enable if the valgrind headers were found. The operations then we be conditionalized on RUNNING_UNDER_VALGRIND. But in a few places we had code which was conditionalized on VALGRIND, i.e. the config option. I noticed because I compiled with -Dvalgrind=true on a machine that didn't have valgrind.h, and the build failed because RUNNING_UNDER_VALGRIND was not defined. My first idea was to add a check that the header is present if the option is set, but it seems better to just remove the option. The code to support valgrind is trivial, and if we're !RUNNING_UNDER_VALGRIND, it has negligible cost. And the case of running under valgrind is always some special testing/debugging mode, so we should just do those extra steps to make valgrind output cleaner. Removing the option makes things simpler and we don't have to think if something should be covered by the one or the other configuration bit. I had a vague recollection that in some places we used -Dvalgrind=true not for valgrind support, but to enable additional cleanup under other sanitizers. But that code would fail to build without the valgrind headers anyway, so I'm not sure if that was still used. If there are uses like that, we can extend the condition for cleanup_pools(). --- README | 12 ++++++------ meson.build | 3 --- meson_options.txt | 2 -- src/basic/hashmap.c | 8 ++++++-- src/core/main.c | 6 +++--- src/libsystemd-network/test-dhcp-client.c | 8 ++++++-- src/libsystemd/sd-journal/journal-send.c | 8 ++++---- src/libsystemd/sd-journal/journal-send.h | 5 ----- src/libsystemd/sd-journal/lookup3.c | 8 +++++--- 9 files changed, 30 insertions(+), 30 deletions(-) diff --git a/README b/README index 857cb38bf8..97338a633d 100644 --- a/README +++ b/README @@ -403,12 +403,12 @@ WARNINGS and TAINT FLAGS: See org.freedesktop.systemd1(5) for more information. VALGRIND: - To run systemd under valgrind, compile with meson option - -Dvalgrind=true and have valgrind development headers installed - (i.e. valgrind-devel or equivalent). Otherwise, false positives will be - triggered by code which violates some rules but is actually safe. Note - that valgrind generates nice output only on exit(), hence on shutdown - we don't execve() systemd-shutdown. + To run systemd under valgrind, compile systemd with the valgrind + development headers available (i.e. valgrind-devel or equivalent). + Otherwise, false positives will be triggered by code which violates + some rules but is actually safe. Note that valgrind generates nice + output only on exit(), hence on shutdown we don't execve() + systemd-shutdown. STABLE BRANCHES AND BACKPORTS: Stable branches with backported patches are available in the diff --git a/meson.build b/meson.build index 319246c639..8e37a06b31 100644 --- a/meson.build +++ b/meson.build @@ -1020,8 +1020,6 @@ endforeach conf.set10('ENABLE_DEBUG_HASHMAP', enable_debug_hashmap) conf.set10('ENABLE_DEBUG_MMAP_CACHE', enable_debug_mmap_cache) conf.set10('ENABLE_DEBUG_SIPHASH', enable_debug_siphash) - -conf.set10('VALGRIND', get_option('valgrind')) conf.set10('LOG_TRACE', get_option('log-trace')) default_user_path = get_option('user-path') @@ -4608,7 +4606,6 @@ foreach tuple : [ ['debug hashmap'], ['debug mmap cache'], ['debug siphash'], - ['valgrind', conf.get('VALGRIND') == 1], ['trace logging', conf.get('LOG_TRACE') == 1], ['install tests', install_tests], ['link-udev-shared', get_option('link-udev-shared')], diff --git a/meson_options.txt b/meson_options.txt index f59c399795..95b1162249 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -79,8 +79,6 @@ option('bump-proc-sys-fs-file-max', type : 'boolean', description : 'bump /proc/sys/fs/file-max to LONG_MAX') option('bump-proc-sys-fs-nr-open', type : 'boolean', description : 'bump /proc/sys/fs/nr_open to INT_MAX') -option('valgrind', type : 'boolean', value : false, - description : 'do extra operations to avoid valgrind warnings') option('log-trace', type : 'boolean', value : false, description : 'enable low level debug logging') option('user-path', type : 'string', diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c index 3d6d99e6de..4b8daf4a29 100644 --- a/src/basic/hashmap.c +++ b/src/basic/hashmap.c @@ -5,6 +5,9 @@ #include #include #include +#if HAVE_VALGRIND_VALGRIND_H +# include +#endif #include "alloc-util.h" #include "fileio.h" @@ -295,10 +298,11 @@ void hashmap_trim_pools(void) { mempool_trim(&ordered_hashmap_pool); } -#if VALGRIND +#if HAVE_VALGRIND_VALGRIND_H _destructor_ static void cleanup_pools(void) { /* Be nice to valgrind */ - hashmap_trim_pools(); + if (RUNNING_ON_VALGRIND) + hashmap_trim_pools(); } #endif diff --git a/src/core/main.c b/src/core/main.c index 1af9b8b505..c9849d05c1 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -12,7 +12,7 @@ #include #endif #if HAVE_VALGRIND_VALGRIND_H -#include +# include #endif #include "sd-bus.h" @@ -1866,8 +1866,8 @@ static int do_reexecute( assert(i <= args_size); /* - * We want valgrind to print its memory usage summary before reexecution. Valgrind won't do - * this is on its own on exec(), but it will do it on exit(). Hence, to ensure we get a + * We want valgrind to print its memory usage summary before reexecution. Valgrind won't do + * this is on its own on exec(), but it will do it on exit(). Hence, to ensure we get a * summary here, fork() off a child, let it exit() cleanly, so that it prints the summary, * and wait() for it in the parent, before proceeding into the exec(). */ diff --git a/src/libsystemd-network/test-dhcp-client.c b/src/libsystemd-network/test-dhcp-client.c index 863649f6df..e4354199e1 100644 --- a/src/libsystemd-network/test-dhcp-client.c +++ b/src/libsystemd-network/test-dhcp-client.c @@ -9,6 +9,9 @@ #include #include #include +#if HAVE_VALGRIND_VALGRIND_H +# include +#endif #include "sd-dhcp-client.h" #include "sd-event.h" @@ -546,11 +549,12 @@ int main(int argc, char *argv[]) { test_discover_message(e); test_addr_acq(e); -#if VALGRIND +#if HAVE_VALGRIND_VALGRIND_H /* Make sure the async_close thread has finished. * valgrind would report some of the phread_* structures * as not cleaned up properly. */ - sleep(1); + if (RUNNING_ON_VALGRIND) + sleep(1); #endif return 0; diff --git a/src/libsystemd/sd-journal/journal-send.c b/src/libsystemd/sd-journal/journal-send.c index 3b74d2246e..d0d29818c2 100644 --- a/src/libsystemd/sd-journal/journal-send.c +++ b/src/libsystemd/sd-journal/journal-send.c @@ -7,7 +7,7 @@ #include #include #if HAVE_VALGRIND_VALGRIND_H -#include +# include #endif #define SD_JOURNAL_SUPPRESS_LOCATION @@ -77,9 +77,9 @@ int journal_fd_nonblock(bool nonblock) { return fd_nonblock(r, nonblock); } -#if VALGRIND void close_journal_fd(void) { - /* Be nice to valgrind. This is not atomic. This must be used only in tests. */ +#if HAVE_VALGRIND_VALGRIND_H + /* Be nice to valgrind. This is not atomic, so it is useful mainly for debugging. */ if (!RUNNING_ON_VALGRIND) return; @@ -92,8 +92,8 @@ void close_journal_fd(void) { safe_close(fd_plus_one - 1); fd_plus_one = 0; -} #endif +} _public_ int sd_journal_print(int priority, const char *format, ...) { int r; diff --git a/src/libsystemd/sd-journal/journal-send.h b/src/libsystemd/sd-journal/journal-send.h index 558d39a8c0..24315e249b 100644 --- a/src/libsystemd/sd-journal/journal-send.h +++ b/src/libsystemd/sd-journal/journal-send.h @@ -4,9 +4,4 @@ #include int journal_fd_nonblock(bool nonblock); - -#if VALGRIND void close_journal_fd(void); -#else -static inline void close_journal_fd(void) {} -#endif diff --git a/src/libsystemd/sd-journal/lookup3.c b/src/libsystemd/sd-journal/lookup3.c index 39967f21cd..a6a32e09c5 100644 --- a/src/libsystemd/sd-journal/lookup3.c +++ b/src/libsystemd/sd-journal/lookup3.c @@ -320,7 +320,9 @@ uint32_t jenkins_hashlittle( const void *key, size_t length, uint32_t initval) * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#if !VALGRIND && !HAS_FEATURE_ADDRESS_SANITIZER && !HAS_FEATURE_MEMORY_SANITIZER +#define VALGRIND_LIKE (HAVE_VALGRIND_VALGRIND_H || HAS_FEATURE_ADDRESS_SANITIZER || HAS_FEATURE_MEMORY_SANITIZER) + +#if !VALGRIND_LIKE switch(length) { @@ -505,7 +507,7 @@ void jenkins_hashlittle2( * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#if !VALGRIND && !HAS_FEATURE_ADDRESS_SANITIZER && !HAS_FEATURE_MEMORY_SANITIZER +#if !VALGRIND_LIKE switch(length) { @@ -681,7 +683,7 @@ uint32_t jenkins_hashbig( const void *key, size_t length, uint32_t initval) * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#if !VALGRIND && !HAS_FEATURE_ADDRESS_SANITIZER && !HAS_FEATURE_MEMORY_SANITIZER +#if !VALGRIND_LIKE switch(length) { From 3dc6b0fcb2ca7231cca754175f625274a1230f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 22 Feb 2023 11:35:25 +0100 Subject: [PATCH 4/5] sd-journal: use a dynamic check for valgrind I left this one as a separate commit because it is more involved. We want people to compile with valgrind support, but we don't want to use a slow hash function unless we're actually running under valgrind. So the compile-time check is changed to a runtime check. When compiled with optimization, the compiler should elide the checks on the constants, and only leave the check for RUNNING_ON_VALGRIND. It is wrapped with _unlikely_ so that the else branch is put in the hot path. --- src/libsystemd/sd-journal/lookup3.c | 136 +++++++++++++--------------- 1 file changed, 65 insertions(+), 71 deletions(-) diff --git a/src/libsystemd/sd-journal/lookup3.c b/src/libsystemd/sd-journal/lookup3.c index a6a32e09c5..c2a640687c 100644 --- a/src/libsystemd/sd-journal/lookup3.c +++ b/src/libsystemd/sd-journal/lookup3.c @@ -4,6 +4,12 @@ #include "lookup3.h" +#if HAVE_VALGRIND_VALGRIND_H +# include +#else +# define RUNNING_ON_VALGRIND 0 +#endif + /* ------------------------------------------------------------------------------- lookup3.c, by Bob Jenkins, May 2006, Public Domain. @@ -320,29 +326,28 @@ uint32_t jenkins_hashlittle( const void *key, size_t length, uint32_t initval) * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#define VALGRIND_LIKE (HAVE_VALGRIND_VALGRIND_H || HAS_FEATURE_ADDRESS_SANITIZER || HAS_FEATURE_MEMORY_SANITIZER) +#define VALGRIND_LIKE (_unlikely_(HAS_FEATURE_ADDRESS_SANITIZER || \ + HAS_FEATURE_MEMORY_SANITIZER || \ + RUNNING_ON_VALGRIND)) -#if !VALGRIND_LIKE - - switch(length) - { - case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; - case 11: c+=k[2]&0xffffff; b+=k[1]; a+=k[0]; break; - case 10: c+=k[2]&0xffff; b+=k[1]; a+=k[0]; break; - case 9 : c+=k[2]&0xff; b+=k[1]; a+=k[0]; break; - case 8 : b+=k[1]; a+=k[0]; break; - case 7 : b+=k[1]&0xffffff; a+=k[0]; break; - case 6 : b+=k[1]&0xffff; a+=k[0]; break; - case 5 : b+=k[1]&0xff; a+=k[0]; break; - case 4 : a+=k[0]; break; - case 3 : a+=k[0]&0xffffff; break; - case 2 : a+=k[0]&0xffff; break; - case 1 : a+=k[0]&0xff; break; - case 0 : return c; /* zero length strings require no mixing */ - } - -#else /* make valgrind happy */ - { + if (!VALGRIND_LIKE) { + switch(length) + { + case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; + case 11: c+=k[2]&0xffffff; b+=k[1]; a+=k[0]; break; + case 10: c+=k[2]&0xffff; b+=k[1]; a+=k[0]; break; + case 9 : c+=k[2]&0xff; b+=k[1]; a+=k[0]; break; + case 8 : b+=k[1]; a+=k[0]; break; + case 7 : b+=k[1]&0xffffff; a+=k[0]; break; + case 6 : b+=k[1]&0xffff; a+=k[0]; break; + case 5 : b+=k[1]&0xff; a+=k[0]; break; + case 4 : a+=k[0]; break; + case 3 : a+=k[0]&0xffffff; break; + case 2 : a+=k[0]&0xffff; break; + case 1 : a+=k[0]&0xff; break; + case 0 : return c; /* zero length strings require no mixing */ + } + } else { const uint8_t *k8 = (const uint8_t *) k; switch(length) @@ -363,8 +368,6 @@ uint32_t jenkins_hashlittle( const void *key, size_t length, uint32_t initval) } } -#endif /* !valgrind */ - } else if (HASH_LITTLE_ENDIAN && ((u.i & 0x1) == 0)) { const uint16_t *k = (const uint16_t *)key; /* read 16-bit chunks */ const uint8_t *k8; @@ -507,29 +510,26 @@ void jenkins_hashlittle2( * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#if !VALGRIND_LIKE - - switch(length) - { - case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; - case 11: c+=k[2]&0xffffff; b+=k[1]; a+=k[0]; break; - case 10: c+=k[2]&0xffff; b+=k[1]; a+=k[0]; break; - case 9 : c+=k[2]&0xff; b+=k[1]; a+=k[0]; break; - case 8 : b+=k[1]; a+=k[0]; break; - case 7 : b+=k[1]&0xffffff; a+=k[0]; break; - case 6 : b+=k[1]&0xffff; a+=k[0]; break; - case 5 : b+=k[1]&0xff; a+=k[0]; break; - case 4 : a+=k[0]; break; - case 3 : a+=k[0]&0xffffff; break; - case 2 : a+=k[0]&0xffff; break; - case 1 : a+=k[0]&0xff; break; - case 0 : *pc=c; *pb=b; return; /* zero length strings require no mixing */ - } - -#else /* make valgrind happy */ - - { + if (!VALGRIND_LIKE) { + switch(length) + { + case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; + case 11: c+=k[2]&0xffffff; b+=k[1]; a+=k[0]; break; + case 10: c+=k[2]&0xffff; b+=k[1]; a+=k[0]; break; + case 9 : c+=k[2]&0xff; b+=k[1]; a+=k[0]; break; + case 8 : b+=k[1]; a+=k[0]; break; + case 7 : b+=k[1]&0xffffff; a+=k[0]; break; + case 6 : b+=k[1]&0xffff; a+=k[0]; break; + case 5 : b+=k[1]&0xff; a+=k[0]; break; + case 4 : a+=k[0]; break; + case 3 : a+=k[0]&0xffffff; break; + case 2 : a+=k[0]&0xffff; break; + case 1 : a+=k[0]&0xff; break; + case 0 : *pc=c; *pb=b; return; /* zero length strings require no mixing */ + } + } else { const uint8_t *k8 = (const uint8_t *)k; + switch(length) { case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; @@ -548,8 +548,6 @@ void jenkins_hashlittle2( } } -#endif /* !valgrind */ - } else if (HASH_LITTLE_ENDIAN && ((u.i & 0x1) == 0)) { const uint16_t *k = (const uint16_t *)key; /* read 16-bit chunks */ const uint8_t *k8; @@ -683,29 +681,27 @@ uint32_t jenkins_hashbig( const void *key, size_t length, uint32_t initval) * still catch it and complain. The masking trick does make the hash * noticeably faster for short strings (like English words). */ -#if !VALGRIND_LIKE - switch(length) - { - case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; - case 11: c+=k[2]&0xffffff00; b+=k[1]; a+=k[0]; break; - case 10: c+=k[2]&0xffff0000; b+=k[1]; a+=k[0]; break; - case 9 : c+=k[2]&0xff000000; b+=k[1]; a+=k[0]; break; - case 8 : b+=k[1]; a+=k[0]; break; - case 7 : b+=k[1]&0xffffff00; a+=k[0]; break; - case 6 : b+=k[1]&0xffff0000; a+=k[0]; break; - case 5 : b+=k[1]&0xff000000; a+=k[0]; break; - case 4 : a+=k[0]; break; - case 3 : a+=k[0]&0xffffff00; break; - case 2 : a+=k[0]&0xffff0000; break; - case 1 : a+=k[0]&0xff000000; break; - case 0 : return c; /* zero length strings require no mixing */ - } - -#else /* make valgrind happy */ - - { + if (!VALGRIND_LIKE) { + switch(length) + { + case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; + case 11: c+=k[2]&0xffffff00; b+=k[1]; a+=k[0]; break; + case 10: c+=k[2]&0xffff0000; b+=k[1]; a+=k[0]; break; + case 9 : c+=k[2]&0xff000000; b+=k[1]; a+=k[0]; break; + case 8 : b+=k[1]; a+=k[0]; break; + case 7 : b+=k[1]&0xffffff00; a+=k[0]; break; + case 6 : b+=k[1]&0xffff0000; a+=k[0]; break; + case 5 : b+=k[1]&0xff000000; a+=k[0]; break; + case 4 : a+=k[0]; break; + case 3 : a+=k[0]&0xffffff00; break; + case 2 : a+=k[0]&0xffff0000; break; + case 1 : a+=k[0]&0xff000000; break; + case 0 : return c; /* zero length strings require no mixing */ + } + } else { const uint8_t *k8 = (const uint8_t *)k; + switch(length) /* all the case statements fall through */ { case 12: c+=k[2]; b+=k[1]; a+=k[0]; break; @@ -724,8 +720,6 @@ uint32_t jenkins_hashbig( const void *key, size_t length, uint32_t initval) } } -#endif /* !VALGRIND */ - } else { /* need to read the key one byte at a time */ const uint8_t *k = (const uint8_t *)key; From 18e100172c8b68fd118979dd535be80748112aad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 22 Feb 2023 11:15:22 +0100 Subject: [PATCH 5/5] test-set: inline two iterator declarations --- src/test/test-set.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/test-set.c b/src/test/test-set.c index cba07c207d..304e3db6c9 100644 --- a/src/test/test-set.c +++ b/src/test/test-set.c @@ -35,10 +35,9 @@ static void item_seen(Item *item) { TEST(set_free_with_destructor) { Set *m; struct Item items[4] = {}; - unsigned i; assert_se(m = set_new(NULL)); - for (i = 0; i < ELEMENTSOF(items) - 1; i++) + for (size_t i = 0; i < ELEMENTSOF(items) - 1; i++) assert_se(set_put(m, items + i) == 1); m = set_free_with_destructor(m, item_seen); @@ -53,10 +52,9 @@ DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(item_hash_ops, void, trivial_hash_ TEST(set_free_with_hash_ops) { Set *m; struct Item items[4] = {}; - unsigned i; assert_se(m = set_new(&item_hash_ops)); - for (i = 0; i < ELEMENTSOF(items) - 1; i++) + for (size_t i = 0; i < ELEMENTSOF(items) - 1; i++) assert_se(set_put(m, items + i) == 1); m = set_free(m);