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

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

[Ard Biesheuvel]

On 12 November 2014 00:48, kbuild test robot <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
> head:   8266e31ed0fedb7ee16ebc86e80468f7cc1bbb4e
> commit: 243b6754cd17112bbf0724ed3c13446b48cf6a28 [2/3] efi/x86: Move x86 back to libstub
> reproduce:
>   # apt-get install sparse
>   git checkout 243b6754cd17112bbf0724ed3c13446b48cf6a28
>   make ARCH=x86_64 allmodconfig
>   make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
>>> arch/x86/boot/compressed/eboot.c:26:16: sparse: incorrect type in return expression (different modifiers)
>    arch/x86/boot/compressed/eboot.c:26:16:    expected struct efi_config const [pure] *
>    arch/x86/boot/compressed/eboot.c:26:16:    got struct efi_config *static [toplevel] efi_early
>

This smells like a sparse bug: __pure applies to functions only, so
there is no way we could ever return something with the __pure
modifier attached.

-- 
Ard.


> vim +26 arch/x86/boot/compressed/eboot.c
>
>     10  #include <linux/efi.h>
>     11  #include <linux/pci.h>
>     12  #include <asm/efi.h>
>     13  #include <asm/setup.h>
>     14  #include <asm/desc.h>
>     15
>     16  #undef memcpy                   /* Use memcpy from misc.c */
>     17
>     18  #include "eboot.h"
>     19
>     20  static efi_system_table_t *sys_table;
>     21
>     22  static struct efi_config *efi_early;
>     23
>     24  __pure const struct efi_config *__efi_early(void)
>     25  {
>   > 26          return efi_early;
>     27  }
>     28
>     29  #define BOOT_SERVICES(bits)                                             \
>     30  static void setup_boot_services##bits(struct efi_config *c)             \
>     31  {                                                                       \
>     32          efi_system_table_##bits##_t *table;                             \
>     33          efi_boot_services_##bits##_t *bt;                               \
>     34                                                                          \
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> http://lists.01.org/mailman/listinfo/kbuild                 Intel Corporation

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

[Christopher Li]

On Wed, Nov 12, 2014 at 9:35 PM, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> (Pulling sparse list in)
>
> On Wed, 2014-11-12 at 11:31 +0100, Ard Biesheuvel wrote:
>> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
>> >>> arch/x86/boot/compressed/eboot.c:26:16: sparse: incorrect type in return expression (different modifiers)
>> >    arch/x86/boot/compressed/eboot.c:26:16:    expected struct efi_config const [pure] *
>> >    arch/x86/boot/compressed/eboot.c:26:16:    got struct efi_config *static [toplevel] efi_early
>> >
>>
>> This smells like a sparse bug: __pure applies to functions only, so
>> there is no way we could ever return something with the __pure
>> modifier attached.

That make sense.

>
> Yes, that warning does look a little strange.
>
> Christopher, Fengguang, what do you guys think?
>

Do you get me a small test function that reproduce the error with sparse?

We can even add a test case for it.

Thanks

Chris

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

[Ard Biesheuvel]

On 12 November 2014 16:22, Christopher Li <sparse-55XgFHCVCFZAfugRpC6u6w@public.gmane.org> wrote:
> On Wed, Nov 12, 2014 at 9:35 PM, Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> (Pulling sparse list in)
>>
>> On Wed, 2014-11-12 at 11:31 +0100, Ard Biesheuvel wrote:
>>> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
>>> >>> arch/x86/boot/compressed/eboot.c:26:16: sparse: incorrect type in return expression (different modifiers)
>>> >    arch/x86/boot/compressed/eboot.c:26:16:    expected struct efi_config const [pure] *
>>> >    arch/x86/boot/compressed/eboot.c:26:16:    got struct efi_config *static [toplevel] efi_early
>>> >
>>>
>>> This smells like a sparse bug: __pure applies to functions only, so
>>> there is no way we could ever return something with the __pure
>>> modifier attached.
>
> That make sense.
>
>>
>> Yes, that warning does look a little strange.
>>
>> Christopher, Fengguang, what do you guys think?
>>
>
> Do you get me a small test function that reproduce the error with sparse?
>
> We can even add a test case for it.
>

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;
}

and produces

pure.c:11:16: warning: incorrect type in return expression (different modifiers)
pure.c:11:16:    expected void [pure] *
pure.c:11:16:    got void *i

