[PATCH 1/2] eal: fix undefined behavior in fbarray

[PATCH 1/2] eal: fix undefined behavior in fbarray

[Adrien Mazarguil]

According to GCC documentation [1], the __builtin_clz() family of functions
yield undefined behavior when fed a zero value. There is one instance in
the fbarray code where this can occur.

Clang (at least version 3.8.0-2ubuntu4) seems much more sensitive to this
than GCC and yields random results when compiling optimized code, as shown
below:

 #include <stdio.h>

 int main(void)
 {
         volatile unsigned long long moo;
         int x;

         moo = 0;
         x = __builtin_clzll(moo);
         printf("%d\n", x);
         return 0;
 }

 $ gcc -O3 -o test test.c && ./test
 63
 $ clang -O3 -o test test.c && ./test
 1742715559
 $ clang -O0 -o test test.c && ./test
 63

Even 63 can be considered an unexpected result given the number of leading
zeroes should be the full width of the underlying type, i.e. 64.

In practice it causes find_next_n() to sometimes return negative values
interpreted as errors by caller functions, which prevents DPDK applications
from starting due to inability to find free memory segments:

 # testpmd [...]
 EAL: Detected 32 lcore(s)
 EAL: Detected 2 NUMA nodes
 EAL: No free hugepages reported in hugepages-1048576kB
 EAL: Multi-process socket /var/run/.rte_unix
 EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list
 EAL: FATAL: Cannot init memory

 EAL: Cannot init memory

 PANIC in main():
 Cannot init EAL
 4: [./build/app/testpmd(_start+0x29) [0x462289]]
 3: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)
     [0x7f19d54fc830]]
 2: [./build/app/testpmd(main+0x8a3) [0x466193]]
 1: [./build/app/testpmd(__rte_panic+0xd6) [0x4efaa6]]
 Aborted

