Fix unaligned memory access in librt internal helper function _write_short #20474
Conversation
| #define _READ(result, data, type) \ | ||
| do { \ | ||
| *(result) = *(type *)(((ReadBufferObject *)data)->ptr); \ | ||
| memcpy((void *) result, ((ReadBufferObject *)data)->ptr, sizeof(type)); \ |
There was a problem hiding this comment.
The spacing between the cast and variable is inconsistent:
(void *) result vs (ReadBufferObject *)data. Since the previous code uses no spacing here, I suggest changing it to (void *)result.
| do { \ | ||
| *(type *)(((WriteBufferObject *)data)->ptr) = v; \ | ||
| type temp = v; \ | ||
| memcpy(((WriteBufferObject *)data)->ptr, (const void *) &temp, sizeof(type)); \ |
There was a problem hiding this comment.
Same spacing inconsistency issue here.
| #define _WRITE(data, type, v) \ | ||
| do { \ | ||
| *(type *)(((WriteBufferObject *)data)->ptr) = v; \ | ||
| type temp = v; \ |
There was a problem hiding this comment.
Looking at the usage of _WRITE(), I'm not 100% sure whether the temporary variable is needed, but it's probably safer when dealing with variables passed as a function parameter.
|
Besides two minor nitpicks with the formatting (spacing), the changes look good to me and I think it's the way to go to avoid unaligned access which can cause performance issues on other architectures besides the Thanks for fixing this! |
|
As I mentioned in original version of this PR, I think this is a too big change, there are only couple problematic uses of |
I don't think it's actually a big change. It just replaces a problematic assignment of type-cast memory with the safer |
I don't think this makes a lot of sense, because the two helper functions are internal and not used anywhere else besides this source file, and because the same alignment issues would also appear if they were used anywhere else. It's better to just fix all potential uses of these macros. As mentioned, there is no expected impact on platforms where unaligned memory access doesn't matter, but the memcpy will ensure that the correct instructions are generated where it does. |
|
Also, doing it selectively implies there aren't any aliasing issues here, and it looks like there might be (the PR as-is would fix that). |
|
@glaubitz Regarding your suggestions: I tried without the temporary variable first, but got an error, because some of the usages simply pass a constant value that can't be used as a reference. |
4 participants
Fixes: #20473
This PR replaces the static upcasting in the helper macros
_READand_WRITEwith amemcpycall.Some of the functions in
librt_internal.cuse these macros to write 16 or 32 bit integer values, while thedata->ptris byte-aligned and may point to a memory address that is not divisible by 2 or 4.This leads to a crash (with SIGBUS) on CPU architectures that have strict memory alignment, or possibly a performance penalty on others.
By replacing the direct assignment with
memcpy, the compiler will figure out the most optimal method to access the unaligned memory.I analyzed the resulting machine code generated by gcc 15 on amd64 and sparc64: The
memcpycall was in fact optimized away, with instructions that avoided the crash on sparc64. On amd64, it made no difference performance-wise (compared to the original code).