UB in ACPI_TO_INTEGER , ACPI_OFFSET

[cemeyer]

It is bogus in C to subtract arbitrary pointers that do not point to the same object (or one beyond the end of the object), and the use of difference for these is not needed. (C11 §6.5.6, constraint 9, p.93 of the last draft.)

C defines offsetof() in stddef.h since C89, but even if we are concerned about running on environments that do not support offsetof, it would be more correct to just cast the corresponding pointers to uintptr_t and skip the subtraction. NULL == 0, regardless of bit representation.

I suggest (something along the lines of):

#define ACPI_TO_INTEGER(p) ((uintptr_t)(p))
#define ACPI_OFFSET(d, f) (offsetof(d, f))

[SchmErik]

uintptr_t isn't portable (we support C90)... Do you have any other alternatives to uintptr_t?

[cemeyer]

intptr is the portable definition. First choice would be losing the 30 year old C compiler support, or at least expecting anyone porting to such platforms to define the type.

Second choice is some crappy compiler-dependent hack like:

#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 199901L
# if defined(_LP64) || defined(__LP64__)
typedef uint64_t uintptr_t;
typedef int64_t intptr_t;
# elif defined(_ILP32) || defined(__ILP32__)
typedef uint32_t uintptr_t;
typedef int32_t intptr_t;
# endif
#endif

(Per https://sourceforge.net/p/predef/wiki/DataModels/ and https://sourceforge.net/p/predef/wiki/Standards/ .)

There is no guarantee that works everywhere, though. It works on most/all sane systems (but so does C99).

I think invoking undefined behavior with modern compilers is probably a worse idea than dropping support for compilers that only support a dialect of C that has been obsolete for 19 years. (To be clear, even though I cited C11 in my initial comment, it was also undefined behavior in C99 and C89.)

[SchmErik]

As it turns out, we already have the ACPI_UINTPTR_T macro that compiles to uintptr_t or void * depending on the compiler. Here's the change that we are thinking about.

-#define ACPI_TO_POINTER(i) ACPI_ADD_PTR (void, (void *) 0, (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_TO_POINTER(i) ACPI_ADD_PTR (void, (ACPI_UINTPTR_T) 0, (ACPI_SIZE) (i))
+#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (ACPI_UINTPTR_T) 0)
+#define ACPI_OFFSET(d, f) ACPI_PTR_DIFF (&(((d *) 0)->f), (ACPI_UINTPTR_T) 0)
#define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i)
#define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i)

[SchmErik]

@cemeyer is this still needed? if so, please review the above comment and I'll make the changes.

[cemeyer]

Yes, it's still relevant. I don't think the proposed change fixes the problem, though. The problematic part is the ACPI_ADD_PTR and ACPI_PTR_DIFF, not the type of the value passed as the second macro argument, which is immediately casted to UINT8* anyway.

[acpibob]

We do in fact need to support old compilers, mostly for our commercial customers who do not like upgrading to new compilers. I don't know all of the users, so we don't really know how far back we must go.

I don't see a problem with defining ACPI_OFFSET as:

#define ACPI_OFFSET(d, f) (ACPI_SIZE) &(((d *) 0)->f)

Which, BTW is the same implementation as visual studio:

#define offsetof(s,m) (size_t)&(((s *)0)->m)

[cemeyer]

@acpibob Hm, it's not clear to me whether that pointer construction is undefined behavior or not. You're generating a pointer to something which is outside the bounds of a valid object and also not a NULL pointer (&((d *)0)->f). See some discussion of the subject on wikipedia here (the conclusion there is that the zero construction is undefined).

I think it may be valid to instead define an ephemeral object with automatic storage duration and perform much the same computation; an optimizing compiler could probably remove the storage for that object entirely. E.g.,

#define ACPI_OFFSET(d,f) \
({ \
    d __tmp; \
    (size_t)((char *)(&__tmp->f) - (char *)&__tmp) \
})

(This construction uses the GCC extension ({}), which is probably dead-on-arrival considering the breadth of compilers the ACPI project intends to support.)

On the other hand, offsetof() has been part of the C standard since C89 — does ACPI really target pre-C89 compilers?

[DimitryAndric]

I ran into this issue again when building recent FreeBSD (which contains acpica) with clang 13.0.0:

sys/contrib/dev/acpica/components/dispatcher/dsopcode.c:708:31: error: performing pointer subtraction with a null pointer has undefined behavior [-Werror,-Wnull-pointer-subtraction]

This is indeed due to the line in sys/contrib/dev/acpica/components/dispatcher/dsopcode.c:

    ObjDesc->Region.Address = ACPI_PTR_TO_PHYSADDR (Table);

with ACPI_PTR_TO_PHYSADDR expanding to ACPI_TO_INTEGER in sys/contrib/dev/acpica/include/actypes.h:

/* Pointer/Integer type conversions */

#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)

Looking through actypes.h there's already a type that sort-of covers intptr_t, which is ACPI_NATIVE_INT, and both uintptr_t and size_t are covered by ACPI_SIZE. So wouldn't it be easier to define ACPI_TO_INTEGER as:

#define ACPI_TO_INTEGER(p)              ((ACPI_SIZE) (p))

This won't work for the ACPI_OFFSET macro though. The only portable way is to use offsetof, but it may be that this isn't supported by older Microsoft compilers. They were notoriously bad at supporting anything resembling modern C standards. (Though they've greatly cleaned up their act nowadays. :) )

[DimitryAndric]

Ok, in FreeBSD's own sys/cdefs.h we use __builtin_offsetof() if the compiler supports it, otherwise:

#define __offsetof(type, field) \
        ((__size_t)(__uintptr_t)((const volatile void *)&((type *)0)->field))

which in acpica could be represented as:

#define ACPI_OFFSET(d, f)               ((ACPI_SIZE)((const volatile void *)&((d *)0)->f))