Page MenuHomeFreeBSD

Fix acpica macros that subtract null pointers
ClosedPublic

Authored by dim on Aug 29 2021, 11:16 AM.

Details

Summary

Clang 13.0.0 produces a new -Werror warning about the ACPI_TO_INTEGER(p)
and ACPI_OFFSET(d, f) macros in acpica's actypes.h:

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]
    ObjDesc->Region.Address = ACPI_PTR_TO_PHYSADDR (Table);
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/contrib/dev/acpica/include/actypes.h:664:41: note: expanded from macro 'ACPI_PTR_TO_PHYSADDR'
#define ACPI_PTR_TO_PHYSADDR(i)         ACPI_TO_INTEGER(i)
                                        ^~~~~~~~~~~~~~~~~~
sys/contrib/dev/acpica/include/actypes.h:661:41: note: expanded from macro 'ACPI_TO_INTEGER'
#define ACPI_TO_INTEGER(p)              ACPI_PTR_DIFF (p, (void *) 0)
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sys/contrib/dev/acpica/include/actypes.h:656:82: note: expanded from macro 'ACPI_PTR_DIFF'
#define ACPI_PTR_DIFF(a, b)             ((ACPI_SIZE) (ACPI_CAST_PTR (UINT8, (a)) - ACPI_CAST_PTR (UINT8, (b))))
                                                                                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

This problem of undefined behavior was also reported to acpica by @cem
in 2018: https://github.com/acpica/acpica/issues/407, but it seems there
was never any fix committed for it upstream.

Instead fix these locally, for ACPI_TO_INTEGER by simply casting the
incoming pointer to ACPI_SIZE (which corresponds roughly to uintptr_t
and size_t), and for ACPI_OFFSET by reusing our offsetof definition from
sys/cdefs.h.

I didn't want to use our offsetof() or builtin_offsetof(), as this
might not be acceptable for upstream. But if upstream is not going to
accept this patch anyway, we might as well just use or own macros.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dim requested review of this revision.Aug 29 2021, 11:16 AM

Until something is fixed upstream, it might be better to use #ifdef __FreeBSD block, and then using our sys/cdefs.h facilities for FreeBSD would be both safer and better explain the intent.

Just my PoV, the last call is the ACPI maintainer' word of course.

I'm good with this or kib's suggestion

This revision is now accepted and ready to land.Aug 29 2021, 5:07 PM

Use conditional FreeBSD block, and our own __offsetof().

This revision now requires review to proceed.Aug 29 2021, 5:50 PM
This revision is now accepted and ready to land.Aug 29 2021, 6:30 PM

LGTM, and it's not the end of the world for us to revert if upstream addresses it differently. But let's give @jkim a couple of days.

This revision was automatically updated to reflect the committed changes.