I also noticed that the following triggers a warning as well:

static void*(*f)(void) = pure2;

(using the definition of pure2 as above)

pure.c:14:26: warning: incorrect type in initializer (different modifiers)
pure.c:14:26:    expected void *( *static [toplevel] f )( ... )
pure.c:14:26:    got void [pure] *( static [toplevel] *<noident> )( ... )

i.e., you are not allowed to assign a pointer to a pure function to a
non-pure function pointer, which is quite ok. The opposite should be
an error, though.

This patch

diff --git a/evaluate.c b/evaluate.c
index a5a830978bda..2e9ad8b7b31d 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1375,7 +1375,7 @@ static int compatible_assignment_types(struct
expression *expr, struct symbol *t
                        goto Cast;
                }
                /* It's OK if the target is more volatile or const
than the source */
-               typediff = type_difference(&t->ctype, &s->ctype, 0, mod1);
+               typediff = type_difference(&t->ctype, &s->ctype, 0,
mod1 | MOD_PURE);
                if (typediff)
                        goto Err;
                return 1;

fixes the first issue, but I am not sure if it is an appropriate fix or not.

-- 
Ard.

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

[Christopher Li]

On Wed, Nov 12, 2014 at 11:34 PM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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?

Chris


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:

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

[Ard Biesheuvel]

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:

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

[Christopher Li]

On Mon, Nov 17, 2014 at 1:58 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 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;

I see. Thanks for the review. So the above case should not raise
error. However, the following one should:

static __pure void *(*f3_err) = non_pure_func;

I got it the other way around. I will add that to the test case
and send out the second round review.

> 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.

That is good to know.

> 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.

I am convinced :-)

Chris

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

[Christopher Li]

On Mon, Nov 17, 2014 at 8:38 AM, Christopher Li <sparse@chrisli.org> wrote:
> I see. Thanks for the review. So the above case should not raise
> error. However, the following one should:
>
> static __pure void *(*f3_err) = non_pure_func;
>
> I got it the other way around. I will add that to the test case
> and send out the second round review.

I send push out the v2 version of the test case at
https://git.kernel.org/cgit/devel/sparse/chrisl/sparse.git/log/?h=review-pure-attr2

I haven't include the fix part yet. Do you see any thing wrong with the test
case?

Thanks

Chris

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

[Ard Biesheuvel]

On 17 November 2014 16:35, Christopher Li <sparse-55XgFHCVCFZAfugRpC6u6w@public.gmane.org> wrote:
> On Mon, Nov 17, 2014 at 8:38 AM, Christopher Li <sparse-55XgFHCVCFZAfugRpC6u6w@public.gmane.org> wrote:
>> I see. Thanks for the review. So the above case should not raise
>> error. However, the following one should:
>>
>> static __pure void *(*f3_err) = non_pure_func;
>>
>> I got it the other way around. I will add that to the test case
>> and send out the second round review.
>
> I send push out the v2 version of the test case at
> https://git.kernel.org/cgit/devel/sparse/chrisl/sparse.git/log/?h=review-pure-attr2
>
> I haven't include the fix part yet. Do you see any thing wrong with the test
> case?
>

No, the test case looks correct now. You may want to add a conflicting
definition, for completeness, as that is an important case for sparse
to verify.
So you could just add

static void *pure1(void); // conflicting declaration

at the bottom, and ensure it is flagged by sparse as an error after
you make your changes.

-- 
Ard.

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

[Christopher Li]

On Tue, Nov 18, 2014 at 12:24 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> No, the test case looks correct now. You may want to add a conflicting
> definition, for completeness, as that is an important case for sparse
> to verify.
> So you could just add
>
> static void *pure1(void); // conflicting declaration

Just to confirm. If function declare as non pure then second time
declare as pure, that  is a conflict error. That is very different from other
C attributes. C allow function declare attribute incrementally.

If what you said is true, then pure attribute is actually acting like a modifier
which don't fix not other non pure attribute.

> at the bottom, and ensure it is flagged by sparse as an error after
> you make your changes.

I am curios how does GCC behave on this incremental declare of pure
attribute. I just test it. GCC does not warn on the conflicting declare.

GCC seems warn on some thing shouldn't warn on the test case:

