Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190717033807.1207-1-cai@lca.pw>
In-Reply-To: <20190717033807.1207-1-cai@lca.pw>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Wed, 17 Jul 2019 15:01:18 -0700
Message-ID: <CAKwvOdmPX2DsUawcA0SzaFacjz==ACcfD8yDsbaS4eP4Es=Wzw@mail.gmail.com>
Subject: Re: [PATCH] acpica: fix -Wnull-pointer-arithmetic warnings
To:     Qian Cai <cai@lca.pw>
Cc:     rafael.j.wysocki@intel.com, robert.moore@intel.com,
        erik.schmauss@intel.com, jkim@freebsd.org,
        Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org,
        devel@acpica.org,
        clang-built-linux <clang-built-linux@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 16, 2019 at 8:38 PM Qian Cai <cai@lca.pw> wrote:
>
> Clang generate quite a few of those warnings.
>
> drivers/acpi/scan.c:759:28: warning: arithmetic on a null pointer
> treated as a cast from integer to pointer is a GNU extension
> [-Wnull-pointer-arithmetic]
>                 status = acpi_get_handle(ACPI_ROOT_OBJECT,
> obj->string.pointer,
>                                          ^~~~~~~~~~~~~~~~
> ./include/acpi/actypes.h:458:56: note: expanded from macro
> 'ACPI_ROOT_OBJECT'
>  #define ACPI_ROOT_OBJECT                ((acpi_handle) ACPI_TO_POINTER
> (ACPI_MAX_PTR))
>                                                         ^~~~~~~~~~~~~~~
> ./include/acpi/actypes.h:509:41: note: expanded from macro
> 'ACPI_TO_POINTER'
>  #define ACPI_TO_POINTER(i)              ACPI_ADD_PTR (void, (void *) 0,
> (acpi_size) (i))
>                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./include/acpi/actypes.h:503:84: note: expanded from macro
> 'ACPI_ADD_PTR'
>  #define ACPI_ADD_PTR(t, a, b)           ACPI_CAST_PTR (t,
> (ACPI_CAST_PTR (u8, (a)) + (acpi_size)(b)))
>                                          ^~~~~~~~~~~~~~~~~
> ./include/acpi/actypes.h:501:66: note: expanded from macro
> 'ACPI_CAST_PTR'
>  #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) (p))
>                                                                   ^
> This is because pointer arithmetic on a pointer not pointing to an array
> is an undefined behavior. Fix it by doing an integer arithmetic
> instead.

Hi Qian, thanks for the patch.  How do I reproduce this issue,
precisely?  I just tried:
$ make CC=clang -j71 drivers/acpi/scan.o
on linux-next today and don't observe the warning.  My clang is ToT
built sometime this week.  It looks like drivers/acpi/scan.o when
CONFIG_ACPI=y, which is set in the defconfig.  Is there another set of
configs to enable to observe the warning?

Also, the fix is curious.  Arithmetic on pointers to different
"objects" (with one element passed the end) may lead to provence
issues due to undefined behavior, but I would have expected some cases
to uintptr_t, then arithmetic on that type, as the solution (which is
what I suspect ACPI_CAST_PTR is doing).

Further, you seem to have modified ACPI_ADD_PTR but not ACPI_SUB_PTR;
I would have expected both to be afflicted together or not at all
based on their existing implementations.

>
> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  include/acpi/actypes.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index ad6892a24015..25b4a32da177 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -500,13 +500,13 @@ typedef u64 acpi_integer;
>
>  #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) (p))
>  #define ACPI_CAST_INDIRECT_PTR(t, p)    ((t **) (acpi_uintptr_t) (p))
> -#define ACPI_ADD_PTR(t, a, b)           ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) + (acpi_size)(b)))
> +#define ACPI_ADD_PTR(t, a, b)           ACPI_CAST_PTR (t, (a) + (acpi_size)(b))
>  #define ACPI_SUB_PTR(t, a, b)           ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) - (acpi_size)(b)))
>  #define ACPI_PTR_DIFF(a, b)             ((acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))))
>
>  /* Pointer/Integer type conversions */
>
> -#define ACPI_TO_POINTER(i)              ACPI_ADD_PTR (void, (void *) 0, (acpi_size) (i))
> +#define ACPI_TO_POINTER(i)              ACPI_ADD_PTR (void, 0, (acpi_size) (i))

IIUC, these are adding `i` to NULL (or (void*)0)? X + 0 == X ?
-- 
Thanks,
~Nick Desaulniers