From 0b80998bb1fa6b9e95e4083f464a7769af180aba Mon Sep 17 00:00:00 2001 From: Marc Herbert Date: Fri, 27 Mar 2020 19:51:39 -0700 Subject: [PATCH] Revert __builtin_assume_aligned(4) probe packet, replace with aligned(4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts and replaces commit 2765b2204962 ("probe-app: assume probe packet aligned"). That commit applied a __builtin_assume_aligned(4) to every pointer assignment of the following type: uint32_t *ptr = struct probe_data_packet *packet; Instead of using __builtin_assume_aligned(4) every time a probe_data_packet is used like this, the probe_data_packet type definition is now 32 bits aligned in only one place thanks to (packed, aligned(4)). struct probe_data_packet is made entirely of uint32_t members. It is (packed) to avoid a 64 bits compiler hypothetically padding and aligning these 32 bits members to 64 bits. However, (packed) alone is roughly equivalent to aligned(1) (neither is part of the C standard) which causes some gcc versions to warn about this: sof/tools/probes/probes_main.c:228:5: error: converting a packed ‘struct probe_data_packet’ pointer (alignment 1) to a ‘uint32_t’ {aka ‘unsigned int’} pointer (alignment 4) may result in an unaligned pointer value [-Werror=address-of-packed-member] 228 | w_ptr = (uint32_t *)packet; | ^~~~~ Unlike "assuming" with __builtin_assume_aligned, (packed, aligned(4)) makes sure probe_data_packet are _actually_ 4 bytes aligned. As a demontration, it stops the following _Static_assert() from failing: modified src/probe/probe.c @ -64,9 +64,12 @ struct probe_pdata { struct probe_dma_ext ext_dma; struct probe_dma_ext inject_dma[CONFIG_PROBE_DMA_MAX]; struct probe_point probe_points[CONFIG_PROBE_POINTS_MAX]; + uint8_t dummy; struct probe_data_packet header; // aligned(4)? struct task dmap_work; }; +_Static_assert(offsetof(struct probe_pdata, header) % 4 == 0 , + "probe_data_packet is not 4 aligned"); In addition to gcc version 8 and 9, I tested and confirmed with _Static_assert and pahole that the same attributes behave the same with clang 9.0.1-2.fc31. Signed-off-by: Marc Herbert --- src/include/ipc/probe.h | 2 +- tools/probes/probes_main.c | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/include/ipc/probe.h b/src/include/ipc/probe.h index 8eff353f5..b70537e13 100644 --- a/src/include/ipc/probe.h +++ b/src/include/ipc/probe.h @@ -94,7 +94,7 @@ struct probe_data_packet { uint32_t checksum; /**< CRC32 of header and payload */ uint32_t data_size_bytes; /**< Size of following audio data */ uint32_t data[]; /**< Audio data extracted from buffer */ -} __attribute__((packed)); +} __attribute__((packed, aligned(4))); /** * Description of probe dma diff --git a/tools/probes/probes_main.c b/tools/probes/probes_main.c index 4ff31a76b..d79baf658 100644 --- a/tools/probes/probes_main.c +++ b/tools/probes/probes_main.c @@ -225,8 +225,7 @@ void parse_data(char *file_in) /* request to copy full data packet */ total_data_to_copy = sizeof(struct probe_data_packet) / sizeof(uint32_t); - /* probe_data_packet forced to align 4 */ - w_ptr = __builtin_assume_aligned((uint32_t *)packet, 4); + w_ptr = (uint32_t *)packet; state = SYNC; } /* data copying section */ @@ -258,8 +257,7 @@ void parse_data(char *file_in) packet = realloc(packet, sizeof(struct probe_data_packet) + packet->data_size_bytes); - /* probe_data_packet forced to align 4 */ - w_ptr = __builtin_assume_aligned((uint32_t *)&packet->data, 4); + w_ptr = (uint32_t *)&packet->data; state = CHECK; break; case CHECK: