Re: [efi:next 2/3] arch/x86/boot/compressed/eboot.c:26:16: sparse: incorrect type in return expression (different modifiers)

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 16 November 2014 11:13, Christopher Li <sparse@chrisli.org> wrote:
> On Wed, Nov 12, 2014 at 11:34 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> Well, I spent some time playing around with this:
>>
>> This one is accepted:
>>
>> static __attribute__((__pure__)) int pure1(void)
>> {
>>     int i = 0;
>>     return i;
>> }
>>
>> This one is not accepted:
>>
>> static __attribute__((__pure__)) void *pure2(void)
>> {
>>     void *i = (void *)0;
>>     return i;
>> }
>>
>
> Thanks for the test case. I have commit your test case with a bit more
> test case regarding function pointer assign.
>
> The change is in chrisl repository reveiw-pure-attr branch:
> https://git.kernel.org/cgit/devel/sparse/chrisl/sparse.git/log/?h=review-pure-attr
>
> I purpose this fix for it. It can pass your test case.
> This patch limit the pure attribute to functions.
> The pure bit should not be a modifier in the first place.
> But that is a much bigger change.
>
> Can you please help me test and review the change?
>

Sure!

I can confirm that this patch removes the incorrect warning during the
kernel build that triggered all of this.
However, I think your testcase is not quite correct: in particular,
this assignment

static void*(*f1_err)(void) = pure1;

is perfectly ok: it is fine to call a pure function through a non-pure
function pointer, but not the other way around. For instance, looking
at the efi example

arch/x86/boot/compressed/eboot.h:

__pure const struct efi_config *__efi_early(void);

#define efi_call_early(f, ...)                                         \
       __efi_early()->call(__efi_early()->f, __VA_ARGS__);

Note the 2 calls to __efi_early(). The purpose of __pure here is to
instruct GCC to emit only a single call to __efi_early(), because it
will return the same value both times.

In other words, GCC is allowed emit fewer calls to a __pure function
than there are calls in the source, and the same applies to calls
through a pure function pointer. However, if the pure pointer points
to a function that is not pure, i.e., back-to-back invocations may
legally return different results, then calling it through a pure
pointer is a bug, and needs to be flagged.

Regards,
Ard.




> diff --git a/evaluate.c b/evaluate.c
> index 035e448..4c8b64a 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -632,6 +632,8 @@ const char *type_difference(struct ctype *c1,
> struct ctype *c2,
>         struct symbol *t1 = c1->base_type;
>         struct symbol *t2 = c2->base_type;
>         int move1 = 1, move2 = 1;
> +       unsigned long ignore = ~MOD_PURE;
> +
>         mod1 |= c1->modifiers;
>         mod2 |= c2->modifiers;
>         for (;;) {
> @@ -728,6 +730,7 @@ const char *type_difference(struct ctype *c1,
> struct ctype *c2,
>                         as1 = t1->ctype.as;
>                         mod2 = t2->ctype.modifiers;
>                         as2 = t2->ctype.as;
> +                       ignore = ~0;
>
>                         if (base1->variadic != base2->variadic)
>                                 return "incompatible variadic arguments";
> @@ -778,7 +781,7 @@ const char *type_difference(struct ctype *c1,
> struct ctype *c2,
>         }
>         if (as1 != as2)
>                 return "different address spaces";
> -       if ((mod1 ^ mod2) & ~MOD_IGNORE & ~MOD_SIGNEDNESS)
> +       if ((mod1 ^ mod2) & ~MOD_IGNORE & ~MOD_SIGNEDNESS & ignore)
>                 return "different modifiers";
>         return NULL;
>  }
> diff --git a/show-parse.c b/show-parse.c
> index fb54375..f274431 100644
> --- a/show-parse.c
> +++ b/show-parse.c
> @@ -326,6 +326,7 @@ deeper:
>                         was_ptr = 0;
>                 }
>                 append(name, "( ... )");
> +               mod = sym->ctype.modifiers;
>                 break;
>
>         case SYM_STRUCT: