[Tarantool-patches] [PATCH 0/3] small: Update to use region_alloc_object

[Tarantool-patches] [PATCH 0/3] small: Update to use region_alloc_object

[Cyrill Gorcunov]

A few commits from the master branch backporting commits
to 1.10 series as Vlad requested.

Since I've to update "small: sanitized rlist, bug in lsregion,
and new region API" patch significantly during backport I
put my "author" tag so any errors are mine while the real
functionality is introduced by Vlad.

issue https://github.com/tarantool/tarantool/issues/5060
branch gorcunov/gh-5060-small-110

Note the patches are for 1.10 series!

Cyrill Gorcunov (2):
  Prepare for small library update
  small: sanitized rlist, bug in lsregion, and new region API

Vladislav Shpilevoy (1):
  cmake: ignore warnings on alignof() and offsetof()

 cmake/compiler.cmake        |   8 ++-
 src/box/memtx_engine.c      |   9 +--
 src/box/txn.c               |  23 +++---
 src/box/vy_write_iterator.c |   7 +-
 src/lib/small               |   2 +-
 test/unit/CMakeLists.txt    |   2 -
 test/unit/rlist.c           | 137 ------------------------------------
 test/unit/rlist.result      |  88 -----------------------
 8 files changed, 31 insertions(+), 245 deletions(-)
 delete mode 100644 test/unit/rlist.c
 delete mode 100644 test/unit/rlist.result


base-commit: ee8278f214f8752088ebdd9aa9260493594a26e9
-- 
2.26.2

[Tarantool-patches] [PATCH 1/3] cmake: ignore warnings on alignof() and offsetof()

[Cyrill Gorcunov]

From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

Warning about invalid offsetof() (used on non-POD types) was set
for g++, but wasn't for clang++.

Warning about invalid alignof() (when expression is passed to it
instead of a type) wasn't ignored, but is going to be very
useful in upcoming unaligned memory access patches. That allows
to write something like:

    struct some_long_type *object = region_aligned_alloc(
            region, size, alignof(*object));

This will work even if type of 'object' will change in future,
and so it is safer. And shorter.

Part of #4609
---
 cmake/compiler.cmake | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 56429dc20..b0908f3b3 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -276,11 +276,17 @@ macro(enable_tnt_compile_flags)
         add_compile_flags("C;CXX" "-Wno-format-truncation")
     endif()
 
-    if (CMAKE_COMPILER_IS_GNUCXX)
+    if (CMAKE_COMPILER_IS_CLANG OR CMAKE_COMPILER_IS_GNUCXX)
         # G++ bug. http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31488
+        # Also offsetof() is widely used in Tarantool source code
+        # for classes and structs to implement intrusive lists and
+        # some other data structures. G++ and clang++ both
+        # complain about classes, having a virtual table. They
+        # complain fair, but this can't be fixed for now.
         add_compile_flags("CXX"
             "-Wno-invalid-offsetof"
         )
+        add_compile_flags("C;CXX" "-Wno-gnu-alignof-expression")
     endif()
 
     if (CMAKE_COMPILER_IS_GNUCC)
-- 
2.26.2

[Tarantool-patches] [PATCH 2/3] Prepare for small library update

[Cyrill Gorcunov]

Backport of the commit 277fc6958fda07f212251d30e471b329d1028638,
note that we do NOT update small library here immediately otherwise
we simply won't built after this commit.

Thus real update will be done a bit later.

To bring new rlist methods - rlist_foreach_entry_safe_reverse and
rlist_cut_before. Also, remove unit/rlist test as it is now a part
of the small suite.

Part-of #5060

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 test/unit/CMakeLists.txt |   2 -
 test/unit/rlist.c        | 137 ---------------------------------------
 test/unit/rlist.result   |  88 -------------------------
 3 files changed, 227 deletions(-)
 delete mode 100644 test/unit/rlist.c
 delete mode 100644 test/unit/rlist.result

diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index cdafd785f..d0138e8fd 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -18,8 +18,6 @@ add_executable(heap.test heap.c)
 target_link_libraries(heap.test unit)
 add_executable(heap_iterator.test heap_iterator.c)
 target_link_libraries(heap_iterator.test unit)
-add_executable(rlist.test rlist.c)
-target_link_libraries(rlist.test unit)
 add_executable(stailq.test stailq.c)
 target_link_libraries(stailq.test unit)
 add_executable(uri.test uri.c unit.c)
