[PATCH v3] sparse: treat function pointers as pointers to const data

[PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

This code snippet:

static void bar(void const *arg)
{
	int (*foo)(void) = arg;
}

produces the following warning:

test.c:4:28: warning: incorrect type in initializer (different modifiers)
test.c:4:28:    expected int ( *foo )( ... )
test.c:4:28:    got void const *arg

which is caused by the fact that the function pointer 'foo' is not annotated
as being a pointer to const data. However, dereferencing a function pointer
does not produce an lvalue, so a function pointer points to const data by
definition, and we should treat it accordingly.

To avoid producing a warning on the inverse case, i.e.,

static void bar(void)
{
	void *foo = bar;
}

we only address the case where the function pointer is the target of
an assignment.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
v3: add rationale for restriction to assignments to commit message
    add R-b

v2: only treat function pointers as pointers to const data when they are the
    target of an assignment

 evaluate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/evaluate.c b/evaluate.c
index 66556150ddac..a5a830978bda 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1359,6 +1359,15 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
 				typediff = "different address spaces";
 				goto Err;
 			}
+			/*
+			 * If this is a function pointer assignment, it is
+			 * actually fine to assign a pointer to const data to
+			 * it, as a function pointer points to const data
+			 * implicitly, i.e., dereferencing it does not produce
+			 * an lvalue.
+			 */
+			if (b1->type == SYM_FN)
+				mod1 |= MOD_CONST;
 			if (mod2 & ~mod1) {
 				typediff = "different modifiers";
 				goto Err;
-- 
1.8.3.2

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

On 8 September 2014 08:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This code snippet:
>
> static void bar(void const *arg)
> {
>         int (*foo)(void) = arg;
> }
>
> produces the following warning:
>
> test.c:4:28: warning: incorrect type in initializer (different modifiers)
> test.c:4:28:    expected int ( *foo )( ... )
> test.c:4:28:    got void const *arg
>
> which is caused by the fact that the function pointer 'foo' is not annotated
> as being a pointer to const data. However, dereferencing a function pointer
> does not produce an lvalue, so a function pointer points to const data by
> definition, and we should treat it accordingly.
>
> To avoid producing a warning on the inverse case, i.e.,
>
> static void bar(void)
> {
>         void *foo = bar;
> }
>
> we only address the case where the function pointer is the target of
> an assignment.
>
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v3: add rationale for restriction to assignments to commit message
>     add R-b
>
> v2: only treat function pointers as pointers to const data when they are the
>     target of an assignment
>
>  evaluate.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/evaluate.c b/evaluate.c
> index 66556150ddac..a5a830978bda 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -1359,6 +1359,15 @@ static int compatible_assignment_types(struct expression *expr, struct symbol *t
>                                 typediff = "different address spaces";
>                                 goto Err;
>                         }
> +                       /*
> +                        * If this is a function pointer assignment, it is
> +                        * actually fine to assign a pointer to const data to
> +                        * it, as a function pointer points to const data
> +                        * implicitly, i.e., dereferencing it does not produce
> +                        * an lvalue.
> +                        */
> +                       if (b1->type == SYM_FN)
> +                               mod1 |= MOD_CONST;
>                         if (mod2 & ~mod1) {
>                                 typediff = "different modifiers";
>                                 goto Err;
> --
> 1.8.3.2
>

Ping?

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Christopher Li]

On Mon, Oct 6, 2014 at 10:44 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>> we only address the case where the function pointer is the target of
>> an assignment.

I apply the patch in my private branch but haven't push out yet.

I just want to confirm, you didn't find this kind of assignment in the
kernel, right?

Chris

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

On 8 October 2014 20:00, Christopher Li <sparse@chrisli.org> wrote:
> On Mon, Oct 6, 2014 at 10:44 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>> we only address the case where the function pointer is the target of
>>> an assignment.
>
> I apply the patch in my private branch but haven't push out yet.
>
> I just want to confirm, you didn't find this kind of assignment in the
> kernel, right?
>

I noticed this patch

https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=de56fb1923ca11f428bf557870e0faa99f38762e

and saw that it was bogus: changing the return type of the pointed-to
function to 'const int' makes no sense at all, and yet it shuts up
sparse.

With my sparse patch, that patch could have been dropped (although I
think it was merged after all)

-- 
Ard.

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

On 8 October 2014 20:06, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 October 2014 20:00, Christopher Li <sparse@chrisli.org> wrote:
>> On Mon, Oct 6, 2014 at 10:44 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>>> we only address the case where the function pointer is the target of
>>>> an assignment.
>>
>> I apply the patch in my private branch but haven't push out yet.
>>
>> I just want to confirm, you didn't find this kind of assignment in the
>> kernel, right?
>>
>
> I noticed this patch
>
> https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=de56fb1923ca11f428bf557870e0faa99f38762e
>
> and saw that it was bogus: changing the return type of the pointed-to
> function to 'const int' makes no sense at all, and yet it shuts up
> sparse.
>
> With my sparse patch, that patch could have been dropped (although I
> think it was merged after all)
>

Ping?

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Christopher Li]

On Tue, Oct 14, 2014 at 8:55 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> Ping?

It is already applied and pushed as c1763a249aba0a40bd326c845c2a146132a02448

Chris

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

On 15 October 2014 03:34, Christopher Li <sparse@chrisli.org> wrote:
> On Tue, Oct 14, 2014 at 8:55 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> Ping?
>
> It is already applied and pushed as c1763a249aba0a40bd326c845c2a146132a02448
>

OK, thanks!

Which upstream repo is that? I couldn't find it on
git://git.kernel.org/pub/scm/devel/sparse/sparse.git

regards,
Ard.

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Christopher Li]

On Wed, Oct 15, 2014 at 1:16 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> Which upstream repo is that? I couldn't find it on
> git://git.kernel.org/pub/scm/devel/sparse/sparse.git

https://git.kernel.org/cgit/devel/sparse/chrisl/sparse.git/

Chris

Re: [PATCH v3] sparse: treat function pointers as pointers to const data

[Ard Biesheuvel]

On 15 October 2014 09:06, Christopher Li <sparse@chrisli.org> wrote:
> On Wed, Oct 15, 2014 at 1:16 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> Which upstream repo is that? I couldn't find it on
>> git://git.kernel.org/pub/scm/devel/sparse/sparse.git
>
> https://git.kernel.org/cgit/devel/sparse/chrisl/sparse.git/
>

OK, thanks!