[PATCH v5] linux-gen: random: make random data independent of destination buffer alignment #2107
Conversation
odpbuild
changed the title
JereLeppanen
force-pushed
the
dev/fix-random-data-align
branch
from
cc3bafc
to
a13a3ab
Compare
odpbuild
changed the title
JereLeppanen
force-pushed
the
dev/fix-random-data-align
branch
from
a13a3ab
to
9f052d1
Compare
odpbuild
changed the title
| @@ -40,38 +41,35 @@ static int32_t _random_data(uint8_t *buf, uint32_t len, uint64_t *seed) | |||
| { | |||
| const uint32_t ret = len; | |||
|
|
|||
| if (!_ODP_UNALIGNED) { | |||
| uint32_t r = xorshift64s32(seed); | |||
| if (!_ODP_UNALIGNED && (uintptr_t)buf & 3) { | |||
There was a problem hiding this comment.
I wonder if it is worth the effort and code complecity to keep optimizing the !_ODP_UNALIGNED case here. All relevant environments seem to have _ODP_UNALIGNED set 1. In my test systems I get _ODP_UNALIGNED set 0 only if compiling for 32-bit Arm with -mno-unaligned-access.
There was a problem hiding this comment.
Removed in v4.
| r >>= 8; | ||
| buf += 1; | ||
| len -= 1; | ||
| memcpy(buf, &r, 4); |
There was a problem hiding this comment.
This memcpy does not appear to get optimized away when compiling with GCC 12 for armv6+fp with -mno-unaligned-access. So if we do want to optimize for the _ODP_UNALIGNED == 0 case, the memcpy should perhaps be replaced by direct assignments to buf.
| } | ||
|
|
||
| for (uint32_t i = 0; i < len / 4; i++) { | ||
| *(odp_una_u32_t *)buf = xorshift64s32(seed); |
There was a problem hiding this comment.
This could be memcpy (at least if we do not care too much about _ODP_UNALIGNED 0) to avoid alignment and aliasing problems and the need to use weird type attributes. The memcpy would get optimized away when compiling for x86_64 or A64. And same comments for the other occurences of this sort of assignments.
There was a problem hiding this comment.
I'm not in favor of replacing every unaligned integer assignment with memcpy. But in any case that's outside the scope of this PR, so I left this as-is.
test/validation/api/random/random.c
Outdated
| @@ -149,6 +149,26 @@ static void random_test_align_and_overflow_true(void) | |||
| random_test_align_and_overflow(ODP_RANDOM_TRUE); | |||
| } | |||
|
|
|||
| static void random_test_align_and_len_test(void) | |||
There was a problem hiding this comment.
This looks good. I just wonder if it would be worthwhile to add checks that the function is not writing more than len bytes in the buffer.
There was a problem hiding this comment.
That and the destination buffer alignment are already tested in random_test_align_and_overflow. So if random test data is allowed to differ for different len values, even if seed is the same, then there's no need for this new test. Removed in v4.
JereLeppanen
force-pushed
the
dev/fix-random-data-align
branch
from
9f052d1
to
e2096f9
Compare
odpbuild
changed the title
|
v4:
|
… alignment Make random test data independent of destination buffer alignment when efficient unaligned access is not available. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com>
MatiasElo
force-pushed
the
dev/fix-random-data-align
branch
from
e2096f9
to
a8d3473
Compare
odpbuild
changed the title
No description provided.