From e2f3f0123c0eb0fdcb68f9c5b7850b068d36eccc Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Wed, 14 Sep 2022 23:19:06 +0000 Subject: [PATCH 1/8] clang-format: add spaces before parens Previously, clang-format was not adding a space after sizeof. --- .clang-format | 1 + 1 file changed, 1 insertion(+) diff --git a/.clang-format b/.clang-format index 6c1859a6..93b8bfd7 100644 --- a/.clang-format +++ b/.clang-format @@ -3,3 +3,4 @@ AlwaysBreakAfterDefinitionReturnType: All BreakBeforeBinaryOperators: None BinPackParameters: false SpaceAfterCStyleCast: true +SpaceBeforeParens: Always From 8c0d9709f3a4e80b0faccc0aee0aa24516902257 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Wed, 14 Sep 2022 23:55:31 +0000 Subject: [PATCH 2/8] clang-format: don't align escaped newlines in macro definitions --- .clang-format | 1 + 1 file changed, 1 insertion(+) diff --git a/.clang-format b/.clang-format index 93b8bfd7..62f39b58 100644 --- a/.clang-format +++ b/.clang-format @@ -4,3 +4,4 @@ BreakBeforeBinaryOperators: None BinPackParameters: false SpaceAfterCStyleCast: true SpaceBeforeParens: Always +AlignEscapedNewlines: DontAlign From 464b51acdeb46620114a54ebf74f5feb39ebb55c Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Fri, 12 Aug 2022 10:18:05 +0000 Subject: [PATCH 3/8] dbus-internals: use size_t in _DBUS_ALIGN_VALUE() When targeting CHERI-enabled architectures such as Arm Morello, performing a bitwise and with uintptr_t values can result in an ambiguous operation compiler warning. Fix this warning by telling compiler which operand is (potentially) a pointer and which one is an integer by changing the boundary type to size_t. This change has no functional effect on other architectures but is required to build with -Werror for Morello. Example warning message: ``` warning: binary expression on capability types 'unsigned __intcap' and 'unsigned __intcap'; it is not clear which should be used as the source of provenance; currently provenance is inherited from the left-hand side [-Wcheri-provenance] _dbus_assert (_DBUS_ALIGN_VALUE (insert_at, 8) == (unsigned) insert_at); ``` --- dbus/dbus-internals.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index c7967d24..deee366f 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -277,7 +277,8 @@ _dbus_assert_error_xor_bool (const DBusError *error, */ #define _DBUS_ALIGN_VALUE(this, boundary) \ - (( ((uintptr_t)(this)) + (((uintptr_t)(boundary)) -1)) & (~(((uintptr_t)(boundary))-1))) + ((((uintptr_t) (this)) + (((size_t) (boundary)) - 1)) & \ + (~(((size_t) (boundary)) - 1))) #define _DBUS_ALIGN_ADDRESS(this, boundary) \ ((void*)_DBUS_ALIGN_VALUE(this, boundary)) From c4a8c2d920002a961d13dab4247f91e9f2c18df6 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Wed, 14 Sep 2022 23:47:21 +0000 Subject: [PATCH 4/8] dbus-mempool.c: use size_t for variables holding object sizes --- dbus/dbus-mempool.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/dbus/dbus-mempool.c b/dbus/dbus-mempool.c index 38c019a7..c6565351 100644 --- a/dbus/dbus-mempool.c +++ b/dbus/dbus-mempool.c @@ -82,10 +82,10 @@ struct DBusMemBlock * when we free the mem pool. */ - /* this is a long so that "elements" is aligned */ - long used_so_far; /**< bytes of this block already allocated as elements. */ - - unsigned char elements[]; /**< the block data, actually allocated to required size */ + /* this is a size_t so that "elements" is aligned */ + size_t used_so_far; /**< bytes of this block already allocated as elements. */ + + unsigned char elements[]; /**< the block data, actually allocated to required size */ }; /** @@ -93,8 +93,8 @@ struct DBusMemBlock */ struct DBusMemPool { - int element_size; /**< size of a single object in the pool */ - int block_size; /**< size of most recently allocated block */ + size_t element_size; /**< size of a single object in the pool */ + size_t block_size; /**< size of most recently allocated block */ unsigned int zero_elements : 1; /**< whether to zero-init allocated elements */ DBusFreedElement *free_elements; /**< a free list of elements to recycle */ @@ -153,7 +153,7 @@ _dbus_mem_pool_new (int element_size, _dbus_assert (element_size >= (int) sizeof (DBusFreedElement)); /* align the element size to a pointer boundary so we won't get bus - * errors under other architectures. + * errors under other architectures. */ pool->element_size = _DBUS_ALIGN_VALUE (element_size, sizeof (void *)); @@ -215,7 +215,7 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) if (_dbus_disable_mem_pools ()) { DBusMemBlock *block; - int alloc_size; + size_t alloc_size; /* This is obviously really silly, but it's * debug-mode-only code that is compiled out @@ -223,9 +223,9 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) * is a constant expression FALSE so this block * should vanish) */ - + alloc_size = sizeof (DBusMemBlock) + pool->element_size; - + if (pool->zero_elements) block = dbus_malloc0 (alloc_size); else @@ -276,7 +276,7 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) { /* Need a new block */ DBusMemBlock *block; - int alloc_size; + size_t alloc_size; #ifdef DBUS_ENABLE_EMBEDDED_TESTS int saved_counter; #endif @@ -319,9 +319,9 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) block->next = pool->blocks; pool->blocks = block; } - + element = &pool->blocks->elements[pool->blocks->used_so_far]; - + pool->blocks->used_so_far += pool->element_size; pool->allocated_elements += 1; From 33dbeb5ebe28aadcde9f6f6bf22eb86fa7023dfa Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Thu, 15 Sep 2022 18:53:30 +0000 Subject: [PATCH 5/8] dbus-mempool.c: ensure that all alignments are aligned to max_align_t This is required e.g. for CHERI-enabled targets such as Arm Morello where aligning to sizeof(long) is not sufficient to load/store pointers (which need 16 byte alignment instead of 8 bytes). As we can't depend on C11 yet, this commit adds a max_align_t emulation to dbus-internals.h. --- dbus/dbus-internals.h | 15 +++++++++++++++ dbus/dbus-mempool.c | 32 ++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index deee366f..4643053b 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -283,6 +283,21 @@ _dbus_assert_error_xor_bool (const DBusError *error, #define _DBUS_ALIGN_ADDRESS(this, boundary) \ ((void*)_DBUS_ALIGN_VALUE(this, boundary)) +#define _DBUS_IS_ALIGNED(this, boundary) \ + ((((size_t) (uintptr_t) (this)) & ((size_t) (boundary) - 1)) == 0) + +/** + * Aligning a pointer to _DBUS_ALIGNOF(dbus_max_align_t) guarantees that all + * scalar types can be loaded/stored from/to such an address without incurring + * an alignment fault (or a slow misaligned access). + * This is based on C11 max_align_t, but falls back to DBusBasicValue for + * older C standards. + */ +#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) +typedef max_align_t dbus_max_align_t; +#else +typedef DBusBasicValue dbus_max_align_t; +#endif DBUS_PRIVATE_EXPORT char* _dbus_strdup (const char *str); diff --git a/dbus/dbus-mempool.c b/dbus/dbus-mempool.c index c6565351..dc0b4467 100644 --- a/dbus/dbus-mempool.c +++ b/dbus/dbus-mempool.c @@ -82,12 +82,24 @@ struct DBusMemBlock * when we free the mem pool. */ - /* this is a size_t so that "elements" is aligned */ size_t used_so_far; /**< bytes of this block already allocated as elements. */ - +#if defined(__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) + /* + * Ensure that elements is aligned correctly. For all supported pre-C11 + * targets, the size_t above should ensure that the elements array is + * sufficiently aligned (this is checked in the static assert below). + */ + _Alignas (dbus_max_align_t) +#endif unsigned char elements[]; /**< the block data, actually allocated to required size */ }; +_DBUS_STATIC_ASSERT (_DBUS_IS_ALIGNED (sizeof (struct DBusMemBlock), + _DBUS_ALIGNOF (dbus_max_align_t))); +_DBUS_STATIC_ASSERT (_DBUS_IS_ALIGNED (offsetof (struct DBusMemBlock, + elements), + _DBUS_ALIGNOF (dbus_max_align_t))); + /** * Internals fields of DBusMemPool */ @@ -152,10 +164,11 @@ _dbus_mem_pool_new (int element_size, _dbus_assert (element_size >= (int) sizeof (void*)); _dbus_assert (element_size >= (int) sizeof (DBusFreedElement)); - /* align the element size to a pointer boundary so we won't get bus - * errors under other architectures. + /* align the element size to be suitable for the most-aligned type + * that we care about (in practice usually a pointer). */ - pool->element_size = _DBUS_ALIGN_VALUE (element_size, sizeof (void *)); + pool->element_size = + _DBUS_ALIGN_VALUE (element_size, _DBUS_ALIGNOF (dbus_max_align_t)); pool->zero_elements = zero_elements != FALSE; @@ -239,6 +252,8 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) VALGRIND_MEMPOOL_ALLOC (pool, (void *) &block->elements[0], pool->element_size); + _dbus_assert (_DBUS_IS_ALIGNED (&block->elements[0], + _DBUS_ALIGNOF (dbus_max_align_t))); return (void*) &block->elements[0]; } else @@ -264,7 +279,8 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) memset (element, '\0', pool->element_size); pool->allocated_elements += 1; - + _dbus_assert ( + _DBUS_IS_ALIGNED (element, _DBUS_ALIGNOF (dbus_max_align_t))); return element; } else @@ -306,6 +322,8 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) block = dbus_malloc0 (alloc_size); else block = dbus_malloc (alloc_size); + _dbus_assert ( + _DBUS_IS_ALIGNED (block, _DBUS_ALIGNOF (dbus_max_align_t))); #ifdef DBUS_ENABLE_EMBEDDED_TESTS _dbus_set_fail_alloc_counter (saved_counter); @@ -327,6 +345,8 @@ _dbus_mem_pool_alloc (DBusMemPool *pool) pool->allocated_elements += 1; VALGRIND_MEMPOOL_ALLOC (pool, element, pool->element_size); + _dbus_assert ( + _DBUS_IS_ALIGNED (element, _DBUS_ALIGNOF (dbus_max_align_t))); return element; } } From 91f4ac9cf62f8bb99c28ddfa4f376e45fd1b2b94 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Fri, 12 Aug 2022 10:34:06 +0000 Subject: [PATCH 6/8] dbus-marshal-recursive.h: reduce padding in DBusType{Reader,Writer} When building for Arm Morello (where pointers are 16 bytes), I hit the static assertion that sizeof (DBusMessageRealIter) <= sizeof (DBusMessageIter) inside _dbus_message_iter_init_common() otherwise. This can be fixed by moving the pointers to the beginning of the struct to remove padding. --- dbus/dbus-marshal-recursive.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-marshal-recursive.h b/dbus/dbus-marshal-recursive.h index c3b96819..5822e058 100644 --- a/dbus/dbus-marshal-recursive.h +++ b/dbus/dbus-marshal-recursive.h @@ -38,18 +38,19 @@ typedef struct DBusArrayLenFixup DBusArrayLenFixup; */ struct DBusTypeReader { + const DBusTypeReaderClass *klass; /**< the vtable for the reader */ + const DBusString *type_str; /**< string containing signature of block */ + const DBusString *value_str; /**< string containing values of block */ + dbus_uint32_t byte_order : 8; /**< byte order of the block */ dbus_uint32_t finished : 1; /**< marks we're at end iterator for cases * where we don't have another way to tell */ dbus_uint32_t array_len_offset : 3; /**< bytes back from start_pos that len ends */ - const DBusString *type_str; /**< string containing signature of block */ int type_pos; /**< current position in signature */ - const DBusString *value_str; /**< string containing values of block */ int value_pos; /**< current position in values */ - const DBusTypeReaderClass *klass; /**< the vtable for the reader */ union { struct { @@ -63,6 +64,8 @@ struct DBusTypeReader */ struct DBusTypeWriter { + DBusString *type_str; /**< where to write typecodes (or read type expectations) */ + DBusString *value_str; /**< where to write values */ dbus_uint32_t byte_order : 8; /**< byte order to write values with */ dbus_uint32_t container_type : 8; /**< what are we inside? (e.g. struct, variant, array) */ @@ -71,9 +74,7 @@ struct DBusTypeWriter dbus_uint32_t enabled : 1; /**< whether to write values */ - DBusString *type_str; /**< where to write typecodes (or read type expectations) */ int type_pos; /**< current pos in type_str */ - DBusString *value_str; /**< where to write values */ int value_pos; /**< next position to write */ union From 6933a9263e30e2dbfb36a038660b9387ee632d32 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Sun, 14 Aug 2022 16:28:56 +0000 Subject: [PATCH 7/8] dbus-message.c: skip 1.10 layout compatibility test on new architectures These static assertions fail on CHERI-enabled architectures such as Arm Morello, where pointers are 128 bits. Architectures with 128-bit pointers were not supported in DBus 1.10, so we can skip the checks for DBus 1.10 structure layout compatibility for architectures with pointer size > 64 bit. --- CMakeLists.txt | 1 + cmake/ConfigureChecks.cmake | 2 ++ configure.ac | 7 ++++++- dbus/dbus-arch-deps.h.in | 2 ++ dbus/dbus-message.c | 12 ++++++++++++ meson.build | 3 +++ 6 files changed, 26 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 58ed3d89..28017a4e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -735,6 +735,7 @@ message(" cxxflags release: ${CMAKE_CXX_FLAGS_RELEASE} " message(" 64-bit int: ${DBUS_INT64_TYPE} ") message(" 32-bit int: ${DBUS_INT32_TYPE} ") message(" 16-bit int: ${DBUS_INT16_TYPE} ") +message(" pointer size: ${DBUS_SIZEOF_VOID_P} ") message(" Doxygen: ${DOXYGEN} ") message(" Docbook Generator: ${DOCBOOK_GENERATOR_NAME} ") diff --git a/cmake/ConfigureChecks.cmake b/cmake/ConfigureChecks.cmake index bb2f021e..172db213 100644 --- a/cmake/ConfigureChecks.cmake +++ b/cmake/ConfigureChecks.cmake @@ -84,6 +84,8 @@ int main() { } " DBUS_USE_SYNC) +set(DBUS_SIZEOF_VOID_P ${CMAKE_SIZEOF_VOID_P}) + check_type_size("short" SIZEOF_SHORT) check_type_size("int" SIZEOF_INT) check_type_size("long" SIZEOF_LONG) diff --git a/configure.ac b/configure.ac index 033a1f8e..aee08dc7 100644 --- a/configure.ac +++ b/configure.ac @@ -429,13 +429,17 @@ winsock2.h ws2tcpip.h ]) +#### Pointer size +AC_CHECK_SIZEOF(void *) +DBUS_SIZEOF_VOID_P=$ac_cv_sizeof_void_p +AC_SUBST(DBUS_SIZEOF_VOID_P) + #### Integer sizes AC_CHECK_SIZEOF(char) AC_CHECK_SIZEOF(short) AC_CHECK_SIZEOF(long) AC_CHECK_SIZEOF(int) -AC_CHECK_SIZEOF(void *) AC_CHECK_SIZEOF(long long) AC_CHECK_SIZEOF(__int64) @@ -1706,6 +1710,7 @@ echo " 64-bit int: ${DBUS_INT64_TYPE} 32-bit int: ${DBUS_INT32_TYPE} 16-bit int: ${DBUS_INT16_TYPE} + pointer size: ${DBUS_SIZEOF_VOID_P} Doxygen: ${DOXYGEN:-not found} xmlto: ${XMLTO:-not found} ducktype: ${DUCKTYPE:-not found} diff --git a/dbus/dbus-arch-deps.h.in b/dbus/dbus-arch-deps.h.in index 2dc58945..7b6328bc 100644 --- a/dbus/dbus-arch-deps.h.in +++ b/dbus/dbus-arch-deps.h.in @@ -46,6 +46,8 @@ typedef unsigned @DBUS_INT32_TYPE@ dbus_uint32_t; typedef @DBUS_INT16_TYPE@ dbus_int16_t; typedef unsigned @DBUS_INT16_TYPE@ dbus_uint16_t; +#define DBUS_SIZEOF_VOID_P @DBUS_SIZEOF_VOID_P@ + /* This is not really arch-dependent, but it's not worth * creating an additional generated header just for this */ diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index a87043e3..de355b07 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -136,6 +136,15 @@ struct DBusMessageRealIter } u; /**< the type writer or reader that does all the work */ }; +#if DBUS_SIZEOF_VOID_P > 8 +/* + * Architectures with 128-bit pointers were not supported in DBus 1.10, so we + * do no check for DBus 1.10 structure layout compatibility for such + * architectures (e.g. Arm Morello). + */ +#define CHECK_DBUS_1_10_BINARY_COMPATIBILITY 0 +#else +#define CHECK_DBUS_1_10_BINARY_COMPATIBILITY 1 /** * Layout of a DBusMessageIter on the stack in dbus 1.10.0. This is no * longer used, but for ABI compatibility we need to assert that the @@ -158,6 +167,7 @@ typedef struct int pad2; void *pad3; } DBusMessageIter_1_10_0; +#endif static void get_const_signature (DBusHeader *header, @@ -2069,12 +2079,14 @@ _dbus_message_iter_init_common (DBusMessage *message, _DBUS_STATIC_ASSERT (sizeof (DBusMessageRealIter) <= sizeof (DBusMessageIter)); _DBUS_STATIC_ASSERT (_DBUS_ALIGNOF (DBusMessageRealIter) <= _DBUS_ALIGNOF (DBusMessageIter)); +#if CHECK_DBUS_1_10_BINARY_COMPATIBILITY /* A failure of these two assertions would indicate that we've broken * ABI on this platform since 1.10.0. */ _DBUS_STATIC_ASSERT (sizeof (DBusMessageIter_1_10_0) == sizeof (DBusMessageIter)); _DBUS_STATIC_ASSERT (_DBUS_ALIGNOF (DBusMessageIter_1_10_0) == _DBUS_ALIGNOF (DBusMessageIter)); +#endif /* If this static assertion fails, it means the DBusMessageIter struct * is not "packed", which might result in "iter = other_iter" not copying * every byte. */ diff --git a/meson.build b/meson.build index 9103bb0e..31c6df51 100644 --- a/meson.build +++ b/meson.build @@ -276,6 +276,8 @@ foreach type : int_types endif endforeach +arch_config.set('DBUS_SIZEOF_VOID_P', cc.sizeof('void *')) + ############################################################################### # Dependencies @@ -1195,6 +1197,7 @@ summary_dict += { '64-bit int': arch_config.get('DBUS_INT64_TYPE'), '32-bit int': arch_config.get('DBUS_INT32_TYPE'), '16-bit int': arch_config.get('DBUS_INT16_TYPE'), + 'pointer size': arch_config.get('DBUS_SIZEOF_VOID_P'), 'xsltproc': xsltproc.found() ? xsltproc.full_path() : '', 'Doxygen': doxygen.found() ? doxygen.full_path() : '', 'ducktype': ducktype.found() ? ducktype.full_path() : '', From c5686f0c24886f7c05be6ce415053a39c9053080 Mon Sep 17 00:00:00 2001 From: Alex Richardson Date: Sun, 14 Aug 2022 16:31:10 +0000 Subject: [PATCH 8/8] DBusMessageIter: ensure contiguous layout with 128-bit pointers I am building DBus targeting the Arm Morello board and the "no padding" layout assertion fails here since pointers require 16-byte alignment, and therefore we have to add two additional ints to the DBusMessageIter struct. As this is a new architecture, where DBus previously failed to compiled we do not have any layout backwards compatibility requirements, so we can simplify the DBusMessageIter structure to allocate space for 16 pointers (which should give us a lot of space for any further changes). --- dbus/dbus-message.c | 4 ++++ dbus/dbus-message.h | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index de355b07..6f2a518d 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -2090,8 +2090,12 @@ _dbus_message_iter_init_common (DBusMessage *message, /* If this static assertion fails, it means the DBusMessageIter struct * is not "packed", which might result in "iter = other_iter" not copying * every byte. */ +#if DBUS_SIZEOF_VOID_P > 8 + _DBUS_STATIC_ASSERT (sizeof (DBusMessageIter) == 16 * sizeof (void *)); +#else _DBUS_STATIC_ASSERT (sizeof (DBusMessageIter) == 4 * sizeof (void *) + sizeof (dbus_uint32_t) + 9 * sizeof (int)); +#endif /* Since the iterator will read or write who-knows-what from the * message, we need to get in the right byte order diff --git a/dbus/dbus-message.h b/dbus/dbus-message.h index da2f2d9d..931917f5 100644 --- a/dbus/dbus-message.h +++ b/dbus/dbus-message.h @@ -59,7 +59,10 @@ typedef struct DBusMessageIter DBusMessageIter; * DBusMessageIter struct; contains no public fields. */ struct DBusMessageIter -{ +{ +#if DBUS_SIZEOF_VOID_P > 8 + void *dummy[16]; /**< Don't use this */ +#else void *dummy1; /**< Don't use this */ void *dummy2; /**< Don't use this */ dbus_uint32_t dummy3; /**< Don't use this */ @@ -74,12 +77,24 @@ struct DBusMessageIter int pad1; /**< Don't use this */ void *pad2; /**< Don't use this */ void *pad3; /**< Don't use this */ +#endif }; /** * A message iterator for which dbus_message_iter_abandon_container_if_open() * is the only valid operation. */ +#if DBUS_SIZEOF_VOID_P > 8 +#define DBUS_MESSAGE_ITER_INIT_CLOSED \ +{ \ + { \ + NULL, NULL, NULL, NULL, \ + NULL, NULL, NULL, NULL, \ + NULL, NULL, NULL, NULL, \ + NULL, NULL, NULL, NULL \ + } \ +} +#else #define DBUS_MESSAGE_ITER_INIT_CLOSED \ { \ NULL, /* dummy1 */ \ @@ -97,6 +112,7 @@ struct DBusMessageIter NULL, /* pad2 */ \ NULL /* pad3 */ \ } +#endif DBUS_EXPORT DBusMessage* dbus_message_new (int message_type);