From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 102241C686 for ; Fri, 13 Apr 2018 18:09:03 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Apr 2018 09:09:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,446,1517904000"; d="scan'208";a="33464545" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.128]) ([10.237.220.128]) by orsmga008.jf.intel.com with ESMTP; 13 Apr 2018 09:09:02 -0700 To: Adrien Mazarguil Cc: dev@dpdk.org References: <20180413153749.28208-1-adrien.mazarguil@6wind.com> <20180413155417.29643-1-adrien.mazarguil@6wind.com> <20180413155417.29643-2-adrien.mazarguil@6wind.com> From: "Burakov, Anatoly" Message-ID: <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com> Date: Fri, 13 Apr 2018 17:09:01 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180413155417.29643-2-adrien.mazarguil@6wind.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 2/2] eal: fix signed integers in fbarray X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Apr 2018 16:09:05 -0000 On 13-Apr-18 4:56 PM, Adrien Mazarguil wrote: > While debugging startup issues encountered with Clang (see "eal: fix > undefined behavior in fbarray"), I noticed that fbarray stores indices, > sizes and masks on signed integers involved in bitwise operations. > > Such operations almost invariably cause undefined behavior with values that > cannot be represented by the result type, as is often the case with > bit-masks and left-shifts. > > This patch replaces them with unsigned integers as a safety measure and > promotes a few internal variables to larger types for consistency. > > Fixes: c44d09811b40 ("eal: add shared indexed file-backed array") > Cc: Anatoly Burakov > > Signed-off-by: Adrien Mazarguil > > -- > > v2 changes: > > Removed unnecessary "(unsigned int)" cast leftovers. Thanks for figuring this out! In general, i'm OK with the change, however... > --- > lib/librte_eal/common/eal_common_fbarray.c | 97 ++++++++++++------------ > lib/librte_eal/common/include/rte_fbarray.h | 33 ++++---- > 2 files changed, 68 insertions(+), 62 deletions(-) > > diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c > index 11aa3f22a..368290654 100644 > --- a/lib/librte_eal/common/eal_common_fbarray.c > +++ b/lib/librte_eal/common/eal_common_fbarray.c > @@ -21,7 +21,7 @@ > #include "rte_fbarray.h" > > #define MASK_SHIFT 6ULL > -#define MASK_ALIGN (1 << MASK_SHIFT) > +#define MASK_ALIGN (1ULL << MASK_SHIFT) > #define MASK_LEN_TO_IDX(x) ((x) >> MASK_SHIFT) > #define MASK_LEN_TO_MOD(x) ((x) - RTE_ALIGN_FLOOR(x, MASK_ALIGN)) > #define MASK_GET_IDX(idx, mod) ((idx << MASK_SHIFT) + mod) > @@ -32,12 +32,12 @@ > */ <...> > > int __rte_experimental > -rte_fbarray_find_next_free(struct rte_fbarray *arr, int start) > +rte_fbarray_find_next_free(struct rte_fbarray *arr, unsigned int start) > { This leads to inconsistency here. As it is, we can specify len and start value up to UINT32_MAX, but this (and others like it) function will only return values up to INT32_MAX. One way to fix this would be to prohibit len being >= INT32_MAX on array creation. The place to fix this would probably be fully_validate(). > int ret = -1; > > - if (arr == NULL || start < 0 || start >= arr->len) { > + if (arr == NULL || start >= arr->len) { > rte_errno = EINVAL; > return -1; > } > @@ -683,11 +686,11 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start) -- Thanks, Anatoly