| Message ID | CANeU7Q=6mB2yoSy9PpnNjeYcvvzD8DEUyyeXQhW2YE2aSGq_gg@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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: -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 17, 2014 at 1:58 AM, Ard Biesheuvel <ard.biesheuvel@linaro.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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17 November 2014 16:35, Christopher Li <sparse@chrisli.org> wrote: > 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? > 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.
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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 19, 2014 at 12:38 AM, Ard Biesheuvel <ard.biesheuvel@linaro.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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19 November 2014 03:53, Christopher Li <sparse@chrisli.org> wrote: > On Wed, Nov 19, 2014 at 12:38 AM, Ard Biesheuvel > <ard.biesheuvel@linaro.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?
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: