[PATCH v2] Silence misguided GCC warning about alignment issues

[Daniel Thompson]

On 07/08/17 21:58, Johannes Schindelin wrote:
> When compiling xz_dec_stream.c with GCC (at least with versions 5.4.0
> and 7.1.0), it complains thusly:
> 
> 	In function 'dec_stream_footer':
> 	error: dereferencing type-punned pointer will break
> 	      strict-aliasing rules [-Werror=strict-aliasing]
> 	   if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf))
> 	       ^~

A different title might be better.

This is *not* a warning about alignment issue; it is a warning about a 
potential aliasing issue: two pointers of different (non-char) types 
that could point to the same memory.


Daniel.


> 
> Despite the arrow pointing to the xz_crc32() call, the actual problem is
> the get_le32() macro expansion.
> 
> The thing is, the `buf` field was put just after two fields of type
> size_t for the express purpose of avoiding alignment issues, as per the
> comment above the `temp` struct >
> Meaning: GCC gets this all wrong and should not complain.
> 
> An earlier attempt to fix (76b65624b (unxz: get_le32
> macro is obviously wrong, 2017-08-07)) this by simply forcing
> get_unaligned_32() (which is unnecessarily slow because it rebuilds a
> 32-bit int from accessing already-aligned bytes one by one) was a bit
> heavy-handed.
> 
> But we can help GCC by turning the get_le32() macro into an inline
> function. So let's do that, it also helps compile time safety by making
> the code a little bit stricter.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin at gmx.de>
> ---
> Published-As: https://github.com/dscho/busybox-w32/releases/tag/busybox-type-punned-warning-v2
> Fetch-It-Via: git fetch https://github.com/dscho/busybox-w32 busybox-type-punned-warning-v2
> 
> Interdiff vs v1:
>   diff --git a/archival/libarchive/decompress_unxz.c b/archival/libarchive/decompress_unxz.c
>   index 0be85500c..e24cff98b 100644
>   --- a/archival/libarchive/decompress_unxz.c
>   +++ b/archival/libarchive/decompress_unxz.c
>   @@ -37,6 +37,11 @@ static uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)
>     || !defined(put_unaligned_be32)
>    # error get_unaligned_le32 accessors are not defined
>    #endif
>   +static ALWAYS_INLINE uint32_t get_le32_fast(const void *p)
>   +{
>   +  return *(uint32_t *)p;
>   +}
>   +#define get_le32 get_le32_fast
>    
>    #include "unxz/xz_dec_bcj.c"
>    #include "unxz/xz_dec_lzma2.c"
>   diff --git a/archival/libarchive/unxz/xz_dec_stream.c b/archival/libarchive/unxz/xz_dec_stream.c
>   index 8131ee30a..bf791055b 100644
>   --- a/archival/libarchive/unxz/xz_dec_stream.c
>   +++ b/archival/libarchive/unxz/xz_dec_stream.c
>   @@ -423,15 +423,6 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct xz_dec *s)
>    	if (!memeq(s->temp.buf + 10, FOOTER_MAGIC, FOOTER_MAGIC_SIZE))
>    		return XZ_DATA_ERROR;
>    
>   -#if defined(__GNUC__)
>   -	/*
>   -	 * The temp.buf field is put just after two fields of type size_t for
>   -	 * the express purpose of avoiding alignment issues. But GCC complains
>   -	 * about it nevertheless... so: shut GCC up for a few lines.
>   -	 */
>   -#pragma GCC diagnostic push
>   -#pragma GCC diagnostic ignored "-Wstrict-aliasing"
>   -#endif
>    	if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf))
>    		return XZ_DATA_ERROR;
>    
>   @@ -442,9 +433,6 @@ static enum xz_ret XZ_FUNC dec_stream_footer(struct xz_dec *s)
>    	 */
>    	if ((s->index.size >> 2) != get_le32(s->temp.buf + 4))
>    		return XZ_DATA_ERROR;
>   -#if defined(__GNUC__)
>   -#pragma GCC diagnostic pop
>   -#endif
>    
>    	if (s->temp.buf[8] != 0 || s->temp.buf[9] != s->check_type)
>    		return XZ_DATA_ERROR;
>   archival/libarchive/decompress_unxz.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/archival/libarchive/decompress_unxz.c b/archival/libarchive/decompress_unxz.c
> index 0be85500c..e24cff98b 100644
> --- a/archival/libarchive/decompress_unxz.c
> +++ b/archival/libarchive/decompress_unxz.c
> @@ -37,6 +37,11 @@ static uint32_t xz_crc32(const uint8_t *buf, size_t size, uint32_t crc)
>    || !defined(put_unaligned_be32)
>   # error get_unaligned_le32 accessors are not defined
>   #endif
> +static ALWAYS_INLINE uint32_t get_le32_fast(const void *p)
> +{
> +  return *(uint32_t *)p;
> +}
> +#define get_le32 get_le32_fast
>   
>   #include "unxz/xz_dec_bcj.c"
>   #include "unxz/xz_dec_lzma2.c"
> 
> base-commit: a907b828d6e9f1357fc2e1db09d3eb1d3fb9b826
>