| Message ID | 20161117095503.8754-1-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Thu, Nov 17, 2016 at 5:55 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > Since gcc does this, it's apparently valid to write > > switch (x) { > case __builtin_bswap16(12): > break; > } > Applied. Thanks for the patch. That will definite stop a lot of warning. There is other possible __builtin_xxx() function gcc expected to get a const result when the input arguments are constant. Ideally, we should be able to mark __builtin_bswap16 as a special constant "pass through" function. So during the evaluation and constant propagation, sparse can pass through the native __builtin_xxx() call to get the const result. In other words, sparse shouldn't need to implement "__builtin_bswap16()" by itself. It can just call to the gcc one to get the result. Then we can treat all other similar __builtin_xxx function the same way. That is a separate patch 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
> There is other possible __builtin_xxx() function gcc expected to get > a const result when the input arguments are constant. Ideally, we > should be able to mark __builtin_bswap16 as a special constant "pass > through" function. So during the evaluation and constant propagation, > sparse can pass through the native __builtin_xxx() call to get the > const result. In other words, sparse shouldn't need to implement > "__builtin_bswap16()" by itself. It can just call to the gcc > one to get the result. Then we can treat all other similar > __builtin_xxx function the same way. Yes, that'd make some sense. I have no idea how to pull it off though :) 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 Mon, Nov 21, 2016 at 5:45 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Yes, that'd make some sense. I have no idea how to pull it off though
I take a brief look.
It seems that you need to implement a symbol with ".expand" implement as
"expand_bswap", similar to "expand_constant_p".
Later in in "eval_init_table", just add one entry for __builtin_bswap.
eval_init_table[] = {
{ "__builtin_constant_p", &builtin_fn_type, MOD_TOPLEVEL, &constant_p_op },
{ "__builtin_safe_p", &builtin_fn_type, MOD_TOPLEVEL, &safe_p_op },
{ "__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 },
{ NULL, NULL, 0 }
};
As you can see in "expand_call", the cost of arguments is zero
if all arguments are constant.
The function "expand_symbol_call" even have the comment about expanding
the builtins there.
In side the "expand_bswap", it should abort if cost is not zero.
It means arguments are not constant, this expression can't
expand into a constant value.
Otherwise, expand_bswap should proceed to calculate the return
value of the bswap call and rewrite the call expression into a constant.
Some what similar to the "__builtin_constant_p".
I will see if I can hack up some thing very quick.
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 Tue, Nov 22, 2016 at 7:13 PM, Christopher Li <sparse@chrisli.org> wrote: > On Mon, Nov 21, 2016 at 5:45 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: >> Yes, that'd make some sense. I have no idea how to pull it off though > > I take a brief look. > > It seems that you need to implement a symbol with ".expand" implement as > "expand_bswap", similar to "expand_constant_p". Never mind that, I just find out expand stage happen after the evaluation stage. The warning happen in the evaluation stage. It will need to make some plan change to get it right. 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 Tue, Nov 22, 2016 at 07:13:11PM +0800, Christopher Li wrote: > > Otherwise, expand_bswap should proceed to calculate the return > value of the bswap call and rewrite the call expression into a constant. > Some what similar to the "__builtin_constant_p". > > I will see if I can hack up some thing very quick. > > Chris > -- 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 Not only this serie fixes several issues but it handle the expression constantnes in a much more systematic way. Also, I wouldn't like that changes in this area make it more difficult to integrate this serie (which is already waiting since July or August 2015). 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
diff --git a/lib.c b/lib.c index d5b56b012a21..aa2af68bf8d2 100644 --- a/lib.c +++ b/lib.c @@ -818,9 +818,38 @@ void declare_builtin_functions(void) 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("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"); diff --git a/validation/bswap-constant-folding.c b/validation/bswap-constant-folding.c new file mode 100644 index 000000000000..c6511fe62041 --- /dev/null +++ b/validation/bswap-constant-folding.c @@ -0,0 +1,28 @@ +typedef unsigned short __be16; +typedef unsigned short __u16; +typedef unsigned short u16; +#define __force + +#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) +/* the test behaves as though it's always on a little-endian machine */ +#define __cpu_to_be16(x) ((__force __be16)__swab16((x))) +#define ___htons(x) __cpu_to_be16(x) +#define htons(x) ___htons(x) + +#define ETH_P_IPV6 0x86DD + +static u16 protocol; + +static void test(void) +{ + switch (protocol) { + case htons(ETH_P_IPV6): + break; + } +} + +/* + * check-name: constant folding in bswap builtins + * check-error-start + * check-error-end + */
Since gcc does this, it's apparently valid to write switch (x) { case __builtin_bswap16(12): break; } but sparse will flag it as an error today. The constant folding used to be done in the kernel's htons() and friends, but due to gcc bugs that isn't done anymore since commit 7322dd755e7d ("byteswap: try to avoid __builtin_constant_p gcc bug"). To get rid of the sparse errors on every such instance now, just add constant folding to __builtin_bswap*() in sparse. Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- lib.c | 35 ++++++++++++++++++++++++++++++++--- validation/bswap-constant-folding.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 validation/bswap-constant-folding.c