| Message ID | 1396939408-10420-1-git-send-email-santosh.shukla@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
* if (find_and_del_tmo(&tick->list, tmo) == 0) { -
ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, tmo,
tick_idx); + ODP_ERR("cancel failed for tim id %d tmo id :%d
tick idx %"PRIu64"\n", id, tmo, tick_idx);
odp_spinlock_unlock(&tick->lock);*
Invoking ODP_ERR() with a spinlock held? Seems like a bad idea to me. You
need 100% control over the code that is executed while a spinlock is held.
The processing needs to be finite (limited) in time.
What happens if the tmo has already expired and odp_timer_cancel_tmo() is
called? The tmo will not be in the tick list anymore I assume. Why are we
calling ODP_ERR() for a legal situation (cancel of expired tmo)?
On 8 April 2014 08:43, Santosh Shukla <santosh.shukla@linaro.org> wrote:
> source/odp_timer.c: In function 'odp_timer_cancel_tmo':wq!
> source/odp_timer.c:136:3: error: format '%lu' expects argument of type
> 'long unsigned int',
> but argument 8 has type 'uint64_t' [-Werror=format=]
> ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id,
> tmo, tick_idx);
>
> odp_timer_ping.c: In function 'ping_init':
> odp_timer_ping.c:257:31: error: cast increases required alignment of
> target type [-Werror=cast-align]
> dst_addr.sin_addr.s_addr = *(long *)hname->h_addr;
>
> Signed-off-by: santosh shukla <santosh.shukla@linaro.org>
> ---
> platform/linux-generic/source/odp_timer.c | 2 +-
> test/api_test/odp_timer_ping.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/platform/linux-generic/source/odp_timer.c
> b/platform/linux-generic/source/odp_timer.c
> index 075de29..875244d 100644
> --- a/platform/linux-generic/source/odp_timer.c
> +++ b/platform/linux-generic/source/odp_timer.c
> @@ -133,7 +133,7 @@ int odp_timer_cancel_tmo(odp_timer_t timer,
> odp_timer_tmo_t tmo)
> odp_spinlock_lock(&tick->lock);
> /* search and delete tmo from tick list */
> if (find_and_del_tmo(&tick->list, tmo) == 0) {
> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx
> %lu\n", id, tmo, tick_idx);
> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx
> %"PRIu64"\n", id, tmo, tick_idx);
> odp_spinlock_unlock(&tick->lock);
> return -1;
> }
> diff --git a/test/api_test/odp_timer_ping.c
> b/test/api_test/odp_timer_ping.c
> index 6c84a56..8b297d8 100644
> --- a/test/api_test/odp_timer_ping.c
> +++ b/test/api_test/odp_timer_ping.c
> @@ -254,7 +254,7 @@ static int ping_init(int count, char *name[])
> bzero(&dst_addr, sizeof(dst_addr));
> dst_addr.sin_family = hname->h_addrtype;
> dst_addr.sin_port = 0;
> - dst_addr.sin_addr.s_addr = *(long *)hname->h_addr;
> + dst_addr.sin_addr.s_addr = *(long *)(void *)hname->h_addr;
> }
> printf("ping to addr %s\n", name[1]);
>
> --
> 1.7.9.5
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
On 8 April 2014 14:18, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > if (find_and_del_tmo(&tick->list, tmo) == 0) { > - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx > %lu\n", id, tmo, tick_idx); > + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx > %"PRIu64"\n", id, tmo, tick_idx); > odp_spinlock_unlock(&tick->lock); > > Invoking ODP_ERR() with a spinlock held? Seems like a bad idea to me. You > need 100% control over the code that is executed while a spinlock is held. > The processing needs to be finite (limited) in time. > valid one. > What happens if the tmo has already expired and odp_timer_cancel_tmo() is > called? The tmo will not be in the tick list anymore I assume. Why are we > calling ODP_ERR() for a legal situation (cancel of expired tmo)? > We wont, in next patch version. Thanks for feedback. > > > On 8 April 2014 08:43, Santosh Shukla <santosh.shukla@linaro.org> wrote: >> >> source/odp_timer.c: In function 'odp_timer_cancel_tmo':wq! >> source/odp_timer.c:136:3: error: format '%lu' expects argument of type >> 'long unsigned int', >> but argument 8 has type 'uint64_t' [-Werror=format=] >> ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, >> tmo, tick_idx); >> >> odp_timer_ping.c: In function 'ping_init': >> odp_timer_ping.c:257:31: error: cast increases required alignment of >> target type [-Werror=cast-align] >> dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >> >> Signed-off-by: santosh shukla <santosh.shukla@linaro.org> >> --- >> platform/linux-generic/source/odp_timer.c | 2 +- >> test/api_test/odp_timer_ping.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/platform/linux-generic/source/odp_timer.c >> b/platform/linux-generic/source/odp_timer.c >> index 075de29..875244d 100644 >> --- a/platform/linux-generic/source/odp_timer.c >> +++ b/platform/linux-generic/source/odp_timer.c >> @@ -133,7 +133,7 @@ int odp_timer_cancel_tmo(odp_timer_t timer, >> odp_timer_tmo_t tmo) >> odp_spinlock_lock(&tick->lock); >> /* search and delete tmo from tick list */ >> if (find_and_del_tmo(&tick->list, tmo) == 0) { >> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >> %lu\n", id, tmo, tick_idx); >> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >> %"PRIu64"\n", id, tmo, tick_idx); >> odp_spinlock_unlock(&tick->lock); >> return -1; >> } >> diff --git a/test/api_test/odp_timer_ping.c >> b/test/api_test/odp_timer_ping.c >> index 6c84a56..8b297d8 100644 >> --- a/test/api_test/odp_timer_ping.c >> +++ b/test/api_test/odp_timer_ping.c >> @@ -254,7 +254,7 @@ static int ping_init(int count, char *name[]) >> bzero(&dst_addr, sizeof(dst_addr)); >> dst_addr.sin_family = hname->h_addrtype; >> dst_addr.sin_port = 0; >> - dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >> + dst_addr.sin_addr.s_addr = *(long *)(void *)hname->h_addr; >> } >> printf("ping to addr %s\n", name[1]); >> >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > >
which gcc version are you using to catch this bug? Maxim. On 04/08/2014 01:26 PM, Santosh Shukla wrote: > On 8 April 2014 14:18, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> if (find_and_del_tmo(&tick->list, tmo) == 0) { >> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >> %lu\n", id, tmo, tick_idx); >> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >> %"PRIu64"\n", id, tmo, tick_idx); >> odp_spinlock_unlock(&tick->lock); >> >> Invoking ODP_ERR() with a spinlock held? Seems like a bad idea to me. You >> need 100% control over the code that is executed while a spinlock is held. >> The processing needs to be finite (limited) in time. >> > valid one. >> What happens if the tmo has already expired and odp_timer_cancel_tmo() is >> called? The tmo will not be in the tick list anymore I assume. Why are we >> calling ODP_ERR() for a legal situation (cancel of expired tmo)? >> > We wont, in next patch version. Thanks for feedback. >> >> On 8 April 2014 08:43, Santosh Shukla <santosh.shukla@linaro.org> wrote: >>> source/odp_timer.c: In function 'odp_timer_cancel_tmo':wq! >>> source/odp_timer.c:136:3: error: format '%lu' expects argument of type >>> 'long unsigned int', >>> but argument 8 has type 'uint64_t' [-Werror=format=] >>> ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, >>> tmo, tick_idx); >>> >>> odp_timer_ping.c: In function 'ping_init': >>> odp_timer_ping.c:257:31: error: cast increases required alignment of >>> target type [-Werror=cast-align] >>> dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >>> >>> Signed-off-by: santosh shukla <santosh.shukla@linaro.org> >>> --- >>> platform/linux-generic/source/odp_timer.c | 2 +- >>> test/api_test/odp_timer_ping.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/platform/linux-generic/source/odp_timer.c >>> b/platform/linux-generic/source/odp_timer.c >>> index 075de29..875244d 100644 >>> --- a/platform/linux-generic/source/odp_timer.c >>> +++ b/platform/linux-generic/source/odp_timer.c >>> @@ -133,7 +133,7 @@ int odp_timer_cancel_tmo(odp_timer_t timer, >>> odp_timer_tmo_t tmo) >>> odp_spinlock_lock(&tick->lock); >>> /* search and delete tmo from tick list */ >>> if (find_and_del_tmo(&tick->list, tmo) == 0) { >>> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>> %lu\n", id, tmo, tick_idx); >>> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>> %"PRIu64"\n", id, tmo, tick_idx); >>> odp_spinlock_unlock(&tick->lock); >>> return -1; >>> } >>> diff --git a/test/api_test/odp_timer_ping.c >>> b/test/api_test/odp_timer_ping.c >>> index 6c84a56..8b297d8 100644 >>> --- a/test/api_test/odp_timer_ping.c >>> +++ b/test/api_test/odp_timer_ping.c >>> @@ -254,7 +254,7 @@ static int ping_init(int count, char *name[]) >>> bzero(&dst_addr, sizeof(dst_addr)); >>> dst_addr.sin_family = hname->h_addrtype; >>> dst_addr.sin_port = 0; >>> - dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >>> + dst_addr.sin_addr.s_addr = *(long *)(void *)hname->h_addr; >>> } >>> printf("ping to addr %s\n", name[1]); >>> >>> -- >>> 1.7.9.5 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >> > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On Arndale, I found this bug for gcc version - root@linaro-server:~/odp# gcc --version gcc (Ubuntu/Linaro 4.8.1-10ubuntu8) 4.8.1 Copyright (C) 2013 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. On 8 April 2014 15:04, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > which gcc version are you using to catch this bug? > > Maxim. > > > On 04/08/2014 01:26 PM, Santosh Shukla wrote: >> >> On 8 April 2014 14:18, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >>> >>> if (find_and_del_tmo(&tick->list, tmo) == 0) { >>> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>> %lu\n", id, tmo, tick_idx); >>> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>> %"PRIu64"\n", id, tmo, tick_idx); >>> odp_spinlock_unlock(&tick->lock); >>> >>> Invoking ODP_ERR() with a spinlock held? Seems like a bad idea to me. You >>> need 100% control over the code that is executed while a spinlock is >>> held. >>> The processing needs to be finite (limited) in time. >>> >> valid one. >>> >>> What happens if the tmo has already expired and odp_timer_cancel_tmo() is >>> called? The tmo will not be in the tick list anymore I assume. Why are we >>> calling ODP_ERR() for a legal situation (cancel of expired tmo)? >>> >> We wont, in next patch version. Thanks for feedback. >>> >>> >>> On 8 April 2014 08:43, Santosh Shukla <santosh.shukla@linaro.org> wrote: >>>> >>>> source/odp_timer.c: In function 'odp_timer_cancel_tmo':wq! >>>> source/odp_timer.c:136:3: error: format '%lu' expects argument of type >>>> 'long unsigned int', >>>> but argument 8 has type 'uint64_t' [-Werror=format=] >>>> ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, >>>> tmo, tick_idx); >>>> >>>> odp_timer_ping.c: In function 'ping_init': >>>> odp_timer_ping.c:257:31: error: cast increases required alignment of >>>> target type [-Werror=cast-align] >>>> dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >>>> >>>> Signed-off-by: santosh shukla <santosh.shukla@linaro.org> >>>> --- >>>> platform/linux-generic/source/odp_timer.c | 2 +- >>>> test/api_test/odp_timer_ping.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/source/odp_timer.c >>>> b/platform/linux-generic/source/odp_timer.c >>>> index 075de29..875244d 100644 >>>> --- a/platform/linux-generic/source/odp_timer.c >>>> +++ b/platform/linux-generic/source/odp_timer.c >>>> @@ -133,7 +133,7 @@ int odp_timer_cancel_tmo(odp_timer_t timer, >>>> odp_timer_tmo_t tmo) >>>> odp_spinlock_lock(&tick->lock); >>>> /* search and delete tmo from tick list */ >>>> if (find_and_del_tmo(&tick->list, tmo) == 0) { >>>> - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>>> %lu\n", id, tmo, tick_idx); >>>> + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx >>>> %"PRIu64"\n", id, tmo, tick_idx); >>>> odp_spinlock_unlock(&tick->lock); >>>> return -1; >>>> } >>>> diff --git a/test/api_test/odp_timer_ping.c >>>> b/test/api_test/odp_timer_ping.c >>>> index 6c84a56..8b297d8 100644 >>>> --- a/test/api_test/odp_timer_ping.c >>>> +++ b/test/api_test/odp_timer_ping.c >>>> @@ -254,7 +254,7 @@ static int ping_init(int count, char *name[]) >>>> bzero(&dst_addr, sizeof(dst_addr)); >>>> dst_addr.sin_family = hname->h_addrtype; >>>> dst_addr.sin_port = 0; >>>> - dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; >>>> + dst_addr.sin_addr.s_addr = *(long *)(void >>>> *)hname->h_addr; >>>> } >>>> printf("ping to addr %s\n", name[1]); >>>> >>>> -- >>>> 1.7.9.5 >>>> >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org >>>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/source/odp_timer.c b/platform/linux-generic/source/odp_timer.c index 075de29..875244d 100644 --- a/platform/linux-generic/source/odp_timer.c +++ b/platform/linux-generic/source/odp_timer.c @@ -133,7 +133,7 @@ int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo) odp_spinlock_lock(&tick->lock); /* search and delete tmo from tick list */ if (find_and_del_tmo(&tick->list, tmo) == 0) { - ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, tmo, tick_idx); + ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %"PRIu64"\n", id, tmo, tick_idx); odp_spinlock_unlock(&tick->lock); return -1; } diff --git a/test/api_test/odp_timer_ping.c b/test/api_test/odp_timer_ping.c index 6c84a56..8b297d8 100644 --- a/test/api_test/odp_timer_ping.c +++ b/test/api_test/odp_timer_ping.c @@ -254,7 +254,7 @@ static int ping_init(int count, char *name[]) bzero(&dst_addr, sizeof(dst_addr)); dst_addr.sin_family = hname->h_addrtype; dst_addr.sin_port = 0; - dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; + dst_addr.sin_addr.s_addr = *(long *)(void *)hname->h_addr; } printf("ping to addr %s\n", name[1]);
source/odp_timer.c: In function ‘odp_timer_cancel_tmo’:wq! source/odp_timer.c:136:3: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 8 has type ‘uint64_t’ [-Werror=format=] ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx %lu\n", id, tmo, tick_idx); odp_timer_ping.c: In function ‘ping_init’: odp_timer_ping.c:257:31: error: cast increases required alignment of target type [-Werror=cast-align] dst_addr.sin_addr.s_addr = *(long *)hname->h_addr; Signed-off-by: santosh shukla <santosh.shukla@linaro.org> --- platform/linux-generic/source/odp_timer.c | 2 +- test/api_test/odp_timer_ping.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)