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 <[email protected]> wrote: > On 12/23/2014 04:01 PM, Bill Fischofer wrote: > >> >> >> On Tue, Dec 23, 2014 at 5:39 AM, Maxim Uvarov <[email protected] >> <mailto:[email protected]>> 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 <[email protected] >> <mailto:[email protected]>> >> --- >> 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 >> [email protected] <mailto:[email protected]> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
