Messages in this thread | | | From | "Kaneda, Erik" <> | Subject | RE: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using __builtin_offsetof | Date | Tue, 16 Jun 2020 21:39:17 +0000 |
| |
> -----Original Message----- > From: Nick Desaulniers <ndesaulniers@google.com> > Sent: Thursday, June 11, 2020 10:06 AM > To: Kaneda, Erik <erik.kaneda@intel.com> > Cc: Jung-uk Kim <jkim@freebsd.org>; Wysocki, Rafael J > <rafael.j.wysocki@intel.com>; Ard Biesheuvel <ardb@kernel.org>; > dvyukov@google.com; glider@google.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > lorenzo.pieralisi@arm.com; mark.rutland@arm.com; pcc@google.com; > rjw@rjwysocki.net; will@kernel.org; stable@vger.kernel.org; linux- > acpi@vger.kernel.org; devel@acpica.org > Subject: Re: [Devel] Re: [PATCH] ACPICA: fix UBSAN warning using > __builtin_offsetof > > On Thu, Jun 11, 2020 at 9:45 AM Kaneda, Erik <erik.kaneda@intel.com> > wrote: > > > > > From: Jung-uk Kim <jkim@FreeBSD.org> > > > > > > Actually, I think we should let platform-specific acfoo.h decide what to > > > do here, i.e., > > > > That's a better solution. For Linux, it would look something like this: > > > > --- a/include/acpi/actypes.h > > +++ b/include/acpi/actypes.h > > @@ -508,10 +508,15 @@ typedef u64 acpi_integer; > > > > #define ACPI_TO_POINTER(i) ACPI_CAST_PTR (void, (acpi_size) (i)) > > #define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) 0) > > -#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void *) > 0) > > #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) > > #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i) > > > > +/* Platforms may want to define their own ACPI_OFFSET */ > > + > > +#ifndef ACPI_OFFSET > > +#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (void > *) 0) > > +#endif > > + > > /* Optimizations for 4-character (32-bit) acpi_name manipulation */ > > > > #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED > > diff --git a/include/acpi/platform/aclinux.h > b/include/acpi/platform/aclinux.h > > index 987e2af7c335..5d1ca6015fce 100644 > > --- a/include/acpi/platform/aclinux.h > > +++ b/include/acpi/platform/aclinux.h > > @@ -71,6 +71,11 @@ > > #undef ACPI_DEBUG_DEFAULT > > #define ACPI_DEBUG_DEFAULT (ACPI_LV_INFO | ACPI_LV_REPAIR) > > > > +/* Use gcc's builtin offset instead of the default */ > > + > > +#undef ACPI_OFFSET > > +#define ACPI_OFFSET(a,b) __builtin_offsetof(a,b) > > + > > #ifndef CONFIG_ACPI > > > Hi,
Sorry about the delayed response.
> Looks good at first glance. Wouldn't actypes.h need to include > platform/acenv.h first though? Otherwise you put some header > inclusion order dependency on folks who include actypes.h to first > include acenv.h otherwise we're not getting the definition in terms of > __builtin_offsetof.
Actypes.h is intended for ACPICA use. For files using ACPI_OFFSET, they include headers like acpi.h that ends up including aclinux.h before including actypes.h. For those using ACPI_OFFSET, precedence will be given to the definition in aclinux.h before falling back to the definition in actypes.
Erik
> > -- > Thanks, > ~Nick Desaulniers
| |