Message ID | CANeU7QmSd5Zd_H2jDQyTjUMg7C4_6wQOyCR73R=FTdJ=f4+ygg@mail.gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Wed, Nov 23, 2016 at 12:32:52AM +0800, Christopher Li wrote: > > I think it would be best to do any change related tos handling > > of constant expressions on to of Nicolai Stange's serie: > > http://marc.info/?l=linux-sparse&m=145429372932235 > > Yes, it is on my patch list. That series can still apply clean. > I just check it. Great. > My current strategy is apply other patches which have impact > on the sparse warning first. Otherwise my anxiety level is too > high, make me hard to pick up and hack sparse. That integer > series needs a relative big chunk of time to review it. For me > it is more important to keep on making progress rather than > solve all problem in one blow. I understand. > I think I have been catch up with most of the patches. Yes, just from memory, there is still one from Oleg, about dissect. > Should be able to work on that soon. Great. 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, Nov 23, 2016 at 12:32:52AM +0800, Christopher Li wrote: > On Tue, Nov 22, 2016 at 9:15 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > >> > >> I will see if I can hack up some thing very quick. > > I get it working in the end. Not too far from what I thought. > Patch attach here for review. Work great here. If needed here is my Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> -- 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 23, 2016 at 1:12 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> I think I have been catch up with most of the patches. > Yes, just from memory, there is still one from Oleg, about dissect. Another observation is that, the review order does not change how much time it need to review the patches. Here is my current pending list other than the constant integer expression series. Chris 8249571-dissect-teach-do_expression-to-handle-EXPR_OFFSETOF.patch 8250741-dissect-teach-do_initializer-to-handle-the-nested-EXPR_IDENTIFIER-s.patch 8258041-v3-sparse-add-support-for-static-assert.patch 8285451-dissect-s-mode_t-usage_t-in-report_member.patch 8335781-FAIL-1-5-parse-Add-comment-to-struct-statement.patch 8335791-FAIL-3-5-linearize-Add-context-symbol-name-when-showing-context-instructions.patch 8335811-FAIL-5-5-test-locks-Add-lock-tester.patch 8335821-FAIL-4-5-parse-Add-acquire-release-requires-and-guarded_by-attributes.patch 8335851-FAIL-2-5-inspect-Show-context-information.patch 8655361-1-2-compiler.h-add-support-for-malloc-attribute.patch 9013811-2-2-validation-Check-C99-for-loop-variables.patch 9428345-RFC-bits_in_bool-should-be-8.patch 9434581-7-8-explicitely-ignore-killing-OP_ENTRY.patch 9434607-3-5-testsuite-report-as-error-tests-known-to-fail-but-which-succeed.patch 9434617-8-8-cleanup-kill_instruction.patch 9434767-1-2-add-missing-PACK_PTR_LIST.patch 9434769-2-2-mark-lists-to-be-repacked-as-dirty.patch 9434773-4-8-fix-killing-OP_CAST-friends.patch 9435027-6-8-fix-killing-OP_COMPUTEDGOTO.patch 9435155-5-8-fix-killing-OP_SELECT.patch 9435157-3-8-fix-killing-OP_PHI-instructions.patch 9435159-1-8-kill-uses-of-replaced-instructions.patch 9435161-2-8-fix-killing-OP_SETVAL-instructions.patch 9435305-0-2-be-more-generous-with-ptrlist-repacking.patch -- 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 23, 2016 at 4:16 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > Work great here. > > If needed here is my > Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Great, that is what I am looking for. Will apply that patch. 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
> I get it working in the end. Not too far from what I thought. > Patch attach here for review. Just a small comment: > + if (expression_list_size(args) != 1) { > + sparse_error(expr->pos, "not enough arguments for function %s", > + show_ident(sym->ident)); Should that say "invalid number of arguments (expected 1)" or so? johannes -- 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 23, 2016 at 2:28 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > > Just a small comment: > >> + if (expression_list_size(args) != 1) { >> + sparse_error(expr->pos, "not enough arguments for function %s", >> + show_ident(sym->ident)); > > Should that say "invalid number of arguments (expected 1)" or so? > Right, that is better. 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 Wed, Nov 23, 2016 at 09:25:18AM +0800, Christopher Li wrote: > On Wed, Nov 23, 2016 at 4:16 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > Work great here. > > > > If needed here is my > > Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > There is a problem though, with non-constant args. Now the predeclarations of the 3 __builtin_bswap16/32/64 are gone, no more prototype and thus no more typing information. For example, if you call __builtin_bswap64() with an int argument, there is no way for the next steps to know the arg must first be 64 bit extended. Same for the result's type. 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 Thu, Nov 24, 2016 at 4:48 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > There is a problem though, with non-constant args. > Now the predeclarations of the 3 __builtin_bswap16/32/64 > are gone, no more prototype and thus no more typing information. > > For example, if you call __builtin_bswap64() with an int argument, > there is no way for the next steps to know the arg must first > be 64 bit extended. Same for the result's type. That is a very good point. Need more work on that. The currently problem if I include the function prototype, the function prototype symbol will over shadow the symbol contain swap_op. We need to make sure the function prototype symbol is the one loading with correct swap_op. Ideally, all the "__builtin_xxx" related stuff should be move to a separate builtin.c. Right now the function prototype is in lib.c The implementation is sprinkle around symbol.c, evaluation.c and expand.c. Re-attach the V2 patch as last email I forget to CC sparse mailing list. This V2 patch still have the missing prototype problem you describe here. 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 Thu, Nov 24, 2016 at 08:56:45AM +0800, Christopher Li wrote: > On Thu, Nov 24, 2016 at 4:48 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > There is a problem though, with non-constant args. > > Now the predeclarations of the 3 __builtin_bswap16/32/64 > > are gone, no more prototype and thus no more typing information. > > > > For example, if you call __builtin_bswap64() with an int argument, > > there is no way for the next steps to know the arg must first > > be 64 bit extended. Same for the result's type. > > That is a very good point. Need more work on that. > > The currently problem if I include the function prototype, the > function prototype symbol will over shadow the symbol contain > swap_op. We need to make sure the function prototype symbol > is the one loading with correct swap_op. Yes indeed. Maybe another approach is possible: - note the fact that "stuff" (I intentionally don't want to use "function") like __builtin_bswap*() behave like an operator or a function in the mathematical sense: they only depend on their args, have no side-effect, ... in others word they are "pure" like in MOD_PURE. - because they're pure they necessarily return constant value if given constant args. This may be used early, the value itself is only needed in later phases. It may also not be a bad idea to have a specific instructions for these (like a few others, I'm thinking to rotate), after all it's not without reasons that common CPU archs have these as real instructions (but this won't directly help our present problem). > Ideally, all the "__builtin_xxx" related stuff should be move to a > separate builtin.c. Right now the function prototype is in lib.c > The implementation is sprinkle around symbol.c, evaluation.c > and expand.c. It would be good, yes. 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 Thu, Nov 24, 2016 at 9:31 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > Maybe another approach is possible: > - note the fact that "stuff" (I intentionally don't want to use > "function") like __builtin_bswap*() behave like an operator > or a function in the mathematical sense: they only depend on > their args, have no side-effect, ... in others word they are "pure" > like in MOD_PURE. > - because they're pure they necessarily return constant value if given > constant args. This may be used early, the value itself is only needed > in later phases. I think that is exactly why __builtins_xxx exist. There is also pure function and inline function etc. I guess I will still call it a function rather than "stuff". > It may also not be a bad idea to have a specific instructions for these > (like a few others, I'm thinking to rotate), after all it's not without > reasons that common CPU archs have these as real instructions > (but this won't directly help our present problem). If the function is expanded at compile time. It does not matter what instruction it was used. It only matter when it need to emit as real instruction in the back end. The black end still have the flexibility to inline asm it. Agree, it does not help our current problem though. 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/expand.c b/expand.c index 0f6720c..1ef95ea 100644 --- a/expand.c +++ b/expand.c @@ -802,6 +802,44 @@ int expand_safe_p(struct expression *expr, int cost) return 0; } +/* The arguments are constant if the cost of all of them is zero */ +int expand_bswap(struct expression *expr, int cost) +{ + struct symbol *sym; + struct expression_list *args = expr->args; + long long input; + + if (cost) + return cost; + + sym = expr->fn->ctype; + if (expression_list_size(args) != 1) { + sparse_error(expr->pos, "not enough arguments for function %s", + show_ident(sym->ident)); + return SIDE_EFFECTS; + } + + input = const_expression_value(first_expression(args)); + + if (sym->ident == &__builtin_bswap16_ident) { + expr->value = __builtin_bswap16(input); + expr->ctype = &ushort_ctype; + } else if (sym->ident == &__builtin_bswap32_ident) { + expr->value = __builtin_bswap32(input); + expr->ctype = &uint_ctype; + } else if (sym->ident == &__builtin_bswap64_ident) { + expr->value = __builtin_bswap64(input); + expr->ctype = &ullong_ctype; + } else { + die("Unexpected __builtin_bswap symbol %s\n", show_ident(sym->ident)); + } + + expr->type = EXPR_VALUE; + expr->taint = 0; + return 0; +} + + /* * expand a call expression with a symbol. This * should expand builtins. diff --git a/ident-list.h b/ident-list.h index b65b667..a683f6c 100644 --- a/ident-list.h +++ b/ident-list.h @@ -35,6 +35,9 @@ IDENT_RESERVED(__sizeof_ptr__); IDENT_RESERVED(__builtin_types_compatible_p); IDENT_RESERVED(__builtin_offsetof); IDENT_RESERVED(__label__); +IDENT(__builtin_bswap16); +IDENT(__builtin_bswap32); +IDENT(__builtin_bswap64); /* Attribute names */ IDENT(packed); IDENT(__packed__); diff --git a/lib.c b/lib.c index 2660575..7e35049 100644 --- a/lib.c +++ b/lib.c @@ -819,40 +819,6 @@ void declare_builtin_functions(void) add_pre_buffer("extern int __builtin_popcountl(unsigned long);\n"); add_pre_buffer("extern int __builtin_popcountll(unsigned long long);\n"); - /* And byte swaps.. */ - add_pre_buffer("extern unsigned short ____builtin_bswap16(unsigned short);\n"); - add_pre_buffer("extern unsigned int ____builtin_bswap32(unsigned int);\n"); - add_pre_buffer("extern unsigned long long ____builtin_bswap64(unsigned long long);\n"); - add_pre_buffer("#define __sparse_constant_swab16(x) ((unsigned short)(" - " (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) |" - " (((unsigned short)(x) & (unsigned short)0xff00U) >> 8)))\n"); - add_pre_buffer("#define __sparse_constant_swab32(x) ((unsigned int)(" - " (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) |" - " (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) |" - " (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) |" - " (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24)))\n"); - add_pre_buffer("#define __sparse_constant_swab64(x) ((unsigned long long)(" - " (((unsigned long long)(x) & (unsigned long long)0x00000000000000ffULL) << 56) |" - " (((unsigned long long)(x) & (unsigned long long)0x000000000000ff00ULL) << 40) |" - " (((unsigned long long)(x) & (unsigned long long)0x0000000000ff0000ULL) << 24) |" - " (((unsigned long long)(x) & (unsigned long long)0x00000000ff000000ULL) << 8) |" - " (((unsigned long long)(x) & (unsigned long long)0x000000ff00000000ULL) >> 8) |" - " (((unsigned long long)(x) & (unsigned long long)0x0000ff0000000000ULL) >> 24) |" - " (((unsigned long long)(x) & (unsigned long long)0x00ff000000000000ULL) >> 40) |" - " (((unsigned long long)(x) & (unsigned long long)0xff00000000000000ULL) >> 56)))\n"); - add_pre_buffer("#define __builtin_bswap16(x)" - " (__builtin_constant_p((unsigned short)(x)) ?" - " __sparse_constant_swab16(x) :" - " ____builtin_bswap16(x))\n"); - add_pre_buffer("#define __builtin_bswap32(x)" - " (__builtin_constant_p((unsigned int)(x)) ?" - " __sparse_constant_swab32(x) :" - " ____builtin_bswap32(x))\n"); - add_pre_buffer("#define __builtin_bswap64(x)" - " (__builtin_constant_p((unsigned long long)(x)) ?" - " __sparse_constant_swab64(x) :" - " ____builtin_bswap64(x))\n"); - /* And atomic memory access functions.. */ add_pre_buffer("extern int __sync_fetch_and_add(void *, ...);\n"); add_pre_buffer("extern int __sync_fetch_and_sub(void *, ...);\n"); diff --git a/lib.h b/lib.h index b778bdc..306ee45 100644 --- a/lib.h +++ b/lib.h @@ -200,6 +200,11 @@ static inline struct instruction *first_instruction(struct instruction_list *hea return first_ptr_list((struct ptr_list *)head); } +static inline struct expression *first_expression(struct expression_list *head) +{ + return first_ptr_list((struct ptr_list *)head); +} + static inline pseudo_t first_pseudo(struct pseudo_list *head) { return first_ptr_list((struct ptr_list *)head); diff --git a/symbol.c b/symbol.c index 92a7a62..e57f207 100644 --- a/symbol.c +++ b/symbol.c @@ -773,6 +773,11 @@ static struct symbol_op choose_op = { .args = arguments_choose, }; +static struct symbol_op bswap_op = { + .evaluate = evaluate_to_integer, + .expand = expand_bswap +}; + /* * Builtin functions */ @@ -788,6 +793,9 @@ static struct sym_init { { "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op }, { "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op }, { "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op }, + { "__builtin_bswap16", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op }, + { "__builtin_bswap32", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op }, + { "__builtin_bswap64", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op }, { NULL, NULL, 0 } }; diff --git a/symbol.h b/symbol.h index 9b3f160..48bbfce 100644 --- a/symbol.h +++ b/symbol.h @@ -130,6 +130,7 @@ struct symbol_op { extern int expand_safe_p(struct expression *expr, int cost); extern int expand_constant_p(struct expression *expr, int cost); +extern int expand_bswap(struct expression *expr, int cost); #define SYM_ATTR_WEAK 0 #define SYM_ATTR_NORMAL 1