diff --git a/test/unit/rlist.c b/test/unit/rlist.c
deleted file mode 100644
index c0c29a3f1..000000000
--- a/test/unit/rlist.c
+++ /dev/null
@@ -1,137 +0,0 @@
-#include "small/rlist.h"
-#include <stdio.h>
-#include <stdarg.h>
-#include "unit.h"
-
-
-#define PLAN		87
-
-#define ITEMS		7
-
-struct test {
-	char ch;
-	int  no;
-	struct rlist list;
-};
-
-static struct test items[ITEMS];
-
-static RLIST_HEAD(head);
-static RLIST_HEAD(head2);
-
-int
-main(void)
-{
-	int i;
-	struct test *it;
-	struct rlist *rlist;
-
-	plan(PLAN);
-	ok(rlist_empty(&head), "list is empty");
-	for (i = 0; i < ITEMS; i++) {
-		items[i].no = i;
-		rlist_add_tail(&head, &(items[i].list));
-	}
-	RLIST_HEAD(empty_list);
-	ok(rlist_empty(&empty_list), "rlist_nil is empty");
-	ok(rlist_empty(&head2), "head2 is empty");
-	rlist_swap(&head2, &empty_list);
-	ok(rlist_empty(&empty_list), "rlist_nil is empty after swap");
-	ok(rlist_empty(&head2), "head2 is empty after swap");
-	rlist_swap(&head, &head2);
-	ok(rlist_empty(&head), "head is empty after swap");
-	is(rlist_first(&head2), &items[0].list, "first item");
-	is(rlist_last(&head2), &items[ITEMS - 1].list, "last item");
-	i = 0;
-	rlist_foreach(rlist, &head2) {
-		is(rlist, &items[i].list, "element (foreach) %d", i);
-		i++;
-	}
-	rlist_foreach_reverse(rlist, &head2) {
-		i--;
-		is(rlist, &items[i].list, "element (foreach_reverse) %d", i);
-	}
-	rlist_swap(&head2, &head);
-
-
-	is(rlist_first(&head), &items[0].list, "first item");
-	isnt(rlist_first(&head), &items[ITEMS - 1].list, "first item");
-
-	is(rlist_last(&head), &items[ITEMS - 1].list, "last item");
-	isnt(rlist_last(&head), &items[0].list, "last item");
-
-	is(rlist_next(&head), &items[0].list, "rlist_next");
-	is(rlist_prev(&head), &items[ITEMS - 1].list, "rlist_prev");
-
-	i = 0;
-	rlist_foreach(rlist, &head) {
-		is(rlist, &items[i].list, "element (foreach) %d", i);
-		i++;
-	}
-	rlist_foreach_reverse(rlist, &head) {
-		i--;
-		is(rlist, &items[i].list, "element (foreach_reverse) %d", i);
-	}
-
-
-	is(rlist_entry(&items[0].list, struct test, list), &items[0],
-		"rlist_entry");
-	is(rlist_first_entry(&head, struct test, list), &items[0],
-		"rlist_first_entry");
-	is(rlist_next_entry(&items[0], list), &items[1], "rlist_next_entry");
-	is(rlist_prev_entry(&items[2], list), &items[1], "rlist_prev_entry");
-
-
-	i = 0;
-	rlist_foreach_entry(it, &head, list) {
-		is(it, items + i, "element (foreach_entry) %d", i);
-		i++;
-	}
-	rlist_foreach_entry_reverse(it, &head, list) {
-		i--;
-		is(it, items + i, "element (foreach_entry_reverse) %d", i);
-	}
-
-	rlist_del(&items[2].list);
-	ok(rlist_empty(&head2), "head2 is empty");
-	rlist_move(&head2, &items[3].list);
-	ok(!rlist_empty(&head2), "head2 isnt empty");
-	is(rlist_first_entry(&head2, struct test, list),
-					&items[3], "Item was moved");
-	rlist_move_tail(&head2, &items[4].list);
-	rlist_foreach_entry(it, &head, list) {
-		is(it, items + i, "element (second deleted) %d", i);
-		i++;
-		if (i == 2)
-			i += 3;
-	}
-	rlist_foreach_entry_reverse(it, &head, list) {
-		i--;
-		if (i == 4)
-			i -= 3;
-		is(it, items + i, "element (second deleted) %d", i);
-	}
-
-
-	rlist_create(&head);
-	ok(rlist_empty(&head), "list is empty");
-	for (i = 0; i < ITEMS; i++) {
-		items[i].no = i;
-		rlist_add(&head, &(items[i].list));
-	}
-	i = 0;
-	rlist_foreach_entry_reverse(it, &head, list) {
-		is(it, items + i, "element (foreach_entry_reverse) %d", i);
-		i++;
-	}
-	rlist_foreach_entry(it, &head, list) {
-		i--;
-		is(it, items + i, "element (foreach_entry) %d", i);
-	}
-	rlist_create(&head);
-	rlist_add_entry(&head, &items[0], list);
-	ok(rlist_prev_entry_safe(&items[0], &head, list) == NULL,
-	   "prev is null");
-	return check_plan();
-}
-
diff --git a/test/unit/rlist.result b/test/unit/rlist.result
deleted file mode 100644
index fa99a87cf..000000000
--- a/test/unit/rlist.result
+++ /dev/null
@@ -1,88 +0,0 @@
-1..87
-ok 1 - list is empty
-ok 2 - rlist_nil is empty
-ok 3 - head2 is empty
-ok 4 - rlist_nil is empty after swap
-ok 5 - head2 is empty after swap
-ok 6 - head is empty after swap
-ok 7 - first item
-ok 8 - last item
-ok 9 - element (foreach) 0
-ok 10 - element (foreach) 1
-ok 11 - element (foreach) 2
-ok 12 - element (foreach) 3
-ok 13 - element (foreach) 4
-ok 14 - element (foreach) 5
-ok 15 - element (foreach) 6
-ok 16 - element (foreach_reverse) 6
-ok 17 - element (foreach_reverse) 5
-ok 18 - element (foreach_reverse) 4
-ok 19 - element (foreach_reverse) 3
-ok 20 - element (foreach_reverse) 2
-ok 21 - element (foreach_reverse) 1
-ok 22 - element (foreach_reverse) 0
-ok 23 - first item
-ok 24 - first item
-ok 25 - last item
-ok 26 - last item
-ok 27 - rlist_next
-ok 28 - rlist_prev
-ok 29 - element (foreach) 0
-ok 30 - element (foreach) 1
-ok 31 - element (foreach) 2
-ok 32 - element (foreach) 3
-ok 33 - element (foreach) 4
-ok 34 - element (foreach) 5
-ok 35 - element (foreach) 6
-ok 36 - element (foreach_reverse) 6
-ok 37 - element (foreach_reverse) 5
-ok 38 - element (foreach_reverse) 4
-ok 39 - element (foreach_reverse) 3
-ok 40 - element (foreach_reverse) 2
-ok 41 - element (foreach_reverse) 1
-ok 42 - element (foreach_reverse) 0
-ok 43 - rlist_entry
-ok 44 - rlist_first_entry
-ok 45 - rlist_next_entry
-ok 46 - rlist_prev_entry
-ok 47 - element (foreach_entry) 0
-ok 48 - element (foreach_entry) 1
-ok 49 - element (foreach_entry) 2
-ok 50 - element (foreach_entry) 3
-ok 51 - element (foreach_entry) 4
-ok 52 - element (foreach_entry) 5
-ok 53 - element (foreach_entry) 6
-ok 54 - element (foreach_entry_reverse) 6
-ok 55 - element (foreach_entry_reverse) 5
-ok 56 - element (foreach_entry_reverse) 4
-ok 57 - element (foreach_entry_reverse) 3
-ok 58 - element (foreach_entry_reverse) 2
-ok 59 - element (foreach_entry_reverse) 1
-ok 60 - element (foreach_entry_reverse) 0
-ok 61 - head2 is empty
-ok 62 - head2 isnt empty
-ok 63 - Item was moved
-ok 64 - element (second deleted) 0
-ok 65 - element (second deleted) 1
-ok 66 - element (second deleted) 5
-ok 67 - element (second deleted) 6
-ok 68 - element (second deleted) 6
-ok 69 - element (second deleted) 5
-ok 70 - element (second deleted) 1
-ok 71 - element (second deleted) 0
-ok 72 - list is empty
-ok 73 - element (foreach_entry_reverse) 0
-ok 74 - element (foreach_entry_reverse) 1
-ok 75 - element (foreach_entry_reverse) 2
-ok 76 - element (foreach_entry_reverse) 3
-ok 77 - element (foreach_entry_reverse) 4
-ok 78 - element (foreach_entry_reverse) 5
-ok 79 - element (foreach_entry_reverse) 6
-ok 80 - element (foreach_entry) 6
-ok 81 - element (foreach_entry) 5
-ok 82 - element (foreach_entry) 4
-ok 83 - element (foreach_entry) 3
-ok 84 - element (foreach_entry) 2
-ok 85 - element (foreach_entry) 1
-ok 86 - element (foreach_entry) 0
-ok 87 - prev is null
-- 
2.26.2