function "f2_ok" and "f5" shouldn't been warned. Strange.

validation/pure-function.c:14:1: warning: '__pure__' attribute ignored
[-Wattributes]
 static __attribute__((__pure__)) void*(*f2_ok)(void) = pure1;
 ^
validation/pure-function.c:15:1: warning: '__pure__' attribute ignored
[-Wattributes]
 static __attribute__((__pure__)) void*(*f3_error)(void) = non_pure1;
 ^
validation/pure-function.c:29:1: warning: '__pure__' attribute ignored
[-Wattributes]
 static __attribute__((__pure__)) int(*f5)(void) = pure2;
 ^
validation/pure-function.c:30:1: warning: '__pure__' attribute ignored
[-Wattributes]
 static __attribute__((__pure__)) int(*f6_error)(void) = non_pure2;


Chris

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

[Ard Biesheuvel]

On 18 November 2014 17:09, Christopher Li <sparse@chrisli.org> wrote:
> On Tue, Nov 18, 2014 at 12:24 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>>
>> No, the test case looks correct now. You may want to add a conflicting
>> definition, for completeness, as that is an important case for sparse
>> to verify.
>> So you could just add
>>
>> static void *pure1(void); // conflicting declaration
>
> Just to confirm. If function declare as non pure then second time
> declare as pure, that  is a conflict error. That is very different from other
> C attributes. C allow function declare attribute incrementally.
>
> If what you said is true, then pure attribute is actually acting like a modifier
> which don't fix not other non pure attribute.
>

Well, that is how sparse currently treats it, and I think that is correct.
If an extern declaration in a .h file specifies  __pure, then GCC may
legally emit fewer calls to that function than appear in the source.
So if the actual implementation in the .c file is not __pure, then you
have an error.

>> at the bottom, and ensure it is flagged by sparse as an error after
>> you make your changes.
>
> I am curios how does GCC behave on this incremental declare of pure
> attribute. I just test it. GCC does not warn on the conflicting declare.
>
> GCC seems warn on some thing shouldn't warn on the test case:
>
> function "f2_ok" and "f5" shouldn't been warned. Strange.
>
> validation/pure-function.c:14:1: warning: '__pure__' attribute ignored
> [-Wattributes]
>  static __attribute__((__pure__)) void*(*f2_ok)(void) = pure1;
>  ^
> validation/pure-function.c:15:1: warning: '__pure__' attribute ignored
> [-Wattributes]
>  static __attribute__((__pure__)) void*(*f3_error)(void) = non_pure1;
>  ^
> validation/pure-function.c:29:1: warning: '__pure__' attribute ignored
> [-Wattributes]
>  static __attribute__((__pure__)) int(*f5)(void) = pure2;
>  ^
> validation/pure-function.c:30:1: warning: '__pure__' attribute ignored
> [-Wattributes]
>  static __attribute__((__pure__)) int(*f6_error)(void) = non_pure2;
>

My apologies. It appears GCC ignores pure attributes on function
pointers, so the assignments are all legal. Sorry to have wasted your
time on this.

So what remains imo is

"""
static __attribute__((__pure__)) void *pure1(void)
{
void *i = (void *)0;
return i;
}

static __attribute__((__pure__)) int pure2(void)
{
int i = 0;
return i;
}
static int pure2(void); // error here
"""

and all the other stuff can be dropped.

Regards,
Ard.

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

[Christopher Li]

On Wed, Nov 19, 2014 at 12:38 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Well, that is how sparse currently treats it, and I think that is correct.
> If an extern declaration in a .h file specifies  __pure, then GCC may
> legally emit fewer calls to that function than appear in the source.
> So if the actual implementation in the .c file is not __pure, then you
> have an error.

OK. I just want to figure out what is the right thing to do here.
It is ambiguous when the second time function declare without pure attribute,
weather it should follow the incremental declare rule or conflicting error.

If follow the C incremental declare rule, there is no error there.
It just an incomplete declare, compiler will internally merge all the
declare in the same scope and apply that to the final function.

e.g.
static __attribute__((noreturn)) void deadloop(void);

static void deadloop(void)
{
}

gcc will treat the second deadloop() function has noreturn attribute,
even though it does not have one in the second time declare.
That is why Gcc warns the function does return:
 /tmp/noreturn.c: In function 'deadloop':
