OrgACRN
/
sof
mirror of https://github.com/thesofproject/sof.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Revert __builtin_assume_aligned(4) probe packet, replace with aligned(4) 

This reverts and replaces commit 2765b22049 ("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 <marc.herbert@intel.com>
This commit is contained in:
Marc Herbert 2020-03-27 19:51:39 -07:00 committed by Liam Girdwood
parent d65495e984
commit 0b80998bb1
2 changed files with 3 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/include/ipc/probe.h
View File

@ -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

6
tools/probes/probes_main.c
View File

@ -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: