Re: [PATCH v2] Use unaligned memory accesses unconditionally

[Fangrui Song]

On Mon, Sep 18, 2023 at 3:59 AM Jean Delvare <jdelvare@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 <maskray@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 *`).


-- 
宋方睿

v3-0001-Use-unaligned-memory-accesses-unconditionally.patch
Description: Text Data