Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply the packed attribute to uint*_u types for Clang #1667

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

MaskRay
Copy link
Contributor

@MaskRay MaskRay commented Aug 25, 2023

... so that the following code

static inline void u32_to_le(uint32_t val, uint8_t *buf) {
    *((uint32_u *) buf) = val;
...

will not cause -fsanitize=alignment failures when building with Clang.

... so that the following code
```
static inline void u32_to_le(uint32_t val, uint8_t *buf) {
    *((uint32_u *) buf) = val;
...
```

will not cause -fsanitize=alignment failures when building with Clang.
@daviesrob
Copy link
Member

clang seems happy enough with this, so I guess it's OK.

This hack was actually put in to prevent certain versions of gcc from using SIMD instructions that expected aligned data. If you want to use sanitizers, I'd recommend building with -DHTS_ALLOW_UNALIGNED=0 in CPPFLAGS which enables the properly standards-conforming version of the code. This will keep -fsanitize=undefined happy.

One day the dodgy typecasts will be replaced by calls to memcpy(), which is the proper way to do unaligned loads. At least these days most compilers manage to replace this with the intended register load (given sufficient optimisation).

@daviesrob daviesrob merged commit 7e20d76 into samtools:develop Aug 25, 2023
@MaskRay MaskRay deleted the clang branch August 25, 2023 21:10
@MaskRay
Copy link
Contributor Author

MaskRay commented Aug 25, 2023

Thank you!

If you want to use sanitizers, I'd recommend building with -DHTS_ALLOW_UNALIGNED=0 in CPPFLAGS which enables the properly standards-conforming version of the code. This will keep -fsanitize=undefined happy.

I think it'd be nice if users don't additionally specify -DHTS_ALLOW_UNALIGNED=0.

For further cleanup, functions like the following can just be replaced with memcpy. See Blosc/c-blosc2#550 (comment) that ((p)[0] | (p)[1]<<8 | (p)[2]<<16 | (p)[3]<<24) may not be optimized well while memcpy is always good:

static inline uint16_t le_to_u16(const uint8_t *buf) {
#if defined(HTS_LITTLE_ENDIAN) && HTS_ALLOW_UNALIGNED != 0
    return *((uint16_u *) buf);
#else
    return (uint16_t) buf[0] | ((uint16_t) buf[1] << 8);
#endif
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants