| Message ID | 1467273705-14626-1-git-send-email-matias.elo@nokia.com |
|---|---|
| State | New |
| Headers | show |
On 30 June 2016 at 04:01, Matias Elo <matias.elo@nokia.com> wrote: > Workaround for a bug in clang version 3.8.0-2ubuntu3 (Ubuntu 16.04). Clang > throws an error 'variable t1 is uninitialized' on line 420 even though > 't1' is always initialized before the line. > > The original code works on clang version 3.4-1ubuntu3 (Ubuntu 14.04) and on > gcc. > > This fixes bug: https://bugs.linaro.org/show_bug.cgi?id=2387 > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > --- > platform/linux-generic/odp_schedule_sp.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_schedule_sp.c > b/platform/linux-generic/odp_schedule_sp.c > index 8c45123..8314640 100644 > --- a/platform/linux-generic/odp_schedule_sp.c > +++ b/platform/linux-generic/odp_schedule_sp.c > @@ -385,7 +385,10 @@ static int schedule_multi(odp_queue_t *from, uint64_t > wait, > uint32_t qi; > int num; > odp_time_t t1; > - > +#ifdef __clang__ > + /* Dummy initialization due to a clang bug */ > + t1 = ODP_TIME_NULL; > +#endif > It feels like a slippery slope to be adding compiler specific fixes, but I assume we cant refactor our way out of this ? If we are adding these workarounds we should also be upstreaming a bug report to get the real compiler issue fixed. It would be nice for this patch to be commented with a clang bug reference so that at some point we know we can clean up. > cmd = sched_cmd(NUM_PRIO); > > if (cmd && cmd->s.type == CMD_PKTIO) { > -- > 1.9.1 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On Thu, Jun 30, 2016 at 10:55 AM, Mike Holmes <mike.holmes@linaro.org> wrote: > On 30 June 2016 at 04:01, Matias Elo <matias.elo@nokia.com> wrote: > > > Workaround for a bug in clang version 3.8.0-2ubuntu3 (Ubuntu 16.04). > Clang > > throws an error 'variable t1 is uninitialized' on line 420 even though > > 't1' is always initialized before the line. > > > > The original code works on clang version 3.4-1ubuntu3 (Ubuntu 14.04) and > on > > gcc. > > > > This fixes bug: https://bugs.linaro.org/show_bug.cgi?id=2387 > > > > Signed-off-by: Matias Elo <matias.elo@nokia.com> > > --- > > platform/linux-generic/odp_schedule_sp.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/platform/linux-generic/odp_schedule_sp.c > > b/platform/linux-generic/odp_schedule_sp.c > > index 8c45123..8314640 100644 > > --- a/platform/linux-generic/odp_schedule_sp.c > > +++ b/platform/linux-generic/odp_schedule_sp.c > > @@ -385,7 +385,10 @@ static int schedule_multi(odp_queue_t *from, > uint64_t > > wait, > > uint32_t qi; > > int num; > > odp_time_t t1; > > - > > +#ifdef __clang__ > > + /* Dummy initialization due to a clang bug */ > > + t1 = ODP_TIME_NULL; > > +#endif > > > > It feels like a slippery slope to be adding compiler specific fixes, but I > assume we cant refactor our way out of this ? > If we are adding these workarounds we should also be upstreaming a bug > report to get the real compiler issue fixed. > It would be nice for this patch to be commented with a clang bug reference > so that at some point we know we can clean up. > > I concur. I'd prefer to see the explicit initialization odp_time_t t1 = ODP_TIME_NULL; and be done with it. This would also allow deletion of the update_t1 variable (and its initialization) since the code: if (update_t1) ... could then be replaced by if (t1 == ODP_TIME_NULL) ... to achieve the same effect so there should be no concerns about less efficient code. > > > > cmd = sched_cmd(NUM_PRIO); > > > > if (cmd && cmd->s.type == CMD_PKTIO) { > > -- > > 1.9.1 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > Mike Holmes > Technical Manager - Linaro Networking Group > Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs > "Work should be fun and collaborative, the rest follows" > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
I've sent an alternate patch to the mailing list which should resolve this issue without #ifdefs and should also be faster since it avoids the repeated checks for whether t1 needs to be initialized on each iteration of the schedule loop in the event we are doing a timed wait. On Fri, Jul 1, 2016 at 1:34 AM, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia-bell-labs.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > Bill > > Fischofer > > Sent: Thursday, June 30, 2016 9:45 PM > > To: Mike Holmes <mike.holmes@linaro.org> > > Cc: Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia-bell-labs.com>; > lng- > > odp <lng-odp@lists.linaro.org> > > Subject: Re: [lng-odp] [PATCH] linux-gen: sched: clang bug workaround > > > > > It feels like a slippery slope to be adding compiler specific fixes, > but > > I > > > assume we cant refactor our way out of this ? > > > If we are adding these workarounds we should also be upstreaming a bug > > > report to get the real compiler issue fixed. > > > It would be nice for this patch to be commented with a clang bug > > reference > > > so that at some point we know we can clean up. > > > > > > > > I concur. I'd prefer to see the explicit initialization > > > > odp_time_t t1 = ODP_TIME_NULL; > > > > and be done with it. This would also allow deletion of the update_t1 > > variable (and its initialization) since the code: > > > > if (update_t1) ... > > > > could then be replaced by > > > > if (t1 == ODP_TIME_NULL) ... > > > > to achieve the same effect so there should be no concerns about less > > efficient code. > > > This is explicitly avoided here since odp_time_t is two 64 bit words... > > typedef struct odp_time_t { > int64_t tv_sec; /**< @internal Seconds */ > int64_t tv_nsec; /**< @internal Nanoseconds */ > } odp_time_t; > > ... and all operations on it would translate atleast 2x (in 64 bit system) > or 4x (in 32 bit system) performance degradation compared to 'int > update_t1'. This is done for every schedule call, so performance does count > here. > > In a 32 bit system this... > if (t1 == ODP_TIME_NULL) > > ... would expand to this. > if (t1.tv_sec.upper == 0 && t1.tv_sec.lower == 0 && t1.tv_nsec.upper == 0 > && t1.tv_nsec.lower == 0) > > > -Petri > > > >
diff --git a/platform/linux-generic/odp_schedule_sp.c b/platform/linux-generic/odp_schedule_sp.c index 8c45123..8314640 100644 --- a/platform/linux-generic/odp_schedule_sp.c +++ b/platform/linux-generic/odp_schedule_sp.c @@ -385,7 +385,10 @@ static int schedule_multi(odp_queue_t *from, uint64_t wait, uint32_t qi; int num; odp_time_t t1; - +#ifdef __clang__ + /* Dummy initialization due to a clang bug */ + t1 = ODP_TIME_NULL; +#endif cmd = sched_cmd(NUM_PRIO); if (cmd && cmd->s.type == CMD_PKTIO) {
Workaround for a bug in clang version 3.8.0-2ubuntu3 (Ubuntu 16.04). Clang throws an error 'variable t1 is uninitialized' on line 420 even though 't1' is always initialized before the line. The original code works on clang version 3.4-1ubuntu3 (Ubuntu 14.04) and on gcc. This fixes bug: https://bugs.linaro.org/show_bug.cgi?id=2387 Signed-off-by: Matias Elo <matias.elo@nokia.com> --- platform/linux-generic/odp_schedule_sp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)