[Tarantool-patches] [PATCH 3/3] small: sanitized rlist, bug in lsregion, and new region API

[Cyrill Gorcunov]

Backport of commit ffec9e40d8695bffe76109b721b1050db84b384e,
I've to update all region_alloc_array calls to use fuber()->gc
as there was no txn->region yet.

Rlist used a hack to implement offsetof() leading to crash under
undefined behaviour clang sanitizer. It was fixed in this update.

Additionally, region_alloc_object() is changed to return the used
size and a new macro region_alloc_array() is added. This small
API change is supposed to simplify switching lots of region
allocations to aligned versions in scope of #4609.

And finally - there was a bug in lsregion, when allocation was
exactly of slab size. It is fixed in small's master.

Part-of #4609
Part-of #5060

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/memtx_engine.c      |  9 +++++----
 src/box/txn.c               | 23 +++++++++++++----------
 src/box/vy_write_iterator.c |  7 +++++--
 src/lib/small               |  2 +-
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index e52397976..cc5bacc28 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -650,10 +650,10 @@ checkpoint_add_space(struct space *sp, void *data)
 		return 0;
 	struct checkpoint *ckpt = (struct checkpoint *)data;
 	struct checkpoint_entry *entry;
-	entry = region_alloc_object(&fiber()->gc, struct checkpoint_entry);
+	int size;
+	entry = region_alloc_object(&fiber()->gc, struct checkpoint_entry, &size);
 	if (entry == NULL) {
-		diag_set(OutOfMemory, sizeof(*entry),
-			 "region", "struct checkpoint_entry");
+		diag_set(OutOfMemory, sizeof(*entry), "region", "checkpoint_entry");
 		return -1;
 	}
 	rlist_add_tail_entry(&ckpt->entries, entry, link);
@@ -715,7 +715,8 @@ memtx_engine_begin_checkpoint(struct engine *engine)
 	struct memtx_engine *memtx = (struct memtx_engine *)engine;
 
 	assert(memtx->checkpoint == NULL);
-	memtx->checkpoint = region_alloc_object(&fiber()->gc, struct checkpoint);
+	int size;
+	memtx->checkpoint = region_alloc_object(&fiber()->gc, struct checkpoint, &size);
 	if (memtx->checkpoint == NULL) {
 		diag_set(OutOfMemory, sizeof(*memtx->checkpoint),
 			 "region", "struct checkpoint");
diff --git a/src/box/txn.c b/src/box/txn.c
index 4823ec0df..4cf107d45 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -48,10 +48,10 @@ txn_add_redo(struct txn_stmt *stmt, struct request *request)
 {
 	/* Create a redo log row for Lua requests */
 	struct xrow_header *row;
-	row = region_alloc_object(&fiber()->gc, struct xrow_header);
+	int size;
+	row = region_alloc_object(&fiber()->gc, struct xrow_header, &size);
 	if (row == NULL) {
-		diag_set(OutOfMemory, sizeof(*row),
-			 "region", "struct xrow_header");
+		diag_set(OutOfMemory, size, "region_alloc_object", "row");
 		return -1;
 	}
 	if (request->header != NULL) {
@@ -85,10 +85,10 @@ static struct txn_stmt *
 txn_stmt_new(struct txn *txn)
 {
 	struct txn_stmt *stmt;
-	stmt = region_alloc_object(&fiber()->gc, struct txn_stmt);
+	int size;
+	stmt = region_alloc_object(&fiber()->gc, struct txn_stmt, &size);
 	if (stmt == NULL) {
-		diag_set(OutOfMemory, sizeof(*stmt),
-			 "region", "struct txn_stmt");
+		diag_set(OutOfMemory, size, "region_alloc_object", "stmt");
 		return NULL;
 	}
 
@@ -141,9 +141,10 @@ txn_begin(bool is_autocommit)
 {
 	static int64_t txn_id = 0;
 	assert(! in_txn());
-	struct txn *txn = region_alloc_object(&fiber()->gc, struct txn);
+	int size;
+	struct txn *txn = region_alloc_object(&fiber()->gc, struct txn, &size);
 	if (txn == NULL) {
-		diag_set(OutOfMemory, sizeof(*txn), "region", "struct txn");
+		diag_set(OutOfMemory, size, "region_alloc_object", "txn");
 		return NULL;
 	}
 	/* Initialize members explicitly to save time on memset() */
@@ -512,12 +513,14 @@ box_txn_savepoint()
 		diag_set(ClientError, ER_SAVEPOINT_NO_TRANSACTION);
 		return NULL;
 	}
+	int size;
 	struct txn_savepoint *svp =
 		(struct txn_savepoint *) region_alloc_object(&fiber()->gc,
-							struct txn_savepoint);
+							struct txn_savepoint,
+							&size);
 	if (svp == NULL) {
 		diag_set(OutOfMemory, sizeof(*svp),
-			 "region", "struct txn_savepoint");
+			 "region_alloc_object", "txn_savepoint");
 		return NULL;
 	}
 	svp->stmt = stailq_last(&txn->stmts);
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 41ef7d04b..2150b172b 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -100,9 +100,12 @@ static inline struct vy_write_history *
 vy_write_history_new(struct tuple *tuple, struct vy_write_history *next)
 {
 	struct vy_write_history *h;
-	h = region_alloc_object(&fiber()->gc, struct vy_write_history);
-	if (h == NULL)
+	int size;
+	h = region_alloc_object(&fiber()->gc, struct vy_write_history, &size);
+	if (h == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc_object", "h");
 		return NULL;
+	}
 	h->tuple = tuple;
 	assert(next == NULL || (next->tuple != NULL &&
 	       vy_stmt_lsn(next->tuple) > vy_stmt_lsn(tuple)));
diff --git a/src/lib/small b/src/lib/small
index 2fb5cf230..fcac155db 160000
--- a/src/lib/small
+++ b/src/lib/small
@@ -1 +1 @@
-Subproject commit 2fb5cf2303956175fd13b738c34d622499efb11b
+Subproject commit fcac155dba18e97b10bc668d1a2fae01184adc13
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 0/3] small: Update to use region_alloc_object

[Cyrill Gorcunov]

On Fri, Sep 11, 2020 at 02:24:28PM +0300, Cyrill Gorcunov wrote:
> A few commits from the master branch backporting commits
> to 1.10 series as Vlad requested.
> 
> Since I've to update "small: sanitized rlist, bug in lsregion,
> and new region API" patch significantly during backport I
> put my "author" tag so any errors are mine while the real
> functionality is introduced by Vlad.
> 
> issue https://github.com/tarantool/tarantool/issues/5060
> branch gorcunov/gh-5060-small-110
> 
> Note the patches are for 1.10 series!

Kirill, these are backports of Vlads' patches I think it
is safe to merge them and close the issue.

Re: [Tarantool-patches] [PATCH 0/3] small: Update to use region_alloc_object

[Kirill Yukhin]

Hello,

On 11 сен 14:24, Cyrill Gorcunov wrote:
> A few commits from the master branch backporting commits
> to 1.10 series as Vlad requested.
> 
> Since I've to update "small: sanitized rlist, bug in lsregion,
> and new region API" patch significantly during backport I
> put my "author" tag so any errors are mine while the real
> functionality is introduced by Vlad.
> 
> issue https://github.com/tarantool/tarantool/issues/5060
> branch gorcunov/gh-5060-small-110
> 
> Note the patches are for 1.10 series!
> 
> Cyrill Gorcunov (2):
>   Prepare for small library update
>   small: sanitized rlist, bug in lsregion, and new region API
> 
> Vladislav Shpilevoy (1):
>   cmake: ignore warnings on alignof() and offsetof()

LGTM.
I've checked your patch set into 1.10.

--
Regards, Kirill Yukhin