| Message ID | 20170123213728.89900-3-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jan 24, 2017 at 5:37 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > So far, builtin functions which had some evaluate/expand method > couldn't also have a prototype because each would have its own symbol > and only the one for the prototype will be seen. > This also meant that the evaluate/expand functions had to take care > to set the correct types for they argumenst & results, which is fine > for some generic builtins like __builtin_constant_p() it's much less > practical for the ones like __builtin_bswap{16,32,64}(). > > Fix this by marking the idents for the builtins we declare some > evaluate/expand methods has being the ident of a builtin function > and later at evaluation time, to share the methods between all the symbols > corresponding to an identifier so marked. This certainly fix the problem for builtin functions. However, I think there is more general bug in sparse not limit to builtin functions. When the symbol is declare twice, the later one did not migrate all the declare information from the previous one. In function external_declaration() check_declaration(decl); if (decl->same_symbol) decl->definition = decl->same_symbol->definition; Here it only migrates the definition. I think if you add symbol->op in the migration as well, it *should* work for your case. I haven't test this myself. If that works for you, it is a more general fix. Want to give it a try? We might want to extract the migration code into a new function and later adding new code it. e.g. function attributes. A more complicate example will involve same symbol in different scope. void __attr_x foo(void); void a(void) { void __attr_y foo(void); // foo should have both __attr_{x,y} { void _attr_z foo(void); // foo should have all __attr_{x,y,z} } // now foo should only have __attr_{x,y} } update: I attach a quick and dirty patch file for you to try. It will replace this 2/3 patch. I just try the test suite and nothing seems complain about it. Chris
On Wed, Feb 08, 2017 at 03:49:54AM +0800, Chris Li wrote: > On Tue, Jan 24, 2017 at 5:37 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > So far, builtin functions which had some evaluate/expand method > > couldn't also have a prototype because each would have its own symbol > > and only the one for the prototype will be seen. > > This also meant that the evaluate/expand functions had to take care > > to set the correct types for they argumenst & results, which is fine > > for some generic builtins like __builtin_constant_p() it's much less > > practical for the ones like __builtin_bswap{16,32,64}(). > > > > Fix this by marking the idents for the builtins we declare some > > evaluate/expand methods has being the ident of a builtin function > > and later at evaluation time, to share the methods between all the symbols > > corresponding to an identifier so marked. > > This certainly fix the problem for builtin functions. However, I think there > is more general bug in sparse not limit to builtin functions. When the symbol > is declare twice, the later one did not migrate all the declare information from > the previous one. I'm very well aware of this bug/problem, it creates all sort of complications but I vaguely understood it was a design choice. To be 100%, you're talking the fact that each declaration create a new symbol only related by their identifier chain, right? > In function external_declaration() > > check_declaration(decl); > if (decl->same_symbol) > decl->definition = decl->same_symbol->definition; > > Here it only migrates the definition. I think if you add symbol->op in > the migration > as well, it *should* work for your case. I haven't test this myself. > If that works for you, it is a more general fix. Want to give it a try? > > We might want to extract the migration code into a new function and > later adding new code it. e.g. function attributes. Yes and diagnose any compatibility. I'll give it a try but I won't be able to do that before the weekend. Luc -- 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, Feb 8, 2017 at 4:12 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Wed, Feb 08, 2017 at 03:49:54AM +0800, Chris Li wrote: >> On Tue, Jan 24, 2017 at 5:37 AM, Luc Van Oostenryck >> <luc.vanoostenryck@gmail.com> wrote: >> > So far, builtin functions which had some evaluate/expand method >> > couldn't also have a prototype because each would have its own symbol >> > and only the one for the prototype will be seen. >> > This also meant that the evaluate/expand functions had to take care >> > to set the correct types for they argumenst & results, which is fine >> > for some generic builtins like __builtin_constant_p() it's much less >> > practical for the ones like __builtin_bswap{16,32,64}(). >> > >> > Fix this by marking the idents for the builtins we declare some >> > evaluate/expand methods has being the ident of a builtin function >> > and later at evaluation time, to share the methods between all the symbols >> > corresponding to an identifier so marked. >> >> This certainly fix the problem for builtin functions. However, I think there >> is more general bug in sparse not limit to builtin functions. When the symbol >> is declare twice, the later one did not migrate all the declare information from >> the previous one. > > I'm very well aware of this bug/problem, it creates all sort of complications > but I vaguely understood it was a design choice. To be 100%, you're talking > the fact that each declaration create a new symbol only related by their > identifier chain, right? > >> In function external_declaration() >> >> check_declaration(decl); >> if (decl->same_symbol) >> decl->definition = decl->same_symbol->definition; >> >> Here it only migrates the definition. I think if you add symbol->op in >> the migration >> as well, it *should* work for your case. I haven't test this myself. >> If that works for you, it is a more general fix. Want to give it a try? >> >> We might want to extract the migration code into a new function and >> later adding new code it. e.g. function attributes. > > Yes and diagnose any compatibility. > I'll give it a try but I won't be able to do that before the weekend. > > Luc -- 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, Feb 08, 2017 at 03:49:54AM +0800, Chris Li wrote: > On Tue, Jan 24, 2017 at 5:37 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > So far, builtin functions which had some evaluate/expand method > > couldn't also have a prototype because each would have its own symbol > > and only the one for the prototype will be seen. > > This also meant that the evaluate/expand functions had to take care > > to set the correct types for they argumenst & results, which is fine > > for some generic builtins like __builtin_constant_p() it's much less > > practical for the ones like __builtin_bswap{16,32,64}(). > > > > Fix this by marking the idents for the builtins we declare some > > evaluate/expand methods has being the ident of a builtin function > > and later at evaluation time, to share the methods between all the symbols > > corresponding to an identifier so marked. > > This certainly fix the problem for builtin functions. However, I think there > is more general bug in sparse not limit to builtin functions. When the symbol > is declare twice, the later one did not migrate all the declare information from > the previous one. > > In function external_declaration() > > check_declaration(decl); > if (decl->same_symbol) > decl->definition = decl->same_symbol->definition; > > Here it only migrates the definition. I think if you add symbol->op in > the migration > as well, it *should* work for your case. I haven't test this myself. > If that works for you, it is a more general fix. Want to give it a try? OK, I've look at this. The difference between your patch and mine is that yours share/ migrate the methods for all symbols while mine purposely did that for builtins only. The reasons I did this way was to avoid to share methods between things that don't need to, for example if someone define a function that have the same name as one of the numerous attributes (which are not reserved keywords and aren't necessarily protected by leading double underscore). But having look at this now, I see that it should never be a problem when this happen (but is it a good idea?). Anyway, I've retest with your patch and everything is fine, as expected, and I'm sending a new version of the serie. Luc -- 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
This serie solves the problem of the expansion of some builtins
like __builtin_bswap16() which gcc consider as an integer constant
expression when the arg is itself an integer constant.
Such builtins are used as such in the kernel and their non-expansion
create undesirable warnings from sparse.
This serie needs to be applied on top of Johannes Berg's patch
concerning the same problem and the tests depend on the testsuite
extensions posted previously.
Change since v1:
- simpler and more generic way to share the eval/expand ops
thanks to Christopher Li
- small changes in the log messages
- change the variable name in the bswap expansion method.
- small reorganization of the test files
Luc Van Oostenryck (3):
move evaluation & expansion of builtins in a separate file
let identical symbols share their evaluate/expand methods
expand __builtin_bswap*() with constant args
Makefile | 1 +
builtin.c | 240 +++++++++++++++++++++
expand.c | 24 +--
expand.h | 34 +++
lib.c | 35 +--
lib.h | 5 +
parse.c | 4 +-
symbol.c | 160 +-------------
symbol.h | 3 +-
validation/builtin-args-checking.c | 45 ++++
...in-constant-eval.c => builtin-bswap-constant.c} | 2 +-
validation/builtin-bswap-variable.c | 32 +++
12 files changed, 367 insertions(+), 218 deletions(-)
create mode 100644 builtin.c
create mode 100644 expand.h
create mode 100644 validation/builtin-args-checking.c
rename validation/{builtin-constant-eval.c => builtin-bswap-constant.c} (93%)
create mode 100644 validation/builtin-bswap-variable.c
On Sun, Feb 12, 2017 at 11:03 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > OK, I've look at this. > The difference between your patch and mine is that yours share/ > migrate the methods for all symbols while mine purposely did that > for builtins only. The reasons I did this way was to avoid to share That to me is actually not the main difference. Yes, the one line patch is simple and apply op to all symbol has the same name. If we need, we can make that migrate code into a function and do some special handing for builtin etc. I just don't see the need for that right now. The big difference I see is the solve the problem at symbol creating side rather than symbol usage side. In the future, we can make migrate symbol only return symbol are new (and has incremental difference). Totally drop the symbol which is the exact same abstract declare as the previous one. > methods between things that don't need to, for example if someone > define a function that have the same name as one of the numerous > attributes (which are not reserved keywords and aren't necessarily > protected by leading double underscore). But having look at this now, > I see that it should never be a problem when this happen (but is it > a good idea?). Even if that became a problem, we can add some code to deal with it. E.g. conditionally migrate some symbol information. 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 Sun, Feb 12, 2017 at 11:10 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > This serie solves the problem of the expansion of some builtins > like __builtin_bswap16() which gcc consider as an integer constant > expression when the arg is itself an integer constant. > Such builtins are used as such in the kernel and their non-expansion > create undesirable warnings from sparse. Applied in sparse-next. 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
diff --git a/builtin.c b/builtin.c index c6c97ed85..ddc71f785 100644 --- a/builtin.c +++ b/builtin.c @@ -205,6 +205,7 @@ void init_builtins(int stream) sym = create_symbol(stream, ptr->name, SYM_NODE, NS_SYMBOL); sym->ctype.base_type = ptr->base_type; sym->ctype.modifiers = ptr->modifiers; + sym->ident->builtin = 1; sym->op = ptr->op; } } diff --git a/evaluate.c b/evaluate.c index 6d44e0d9c..f89e44b75 100644 --- a/evaluate.c +++ b/evaluate.c @@ -2829,6 +2829,12 @@ static int evaluate_symbol_call(struct expression *expr) if (fn->type != EXPR_PREOP) return 0; + if (ctype->ident->builtin) { + struct symbol *next = ctype->next_id; + if (!ctype->op && next) + ctype->op = next->op; + } + if (ctype->op && ctype->op->evaluate) return ctype->op->evaluate(expr); diff --git a/token.h b/token.h index f7d88eb45..7b9c7ab74 100644 --- a/token.h +++ b/token.h @@ -73,6 +73,7 @@ struct ident { unsigned char len; /* Length of identifier name */ unsigned char tainted:1, reserved:1, + builtin:1, keyword:1; char name[]; /* Actual identifier */ };
So far, builtin functions which had some evaluate/expand method couldn't also have a prototype because each would have its own symbol and only the one for the prototype will be seen. This also meant that the evaluate/expand functions had to take care to set the correct types for they argumenst & results, which is fine for some generic builtins like __builtin_constant_p() it's much less practical for the ones like __builtin_bswap{16,32,64}(). Fix this by marking the idents for the builtins we declare some evaluate/expand methods has being the ident of a builtin function and later at evaluation time, to share the methods between all the symbols corresponding to an identifier so marked. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- builtin.c | 1 + evaluate.c | 6 ++++++ token.h | 1 + 3 files changed, 8 insertions(+)