This problem appears with commit 66cc45e293ed ("mem: replace memseg with
memseg lists") however the root cause is introduced by a prior patch.

[1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: Anatoly Burakov <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index f65875dd9..11aa3f22a 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -173,7 +173,10 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		 */
 
 		/* count leading zeroes on inverted mask */
-		clz = __builtin_clzll(~cur_msk);
+		if (~cur_msk == 0)
+			clz = sizeof(cur_msk) * 8;
+		else
+			clz = __builtin_clzll(~cur_msk);
 
 		/* if there aren't any runs at the end either, just continue */
 		if (clz == 0)
-- 
2.11.0

[PATCH 2/2] eal: fix signed integers in fbarray

[Adrien Mazarguil]

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 <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_eal/common/eal_common_fbarray.c  | 101 ++++++++++++-----------
 lib/librte_eal/common/include/rte_fbarray.h |  33 ++++----
 2 files changed, 70 insertions(+), 64 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 11aa3f22a..26c88be88 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 @@
  */
 
 struct used_mask {
-	int n_masks;
+	unsigned int n_masks;
 	uint64_t data[];
 };
 
 static size_t
-calc_mask_size(int len)
+calc_mask_size(unsigned int len)
 {
 	/* mask must be multiple of MASK_ALIGN, even though length of array
 	 * itself may not be aligned on that boundary.
@@ -48,7 +48,7 @@ calc_mask_size(int len)
 }
 
 static size_t
-calc_data_size(size_t page_sz, int elt_sz, int len)
+calc_data_size(size_t page_sz, unsigned int elt_sz, unsigned int len)
 {
 	size_t data_sz = elt_sz * len;
 	size_t msk_sz = calc_mask_size(len);
@@ -56,7 +56,7 @@ calc_data_size(size_t page_sz, int elt_sz, int len)
 }
 
 static struct used_mask *
-get_used_mask(void *data, int elt_sz, int len)
+get_used_mask(void *data, unsigned int elt_sz, unsigned int len)
 {
 	return (struct used_mask *) RTE_PTR_ADD(data, elt_sz * len);
 }
@@ -86,13 +86,14 @@ resize_and_map(int fd, void *addr, size_t len)
 }
 
 static int
-find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
+find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
+	    bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int msk_idx, lookahead_idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int msk_idx, lookahead_idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -108,11 +109,11 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 	 */
 	last = MASK_LEN_TO_IDX(arr->len);
 	last_mod = MASK_LEN_TO_MOD(arr->len);
-	last_msk = ~(-(1ULL) << last_mod);
+	last_msk = ~(-1ULL << last_mod);
 
 	for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
 		uint64_t cur_msk, lookahead_msk;
-		int run_start, clz, left;
+		unsigned int run_start, clz, left;
 		bool found = false;
 		/*
 		 * The process of getting n consecutive bits for arbitrary n is
@@ -155,9 +156,9 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		}
 
 		/* if n can fit in within a single mask, do a search */
-		if (n <= MASK_ALIGN) {
+		if ((unsigned int)n <= MASK_ALIGN) {
 			uint64_t tmp_msk = cur_msk;
-			int s_idx;
+			unsigned int s_idx;
 			for (s_idx = 0; s_idx < n - 1; s_idx++)
 				tmp_msk &= tmp_msk >> 1ULL;
 			/* we found what we were looking for */
@@ -188,7 +189,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 
 		for (lookahead_idx = msk_idx + 1; lookahead_idx < msk->n_masks;
 				lookahead_idx++) {
-			int s_idx, need;
+			unsigned int s_idx, need;
 			lookahead_msk = msk->data[lookahead_idx];
 
 			/* if we're looking for free space, invert the mask */
@@ -196,7 +197,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 				lookahead_msk = ~lookahead_msk;
 
 			/* figure out how many consecutive bits we need here */
-			need = RTE_MIN(left, MASK_ALIGN);
+			need = RTE_MIN((unsigned int)left, MASK_ALIGN);
 
 			for (s_idx = 0; s_idx < need - 1; s_idx++)
 				lookahead_msk &= lookahead_msk >> 1ULL;
@@ -234,13 +235,13 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 }
 
 static int
-find_next(const struct rte_fbarray *arr, int start, bool used)
+find_next(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -290,13 +291,14 @@ find_next(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-find_contig(const struct rte_fbarray *arr, int start, bool used)
+find_contig(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	int need_len, result = 0;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk;
+	unsigned int need_len, result = 0;
 
 	/* array length may not be aligned, so calculate ignore mask for last
 	 * mask index.
@@ -309,7 +311,7 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 	first_mod = MASK_LEN_TO_MOD(start);
 	for (idx = first; idx < msk->n_masks; idx++, result += need_len) {
 		uint64_t cur = msk->data[idx];
-		int run_len;
+		unsigned int run_len;
 
 		need_len = MASK_ALIGN;
 
@@ -350,15 +352,15 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-set_used(struct rte_fbarray *arr, int idx, bool used)
+set_used(struct rte_fbarray *arr, unsigned int idx, bool used)
 {
 	struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz, arr->len);
 	uint64_t msk_bit = 1ULL << MASK_LEN_TO_MOD(idx);
-	int msk_idx = MASK_LEN_TO_IDX(idx);
+	unsigned int msk_idx = MASK_LEN_TO_IDX(idx);
 	bool already_used;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -402,7 +404,8 @@ fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
 }
 
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len, int elt_sz)
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz)
 {
 	size_t page_sz, mmap_len;
 	char path[PATH_MAX];
@@ -601,10 +604,10 @@ rte_fbarray_destroy(struct rte_fbarray *arr)
 }
 
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx)
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx)
 {
 	void *ret = NULL;
-	if (arr == NULL || idx < 0) {
+	if (arr == NULL) {
 		rte_errno = EINVAL;
 		return NULL;
 	}
@@ -620,26 +623,26 @@ rte_fbarray_get(const struct rte_fbarray *arr, int idx)
 }
 
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, true);
 }
 
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, false);
 }
 
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	struct used_mask *msk;
 	int msk_idx;
 	uint64_t msk_bit;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -659,11 +662,11 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
 }
 
 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)
 {
 	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)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -707,12 +710,12 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -732,12 +735,12 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -757,11 +760,11 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_free(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -786,11 +789,11 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -834,7 +837,7 @@ void __rte_experimental
 rte_fbarray_dump_metadata(struct rte_fbarray *arr, FILE *f)
 {
 	struct used_mask *msk;
-	int i;
+	unsigned int i;
 
 	if (arr == NULL || f == NULL) {
 		rte_errno = EINVAL;
diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
index 97df945ea..3e61fffee 100644
--- a/lib/librte_eal/common/include/rte_fbarray.h
+++ b/lib/librte_eal/common/include/rte_fbarray.h
@@ -44,9 +44,9 @@ extern "C" {
 
 struct rte_fbarray {
 	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
-	int count;                       /**< number of entries stored */
-	int len;                         /**< current length of the array */
-	int elt_sz;                      /**< size of each element */
+	unsigned int count;              /**< number of entries stored */
+	unsigned int len;                /**< current length of the array */
+	unsigned int elt_sz;             /**< size of each element */
 	void *data;                      /**< data pointer */
 	rte_rwlock_t rwlock;             /**< multiprocess lock */
 };
@@ -76,8 +76,8 @@ struct rte_fbarray {
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len,
-		int elt_sz);
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz);
 
 
 /**
@@ -154,7 +154,7 @@ rte_fbarray_detach(struct rte_fbarray *arr);
  *  - NULL on failure, with ``rte_errno`` indicating reason for failure.
  */
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx);
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -188,7 +188,7 @@ rte_fbarray_find_idx(const struct rte_fbarray *arr, const void *elt);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -205,7 +205,7 @@ rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -223,7 +223,7 @@ rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -240,7 +240,7 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 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);
 
 
 /**
@@ -257,7 +257,7 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
@@ -277,7 +277,8 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -297,7 +298,8 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -314,7 +316,8 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_free(struct rte_fbarray *arr,
+		unsigned int start);
 
 
 /**
@@ -331,7 +334,7 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
-- 
2.11.0

[PATCH v2 1/2] eal: fix undefined behavior in fbarray

[Adrien Mazarguil]

According to GCC documentation [1], the __builtin_clz() family of functions
yield undefined behavior when fed a zero value. There is one instance in
the fbarray code where this can occur.

Clang (at least version 3.8.0-2ubuntu4) seems much more sensitive to this
than GCC and yields random results when compiling optimized code, as shown
below:

 #include <stdio.h>

 int main(void)
 {
         volatile unsigned long long moo;
         int x;

         moo = 0;
         x = __builtin_clzll(moo);
         printf("%d\n", x);
         return 0;
 }

 $ gcc -O3 -o test test.c && ./test
 63
 $ clang -O3 -o test test.c && ./test
 1742715559
 $ clang -O0 -o test test.c && ./test
 63

Even 63 can be considered an unexpected result given the number of leading
zeroes should be the full width of the underlying type, i.e. 64.

In practice it causes find_next_n() to sometimes return negative values
interpreted as errors by caller functions, which prevents DPDK applications
from starting due to inability to find free memory segments:

 # testpmd [...]
 EAL: Detected 32 lcore(s)
 EAL: Detected 2 NUMA nodes
 EAL: No free hugepages reported in hugepages-1048576kB
 EAL: Multi-process socket /var/run/.rte_unix
 EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list
 EAL: FATAL: Cannot init memory

 EAL: Cannot init memory

 PANIC in main():
 Cannot init EAL
 4: [./build/app/testpmd(_start+0x29) [0x462289]]
 3: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)
     [0x7f19d54fc830]]
 2: [./build/app/testpmd(main+0x8a3) [0x466193]]
 1: [./build/app/testpmd(__rte_panic+0xd6) [0x4efaa6]]
 Aborted

This problem appears with commit 66cc45e293ed ("mem: replace memseg with
memseg lists") however the root cause is introduced by a prior patch.

[1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: Anatoly Burakov <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index f65875dd9..11aa3f22a 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -173,7 +173,10 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		 */
 
 		/* count leading zeroes on inverted mask */
-		clz = __builtin_clzll(~cur_msk);
+		if (~cur_msk == 0)
+			clz = sizeof(cur_msk) * 8;
+		else
+			clz = __builtin_clzll(~cur_msk);
 
 		/* if there aren't any runs at the end either, just continue */
 		if (clz == 0)
-- 
2.11.0

[PATCH v2 2/2] eal: fix signed integers in fbarray

[Adrien Mazarguil]

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 <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

--

v2 changes:

Removed unnecessary "(unsigned int)" cast leftovers.
---
 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 @@
  */
 
 struct used_mask {
-	int n_masks;
+	unsigned int n_masks;
 	uint64_t data[];
 };
 
 static size_t
-calc_mask_size(int len)
+calc_mask_size(unsigned int len)
 {
 	/* mask must be multiple of MASK_ALIGN, even though length of array
 	 * itself may not be aligned on that boundary.
@@ -48,7 +48,7 @@ calc_mask_size(int len)
 }
 
 static size_t
-calc_data_size(size_t page_sz, int elt_sz, int len)
+calc_data_size(size_t page_sz, unsigned int elt_sz, unsigned int len)
 {
 	size_t data_sz = elt_sz * len;
 	size_t msk_sz = calc_mask_size(len);
@@ -56,7 +56,7 @@ calc_data_size(size_t page_sz, int elt_sz, int len)
 }
 
 static struct used_mask *
-get_used_mask(void *data, int elt_sz, int len)
+get_used_mask(void *data, unsigned int elt_sz, unsigned int len)
 {
 	return (struct used_mask *) RTE_PTR_ADD(data, elt_sz * len);
 }
@@ -86,13 +86,14 @@ resize_and_map(int fd, void *addr, size_t len)
 }
 
 static int
-find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
+find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
+	    bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int msk_idx, lookahead_idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int msk_idx, lookahead_idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -108,11 +109,11 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 	 */
 	last = MASK_LEN_TO_IDX(arr->len);
 	last_mod = MASK_LEN_TO_MOD(arr->len);
-	last_msk = ~(-(1ULL) << last_mod);
+	last_msk = ~(-1ULL << last_mod);
 
 	for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
 		uint64_t cur_msk, lookahead_msk;
-		int run_start, clz, left;
+		unsigned int run_start, clz, left;
 		bool found = false;
 		/*
 		 * The process of getting n consecutive bits for arbitrary n is
@@ -157,7 +158,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		/* if n can fit in within a single mask, do a search */
 		if (n <= MASK_ALIGN) {
 			uint64_t tmp_msk = cur_msk;
-			int s_idx;
+			unsigned int s_idx;
 			for (s_idx = 0; s_idx < n - 1; s_idx++)
 				tmp_msk &= tmp_msk >> 1ULL;
 			/* we found what we were looking for */
@@ -188,7 +189,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 
 		for (lookahead_idx = msk_idx + 1; lookahead_idx < msk->n_masks;
 				lookahead_idx++) {
-			int s_idx, need;
+			unsigned int s_idx, need;
 			lookahead_msk = msk->data[lookahead_idx];
 
 			/* if we're looking for free space, invert the mask */
@@ -234,13 +235,13 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 }
 
 static int
-find_next(const struct rte_fbarray *arr, int start, bool used)
+find_next(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -290,13 +291,14 @@ find_next(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-find_contig(const struct rte_fbarray *arr, int start, bool used)
+find_contig(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	int need_len, result = 0;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk;
+	unsigned int need_len, result = 0;
 
 	/* array length may not be aligned, so calculate ignore mask for last
 	 * mask index.
@@ -309,7 +311,7 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 	first_mod = MASK_LEN_TO_MOD(start);
 	for (idx = first; idx < msk->n_masks; idx++, result += need_len) {
 		uint64_t cur = msk->data[idx];
-		int run_len;
+		unsigned int run_len;
 
 		need_len = MASK_ALIGN;
 
@@ -350,15 +352,15 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-set_used(struct rte_fbarray *arr, int idx, bool used)
+set_used(struct rte_fbarray *arr, unsigned int idx, bool used)
 {
 	struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz, arr->len);
 	uint64_t msk_bit = 1ULL << MASK_LEN_TO_MOD(idx);
-	int msk_idx = MASK_LEN_TO_IDX(idx);
+	unsigned int msk_idx = MASK_LEN_TO_IDX(idx);
 	bool already_used;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -402,7 +404,8 @@ fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
 }
 
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len, int elt_sz)
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz)
 {
 	size_t page_sz, mmap_len;
 	char path[PATH_MAX];
@@ -601,10 +604,10 @@ rte_fbarray_destroy(struct rte_fbarray *arr)
 }
 
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx)
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx)
 {
 	void *ret = NULL;
-	if (arr == NULL || idx < 0) {
+	if (arr == NULL) {
 		rte_errno = EINVAL;
 		return NULL;
 	}
@@ -620,26 +623,26 @@ rte_fbarray_get(const struct rte_fbarray *arr, int idx)
 }
 
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, true);
 }
 
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, false);
 }
 
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	struct used_mask *msk;
 	int msk_idx;
 	uint64_t msk_bit;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -659,11 +662,11 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
 }
 
 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)
 {
 	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)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -707,12 +710,12 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -732,12 +735,12 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -757,11 +760,11 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_free(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -786,11 +789,11 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -834,7 +837,7 @@ void __rte_experimental
 rte_fbarray_dump_metadata(struct rte_fbarray *arr, FILE *f)
 {
 	struct used_mask *msk;
-	int i;
+	unsigned int i;
 
 	if (arr == NULL || f == NULL) {
 		rte_errno = EINVAL;
diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
index 97df945ea..3e61fffee 100644
--- a/lib/librte_eal/common/include/rte_fbarray.h
+++ b/lib/librte_eal/common/include/rte_fbarray.h
@@ -44,9 +44,9 @@ extern "C" {
 
 struct rte_fbarray {
 	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
-	int count;                       /**< number of entries stored */
-	int len;                         /**< current length of the array */
-	int elt_sz;                      /**< size of each element */
+	unsigned int count;              /**< number of entries stored */
+	unsigned int len;                /**< current length of the array */
+	unsigned int elt_sz;             /**< size of each element */
 	void *data;                      /**< data pointer */
 	rte_rwlock_t rwlock;             /**< multiprocess lock */
 };
@@ -76,8 +76,8 @@ struct rte_fbarray {
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len,
-		int elt_sz);
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz);
 
 
 /**
@@ -154,7 +154,7 @@ rte_fbarray_detach(struct rte_fbarray *arr);
  *  - NULL on failure, with ``rte_errno`` indicating reason for failure.
  */
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx);
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -188,7 +188,7 @@ rte_fbarray_find_idx(const struct rte_fbarray *arr, const void *elt);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -205,7 +205,7 @@ rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -223,7 +223,7 @@ rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -240,7 +240,7 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 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);
 
 
 /**
@@ -257,7 +257,7 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
@@ -277,7 +277,8 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -297,7 +298,8 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -314,7 +316,8 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_free(struct rte_fbarray *arr,
+		unsigned int start);
 
 
 /**
@@ -331,7 +334,7 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
-- 
2.11.0

Re: [PATCH v2 2/2] eal: fix signed integers in fbarray

[Burakov, Anatoly]

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 <anatoly.burakov@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> --
> 
> 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

Re: [PATCH v2 1/2] eal: fix undefined behavior in fbarray

[Burakov, Anatoly]

On 13-Apr-18 4:56 PM, Adrien Mazarguil wrote:
> According to GCC documentation [1], the __builtin_clz() family of functions
> yield undefined behavior when fed a zero value. There is one instance in
> the fbarray code where this can occur.
> 
> Clang (at least version 3.8.0-2ubuntu4) seems much more sensitive to this
> than GCC and yields random results when compiling optimized code, as shown
> below:
> 
>   #include <stdio.h>
> 
>   int main(void)
>   {
>           volatile unsigned long long moo;
>           int x;
> 
>           moo = 0;
>           x = __builtin_clzll(moo);
>           printf("%d\n", x);
>           return 0;
>   }
> 
>   $ gcc -O3 -o test test.c && ./test
>   63
>   $ clang -O3 -o test test.c && ./test
>   1742715559
>   $ clang -O0 -o test test.c && ./test
>   63
> 
> Even 63 can be considered an unexpected result given the number of leading
> zeroes should be the full width of the underlying type, i.e. 64.
> 
> In practice it causes find_next_n() to sometimes return negative values
> interpreted as errors by caller functions, which prevents DPDK applications
> from starting due to inability to find free memory segments:
> 
>   # testpmd [...]
>   EAL: Detected 32 lcore(s)
>   EAL: Detected 2 NUMA nodes
>   EAL: No free hugepages reported in hugepages-1048576kB
>   EAL: Multi-process socket /var/run/.rte_unix
>   EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list
>   EAL: FATAL: Cannot init memory
> 
>   EAL: Cannot init memory
> 
>   PANIC in main():
>   Cannot init EAL
>   4: [./build/app/testpmd(_start+0x29) [0x462289]]
>   3: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)
>       [0x7f19d54fc830]]
>   2: [./build/app/testpmd(main+0x8a3) [0x466193]]
>   1: [./build/app/testpmd(__rte_panic+0xd6) [0x4efaa6]]
>   Aborted
> 
> This problem appears with commit 66cc45e293ed ("mem: replace memseg with
> memseg lists") however the root cause is introduced by a prior patch.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
> 
> Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
> Cc: Anatoly Burakov <anatoly.burakov@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v2 2/2] eal: fix signed integers in fbarray

[Adrien Mazarguil]

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 <anatoly.burakov@intel.com>
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > 
> > --
> > 
> > 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

[PATCH v3 1/2] eal: fix undefined behavior in fbarray

[Adrien Mazarguil]

According to GCC documentation [1], the __builtin_clz() family of functions
yield undefined behavior when fed a zero value. There is one instance in
the fbarray code where this can occur.

Clang (at least version 3.8.0-2ubuntu4) seems much more sensitive to this
than GCC and yields random results when compiling optimized code, as shown
below:

 #include <stdio.h>

 int main(void)
 {
         volatile unsigned long long moo;
         int x;

         moo = 0;
         x = __builtin_clzll(moo);
         printf("%d\n", x);
         return 0;
 }

 $ gcc -O3 -o test test.c && ./test
 63
 $ clang -O3 -o test test.c && ./test
 1742715559
 $ clang -O0 -o test test.c && ./test
 63

Even 63 can be considered an unexpected result given the number of leading
zeroes should be the full width of the underlying type, i.e. 64.

In practice it causes find_next_n() to sometimes return negative values
interpreted as errors by caller functions, which prevents DPDK applications
from starting due to inability to find free memory segments:

 # testpmd [...]
 EAL: Detected 32 lcore(s)
 EAL: Detected 2 NUMA nodes
 EAL: No free hugepages reported in hugepages-1048576kB
 EAL: Multi-process socket /var/run/.rte_unix
 EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list
 EAL: FATAL: Cannot init memory

 EAL: Cannot init memory

 PANIC in main():
 Cannot init EAL
 4: [./build/app/testpmd(_start+0x29) [0x462289]]
 3: [/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)
     [0x7f19d54fc830]]
 2: [./build/app/testpmd(main+0x8a3) [0x466193]]
 1: [./build/app/testpmd(__rte_panic+0xd6) [0x4efaa6]]
 Aborted

This problem appears with commit 66cc45e293ed ("mem: replace memseg with
memseg lists") however the root cause is introduced by a prior patch.

[1] https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

Fixes: c44d09811b40 ("eal: add shared indexed file-backed array")
Cc: Anatoly Burakov <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index f65875dd9..11aa3f22a 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -173,7 +173,10 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		 */
 
 		/* count leading zeroes on inverted mask */
-		clz = __builtin_clzll(~cur_msk);
+		if (~cur_msk == 0)
+			clz = sizeof(cur_msk) * 8;
+		else
+			clz = __builtin_clzll(~cur_msk);
 
 		/* if there aren't any runs at the end either, just continue */
 		if (clz == 0)
-- 
2.11.0

[PATCH v3 2/2] eal: fix signed integers in fbarray

[Adrien Mazarguil]

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 <anatoly.burakov@intel.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

--

v3 changes:

- Added INT_MAX upper bound check in fully_validate() as suggested by
  Anatoly.
- Added a sysconf() result check to appease Coverity since calc_data_size()
  now takes an unsigned page size (Coverity issues 272598 and 272599).

v2 changes:

Removed unnecessary "(unsigned int)" cast leftovers.
---
 lib/librte_eal/common/eal_common_fbarray.c  | 104 ++++++++++++-----------
 lib/librte_eal/common/include/rte_fbarray.h |  33 +++----
 2 files changed, 74 insertions(+), 63 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 11aa3f22a..9d9d67d3f 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -3,6 +3,7 @@
  */
 
 #include <inttypes.h>
+#include <limits.h>
 #include <sys/mman.h>
 #include <stdint.h>
 #include <errno.h>
@@ -21,7 +22,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 +33,12 @@
  */
 
 struct used_mask {
-	int n_masks;
+	unsigned int n_masks;
 	uint64_t data[];
 };
 
 static size_t
-calc_mask_size(int len)
+calc_mask_size(unsigned int len)
 {
 	/* mask must be multiple of MASK_ALIGN, even though length of array
 	 * itself may not be aligned on that boundary.
@@ -48,7 +49,7 @@ calc_mask_size(int len)
 }
 
 static size_t
-calc_data_size(size_t page_sz, int elt_sz, int len)
+calc_data_size(size_t page_sz, unsigned int elt_sz, unsigned int len)
 {
 	size_t data_sz = elt_sz * len;
 	size_t msk_sz = calc_mask_size(len);
@@ -56,7 +57,7 @@ calc_data_size(size_t page_sz, int elt_sz, int len)
 }
 
 static struct used_mask *
-get_used_mask(void *data, int elt_sz, int len)
+get_used_mask(void *data, unsigned int elt_sz, unsigned int len)
 {
 	return (struct used_mask *) RTE_PTR_ADD(data, elt_sz * len);
 }
@@ -86,13 +87,14 @@ resize_and_map(int fd, void *addr, size_t len)
 }
 
 static int
-find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
+find_next_n(const struct rte_fbarray *arr, unsigned int start, unsigned int n,
+	    bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int msk_idx, lookahead_idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int msk_idx, lookahead_idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -108,11 +110,11 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 	 */
 	last = MASK_LEN_TO_IDX(arr->len);
 	last_mod = MASK_LEN_TO_MOD(arr->len);
-	last_msk = ~(-(1ULL) << last_mod);
+	last_msk = ~(-1ULL << last_mod);
 
 	for (msk_idx = first; msk_idx < msk->n_masks; msk_idx++) {
 		uint64_t cur_msk, lookahead_msk;
-		int run_start, clz, left;
+		unsigned int run_start, clz, left;
 		bool found = false;
 		/*
 		 * The process of getting n consecutive bits for arbitrary n is
@@ -157,7 +159,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 		/* if n can fit in within a single mask, do a search */
 		if (n <= MASK_ALIGN) {
 			uint64_t tmp_msk = cur_msk;
-			int s_idx;
+			unsigned int s_idx;
 			for (s_idx = 0; s_idx < n - 1; s_idx++)
 				tmp_msk &= tmp_msk >> 1ULL;
 			/* we found what we were looking for */
@@ -188,7 +190,7 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 
 		for (lookahead_idx = msk_idx + 1; lookahead_idx < msk->n_masks;
 				lookahead_idx++) {
-			int s_idx, need;
+			unsigned int s_idx, need;
 			lookahead_msk = msk->data[lookahead_idx];
 
 			/* if we're looking for free space, invert the mask */
@@ -234,13 +236,13 @@ find_next_n(const struct rte_fbarray *arr, int start, int n, bool used)
 }
 
 static int
-find_next(const struct rte_fbarray *arr, int start, bool used)
+find_next(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	uint64_t ignore_msk;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk, ignore_msk;
 
 	/*
 	 * mask only has granularity of MASK_ALIGN, but start may not be aligned
@@ -290,13 +292,14 @@ find_next(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-find_contig(const struct rte_fbarray *arr, int start, bool used)
+find_contig(const struct rte_fbarray *arr, unsigned int start, bool used)
 {
 	const struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz,
 			arr->len);
-	int idx, first, first_mod;
-	int last, last_mod, last_msk;
-	int need_len, result = 0;
+	unsigned int idx, first, first_mod;
+	unsigned int last, last_mod;
+	uint64_t last_msk;
+	unsigned int need_len, result = 0;
 
 	/* array length may not be aligned, so calculate ignore mask for last
 	 * mask index.
@@ -309,7 +312,7 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 	first_mod = MASK_LEN_TO_MOD(start);
 	for (idx = first; idx < msk->n_masks; idx++, result += need_len) {
 		uint64_t cur = msk->data[idx];
-		int run_len;
+		unsigned int run_len;
 
 		need_len = MASK_ALIGN;
 
@@ -350,15 +353,15 @@ find_contig(const struct rte_fbarray *arr, int start, bool used)
 }
 
 static int
-set_used(struct rte_fbarray *arr, int idx, bool used)
+set_used(struct rte_fbarray *arr, unsigned int idx, bool used)
 {
 	struct used_mask *msk = get_used_mask(arr->data, arr->elt_sz, arr->len);
 	uint64_t msk_bit = 1ULL << MASK_LEN_TO_MOD(idx);
-	int msk_idx = MASK_LEN_TO_IDX(idx);
+	unsigned int msk_idx = MASK_LEN_TO_IDX(idx);
 	bool already_used;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -389,7 +392,7 @@ set_used(struct rte_fbarray *arr, int idx, bool used)
 static int
 fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
 {
-	if (name == NULL || elt_sz == 0 || len == 0) {
+	if (name == NULL || elt_sz == 0 || len == 0 || len > INT_MAX) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -402,7 +405,8 @@ fully_validate(const char *name, unsigned int elt_sz, unsigned int len)
 }
 
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len, int elt_sz)
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz)
 {
 	size_t page_sz, mmap_len;
 	char path[PATH_MAX];
@@ -419,6 +423,8 @@ rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len, int elt_sz)
 		return -1;
 
 	page_sz = sysconf(_SC_PAGESIZE);
+	if (page_sz == (size_t)-1)
+		goto fail;
 
 	/* calculate our memory limits */
 	mmap_len = calc_data_size(page_sz, elt_sz, len);
@@ -508,6 +514,8 @@ rte_fbarray_attach(struct rte_fbarray *arr)
 		return -1;
 
 	page_sz = sysconf(_SC_PAGESIZE);
+	if (page_sz == (size_t)-1)
+		goto fail;
 
 	mmap_len = calc_data_size(page_sz, arr->elt_sz, arr->len);
 
@@ -601,10 +609,10 @@ rte_fbarray_destroy(struct rte_fbarray *arr)
 }
 
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx)
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx)
 {
 	void *ret = NULL;
-	if (arr == NULL || idx < 0) {
+	if (arr == NULL) {
 		rte_errno = EINVAL;
 		return NULL;
 	}
@@ -620,26 +628,26 @@ rte_fbarray_get(const struct rte_fbarray *arr, int idx)
 }
 
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, true);
 }
 
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx)
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx)
 {
 	return set_used(arr, idx, false);
 }
 
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx)
 {
 	struct used_mask *msk;
 	int msk_idx;
 	uint64_t msk_bit;
 	int ret = -1;
 
-	if (arr == NULL || idx < 0 || idx >= arr->len) {
+	if (arr == NULL || idx >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -659,11 +667,11 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx)
 }
 
 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)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -683,11 +691,11 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -707,12 +715,12 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -732,12 +740,12 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len ||
-			n < 0 || n > arr->len) {
+	if (arr == NULL || start >= arr->len || n > arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -757,11 +765,11 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_free(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -786,11 +794,11 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start)
 }
 
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start)
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start)
 {
 	int ret = -1;
 
-	if (arr == NULL || start < 0 || start >= arr->len) {
+	if (arr == NULL || start >= arr->len) {
 		rte_errno = EINVAL;
 		return -1;
 	}
@@ -834,7 +842,7 @@ void __rte_experimental
 rte_fbarray_dump_metadata(struct rte_fbarray *arr, FILE *f)
 {
 	struct used_mask *msk;
-	int i;
+	unsigned int i;
 
 	if (arr == NULL || f == NULL) {
 		rte_errno = EINVAL;
diff --git a/lib/librte_eal/common/include/rte_fbarray.h b/lib/librte_eal/common/include/rte_fbarray.h
index 97df945ea..3e61fffee 100644
--- a/lib/librte_eal/common/include/rte_fbarray.h
+++ b/lib/librte_eal/common/include/rte_fbarray.h
@@ -44,9 +44,9 @@ extern "C" {
 
 struct rte_fbarray {
 	char name[RTE_FBARRAY_NAME_LEN]; /**< name associated with an array */
-	int count;                       /**< number of entries stored */
-	int len;                         /**< current length of the array */
-	int elt_sz;                      /**< size of each element */
+	unsigned int count;              /**< number of entries stored */
+	unsigned int len;                /**< current length of the array */
+	unsigned int elt_sz;             /**< size of each element */
 	void *data;                      /**< data pointer */
 	rte_rwlock_t rwlock;             /**< multiprocess lock */
 };
@@ -76,8 +76,8 @@ struct rte_fbarray {
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_init(struct rte_fbarray *arr, const char *name, int len,
-		int elt_sz);
+rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
+		unsigned int elt_sz);
 
 
 /**
@@ -154,7 +154,7 @@ rte_fbarray_detach(struct rte_fbarray *arr);
  *  - NULL on failure, with ``rte_errno`` indicating reason for failure.
  */
 void * __rte_experimental
-rte_fbarray_get(const struct rte_fbarray *arr, int idx);
+rte_fbarray_get(const struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -188,7 +188,7 @@ rte_fbarray_find_idx(const struct rte_fbarray *arr, const void *elt);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -205,7 +205,7 @@ rte_fbarray_set_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
+rte_fbarray_set_free(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -223,7 +223,7 @@ rte_fbarray_set_free(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
+rte_fbarray_is_used(struct rte_fbarray *arr, unsigned int idx);
 
 
 /**
@@ -240,7 +240,7 @@ rte_fbarray_is_used(struct rte_fbarray *arr, int idx);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 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);
 
 
 /**
@@ -257,7 +257,7 @@ rte_fbarray_find_next_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_next_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
@@ -277,7 +277,8 @@ rte_fbarray_find_next_used(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_free(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -297,7 +298,8 @@ rte_fbarray_find_next_n_free(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
+rte_fbarray_find_next_n_used(struct rte_fbarray *arr, unsigned int start,
+		unsigned int n);
 
 
 /**
@@ -314,7 +316,8 @@ rte_fbarray_find_next_n_used(struct rte_fbarray *arr, int start, int n);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_free(struct rte_fbarray *arr,
+		unsigned int start);
 
 
 /**
@@ -331,7 +334,7 @@ rte_fbarray_find_contig_free(struct rte_fbarray *arr, int start);
  *  - -1 on failure, with ``rte_errno`` indicating reason for failure.
  */
 int __rte_experimental
-rte_fbarray_find_contig_used(struct rte_fbarray *arr, int start);
+rte_fbarray_find_contig_used(struct rte_fbarray *arr, unsigned int start);
 
 
 /**
-- 
2.11.0

Re: [PATCH v3 2/2] eal: fix signed integers in fbarray

[Burakov, Anatoly]

On 13-Apr-18 7:43 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 <anatoly.burakov@intel.com>
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> --
> 
> v3 changes:
> 
> - Added INT_MAX upper bound check in fully_validate() as suggested by
>    Anatoly.
> - Added a sysconf() result check to appease Coverity since calc_data_size()
>    now takes an unsigned page size (Coverity issues 272598 and 272599).
> 
> v2 changes:
> 
> Removed unnecessary "(unsigned int)" cast leftovers.

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [PATCH v3 2/2] eal: fix signed integers in fbarray

[Burakov, Anatoly]

On 14-Apr-18 11:03 AM, Burakov, Anatoly wrote:
> On 13-Apr-18 7:43 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 <anatoly.burakov@intel.com>
>>
>> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
>>
>> -- 
>>
>> v3 changes:
>>
>> - Added INT_MAX upper bound check in fully_validate() as suggested by
>>    Anatoly.
>> - Added a sysconf() result check to appease Coverity since 
>> calc_data_size()
>>    now takes an unsigned page size (Coverity issues 272598 and 272599).
>>
>> v2 changes:
>>
>> Removed unnecessary "(unsigned int)" cast leftovers.
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Still ack, but i think Coverity issues need to be moved to main commit 
message, as opposed to being in patch notes.

-- 
Thanks,
Anatoly

Re: [PATCH v3 2/2] eal: fix signed integers in fbarray

[Thomas Monjalon]

17/04/2018 13:14, Burakov, Anatoly:
> On 14-Apr-18 11:03 AM, Burakov, Anatoly wrote:
> > On 13-Apr-18 7:43 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 <anatoly.burakov@intel.com>
> >>
> >> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> >>
> >>
> >> v3 changes:
> >>
> >> - Added INT_MAX upper bound check in fully_validate() as suggested by
> >>    Anatoly.
> >> - Added a sysconf() result check to appease Coverity since 
> >> calc_data_size()
> >>    now takes an unsigned page size (Coverity issues 272598 and 272599).
> >>
> >> v2 changes:
> >>
> >> Removed unnecessary "(unsigned int)" cast leftovers.
> > 
> > Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > 
> 
> Still ack, but i think Coverity issues need to be moved to main commit 
> message, as opposed to being in patch notes.

Applied (with Coverity ids added), thanks