| Message ID | 1484810497-21359-1-git-send-email-yi.he@linaro.org |
|---|---|
| State | New |
| Headers | show |
This is not correct. With e.g. uint64_t x = 1; __builtin_clz(x) == 63 and 8*sizeof(unsigned int) == 32, the shift value would negative. Sizeof is there to calculate bits of the variable that deliver x. You cannot fix it to 'unsigned int' without fixing the variable size seen by __builtin_clz(). It seems a false alarm to me. -Petri > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Yi He > Sent: Thursday, January 19, 2017 9:22 AM > To: bill.fischofer@linaro.org; lng-odp@lists.linaro.org > Subject: [lng-odp] [API-NEXT PATCH] linux-gen: align: fix round up power > of two > > Fix Bug https://bugs.linaro.org/show_bug.cgi?id=2828 incorrect > sizeof expression (BAD_SIZEOF) of ODP_ROUNDUP_POWER_2(literal constant) > usage. > > Signed-off-by: Yi He <yi.he@linaro.org> > --- > platform/linux-generic/include/odp_align_internal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/include/odp_align_internal.h > b/platform/linux-generic/include/odp_align_internal.h > index d9cd30b..2c2f1f5 100644 > --- a/platform/linux-generic/include/odp_align_internal.h > +++ b/platform/linux-generic/include/odp_align_internal.h > @@ -40,7 +40,7 @@ extern "C" { > * power of two value. Zero is not supported as an input value. > */ > #define ODP_ROUNDUP_POWER_2(x)\ > - (1 << (((int)(8 * sizeof(x))) - __builtin_clz((x) - 1))) > + (1 << (((int)(8 * sizeof(unsigned int))) - __builtin_clz((x) - 1))) > > /** > * @internal > -- > 2.7.4
Hi, Petri As replied in bug tracking, just worried uint64_t need to use __builtin_clzl() or __builtin_clzll() the __builtin_clz()'s parameter is unsigned int, if mixed uint64_t with __builtin_clz() it may result quite large value. But tested on my development machine (Intel i7) did not show problem, so maybe __builtin_clz() covers both uint32_t and uint64_t cases. I'll follow the decision for this bug. Best Regards, Yi On 19 January 2017 at 16:08, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > This is not correct. With e.g. > > uint64_t x = 1; > > __builtin_clz(x) == 63 and 8*sizeof(unsigned int) == 32, the shift value > would negative. > > Sizeof is there to calculate bits of the variable that deliver x. You > cannot fix it to 'unsigned int' without fixing the variable size seen by > __builtin_clz(). It seems a false alarm to me. > > -Petri > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Yi > He > > Sent: Thursday, January 19, 2017 9:22 AM > > To: bill.fischofer@linaro.org; lng-odp@lists.linaro.org > > Subject: [lng-odp] [API-NEXT PATCH] linux-gen: align: fix round up power > > of two > > > > Fix Bug https://bugs.linaro.org/show_bug.cgi?id=2828 incorrect > > sizeof expression (BAD_SIZEOF) of ODP_ROUNDUP_POWER_2(literal constant) > > usage. > > > > Signed-off-by: Yi He <yi.he@linaro.org> > > --- > > platform/linux-generic/include/odp_align_internal.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/platform/linux-generic/include/odp_align_internal.h > > b/platform/linux-generic/include/odp_align_internal.h > > index d9cd30b..2c2f1f5 100644 > > --- a/platform/linux-generic/include/odp_align_internal.h > > +++ b/platform/linux-generic/include/odp_align_internal.h > > @@ -40,7 +40,7 @@ extern "C" { > > * power of two value. Zero is not supported as an input value. > > */ > > #define ODP_ROUNDUP_POWER_2(x)\ > > - (1 << (((int)(8 * sizeof(x))) - __builtin_clz((x) - 1))) > > + (1 << (((int)(8 * sizeof(unsigned int))) - __builtin_clz((x) - 1))) > > > > /** > > * @internal > > -- > > 2.7.4 > >
Hi, OK, didn't notice those other clz builtins. Anyway, I decided to write a macro instead since it was not too convenient to combine 'unsigned int' and 'long long' builtins. Also macro allows more optimal code since preprocessor calculate the constant value only once (instead of generating code with clz instruction). -Petri From: Yi He [mailto:yi.he@linaro.org] Sent: Thursday, January 19, 2017 10:43 AM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> Cc: bill.fischofer@linaro.org; lng-odp@lists.linaro.org Subject: Re: [lng-odp] [API-NEXT PATCH] linux-gen: align: fix round up power of two Hi, Petri As replied in bug tracking, just worried uint64_t need to use __builtin_clzl() or __builtin_clzll() the __builtin_clz()'s parameter is unsigned int, if mixed uint64_t with __builtin_clz() it may result quite large value. But tested on my development machine (Intel i7) did not show problem, so maybe __builtin_clz() covers both uint32_t and uint64_t cases. I'll follow the decision for this bug. Best Regards, Yi On 19 January 2017 at 16:08, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-labs.com> wrote: This is not correct. With e.g. uint64_t x = 1; __builtin_clz(x) == 63 and 8*sizeof(unsigned int) == 32, the shift value would negative. Sizeof is there to calculate bits of the variable that deliver x. You cannot fix it to 'unsigned int' without fixing the variable size seen by __builtin_clz(). It seems a false alarm to me. -Petri > -----Original Message----- > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Yi He > Sent: Thursday, January 19, 2017 9:22 AM > To: bill.fischofer@linaro.org; lng-odp@lists.linaro.org > Subject: [lng-odp] [API-NEXT PATCH] linux-gen: align: fix round up power > of two > > Fix Bug https://bugs.linaro.org/show_bug.cgi?id=2828 incorrect > sizeof expression (BAD_SIZEOF) of ODP_ROUNDUP_POWER_2(literal constant) > usage. > > Signed-off-by: Yi He <yi.he@linaro.org> > --- > platform/linux-generic/include/odp_align_internal.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/include/odp_align_internal.h > b/platform/linux-generic/include/odp_align_internal.h > index d9cd30b..2c2f1f5 100644 > --- a/platform/linux-generic/include/odp_align_internal.h > +++ b/platform/linux-generic/include/odp_align_internal.h > @@ -40,7 +40,7 @@ extern "C" { > * power of two value. Zero is not supported as an input value. > */ > #define ODP_ROUNDUP_POWER_2(x)\ > - (1 << (((int)(8 * sizeof(x))) - __builtin_clz((x) - 1))) > + (1 << (((int)(8 * sizeof(unsigned int))) - __builtin_clz((x) - 1))) > > /** > * @internal > -- > 2.7.4
diff --git a/platform/linux-generic/include/odp_align_internal.h b/platform/linux-generic/include/odp_align_internal.h index d9cd30b..2c2f1f5 100644 --- a/platform/linux-generic/include/odp_align_internal.h +++ b/platform/linux-generic/include/odp_align_internal.h @@ -40,7 +40,7 @@ extern "C" { * power of two value. Zero is not supported as an input value. */ #define ODP_ROUNDUP_POWER_2(x)\ - (1 << (((int)(8 * sizeof(x))) - __builtin_clz((x) - 1))) + (1 << (((int)(8 * sizeof(unsigned int))) - __builtin_clz((x) - 1))) /** * @internal
Fix Bug https://bugs.linaro.org/show_bug.cgi?id=2828 incorrect sizeof expression (BAD_SIZEOF) of ODP_ROUNDUP_POWER_2(literal constant) usage. Signed-off-by: Yi He <yi.he@linaro.org> --- platform/linux-generic/include/odp_align_internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.7.4