| Message ID | 57060061.8070904@linaro.org |
|---|---|
| State | New |
| Headers | show |
After reviewing the code again, I think the patch stands as the simplest solution to this compiler bug. Both type and handle are needed because they are cached from the queue_entry while it is locked and then referenced after it is unlocked. A direct reference the internal fields is not legal unless the queue_entry lock is held. On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Mike, it think it's better to remove handle and type completely. And if > you already looking to that > function it might be reasonable to make it more readable, use > odp_config_queues() and use int > instead of uint32_t. Something like that: > > > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) > > odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t > *param) > { > - uint32_t i; > + int i; > queue_entry_t *queue; > - odp_queue_t handle = ODP_QUEUE_INVALID; > - odp_queue_type_t type; > odp_queue_param_t default_param; > > if (param == NULL) { > @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const > odp_queue_param_t *param) > param = &default_param; > } > > - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { > + for (i = 0; i < odp_config_queues(); i++) { > queue = &queue_tbl->queue[i]; > > if (queue->s.status != QUEUE_STATUS_FREE) > @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name, const > odp_queue_param_t *param) > > LOCK(&queue->s.lock); > if (queue->s.status == QUEUE_STATUS_FREE) { > - if (queue_init(queue, name, param)) { > + if (queue_init(queue, name, param) || > + queue->s.handle == ODP_QUEUE_INVALID) { > UNLOCK(&queue->s.lock); > - return handle; > + return ODP_QUEUE_INVALID; > } > > - type = queue->s.type; > - > - if (type == ODP_QUEUE_TYPE_SCHED) > + if (queue->s.type == ODP_QUEUE_TYPE_SCHED) { > queue->s.status = QUEUE_STATUS_NOTSCHED; > - else > + if (schedule_queue_init(queue)) { > + ODP_ERR("schedule queue init > failed\n"); > + UNLOCK(&queue->s.lock); > + return ODP_QUEUE_INVALID; > + } > + } else { > queue->s.status = QUEUE_STATUS_READY; > - > - handle = queue->s.handle; > + } > UNLOCK(&queue->s.lock); > - break; > + return queue->s.handle; > } > UNLOCK(&queue->s.lock); > } > > - if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) { > - if (schedule_queue_init(queue)) { > - ODP_ERR("schedule queue init failed\n"); > - return ODP_QUEUE_INVALID; > - } > - } > - > - return handle; > + return ODP_QUEUE_INVALID; > } > > Maxim. > > On 04/05/16 21:02, Mike Holmes wrote: > >> Any one object to this? >> It impacts the accuracy of the ABI testing tools if -Og cannot be used. >> >> On 1 April 2016 at 19:03, Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> wrote: >> >> Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an >> extraneous variable initialization to avoid a false positive error >> when >> compiling with gcc -Og >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> >> --- >> platform/linux-generic/odp_queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_queue.c >> b/platform/linux-generic/odp_queue.c >> index 342ffa2..5963057 100644 >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const char *name, >> const odp_queue_param_t *param) >> uint32_t i; >> queue_entry_t *queue; >> odp_queue_t handle = ODP_QUEUE_INVALID; >> - odp_queue_type_t type; >> + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; >> odp_queue_param_t default_param; >> >> if (param == NULL) { >> -- >> 2.5.0 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto: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 >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
ping. Maxim, I believe this patch is the simplest solution to the issue. On Fri, Apr 8, 2016 at 4:06 PM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > After reviewing the code again, I think the patch stands as the simplest > solution to this compiler bug. Both type and handle are needed because they > are cached from the queue_entry while it is locked and then referenced > after it is unlocked. A direct reference the internal fields is not legal > unless the queue_entry lock is held. > > On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >> Mike, it think it's better to remove handle and type completely. And if >> you already looking to that >> function it might be reasonable to make it more readable, use >> odp_config_queues() and use int >> instead of uint32_t. Something like that: >> >> >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) >> >> odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t >> *param) >> { >> - uint32_t i; >> + int i; >> queue_entry_t *queue; >> - odp_queue_t handle = ODP_QUEUE_INVALID; >> - odp_queue_type_t type; >> odp_queue_param_t default_param; >> >> if (param == NULL) { >> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const >> odp_queue_param_t *param) >> param = &default_param; >> } >> >> - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { >> + for (i = 0; i < odp_config_queues(); i++) { >> queue = &queue_tbl->queue[i]; >> >> if (queue->s.status != QUEUE_STATUS_FREE) >> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name, >> const odp_queue_param_t *param) >> >> LOCK(&queue->s.lock); >> if (queue->s.status == QUEUE_STATUS_FREE) { >> - if (queue_init(queue, name, param)) { >> + if (queue_init(queue, name, param) || >> + queue->s.handle == ODP_QUEUE_INVALID) { >> UNLOCK(&queue->s.lock); >> - return handle; >> + return ODP_QUEUE_INVALID; >> } >> >> - type = queue->s.type; >> - >> - if (type == ODP_QUEUE_TYPE_SCHED) >> + if (queue->s.type == ODP_QUEUE_TYPE_SCHED) { >> queue->s.status = QUEUE_STATUS_NOTSCHED; >> - else >> + if (schedule_queue_init(queue)) { >> + ODP_ERR("schedule queue init >> failed\n"); >> + UNLOCK(&queue->s.lock); >> + return ODP_QUEUE_INVALID; >> + } >> + } else { >> queue->s.status = QUEUE_STATUS_READY; >> - >> - handle = queue->s.handle; >> + } >> UNLOCK(&queue->s.lock); >> - break; >> + return queue->s.handle; >> } >> UNLOCK(&queue->s.lock); >> } >> >> - if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) { >> - if (schedule_queue_init(queue)) { >> - ODP_ERR("schedule queue init failed\n"); >> - return ODP_QUEUE_INVALID; >> - } >> - } >> - >> - return handle; >> + return ODP_QUEUE_INVALID; >> } >> >> Maxim. >> >> On 04/05/16 21:02, Mike Holmes wrote: >> >>> Any one object to this? >>> It impacts the accuracy of the ABI testing tools if -Og cannot be used. >>> >>> On 1 April 2016 at 19:03, Bill Fischofer <bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>> wrote: >>> >>> Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2159 by adding >>> an >>> extraneous variable initialization to avoid a false positive error >>> when >>> compiling with gcc -Og >>> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>> >>> --- >>> platform/linux-generic/odp_queue.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_queue.c >>> b/platform/linux-generic/odp_queue.c >>> index 342ffa2..5963057 100644 >>> --- a/platform/linux-generic/odp_queue.c >>> +++ b/platform/linux-generic/odp_queue.c >>> @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const char *name, >>> const odp_queue_param_t *param) >>> uint32_t i; >>> queue_entry_t *queue; >>> odp_queue_t handle = ODP_QUEUE_INVALID; >>> - odp_queue_type_t type; >>> + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; >>> odp_queue_param_t default_param; >>> >>> if (param == NULL) { >>> -- >>> 2.5.0 >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto: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 >>> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > >
On 04/14/16 15:45, Bill Fischofer wrote: > ping. Maxim, I believe this patch is the simplest solution to the issue. > Bill, this patch fixes existence problem with initializing variable. But it looks a little bit like a hack, where logically one function slitted on 2 pieces without any reason. (Or I do not see this reason.) Since nobody gave review yet, I will send my patch to list also. And we can merge mine or your. Maxim. > On Fri, Apr 8, 2016 at 4:06 PM, Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote: > > After reviewing the code again, I think the patch stands as the > simplest solution to this compiler bug. Both type and handle are > needed because they are cached from the queue_entry while it is > locked and then referenced after it is unlocked. A direct > reference the internal fields is not legal unless the queue_entry > lock is held. > > On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > Mike, it think it's better to remove handle and type > completely. And if you already looking to that > function it might be reasonable to make it more readable, use > odp_config_queues() and use int > instead of uint32_t. Something like that: > > > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) > > odp_queue_t odp_queue_create(const char *name, const > odp_queue_param_t *param) > { > - uint32_t i; > + int i; > queue_entry_t *queue; > - odp_queue_t handle = ODP_QUEUE_INVALID; > - odp_queue_type_t type; > odp_queue_param_t default_param; > > if (param == NULL) { > @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char > *name, const odp_queue_param_t *param) > param = &default_param; > } > > - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { > + for (i = 0; i < odp_config_queues(); i++) { > queue = &queue_tbl->queue[i]; > > if (queue->s.status != QUEUE_STATUS_FREE) > @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char > *name, const odp_queue_param_t *param) > > LOCK(&queue->s.lock); > if (queue->s.status == QUEUE_STATUS_FREE) { > - if (queue_init(queue, name, param)) { > + if (queue_init(queue, name, param) || > + queue->s.handle == > ODP_QUEUE_INVALID) { > UNLOCK(&queue->s.lock); > - return handle; > + return ODP_QUEUE_INVALID; > } > > - type = queue->s.type; > - > - if (type == ODP_QUEUE_TYPE_SCHED) > + if (queue->s.type == > ODP_QUEUE_TYPE_SCHED) { > queue->s.status = > QUEUE_STATUS_NOTSCHED; > - else > + if (schedule_queue_init(queue)) { > + ODP_ERR("schedule queue init failed\n"); > + UNLOCK(&queue->s.lock); > + return ODP_QUEUE_INVALID; > + } > + } else { > queue->s.status = > QUEUE_STATUS_READY; > - > - handle = queue->s.handle; > + } > UNLOCK(&queue->s.lock); > - break; > + return queue->s.handle; > } > UNLOCK(&queue->s.lock); > } > > - if (handle != ODP_QUEUE_INVALID && type == > ODP_QUEUE_TYPE_SCHED) { > - if (schedule_queue_init(queue)) { > - ODP_ERR("schedule queue init failed\n"); > - return ODP_QUEUE_INVALID; > - } > - } > - > - return handle; > + return ODP_QUEUE_INVALID; > } > > Maxim. > > On 04/05/16 21:02, Mike Holmes wrote: > > Any one object to this? > It impacts the accuracy of the ABI testing tools if -Og > cannot be used. > > On 1 April 2016 at 19:03, Bill Fischofer > <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>>> wrote: > > Resolve Bug > https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an > extraneous variable initialization to avoid a false > positive error > when > compiling with gcc -Og > > Signed-off-by: Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>>> > --- > platform/linux-generic/odp_queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_queue.c > b/platform/linux-generic/odp_queue.c > index 342ffa2..5963057 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const > char *name, > const odp_queue_param_t *param) > uint32_t i; > queue_entry_t *queue; > odp_queue_t handle = ODP_QUEUE_INVALID; > - odp_queue_type_t type; > + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; > odp_queue_param_t default_param; > > if (param == NULL) { > -- > 2.5.0 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto: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 <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > >
On Thu, Apr 14, 2016 at 11:34 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 04/14/16 15:45, Bill Fischofer wrote: > >> ping. Maxim, I believe this patch is the simplest solution to the issue. >> >> > Bill, this patch fixes existence problem with initializing variable. But > it looks a little bit like a hack, where > logically one function slitted on 2 pieces without any reason. (Or I do > not see this reason.) > Since nobody gave review yet, I will send my patch to list also. And we > can merge mine or your. > It is a hack to work around a GCC compiler bug. The variable it's complaining about can never be uninitialized and other optimization levels clearly see that but -Og does not. But it's a very efficient hack. :) > > Maxim. > > > On Fri, Apr 8, 2016 at 4:06 PM, Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> wrote: >> >> After reviewing the code again, I think the patch stands as the >> simplest solution to this compiler bug. Both type and handle are >> needed because they are cached from the queue_entry while it is >> locked and then referenced after it is unlocked. A direct >> reference the internal fields is not legal unless the queue_entry >> lock is held. >> >> On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >> >> Mike, it think it's better to remove handle and type >> completely. And if you already looking to that >> function it might be reasonable to make it more readable, use >> odp_config_queues() and use int >> instead of uint32_t. Something like that: >> >> >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) >> >> odp_queue_t odp_queue_create(const char *name, const >> odp_queue_param_t *param) >> { >> - uint32_t i; >> + int i; >> queue_entry_t *queue; >> - odp_queue_t handle = ODP_QUEUE_INVALID; >> - odp_queue_type_t type; >> odp_queue_param_t default_param; >> >> if (param == NULL) { >> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char >> *name, const odp_queue_param_t *param) >> param = &default_param; >> } >> >> - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { >> + for (i = 0; i < odp_config_queues(); i++) { >> queue = &queue_tbl->queue[i]; >> >> if (queue->s.status != QUEUE_STATUS_FREE) >> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char >> *name, const odp_queue_param_t *param) >> >> LOCK(&queue->s.lock); >> if (queue->s.status == QUEUE_STATUS_FREE) { >> - if (queue_init(queue, name, param)) { >> + if (queue_init(queue, name, param) || >> + queue->s.handle == >> ODP_QUEUE_INVALID) { >> UNLOCK(&queue->s.lock); >> - return handle; >> + return ODP_QUEUE_INVALID; >> } >> >> - type = queue->s.type; >> - >> - if (type == ODP_QUEUE_TYPE_SCHED) >> + if (queue->s.type == >> ODP_QUEUE_TYPE_SCHED) { >> queue->s.status = >> QUEUE_STATUS_NOTSCHED; >> - else >> + if (schedule_queue_init(queue)) { >> + ODP_ERR("schedule queue init failed\n"); >> + UNLOCK(&queue->s.lock); >> + return ODP_QUEUE_INVALID; >> + } >> + } else { >> queue->s.status = >> QUEUE_STATUS_READY; >> - >> - handle = queue->s.handle; >> + } >> UNLOCK(&queue->s.lock); >> - break; >> + return queue->s.handle; >> } >> UNLOCK(&queue->s.lock); >> } >> >> - if (handle != ODP_QUEUE_INVALID && type == >> ODP_QUEUE_TYPE_SCHED) { >> - if (schedule_queue_init(queue)) { >> - ODP_ERR("schedule queue init failed\n"); >> - return ODP_QUEUE_INVALID; >> - } >> - } >> - >> - return handle; >> + return ODP_QUEUE_INVALID; >> } >> >> Maxim. >> >> On 04/05/16 21:02, Mike Holmes wrote: >> >> Any one object to this? >> It impacts the accuracy of the ABI testing tools if -Og >> cannot be used. >> >> On 1 April 2016 at 19:03, Bill Fischofer >> <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org> >> <mailto:bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>>> wrote: >> >> Resolve Bug >> https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an >> extraneous variable initialization to avoid a false >> positive error >> when >> compiling with gcc -Og >> >> Signed-off-by: Bill Fischofer >> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> >> <mailto:bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>>> >> --- >> platform/linux-generic/odp_queue.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_queue.c >> b/platform/linux-generic/odp_queue.c >> index 342ffa2..5963057 100644 >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const >> char *name, >> const odp_queue_param_t *param) >> uint32_t i; >> queue_entry_t *queue; >> odp_queue_t handle = ODP_QUEUE_INVALID; >> - odp_queue_type_t type; >> + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; >> odp_queue_param_t default_param; >> >> if (param == NULL) { >> -- >> 2.5.0 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto: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 <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >
Is this not a reference to cases where we never pass through the following code ? if (queue->s.status == QUEUE_STATUS_FREE) is never true then type is never set. Can a bug elsewhere create such a case that leads to a cascading error that appears far from where it originated? On 14 April 2016 at 15:20, Bill Fischofer <bill.fischofer@linaro.org> wrote: > > > On Thu, Apr 14, 2016 at 11:34 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >> On 04/14/16 15:45, Bill Fischofer wrote: >> >>> ping. Maxim, I believe this patch is the simplest solution to the issue. >>> >>> >> Bill, this patch fixes existence problem with initializing variable. But >> it looks a little bit like a hack, where >> logically one function slitted on 2 pieces without any reason. (Or I do >> not see this reason.) >> Since nobody gave review yet, I will send my patch to list also. And we >> can merge mine or your. >> > > It is a hack to work around a GCC compiler bug. The variable it's > complaining about can never be uninitialized and other optimization levels > clearly see that but -Og does not. But it's a very efficient hack. :) > > >> >> Maxim. >> >> >> On Fri, Apr 8, 2016 at 4:06 PM, Bill Fischofer <bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>> wrote: >>> >>> After reviewing the code again, I think the patch stands as the >>> simplest solution to this compiler bug. Both type and handle are >>> needed because they are cached from the queue_entry while it is >>> locked and then referenced after it is unlocked. A direct >>> reference the internal fields is not legal unless the queue_entry >>> lock is held. >>> >>> On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov >>> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >>> >>> Mike, it think it's better to remove handle and type >>> completely. And if you already looking to that >>> function it might be reasonable to make it more readable, use >>> odp_config_queues() and use int >>> instead of uint32_t. Something like that: >>> >>> >>> --- a/platform/linux-generic/odp_queue.c >>> +++ b/platform/linux-generic/odp_queue.c >>> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) >>> >>> odp_queue_t odp_queue_create(const char *name, const >>> odp_queue_param_t *param) >>> { >>> - uint32_t i; >>> + int i; >>> queue_entry_t *queue; >>> - odp_queue_t handle = ODP_QUEUE_INVALID; >>> - odp_queue_type_t type; >>> odp_queue_param_t default_param; >>> >>> if (param == NULL) { >>> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char >>> *name, const odp_queue_param_t *param) >>> param = &default_param; >>> } >>> >>> - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { >>> + for (i = 0; i < odp_config_queues(); i++) { >>> queue = &queue_tbl->queue[i]; >>> >>> if (queue->s.status != QUEUE_STATUS_FREE) >>> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char >>> *name, const odp_queue_param_t *param) >>> >>> LOCK(&queue->s.lock); >>> if (queue->s.status == QUEUE_STATUS_FREE) { >>> - if (queue_init(queue, name, param)) { >>> + if (queue_init(queue, name, param) || >>> + queue->s.handle == >>> ODP_QUEUE_INVALID) { >>> UNLOCK(&queue->s.lock); >>> - return handle; >>> + return ODP_QUEUE_INVALID; >>> } >>> >>> - type = queue->s.type; >>> - >>> - if (type == ODP_QUEUE_TYPE_SCHED) >>> + if (queue->s.type == >>> ODP_QUEUE_TYPE_SCHED) { >>> queue->s.status = >>> QUEUE_STATUS_NOTSCHED; >>> - else >>> + if (schedule_queue_init(queue)) { >>> + ODP_ERR("schedule queue init failed\n"); >>> + UNLOCK(&queue->s.lock); >>> + return ODP_QUEUE_INVALID; >>> + } >>> + } else { >>> queue->s.status = >>> QUEUE_STATUS_READY; >>> - >>> - handle = queue->s.handle; >>> + } >>> UNLOCK(&queue->s.lock); >>> - break; >>> + return queue->s.handle; >>> } >>> UNLOCK(&queue->s.lock); >>> } >>> >>> - if (handle != ODP_QUEUE_INVALID && type == >>> ODP_QUEUE_TYPE_SCHED) { >>> - if (schedule_queue_init(queue)) { >>> - ODP_ERR("schedule queue init failed\n"); >>> - return ODP_QUEUE_INVALID; >>> - } >>> - } >>> - >>> - return handle; >>> + return ODP_QUEUE_INVALID; >>> } >>> >>> Maxim. >>> >>> On 04/05/16 21:02, Mike Holmes wrote: >>> >>> Any one object to this? >>> It impacts the accuracy of the ABI testing tools if -Og >>> cannot be used. >>> >>> On 1 April 2016 at 19:03, Bill Fischofer >>> <bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org> >>> <mailto:bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>>> wrote: >>> >>> Resolve Bug >>> https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an >>> extraneous variable initialization to avoid a false >>> positive error >>> when >>> compiling with gcc -Og >>> >>> Signed-off-by: Bill Fischofer >>> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org >>> > >>> <mailto:bill.fischofer@linaro.org >>> <mailto:bill.fischofer@linaro.org>>> >>> --- >>> platform/linux-generic/odp_queue.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_queue.c >>> b/platform/linux-generic/odp_queue.c >>> index 342ffa2..5963057 100644 >>> --- a/platform/linux-generic/odp_queue.c >>> +++ b/platform/linux-generic/odp_queue.c >>> @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const >>> char *name, >>> const odp_queue_param_t *param) >>> uint32_t i; >>> queue_entry_t *queue; >>> odp_queue_t handle = ODP_QUEUE_INVALID; >>> - odp_queue_type_t type; >>> + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; >>> odp_queue_param_t default_param; >>> >>> if (param == NULL) { >>> -- >>> 2.5.0 >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>> <mailto:lng-odp@lists.linaro.org >>> <mailto: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 <mailto:lng-odp@lists.linaro.org> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>> https://lists.linaro.org/mailman/listinfo/lng-odp >>> >>> >>> >>> >> > > _______________________________________________ > 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"
On Thu, Apr 14, 2016 at 3:14 PM, Mike Holmes <mike.holmes@linaro.org> wrote: > Is this not a reference to cases where we never pass through the following > code ? > > if (queue->s.status == QUEUE_STATUS_FREE) is never true then type is > never set. > > Can a bug elsewhere create such a case that leads to a cascading error > that appears far from where it originated? > > > No. The compiler is stating (falsely) that type may be uninitialized. But type is only ever referenced if handle != ODP_QUEUE_INVALID and since handle is initialized to ODP_QUEUE_INVALID the only way it gets set to a non-invalid value is if a queue entry is being processed in which case type is set to queue->s.type. GCC clearly sees this at other optimization levels. The bug is with -Og. > > > On 14 April 2016 at 15:20, Bill Fischofer <bill.fischofer@linaro.org> > wrote: > >> >> >> On Thu, Apr 14, 2016 at 11:34 AM, Maxim Uvarov <maxim.uvarov@linaro.org> >> wrote: >> >>> On 04/14/16 15:45, Bill Fischofer wrote: >>> >>>> ping. Maxim, I believe this patch is the simplest solution to the >>>> issue. >>>> >>>> >>> Bill, this patch fixes existence problem with initializing variable. >>> But it looks a little bit like a hack, where >>> logically one function slitted on 2 pieces without any reason. (Or I do >>> not see this reason.) >>> Since nobody gave review yet, I will send my patch to list also. And we >>> can merge mine or your. >>> >> >> It is a hack to work around a GCC compiler bug. The variable it's >> complaining about can never be uninitialized and other optimization levels >> clearly see that but -Og does not. But it's a very efficient hack. :) >> >> >>> >>> Maxim. >>> >>> >>> On Fri, Apr 8, 2016 at 4:06 PM, Bill Fischofer < >>>> bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org>> wrote: >>>> >>>> After reviewing the code again, I think the patch stands as the >>>> simplest solution to this compiler bug. Both type and handle are >>>> needed because they are cached from the queue_entry while it is >>>> locked and then referenced after it is unlocked. A direct >>>> reference the internal fields is not legal unless the queue_entry >>>> lock is held. >>>> >>>> On Thu, Apr 7, 2016 at 1:38 AM, Maxim Uvarov >>>> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: >>>> >>>> Mike, it think it's better to remove handle and type >>>> completely. And if you already looking to that >>>> function it might be reasonable to make it more readable, use >>>> odp_config_queues() and use int >>>> instead of uint32_t. Something like that: >>>> >>>> >>>> --- a/platform/linux-generic/odp_queue.c >>>> +++ b/platform/linux-generic/odp_queue.c >>>> @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t >>>> handle) >>>> >>>> odp_queue_t odp_queue_create(const char *name, const >>>> odp_queue_param_t *param) >>>> { >>>> - uint32_t i; >>>> + int i; >>>> queue_entry_t *queue; >>>> - odp_queue_t handle = ODP_QUEUE_INVALID; >>>> - odp_queue_type_t type; >>>> odp_queue_param_t default_param; >>>> >>>> if (param == NULL) { >>>> @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char >>>> *name, const odp_queue_param_t *param) >>>> param = &default_param; >>>> } >>>> >>>> - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { >>>> + for (i = 0; i < odp_config_queues(); i++) { >>>> queue = &queue_tbl->queue[i]; >>>> >>>> if (queue->s.status != QUEUE_STATUS_FREE) >>>> @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char >>>> *name, const odp_queue_param_t *param) >>>> >>>> LOCK(&queue->s.lock); >>>> if (queue->s.status == QUEUE_STATUS_FREE) { >>>> - if (queue_init(queue, name, param)) { >>>> + if (queue_init(queue, name, param) || >>>> + queue->s.handle == >>>> ODP_QUEUE_INVALID) { >>>> UNLOCK(&queue->s.lock); >>>> - return handle; >>>> + return ODP_QUEUE_INVALID; >>>> } >>>> >>>> - type = queue->s.type; >>>> - >>>> - if (type == ODP_QUEUE_TYPE_SCHED) >>>> + if (queue->s.type == >>>> ODP_QUEUE_TYPE_SCHED) { >>>> queue->s.status = >>>> QUEUE_STATUS_NOTSCHED; >>>> - else >>>> + if (schedule_queue_init(queue)) >>>> { >>>> + ODP_ERR("schedule queue init failed\n"); >>>> + UNLOCK(&queue->s.lock); >>>> + return >>>> ODP_QUEUE_INVALID; >>>> + } >>>> + } else { >>>> queue->s.status = >>>> QUEUE_STATUS_READY; >>>> - >>>> - handle = queue->s.handle; >>>> + } >>>> UNLOCK(&queue->s.lock); >>>> - break; >>>> + return queue->s.handle; >>>> } >>>> UNLOCK(&queue->s.lock); >>>> } >>>> >>>> - if (handle != ODP_QUEUE_INVALID && type == >>>> ODP_QUEUE_TYPE_SCHED) { >>>> - if (schedule_queue_init(queue)) { >>>> - ODP_ERR("schedule queue init failed\n"); >>>> - return ODP_QUEUE_INVALID; >>>> - } >>>> - } >>>> - >>>> - return handle; >>>> + return ODP_QUEUE_INVALID; >>>> } >>>> >>>> Maxim. >>>> >>>> On 04/05/16 21:02, Mike Holmes wrote: >>>> >>>> Any one object to this? >>>> It impacts the accuracy of the ABI testing tools if -Og >>>> cannot be used. >>>> >>>> On 1 April 2016 at 19:03, Bill Fischofer >>>> <bill.fischofer@linaro.org >>>> <mailto:bill.fischofer@linaro.org> >>>> <mailto:bill.fischofer@linaro.org >>>> <mailto:bill.fischofer@linaro.org>>> wrote: >>>> >>>> Resolve Bug >>>> https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an >>>> extraneous variable initialization to avoid a false >>>> positive error >>>> when >>>> compiling with gcc -Og >>>> >>>> Signed-off-by: Bill Fischofer >>>> <bill.fischofer@linaro.org <mailto: >>>> bill.fischofer@linaro.org> >>>> <mailto:bill.fischofer@linaro.org >>>> <mailto:bill.fischofer@linaro.org>>> >>>> --- >>>> platform/linux-generic/odp_queue.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/platform/linux-generic/odp_queue.c >>>> b/platform/linux-generic/odp_queue.c >>>> index 342ffa2..5963057 100644 >>>> --- a/platform/linux-generic/odp_queue.c >>>> +++ b/platform/linux-generic/odp_queue.c >>>> @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const >>>> char *name, >>>> const odp_queue_param_t *param) >>>> uint32_t i; >>>> queue_entry_t *queue; >>>> odp_queue_t handle = ODP_QUEUE_INVALID; >>>> - odp_queue_type_t type; >>>> + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; >>>> odp_queue_param_t default_param; >>>> >>>> if (param == NULL) { >>>> -- >>>> 2.5.0 >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>>> <mailto:lng-odp@lists.linaro.org >>>> <mailto: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 <mailto:lng-odp@lists.linaro.org> >>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >>>> https://lists.linaro.org/mailman/listinfo/lng-odp >>>> >>>> >>>> >>>> >>> >> >> _______________________________________________ >> 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" > > >
--- a/platform/linux-generic/odp_queue.c +++ b/platform/linux-generic/odp_queue.c @@ -234,10 +234,8 @@ int odp_queue_lock_count(odp_queue_t handle) odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param) { - uint32_t i; + int i; queue_entry_t *queue; - odp_queue_t handle = ODP_QUEUE_INVALID; - odp_queue_type_t type; odp_queue_param_t default_param;
Mike, it think it's better to remove handle and type completely. And if you already looking to that function it might be reasonable to make it more readable, use odp_config_queues() and use int instead of uint32_t. Something like that: if (param == NULL) { @@ -245,7 +243,7 @@ odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param) param = &default_param; } - for (i = 0; i < ODP_CONFIG_QUEUES; i++) { + for (i = 0; i < odp_config_queues(); i++) { queue = &queue_tbl->queue[i]; if (queue->s.status != QUEUE_STATUS_FREE) @@ -253,33 +251,29 @@ odp_queue_t odp_queue_create(const char *name, const odp_queue_param_t *param) LOCK(&queue->s.lock); if (queue->s.status == QUEUE_STATUS_FREE) { - if (queue_init(queue, name, param)) { + if (queue_init(queue, name, param) || + queue->s.handle == ODP_QUEUE_INVALID) { UNLOCK(&queue->s.lock); - return handle; + return ODP_QUEUE_INVALID; } - type = queue->s.type; - - if (type == ODP_QUEUE_TYPE_SCHED) + if (queue->s.type == ODP_QUEUE_TYPE_SCHED) { queue->s.status = QUEUE_STATUS_NOTSCHED; - else + if (schedule_queue_init(queue)) { + ODP_ERR("schedule queue init failed\n"); + UNLOCK(&queue->s.lock); + return ODP_QUEUE_INVALID; + } + } else { queue->s.status = QUEUE_STATUS_READY; - - handle = queue->s.handle; + } UNLOCK(&queue->s.lock); - break; + return queue->s.handle; } UNLOCK(&queue->s.lock); } - if (handle != ODP_QUEUE_INVALID && type == ODP_QUEUE_TYPE_SCHED) { - if (schedule_queue_init(queue)) { - ODP_ERR("schedule queue init failed\n"); - return ODP_QUEUE_INVALID; - } - } - - return handle; + return ODP_QUEUE_INVALID; } Maxim. On 04/05/16 21:02, Mike Holmes wrote: > Any one object to this? > It impacts the accuracy of the ABI testing tools if -Og cannot be used. > > On 1 April 2016 at 19:03, Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> wrote: > > Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2159 by adding an > extraneous variable initialization to avoid a false positive error > when > compiling with gcc -Og > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > --- > platform/linux-generic/odp_queue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_queue.c > b/platform/linux-generic/odp_queue.c > index 342ffa2..5963057 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -237,7 +237,7 @@ odp_queue_t odp_queue_create(const char *name, > const odp_queue_param_t *param) > uint32_t i; > queue_entry_t *queue; > odp_queue_t handle = ODP_QUEUE_INVALID; > - odp_queue_type_t type; > + odp_queue_type_t type = ODP_QUEUE_TYPE_PLAIN; > odp_queue_param_t default_param; > > if (param == NULL) { > -- > 2.5.0 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto: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