/tmp/noreturn.c:6:1: warning: 'noreturn' function does return [enabled
by default]
 }

I try it on gcc, gcc does not emit error for the pure attribute conflicting.
That seems indicate gcc follow the incremental declare rule rather than
conflicting error.

Sparse has the symbol->same_symbol chain for that reason as well.
It can be used for merging different declare of the same function.
Some declare provide more detail specification, some did not.

> """
> static __attribute__((__pure__)) int pure2(void)
> {
> int i = 0;
> return i;
> }
> static int pure2(void); // error here
> """

See above. I am not sure that is an error here if we follow
the incremental declare rule.

Chris

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

[Ard Biesheuvel]

On 19 November 2014 03:53, Christopher Li <sparse-55XgFHCVCFZAfugRpC6u6w@public.gmane.org> wrote:
> On Wed, Nov 19, 2014 at 12:38 AM, Ard Biesheuvel
> <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Well, that is how sparse currently treats it, and I think that is correct.
>> If an extern declaration in a .h file specifies  __pure, then GCC may
>> legally emit fewer calls to that function than appear in the source.
>> So if the actual implementation in the .c file is not __pure, then you
>> have an error.
>
> OK. I just want to figure out what is the right thing to do here.
> It is ambiguous when the second time function declare without pure attribute,
> weather it should follow the incremental declare rule or conflicting error.
>
> If follow the C incremental declare rule, there is no error there.
> It just an incomplete declare, compiler will internally merge all the
> declare in the same scope and apply that to the final function.
>
> e.g.
> static __attribute__((noreturn)) void deadloop(void);
>
> static void deadloop(void)
> {
> }
>
> gcc will treat the second deadloop() function has noreturn attribute,
> even though it does not have one in the second time declare.
> That is why Gcc warns the function does return:
>  /tmp/noreturn.c: In function 'deadloop':
> /tmp/noreturn.c:6:1: warning: 'noreturn' function does return [enabled
> by default]
>  }
>
> I try it on gcc, gcc does not emit error for the pure attribute conflicting.
> That seems indicate gcc follow the incremental declare rule rather than
> conflicting error.
>

That may be the case, but GCC also does not warn if you are violating
the pureness rules.
For example, this function

__attribute__((__pure__)) int foo(void)
{
  static int i = 0;
  return ++i;
}

is obviously not pure, but GCC does not warn about that. But yes, GCC
follows the incremental rule, so if you mark a function as 'pure' only
in the header file, but not in the definition, then it will still be
considered pure.

> Sparse has the symbol->same_symbol chain for that reason as well.
> It can be used for merging different declare of the same function.
> Some declare provide more detail specification, some did not.
>
>> """
>> static __attribute__((__pure__)) int pure2(void)
>> {
>> int i = 0;
>> return i;
>> }
>> static int pure2(void); // error here
>> """
>
> See above. I am not sure that is an error here if we follow
> the incremental declare rule.
>

I think you are right. It is fairly common practice to define
attributes for a function /definition/ by preceding it with a
definition. e.g.,

int foo(void) __attribute__((__pure__));
int foo(void)
{
  static int i = 0;
  return ++i;
}

so this is obviously not an error.

So bottom line is that GCC does not warn about violating pureness
rules, and using the incremental declare rule seems appropriate, so
perhaps sparse should ignore pure altogether?

-- 
Ard.

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

[Josh Triplett]

On Sun, Nov 16, 2014 at 06:13:28PM +0800, Christopher Li 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.

Wait, is the above being parsed as applying the pure attribute to the
return value rather than to the function?  Or is that not the issue
here?

- Josh Triplett

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

[Christopher Li]

On Mon, Nov 17, 2014 at 2:22 AM, Josh Triplett <josh@joshtriplett.org> wrote:
>
> Wait, is the above being parsed as applying the pure attribute to the
> return value rather than to the function?  Or is that not the issue
> here?

That seems to be the case here, e.g.  "__pure int pure1(long x);"
Currently sparse parse it as:
<node pure1>
    <function>
        <base type int>  [pure]

Ideally it should be:
<node pure1>
    <function> [pure]
        <base type int>

There is reason sparse does that. Sparse parse "__pure int" as a base
type to hold the __pure. The parse don't know if that is a function or
a function
pointer yet.

I haven't look at move the [pure] to function node. That is likely
a much bigger change.

Chris