On Mon, Sep 18, 2023 at 3:59 AM Jean Delvare <jdelv...@suse.de> wrote:
>
> 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
Hi Jean,
Sorry, the patch did not consider big-endian systems.
The attached patch has fixed it (__builtin_bswap{16,32}) and the `void
*` warnings (should use `const void *`).
--
宋方睿
From 8e9abf6d211fafcdfe164c79ec21de5497ba57fb Mon Sep 17 00:00:00 2001
From: Fangrui Song <mask...@google.com>
Date: Wed, 2 Aug 2023 16:22:56 +0000
Subject: [PATCH v3] Use unaligned memory accesses unconditionally
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 out memcpy. This change tickles compiler
optimizations and may make dmi_table_decode slightly larger.
Signed-off-by: Fangrui Song <mask...@google.com>
---
Makefile | 1 -
config.h | 5 -----
types.h | 36 ++++++++++++++++++++++--------------
3 files changed, 22 insertions(+), 20 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..ef67840 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,7 +28,6 @@ typedef struct {
} u64;
#endif
-#if defined(ALIGNMENT_WORKAROUND) || defined(BIGENDIAN)
static inline u64 U64(u32 low, u32 high)
{
u64 self;
@@ -40,20 +37,31 @@ static inline u64 U64(u32 low, u32 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))
+static inline u16 WORD(const void *x)
+{
+ u16 ret;
+ memcpy(&ret, x, sizeof(ret));
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+ ret = __builtin_bswap16(ret);
+#endif
+ return ret;
+}
+
+static inline u32 DWORD(const void *x)
+{
+ u32 ret;
+ memcpy(&ret, x, sizeof(ret));
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+ ret = __builtin_bswap32(ret);
+#endif
+ return ret;
+}
+
#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 */
#endif
--
2.42.0.515.g380fc7ccd1-goog
