| Message ID | 1419334799-19653-2-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > CID 85006: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) > "handle.pool_id >= 16" is always false regardless of the values of > its operands. This occurs as the logical second operand of '||'. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/include/odp_buffer_inlines.h > b/platform/linux-generic/include/odp_buffer_inlines.h > index f880445..a28e1f1 100644 > --- a/platform/linux-generic/include/odp_buffer_inlines.h > +++ b/platform/linux-generic/include/odp_buffer_inlines.h > @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t > *validate_buf(odp_buffer_t buf) > odp_buffer_hdr_t *buf_hdr; > handle.u32 = buf; > > - /* For buffer handles, segment index must be 0 and pool id in > range */ > - if (handle.seg != 0 || handle.pool_id >= ODP_CONFIG_BUFFER_POOLS) > It is erroneous to delete this check. The coverity warning is a false positive that arises only in cases where ODP_CONFIG_BUFFER_POOLS happens to be a power of two, which is the case here but is not necessarily true. > + /* For buffer handles, segment index must be 0 */ > + if (handle.seg != 0) > return NULL; > > pool_entry_t *pool = odp_pool_to_entry(handle.pool_id); > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 12/23/2014 04:01 PM, Bill Fischofer wrote: > > > On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > CID 85006: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) > "handle.pool_id >= 16" is always false regardless of the values of > its operands. This occurs as the logical second operand of '||'. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/include/odp_buffer_inlines.h > b/platform/linux-generic/include/odp_buffer_inlines.h > index f880445..a28e1f1 100644 > --- a/platform/linux-generic/include/odp_buffer_inlines.h > +++ b/platform/linux-generic/include/odp_buffer_inlines.h > @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t > *validate_buf(odp_buffer_t buf) > odp_buffer_hdr_t *buf_hdr; > handle.u32 = buf; > > - /* For buffer handles, segment index must be 0 and pool id > in range */ > - if (handle.seg != 0 || handle.pool_id >= > ODP_CONFIG_BUFFER_POOLS) > > > It is erroneous to delete this check. The coverity warning is a false > positive that arises only in cases where ODP_CONFIG_BUFFER_POOLS > happens to be a power of two, which is the case here but is not > necessarily true. Bill, I think you might be not correct. If I understand core right then: uint32_t pool_id:ODP_BUFFER_POOL_BITS; #define ODP_CONFIG_BUFFER_POOLS 16 #define ODP_BUFFER_POOL_BITS ODP_BITSIZE(ODP_CONFIG_BUFFER_POOLS) And this macro: #define ODP_BITSIZE(x) \ ((x) <= 2 ? 1 : \ ((x) <= 4 ? 2 : \ ((x) <= 8 ? 3 : \ ((x) <= 16 ? 4 : \ ((x) <= 32 ? 5 : \ ((x) <= 64 ? 6 : \ ((x) <= 128 ? 7 : \ ((x) <= 256 ? 8 : \ ((x) <= 512 ? 9 : \ ((x) <= 1024 ? 10 : \ ((x) <= 2048 ? 11 : \ ((x) <= 4096 ? 12 : \ ((x) <= 8196 ? 13 : \ ((x) <= 16384 ? 14 : \ ((x) <= 32768 ? 15 : \ ((x) <= 65536 ? 16 : \ (0/0))))))))))))))))) So ODP_CONFIG_BUFFER_POOLS must be power of two or it will fall back to 0. Maxim. > + /* For buffer handles, segment index must be 0 */ > + if (handle.seg != 0) > return NULL; > > pool_entry_t *pool = odp_pool_to_entry(handle.pool_id); > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp >
The ODP_BITSIZE macro returns the number of bits needed to store x. For example, ODP_BITSIZE(14) = 4. So if someone specified ODP_CONFIG_BUFFER_POOLS at 14, then the check would ensure that we didn't receive a handle with a pool_id of 14 or 15 (which would be invalid). Because in this case ODP_CONFIG_BUFFER_POOLS is 16 then Coverity says (correctly) that the test can never be true since a 4 bit value can never be >= 16, however this is a false positive because the test isn't there for any specific value but for any value of ODP_CONFIG_BUFFER_POOLS. ODP doesn't require that this be specified as a power of two. Powers of two are fine for small numbers but problematic for larger ones, so no need to force this. The compiler is smart enough to know when the test can never be true and won't generate any additional code if ODP_CONFIG_BUFFER_POOLS is a power of two. But if it isn't a power of two then the code will be generated. So it's there when we need it and not there otherwise, which is exactly what we want. Coverity just needs to ignore this because it's doing simple static analysis and not seeing the bigger picture. On Wed, Dec 24, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/23/2014 04:01 PM, Bill Fischofer wrote: > >> >> >> On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> CID 85006: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) >> "handle.pool_id >= 16" is always false regardless of the values of >> its operands. This occurs as the logical second operand of '||'. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> --- >> platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/platform/linux-generic/include/odp_buffer_inlines.h >> b/platform/linux-generic/include/odp_buffer_inlines.h >> index f880445..a28e1f1 100644 >> --- a/platform/linux-generic/include/odp_buffer_inlines.h >> +++ b/platform/linux-generic/include/odp_buffer_inlines.h >> @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t >> *validate_buf(odp_buffer_t buf) >> odp_buffer_hdr_t *buf_hdr; >> handle.u32 = buf; >> >> - /* For buffer handles, segment index must be 0 and pool id >> in range */ >> - if (handle.seg != 0 || handle.pool_id >= >> ODP_CONFIG_BUFFER_POOLS) >> >> >> It is erroneous to delete this check. The coverity warning is a false >> positive that arises only in cases where ODP_CONFIG_BUFFER_POOLS happens to >> be a power of two, which is the case here but is not necessarily true. >> > > Bill, I think you might be not correct. If I understand core right then: > > uint32_t pool_id:ODP_BUFFER_POOL_BITS; > > #define ODP_CONFIG_BUFFER_POOLS 16 > #define ODP_BUFFER_POOL_BITS ODP_BITSIZE(ODP_CONFIG_BUFFER_POOLS) > > And this macro: > > #define ODP_BITSIZE(x) \ > ((x) <= 2 ? 1 : \ > ((x) <= 4 ? 2 : \ > ((x) <= 8 ? 3 : \ > ((x) <= 16 ? 4 : \ > ((x) <= 32 ? 5 : \ > ((x) <= 64 ? 6 : \ > ((x) <= 128 ? 7 : \ > ((x) <= 256 ? 8 : \ > ((x) <= 512 ? 9 : \ > ((x) <= 1024 ? 10 : \ > ((x) <= 2048 ? 11 : \ > ((x) <= 4096 ? 12 : \ > ((x) <= 8196 ? 13 : \ > ((x) <= 16384 ? 14 : \ > ((x) <= 32768 ? 15 : \ > ((x) <= 65536 ? 16 : \ > (0/0))))))))))))))))) > > So ODP_CONFIG_BUFFER_POOLS must be power of two or it will fall back to 0. > > Maxim. > > + /* For buffer handles, segment index must be 0 */ >> + if (handle.seg != 0) >> return NULL; >> >> pool_entry_t *pool = odp_pool_to_entry(handle.pool_id); >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >
On 12/24/2014 03:32 PM, Bill Fischofer wrote: > The ODP_BITSIZE macro returns the number of bits needed to store x. > For example, ODP_BITSIZE(14) = 4. So if someone specified > ODP_CONFIG_BUFFER_POOLS at 14, then the check would ensure that we > didn't receive a handle with a pool_id of 14 or 15 (which would be > invalid). Because in this case ODP_CONFIG_BUFFER_POOLS is 16 then > Coverity says (correctly) that the test can never be true since a 4 > bit value can never be >= 16, however this is a false positive because > the test isn't there for any specific value but for any value of > ODP_CONFIG_BUFFER_POOLS. ODP doesn't require that this be specified > as a power of two. Powers of two are fine for small numbers but > problematic for larger ones, so no need to force this. > > The compiler is smart enough to know when the test can never be true > and won't generate any additional code if ODP_CONFIG_BUFFER_POOLS is a > power of two. But if it isn't a power of two then the code will be > generated. So it's there when we need it and not there otherwise, > which is exactly what we want. Coverity just needs to ignore this > because it's doing simple static analysis and not seeing the bigger > picture. > Ok, thanks. Mike can you mark that as false positive? Maxim. > On Wed, Dec 24, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 12/23/2014 04:01 PM, Bill Fischofer wrote: > > > > On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > CID 85006: Operands don't affect result > (CONSTANT_EXPRESSION_RESULT) > "handle.pool_id >= 16" is always false regardless of > the values of > its operands. This occurs as the logical second > operand of '||'. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > --- > platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git > a/platform/linux-generic/include/odp_buffer_inlines.h > b/platform/linux-generic/include/odp_buffer_inlines.h > index f880445..a28e1f1 100644 > --- a/platform/linux-generic/include/odp_buffer_inlines.h > +++ b/platform/linux-generic/include/odp_buffer_inlines.h > @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t > *validate_buf(odp_buffer_t buf) > odp_buffer_hdr_t *buf_hdr; > handle.u32 = buf; > > - /* For buffer handles, segment index must be 0 and > pool id > in range */ > - if (handle.seg != 0 || handle.pool_id >= > ODP_CONFIG_BUFFER_POOLS) > > > It is erroneous to delete this check. The coverity warning is > a false positive that arises only in cases where > ODP_CONFIG_BUFFER_POOLS happens to be a power of two, which is > the case here but is not necessarily true. > > > Bill, I think you might be not correct. If I understand core right > then: > > uint32_t pool_id:ODP_BUFFER_POOL_BITS; > > #define ODP_CONFIG_BUFFER_POOLS 16 > #define ODP_BUFFER_POOL_BITS ODP_BITSIZE(ODP_CONFIG_BUFFER_POOLS) > > And this macro: > > #define ODP_BITSIZE(x) \ > ((x) <= 2 ? 1 : \ > ((x) <= 4 ? 2 : \ > ((x) <= 8 ? 3 : \ > ((x) <= 16 ? 4 : \ > ((x) <= 32 ? 5 : \ > ((x) <= 64 ? 6 : \ > ((x) <= 128 ? 7 : \ > ((x) <= 256 ? 8 : \ > ((x) <= 512 ? 9 : \ > ((x) <= 1024 ? 10 : \ > ((x) <= 2048 ? 11 : \ > ((x) <= 4096 ? 12 : \ > ((x) <= 8196 ? 13 : \ > ((x) <= 16384 ? 14 : \ > ((x) <= 32768 ? 15 : \ > ((x) <= 65536 ? 16 : \ > (0/0))))))))))))))))) > > So ODP_CONFIG_BUFFER_POOLS must be power of two or it will fall > back to 0. > > Maxim. > > + /* For buffer handles, segment index must be 0 */ > + if (handle.seg != 0) > return NULL; > > pool_entry_t *pool = > odp_pool_to_entry(handle.pool_id); > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > 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>> > http://lists.linaro.org/mailman/listinfo/lng-odp > > >
The exception mechanism is detailed here https://scan.coverity.com/models I will take a look and try to become a reference for how it works, but we probably want to follow a process where the excption model is extended by the owner of the code that needs it. On 24 December 2014 at 08:06, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/24/2014 03:32 PM, Bill Fischofer wrote: > >> The ODP_BITSIZE macro returns the number of bits needed to store x. For >> example, ODP_BITSIZE(14) = 4. So if someone specified >> ODP_CONFIG_BUFFER_POOLS at 14, then the check would ensure that we didn't >> receive a handle with a pool_id of 14 or 15 (which would be invalid). >> Because in this case ODP_CONFIG_BUFFER_POOLS is 16 then Coverity says >> (correctly) that the test can never be true since a 4 bit value can never >> be >= 16, however this is a false positive because the test isn't there for >> any specific value but for any value of ODP_CONFIG_BUFFER_POOLS. ODP >> doesn't require that this be specified as a power of two. Powers of two >> are fine for small numbers but problematic for larger ones, so no need to >> force this. >> >> The compiler is smart enough to know when the test can never be true and >> won't generate any additional code if ODP_CONFIG_BUFFER_POOLS is a power of >> two. But if it isn't a power of two then the code will be generated. So >> it's there when we need it and not there otherwise, which is exactly what >> we want. Coverity just needs to ignore this because it's doing simple >> static analysis and not seeing the bigger picture. >> >> Ok, thanks. > > Mike can you mark that as false positive? > > Maxim. > > > On Wed, Dec 24, 2014 at 5:39 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 12/23/2014 04:01 PM, Bill Fischofer wrote: >> >> >> >> On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> CID 85006: Operands don't affect result >> (CONSTANT_EXPRESSION_RESULT) >> "handle.pool_id >= 16" is always false regardless of >> the values of >> its operands. This occurs as the logical second >> operand of '||'. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> >> <mailto:maxim.uvarov@linaro.org>>> >> --- >> platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git >> a/platform/linux-generic/include/odp_buffer_inlines.h >> b/platform/linux-generic/include/odp_buffer_inlines.h >> index f880445..a28e1f1 100644 >> --- a/platform/linux-generic/include/odp_buffer_inlines.h >> +++ b/platform/linux-generic/include/odp_buffer_inlines.h >> @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t >> *validate_buf(odp_buffer_t buf) >> odp_buffer_hdr_t *buf_hdr; >> handle.u32 = buf; >> >> - /* For buffer handles, segment index must be 0 and >> pool id >> in range */ >> - if (handle.seg != 0 || handle.pool_id >= >> ODP_CONFIG_BUFFER_POOLS) >> >> >> It is erroneous to delete this check. The coverity warning is >> a false positive that arises only in cases where >> ODP_CONFIG_BUFFER_POOLS happens to be a power of two, which is >> the case here but is not necessarily true. >> >> >> Bill, I think you might be not correct. If I understand core right >> then: >> >> uint32_t pool_id:ODP_BUFFER_POOL_BITS; >> >> #define ODP_CONFIG_BUFFER_POOLS 16 >> #define ODP_BUFFER_POOL_BITS ODP_BITSIZE(ODP_CONFIG_BUFFER_POOLS) >> >> And this macro: >> >> #define ODP_BITSIZE(x) \ >> ((x) <= 2 ? 1 : \ >> ((x) <= 4 ? 2 : \ >> ((x) <= 8 ? 3 : \ >> ((x) <= 16 ? 4 : \ >> ((x) <= 32 ? 5 : \ >> ((x) <= 64 ? 6 : \ >> ((x) <= 128 ? 7 : \ >> ((x) <= 256 ? 8 : \ >> ((x) <= 512 ? 9 : \ >> ((x) <= 1024 ? 10 : \ >> ((x) <= 2048 ? 11 : \ >> ((x) <= 4096 ? 12 : \ >> ((x) <= 8196 ? 13 : \ >> ((x) <= 16384 ? 14 : \ >> ((x) <= 32768 ? 15 : \ >> ((x) <= 65536 ? 16 : \ >> (0/0))))))))))))))))) >> >> So ODP_CONFIG_BUFFER_POOLS must be power of two or it will fall >> back to 0. >> >> Maxim. >> >> + /* For buffer handles, segment index must be 0 */ >> + if (handle.seg != 0) >> return NULL; >> >> pool_entry_t *pool = >> odp_pool_to_entry(handle.pool_id); >> -- >> 1.8.5.1.163.gd7aced9 >> >> >> _______________________________________________ >> 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>> >> 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/include/odp_buffer_inlines.h b/platform/linux-generic/include/odp_buffer_inlines.h index f880445..a28e1f1 100644 --- a/platform/linux-generic/include/odp_buffer_inlines.h +++ b/platform/linux-generic/include/odp_buffer_inlines.h @@ -99,8 +99,8 @@ static inline odp_buffer_hdr_t *validate_buf(odp_buffer_t buf) odp_buffer_hdr_t *buf_hdr; handle.u32 = buf; - /* For buffer handles, segment index must be 0 and pool id in range */ - if (handle.seg != 0 || handle.pool_id >= ODP_CONFIG_BUFFER_POOLS) + /* For buffer handles, segment index must be 0 */ + if (handle.seg != 0) return NULL; pool_entry_t *pool = odp_pool_to_entry(handle.pool_id);
CID 85006: Operands don't affect result (CONSTANT_EXPRESSION_RESULT) "handle.pool_id >= 16" is always false regardless of the values of its operands. This occurs as the logical second operand of '||'. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/odp_buffer_inlines.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)