From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by dpdk.org (Postfix) with ESMTP id E8EB31C6D5 for ; Fri, 13 Apr 2018 19:54:17 +0200 (CEST) Received: by mail-wr0-f193.google.com with SMTP id z73so10181522wrb.0 for ; Fri, 13 Apr 2018 10:54:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ea7JpMeVMNbaFX4v5IKWRdvp3IKWXcfRT299pcfAZII=; b=DiNoXtFfJtke76uaxYv0SIOI1qduK0clQzAL7kLTKhbBhEADuc14vE/GgwcVEK8y6T ckNxnebyBNIbDiBa3/cYTK/6u5zwv9z8Vmbr8s7Q7fKRXISrj5cg3tPglzEIeME8DRK5 UxJ9CddEjnDjlr4kwVfSqeQ8pajzyRffHSf+opP7J2aJBPwmqVj3UHOg7DQlR9MlGgBk bc7asa2NaEl8ZWR8Nx7R12p+7h5UrnEi1T6VtUl/JxtosvBCvEH0GZOtpLmzQfli7zBc sZYxaDpf5iY1vWehvn0Y1jZpsUTpl6Iue12sN8KaOMVN7ASP1ZNGQOpnDit8DTa8YGED s6CQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ea7JpMeVMNbaFX4v5IKWRdvp3IKWXcfRT299pcfAZII=; b=latE2jM+EYfjqsZ0fcR+DBJWIJ+/7REeAcr7u6wd9D/L9at1PXCaQ/4jXVaKKHjrKC TYTkDSADNfRTaawGO3lbXVgRd/9cLOcUaJ/wIIKEA7u6FsCKhy8N9RUJ2O7W7ZB0lcLG COVHUF9PpuH6W8DUSUBhPARl2IgsFZeE60JoHRK8lgD7rfRSKLBnlAAFr0Jeg+k/TETN XlqQcBWDgRpUPooyVirmALJY+i2tToqXoW5ZDuPSD69fuA6/leUq8xLyebTi2aWumKoR 8PBIJ4gnsi4pSTbXNeCHXhvee1MMXFYpZBqylQzPIJTgBu6LofFYm7Uzd4mhic5yuQMo 0N/g== X-Gm-Message-State: ALQs6tAbhQUXARokKUNfCAn5cDdmsWK+6EyiyKjoQQyjpZNZvuc6Gv+/ tRc2v5FhcJgdVe1wi1Bwh7FN9/pM X-Google-Smtp-Source: AIpwx4+O8cKLcmZMzbO9d4yjCSH+cLiq77loVwWwEGeOjHDZXzkVMQL3TgitNKXxFIPK7ZF9jEIHKA== X-Received: by 10.223.150.7 with SMTP id b7mr1381526wra.129.1523642057447; Fri, 13 Apr 2018 10:54:17 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 63sm1429742wrn.12.2018.04.13.10.54.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 13 Apr 2018 10:54:16 -0700 (PDT) Date: Fri, 13 Apr 2018 19:54:03 +0200 From: Adrien Mazarguil To: "Burakov, Anatoly" Cc: dev@dpdk.org Message-ID: <20180413175403.GT4957@6wind.com> References: <20180413153749.28208-1-adrien.mazarguil@6wind.com> <20180413155417.29643-1-adrien.mazarguil@6wind.com> <20180413155417.29643-2-adrien.mazarguil@6wind.com> <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <25801a3b-8c0d-26fb-aa72-40bbd02adbb4@intel.com> 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 17:54:18 -0000 On Fri, Apr 13, 2018 at 05:09:01PM +0100, Burakov, Anatoly wrote: > 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(). Indeed, also I just received a Coverity report about a bunch of other details due to these changes (now it's obvious that calc_data_size() doesn't support negative page sizes), I'll update the patch accordingly and submit v3. Thanks. > > > 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 -- Adrien Mazarguil 6WIND