From d9a460b2b6190096f2a546e1f9cdc89885b512ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Jul 2025 13:39:00 +0200 Subject: [PATCH 1/6] Adjust bitfields in struct Condition As is usually the case, the bitfields don't create the expected space savings, because the field that follows needs to be aligned. But we don't want to fully drop the bitfields here, because then ConditionType and ConditionResult are each 4 bytes, and the whole struct grows from 32 to 40 bytes (on amd64). We potentially have lots of little Conditions and that'd waste some memory. Make each of the four fields one byte. This still allows the compiler to generate simpler code without changing the struct size: E.g. in condition_test: c->result = CONDITION_ERROR; - 78fab: 48 8b 45 e8 mov -0x18(%rbp),%rax - 78faf: 0f b6 50 01 movzbl 0x1(%rax),%edx - 78fb3: 83 e2 03 and $0x3,%edx - 78fb6: 83 ca 0c or $0xc,%edx - 78fb9: 88 50 01 mov %dl,0x1(%rax) + 78f8b: 48 8b 45 e8 mov -0x18(%rbp),%rax + 78f8f: c6 40 03 03 movb $0x3,0x3(%rax) --- src/shared/condition.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/shared/condition.h b/src/shared/condition.h index c25b9643b9..90d896c85c 100644 --- a/src/shared/condition.h +++ b/src/shared/condition.h @@ -58,12 +58,13 @@ typedef enum ConditionResult { } ConditionResult; typedef struct Condition { + /* Use bitfields for ConditionType and ConditionResult to keep the whole struct in 32 bytes. */ ConditionType type:8; - bool trigger:1; - bool negate:1; + bool trigger; + bool negate; - ConditionResult result:6; + ConditionResult result:8; char *parameter; From f283459b9f44538a046bfa0cfd43ef6c1a1546aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 7 Jul 2025 11:13:26 +0200 Subject: [PATCH 2/6] shared/open-file: add line break We don't generally parenthesize additions, so drop that too. --- src/shared/open-file.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/shared/open-file.c b/src/shared/open-file.c index 7dfce2d977..16435490f1 100644 --- a/src/shared/open-file.c +++ b/src/shared/open-file.c @@ -76,8 +76,9 @@ int open_file_validate(const OpenFile *of) { if (!fdname_is_valid(of->fdname)) return -EINVAL; - if ((FLAGS_SET(of->flags, OPENFILE_READ_ONLY) + FLAGS_SET(of->flags, OPENFILE_APPEND) + - FLAGS_SET(of->flags, OPENFILE_TRUNCATE)) > 1) + if (FLAGS_SET(of->flags, OPENFILE_READ_ONLY) + + FLAGS_SET(of->flags, OPENFILE_APPEND) + + FLAGS_SET(of->flags, OPENFILE_TRUNCATE) > 1) return -EINVAL; if ((of->flags & ~_OPENFILE_MASK_PUBLIC) != 0) From c1794666162e653e010acbb0963191de1d293608 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Jul 2025 12:09:31 +0200 Subject: [PATCH 3/6] Merge shared/exec-directory-util.? into basic/unit-def.? Suggested in https://github.com/systemd/systemd/pull/35892#discussion_r2180322856. This is a tiny amount of code and does not warrant having a separate file and spawning a separate instance of the compiler during the build. Note: it took me a while to confirm that the contents of that table and function don't end up in libsystemd.so. The issue is that they _are_ present in it, unless LTO is used. We actually use link_whole[libbasic_static] for libsystemd, so we end up with all that code there. LTO is needed to clean that up. --- src/basic/unit-def.c | 11 +++++++++++ src/basic/unit-def.h | 13 +++++++++++++ src/core/execute.h | 1 - src/shared/exec-directory-util.c | 15 --------------- src/shared/exec-directory-util.h | 19 ------------------- src/shared/meson.build | 1 - src/systemctl/systemctl-show.c | 1 - 7 files changed, 24 insertions(+), 37 deletions(-) delete mode 100644 src/shared/exec-directory-util.c delete mode 100644 src/shared/exec-directory-util.h diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index f98cfd4ae1..04b096cd19 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -361,6 +361,17 @@ static const char* const job_mode_table[_JOB_MODE_MAX] = { DEFINE_STRING_TABLE_LOOKUP(job_mode, JobMode); +/* This table maps ExecDirectoryType to the setting it is configured with in the unit */ +static const char* const exec_directory_type_table[_EXEC_DIRECTORY_TYPE_MAX] = { + [EXEC_DIRECTORY_RUNTIME] = "RuntimeDirectory", + [EXEC_DIRECTORY_STATE] = "StateDirectory", + [EXEC_DIRECTORY_CACHE] = "CacheDirectory", + [EXEC_DIRECTORY_LOGS] = "LogsDirectory", + [EXEC_DIRECTORY_CONFIGURATION] = "ConfigurationDirectory", +}; + +DEFINE_STRING_TABLE_LOOKUP(exec_directory_type, ExecDirectoryType); + Glyph unit_active_state_to_glyph(UnitActiveState state) { static const Glyph map[_UNIT_ACTIVE_STATE_MAX] = { [UNIT_ACTIVE] = GLYPH_BLACK_CIRCLE, diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 7664f2881a..e0b1c6730d 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -296,6 +296,16 @@ typedef enum JobMode { _JOB_MODE_INVALID = -EINVAL, } JobMode; +typedef enum ExecDirectoryType { + EXEC_DIRECTORY_RUNTIME, + EXEC_DIRECTORY_STATE, + EXEC_DIRECTORY_CACHE, + EXEC_DIRECTORY_LOGS, + EXEC_DIRECTORY_CONFIGURATION, + _EXEC_DIRECTORY_TYPE_MAX, + _EXEC_DIRECTORY_TYPE_INVALID = -EINVAL, +} ExecDirectoryType; + char* unit_dbus_path_from_name(const char *name); int unit_name_from_dbus_path(const char *path, char **name); @@ -361,4 +371,7 @@ NotifyAccess notify_access_from_string(const char *s) _pure_; const char* job_mode_to_string(JobMode t) _const_; JobMode job_mode_from_string(const char *s) _pure_; +const char* exec_directory_type_to_string(ExecDirectoryType i) _const_; +ExecDirectoryType exec_directory_type_from_string(const char *s) _pure_; + Glyph unit_active_state_to_glyph(UnitActiveState state); diff --git a/src/core/execute.h b/src/core/execute.h index e0f3ba51e0..da1600a044 100644 --- a/src/core/execute.h +++ b/src/core/execute.h @@ -7,7 +7,6 @@ #include "cgroup-util.h" #include "core-forward.h" #include "cpu-set-util.h" -#include "exec-directory-util.h" #include "exec-util.h" #include "list.h" #include "log-context.h" diff --git a/src/shared/exec-directory-util.c b/src/shared/exec-directory-util.c deleted file mode 100644 index d9effa2502..0000000000 --- a/src/shared/exec-directory-util.c +++ /dev/null @@ -1,15 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ - -#include "exec-directory-util.h" -#include "string-table.h" - -/* This table maps ExecDirectoryType to the setting it is configured with in the unit */ -static const char* const exec_directory_type_table[_EXEC_DIRECTORY_TYPE_MAX] = { - [EXEC_DIRECTORY_RUNTIME] = "RuntimeDirectory", - [EXEC_DIRECTORY_STATE] = "StateDirectory", - [EXEC_DIRECTORY_CACHE] = "CacheDirectory", - [EXEC_DIRECTORY_LOGS] = "LogsDirectory", - [EXEC_DIRECTORY_CONFIGURATION] = "ConfigurationDirectory", -}; - -DEFINE_STRING_TABLE_LOOKUP(exec_directory_type, ExecDirectoryType); diff --git a/src/shared/exec-directory-util.h b/src/shared/exec-directory-util.h deleted file mode 100644 index 0006c73abf..0000000000 --- a/src/shared/exec-directory-util.h +++ /dev/null @@ -1,19 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -#pragma once - -#include - -#include "macro-fundamental.h" - -typedef enum ExecDirectoryType { - EXEC_DIRECTORY_RUNTIME, - EXEC_DIRECTORY_STATE, - EXEC_DIRECTORY_CACHE, - EXEC_DIRECTORY_LOGS, - EXEC_DIRECTORY_CONFIGURATION, - _EXEC_DIRECTORY_TYPE_MAX, - _EXEC_DIRECTORY_TYPE_INVALID = -EINVAL, -} ExecDirectoryType; - -const char* exec_directory_type_to_string(ExecDirectoryType i) _const_; -ExecDirectoryType exec_directory_type_from_string(const char *s) _pure_; diff --git a/src/shared/meson.build b/src/shared/meson.build index afbf7f8033..78e435de35 100644 --- a/src/shared/meson.build +++ b/src/shared/meson.build @@ -69,7 +69,6 @@ shared_sources = files( 'elf-util.c', 'enable-mempool.c', 'ethtool-util.c', - 'exec-directory-util.c', 'exec-util.c', 'exit-status.c', 'extension-util.c', diff --git a/src/systemctl/systemctl-show.c b/src/systemctl/systemctl-show.c index e75e7ac1c1..1db9bf2bea 100644 --- a/src/systemctl/systemctl-show.c +++ b/src/systemctl/systemctl-show.c @@ -15,7 +15,6 @@ #include "cgroup-show.h" #include "cpu-set-util.h" #include "errno-util.h" -#include "exec-directory-util.h" #include "exec-util.h" #include "exit-status.h" #include "extract-word.h" From 1e99c4e2be7bf27ca2bc14b528232aef0b1715aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Jul 2025 12:55:17 +0200 Subject: [PATCH 4/6] test-string-util: add a small test for xsprintf --- src/test/test-string-util.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/test-string-util.c b/src/test/test-string-util.c index 90ac579707..4f12ec710c 100644 --- a/src/test/test-string-util.c +++ b/src/test/test-string-util.c @@ -9,6 +9,19 @@ #include "strv.h" #include "tests.h" +TEST(xsprintf) { + char buf[5]; + + xsprintf(buf, "asdf"); + xsprintf(buf, "%4s", "a"); + xsprintf(buf, "%-4s", "a"); + xsprintf(buf, "%04d", 1); + + ASSERT_SIGNAL(xsprintf(buf, "asdfe"), SIGABRT); + ASSERT_SIGNAL(xsprintf(buf, "asdfefghdhdhdhdhd"), SIGABRT); + ASSERT_SIGNAL(xsprintf(buf, "%5s", "a"), SIGABRT); +} + TEST(string_erase) { char *x; x = strdupa_safe(""); From 048a94c8f606706fbb42882f7b559cd982eb3914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Jul 2025 12:44:06 +0200 Subject: [PATCH 5/6] basic/stdio-util: use a fixed message in xsprintf We put the name of the variable in the message, but it is a local variable and the name does not have global meaning. We end up with pointless copies of the error string: $ strings build/libsystemd.so.0.40.0 | grep 'big enough' xsprintf: p[] must be big enough xsprintf: error[] must be big enough xsprintf: prefix[] must be big enough xsprintf: pty[] must be big enough xsprintf: mode[] must be big enough xsprintf: t[] must be big enough xsprintf: s[] must be big enough xsprintf: spid[] must be big enough xsprintf: header_priority[] must be big enough xsprintf: header_pid[] must be big enough xsprintf: path[] must be big enough xsprintf: buf[] must be big enough The error message already shows the file, line, and function name, which is enough to identify the problem: Assertion 'xsprintf: buffer too small' failed at src/test/test-string-util.c:20, function test_xsprintf(). Aborting. --- src/basic/stdio-util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/stdio-util.h b/src/basic/stdio-util.h index 5464031434..052087ce15 100644 --- a/src/basic/stdio-util.h +++ b/src/basic/stdio-util.h @@ -19,7 +19,7 @@ static inline char* snprintf_ok(char *buf, size_t len, const char *format, ...) } #define xsprintf(buf, fmt, ...) \ - assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, ##__VA_ARGS__), "xsprintf: " #buf "[] must be big enough") + assert_message_se(snprintf_ok(buf, ELEMENTSOF(buf), fmt, ##__VA_ARGS__), "xsprintf: buffer too small") #define VA_FORMAT_ADVANCE(format, ap) \ do { \ From 6eb805f42af331581e9b316b2dc7450e222853c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 8 Jul 2025 13:18:07 +0200 Subject: [PATCH 6/6] meson: drop -ffunction-sections -fdata-sections I added them in 41afb5eb7214727301132aedc381831fbfc78e37 without too much explanation. Most likely the idea was to get rid of unused code in libsystemd.so [1]. But now that I'm testing this, it doesn't seem to have an effect. LTO is needed to get rid of unused functions, and it's enough to have LTO without those options. Those options might have some downsides [2], so let's disable them since there are doubts and no particularly good reason to have them. But keep the -Wl,--gc-sections option. Without this, libsystemd.so grows a little: -rwxr-xr-x 1 zbyszek zbyszek 5532424 07-08 13:24 build/libsystemd.so.0.40.0-orig -rwxr-xr-x 1 zbyszek zbyszek 5614472 07-08 13:26 build/libsystemd.so.0.40.0-no-sections -rwxr-xr-x 1 zbyszek zbyszek 5532392 07-08 13:27 build/libsystemd.so.0.40.0 Let's apply the --gc-sections option always to make the debug and final builds more similar. We need to verify that distro packages don't unexpectedly grow after this. [1] https://unix.stackexchange.com/a/715901 [2] https://stackoverflow.com/a/36033811 --- meson.build | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/meson.build b/meson.build index 127daa419b..6f8397c966 100644 --- a/meson.build +++ b/meson.build @@ -484,6 +484,7 @@ possible_link_flags = [ '-Wl,--fatal-warnings', '-Wl,-z,now', '-Wl,-z,relro', + '-Wl,--gc-sections', ] if get_option('b_sanitize') == 'none' @@ -503,15 +504,6 @@ possible_cc_flags = [ '-fvisibility=hidden', ] -if get_option('buildtype') != 'debug' - possible_cc_flags += [ - '-ffunction-sections', - '-fdata-sections', - ] - - possible_link_flags += '-Wl,--gc-sections' -endif - if get_option('mode') == 'developer' possible_cc_flags += '-fno-omit-frame-pointer' endif