Hi Fangrui,
On Tue, 8 Aug 2023 18:39:58 -0700, Fangrui Song via dmidecode-devel wrote:
> Currently ALIGNMENT_WORKAROUND is only defined for __ia64__ and __arm__.
> However, -fsanitize=alignment (part of UndefinedBehaviorSanitizer) will
> give errors for other architectures like x86, e.g.
>
> dmidecode.c:4941:7: runtime error: load of misaligned address 0x5629323b5f05
> for type 'const u32', which requires 4 byt
> e alignment
> 0x5629323b5f05: note: pointer points here
> 13 0f 01 03 00 00 40 00 ff ff 0f 20 00 02 01 00 00 14 13 00 40 00 00 00
> 00 ff ff 2f 00 00 70 00
> ^
>
> Let's just use memcpy to perform unaligned memory accesses. Modern
> compilers are able to optimize memcpy.
>
> For x86-64 builds using GCC or Clang, the old assembly seems to prefer
> 2 32-bit mov while the new assembly prefers 64-bit mov with shr with 32.
> For aarch64 builds using GCC or Clang, the output sizes do not change.
>
> Signed-off-by: Fangrui Song <mask...@google.com>
> ---
> Makefile | 1 -
> config.h | 5 -----
> types.h | 47 ++++++++++++++++++++++-------------------------
> 3 files changed, 22 insertions(+), 31 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 7aa729d..3e83805 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -25,7 +25,6 @@ CFLAGS += -W -Wall -Wshadow -Wstrict-prototypes
> -Wpointer-arith -Wcast-qual \
> CFLAGS += -D_FILE_OFFSET_BITS=64
>
> #CFLAGS += -DBIGENDIAN
> -#CFLAGS += -DALIGNMENT_WORKAROUND
>
> # Pass linker flags here (can be set from environment too)
> LDFLAGS ?=
> diff --git a/config.h b/config.h
> index 0a1af7d..416d4a6 100644
> --- a/config.h
> +++ b/config.h
> @@ -21,11 +21,6 @@
> #define USE_MMAP
> #endif
>
> -/* Use memory alignment workaround or not */
> -#if defined(__ia64__) || defined(__arm__)
> -#define ALIGNMENT_WORKAROUND
> -#endif
> -
> /* Avoid unaligned memcpy on /dev/mem */
> #ifdef __aarch64__
> #define USE_SLOW_MEMCPY
> diff --git a/types.h b/types.h
> index 51c32d7..80a3605 100644
> --- a/types.h
> +++ b/types.h
> @@ -1,6 +1,8 @@
> #ifndef TYPES_H
> #define TYPES_H
>
> +#include <string.h>
> +
> #include "config.h"
>
> typedef unsigned char u8;
> @@ -12,10 +14,6 @@ typedef unsigned int u32;
> * You may use the following defines to adjust the type definitions
> * depending on the architecture:
> * - Define BIGENDIAN on big-endian systems.
> - * - Define ALIGNMENT_WORKAROUND if your system doesn't support
> - * non-aligned memory access. In this case, we use a slower, but safer,
> - * memory access method. This should be done automatically in config.h
> - * for architectures which need it.
> */
>
> #ifdef BIGENDIAN
> @@ -30,30 +28,29 @@ typedef struct {
> } u64;
> #endif
>
> -#if defined(ALIGNMENT_WORKAROUND) || defined(BIGENDIAN)
> -static inline u64 U64(u32 low, u32 high)
> -{
> - u64 self;
> -
> - self.l = low;
> - self.h = high;
> -
> - return self;
> -}
> -#endif
> -
> /*
> * Per SMBIOS v2.8.0 and later, all structures assume a little-endian
> * ordering convention.
> */
> -#if defined(ALIGNMENT_WORKAROUND) || defined(BIGENDIAN)
> -#define WORD(x) (u16)((x)[0] + ((x)[1] << 8))
> -#define DWORD(x) (u32)((x)[0] + ((x)[1] << 8) + ((x)[2] << 16) + ((x)[3] <<
> 24))
> -#define QWORD(x) (U64(DWORD(x), DWORD(x + 4)))
> -#else /* ALIGNMENT_WORKAROUND || BIGENDIAN */
> -#define WORD(x) (u16)(*(const u16 *)(x))
> -#define DWORD(x) (u32)(*(const u32 *)(x))
> -#define QWORD(x) (*(const u64 *)(x))
> -#endif /* ALIGNMENT_WORKAROUND || BIGENDIAN */
> +static inline u16 WORD(void *x)
> +{
> + u16 ret;
> + memcpy(&ret, x, sizeof(ret));
> + return ret;
> +}
> +
> +static inline u32 DWORD(void *x)
> +{
> + u32 ret;
> + memcpy(&ret, x, sizeof(ret));
> + return ret;
> +}
> +
> +static inline u64 QWORD(void *x)
> +{
> + u64 ret;
> + memcpy(&ret, x, sizeof(ret));
> + return ret;
> +}
I can't see how this would work on big-endian systems.
>
> #endif
I just gave a try to your patch on my system (openSUSE Leap 15.4, gcc
7.5.0, x86-64) and it results in pages and pages of warnings such as:
dmidecode.c: In function ‘dmi_base_board_handles’:
dmidecode.c:592:31: warning: passing argument 1 of ‘WORD’ discards ‘const’
qualifier from pointer target type [-Wdiscarded-qualifiers]
pr_list_item("0x%04X", WORD(p + sizeof(u16) * i));
^
In file included from dmidecode.c:81:0:
types.h:35:19: note: expected ‘void *’ but argument is of type ‘const u8 * {aka
const unsigned char *}’
static inline u16 WORD(void *x)
^~~~
I can't possibly commit a change which generates build warnings, sorry.
--
Jean Delvare
SUSE L3 Support
