[Tarantool-patches] [PATCH v2 00/10] Sanitize unaligned access

[Tarantool-patches] [PATCH v2 00/10] Sanitize unaligned access

[Vladislav Shpilevoy]

The patchset introduces a new cmake option ENABLE_UB_SANITIZER, to
enable clang undefined behaviour sanitizer.

The sanitizer revealed lots of unaligned memory accesses, and the
patchset fixed all of them (which were found).

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4609-sanitize-alignment-full-ci
Issue: https://github.com/tarantool/tarantool/issues/4609

Changes in v2:
- Properly fixed CRC33 and removed NOSANITIZE macro;
- Added UB sanitizer to CI in ASAN builds;
- Rebased to master, and pushed some preliminary patches.

Vladislav Shpilevoy (10):
  small: sanitized rlist and new region API
  cmake: ignore warnings on alignof() and offsetof()
  cmake: add option ENABLE_UB_SANITIZER
  crc32: align memory access
  sql: make BtCursor's memory aligned
  region: use aligned allocations where necessary
  vinyl: align statements and bps tree extents
  tuple: use unaligned store-load for field map
  port: make port_c_entry not PACKED
  xrow: use unaligned store operation in xrow_to_iovec()

 .travis.mk                      |  3 +-
 cmake/compiler.cmake            | 13 +++++-
 src/box/alter.cc                | 39 +++++++++++-------
 src/box/applier.cc              | 22 +++++-----
 src/box/bind.c                  |  7 ++--
 src/box/ck_constraint.c         | 11 ++---
 src/box/field_map.c             | 30 +++++++++-----
 src/box/field_map.h             | 21 ++++++----
 src/box/fk_constraint.h         | 14 ++++---
 src/box/index_def.c             |  9 ++--
 src/box/key_def.c               |  9 ++--
 src/box/lua/execute.c           |  7 ++--
 src/box/lua/key_def.c           |  7 ++--
 src/box/lua/misc.cc             |  8 ++--
 src/box/lua/schema.lua          |  2 +-
 src/box/memtx_tree.c            |  7 ++--
 src/box/port.h                  | 10 +----
 src/box/space_def.c             |  7 ++--
 src/box/sql.c                   | 20 +++++----
 src/box/sql/build.c             | 60 ++++++++++++++++-----------
 src/box/sql/func.c              |  7 ++--
 src/box/sql/select.c            | 14 ++++---
 src/box/sql/update.c            |  6 +--
 src/box/sql/vdbe.c              | 16 ++++----
 src/box/sql/wherecode.c         |  9 ++--
 src/box/tuple_format.c          |  3 +-
 src/box/txn.c                   | 29 +++++++------
 src/box/user.cc                 |  8 ++--
 src/box/vinyl.c                 | 26 +++++++-----
 src/box/vy_log.c                | 33 +++++++--------
 src/box/vy_mem.c                |  9 ++--
 src/box/vy_point_lookup.c       |  9 ++--
 src/box/vy_stmt.c               | 13 +++---
 src/box/vy_write_iterator.c     |  7 +++-
 src/box/xrow.c                  |  2 +-
 src/box/xrow_update_map.c       |  7 ++--
 src/box/xrow_update_route.c     |  7 ++--
 src/cpu_feature.c               | 31 ++++++++++----
 src/lib/core/backtrace.cc       |  6 +--
 src/lib/core/port.h             |  2 +-
 src/lib/small                   |  2 +-
 src/lua/popen.c                 | 20 +++++----
 test/unit/CMakeLists.txt        |  3 ++
 test/unit/crc32.c               | 73 +++++++++++++++++++++++++++++++++
 test/unit/crc32.result          | 11 +++++
 test/vinyl/quota.result         | 10 ++---
 test/vinyl/quota_timeout.result |  4 +-
 test/vinyl/stat.result          |  4 +-
 48 files changed, 429 insertions(+), 248 deletions(-)
 create mode 100644 test/unit/crc32.c
 create mode 100644 test/unit/crc32.result

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 01/10] small: sanitized rlist and new region API

[Vladislav Shpilevoy]

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.

Part of #4609
---
 src/box/txn.c               | 17 +++++++++--------
 src/box/vy_write_iterator.c |  7 +++++--
 src/lib/small               |  2 +-
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 488aa4bdd..b81693c0a 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -54,11 +54,11 @@ static int
 txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
 {
 	/* Create a redo log row. */
+	int size;
 	struct xrow_header *row;
-	row = region_alloc_object(&txn->region, struct xrow_header);
+	row = region_alloc_object(&txn->region, 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) {
@@ -91,11 +91,11 @@ txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
 static struct txn_stmt *
 txn_stmt_new(struct region *region)
 {
+	int size;
 	struct txn_stmt *stmt;
-	stmt = region_alloc_object(region, struct txn_stmt);
+	stmt = region_alloc_object(region, 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;
 	}
 
@@ -178,9 +178,10 @@ txn_new(void)
 	region_create(&region, &cord()->slabc);
 
 	/* Place txn structure on the region. */
-	struct txn *txn = region_alloc_object(&region, struct txn);
+	int size;
+	struct txn *txn = region_alloc_object(&region, struct txn, &size);
 	if (txn == NULL) {
-		diag_set(OutOfMemory, sizeof(*txn), "region", "struct txn");
+		diag_set(OutOfMemory, size, "region_alloc_object", "txn");
 		return NULL;
 	}
 	assert(region_used(&region) == sizeof(*txn));
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 01962faa3..78a52ae5c 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -102,9 +102,12 @@ static inline struct vy_write_history *
 vy_write_history_new(struct vy_entry entry, 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->entry = entry;
 	assert(next == NULL || (next->entry.stmt != NULL &&
 	       vy_stmt_lsn(next->entry.stmt) > vy_stmt_lsn(entry.stmt)));
diff --git a/src/lib/small b/src/lib/small
index 118b53334..fc75e99f5 160000
--- a/src/lib/small
+++ b/src/lib/small
@@ -1 +1 @@
-Subproject commit 118b53334ad785ebe0534758832841dc2786fdc4
+Subproject commit fc75e99f5b2a695c9f52db78198ea7ada7ef6a82
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 01/10] small: sanitized rlist and new region API

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: [PATCH v2 01/10] small: sanitized rlist and new region API
: 

...
: diff --git a/src/lib/small b/src/lib/small
: index 118b53334..fc75e99f5 160000
: --- a/src/lib/small
: +++ b/src/lib/small
: @@ -1 +1 @@
: -Subproject commit 118b53334ad785ebe0534758832841dc2786fdc4
: +Subproject commit fc75e99f5b2a695c9f52db78198ea7ada7ef6a82
: --
: 2.21.1 (Apple Git-122.3)

I guess this is intentional, but just to make sure we are on the 
same line I want to double check - have you actually wanted 
to change small version here? Did you send the relevant patch 
for small? 

Timur

Re: [Tarantool-patches] [PATCH v2 01/10] small: sanitized rlist and new region API

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 28/05/2020 22:41, Timur Safin wrote:
> 
> 
> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : Subject: [PATCH v2 01/10] small: sanitized rlist and new region API
> : 
> 
> ...
> : diff --git a/src/lib/small b/src/lib/small
> : index 118b53334..fc75e99f5 160000
> : --- a/src/lib/small
> : +++ b/src/lib/small
> : @@ -1 +1 @@
> : -Subproject commit 118b53334ad785ebe0534758832841dc2786fdc4
> : +Subproject commit fc75e99f5b2a695c9f52db78198ea7ada7ef6a82
> : --
> : 2.21.1 (Apple Git-122.3)
> 
> I guess this is intentional, but just to make sure we are on the 
> same line I want to double check - have you actually wanted 
> to change small version here? Did you send the relevant patch 
> for small? 

Yes. The whole purpose of the commit - update of 'small' library.
I sent 'small' patch in a separate email thread.

Re: [Tarantool-patches] [PATCH v2 01/10] small: sanitized rlist and new region API

[Vladislav Shpilevoy]

This commit, along with the second one, are pushed to
2.4, 2.3 (in addition to master).

Because the first commit contains a bug fix for lsregion.
And the second commit is needed to be able to build the
project with expressions in offsetof and alignof.

Couldn't push it to 1.10, because seems like something
was changed in small, requiring changes in tarantool's
cmake script:
https://github.com/tarantool/tarantool/issues/5060

[Tarantool-patches] [PATCH v2 10/10] xrow: use unaligned store operation in xrow_to_iovec()

[Vladislav Shpilevoy]

xrow_to_iovec() tried to save a uint32_t value by a not aligned
address. The patch makes it use a special operation for that
instead of regular assignment.

Part of #4609
---
 src/box/xrow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 06473a8bc..bb64864b2 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -892,7 +892,7 @@ xrow_to_iovec(const struct xrow_header *row, struct iovec *out)
 	/* Encode length */
 	char *data = (char *) out[0].iov_base;
 	*(data++) = 0xce; /* MP_UINT32 */
-	*(uint32_t *) data = mp_bswap_u32(len);
+	store_u32(data, mp_bswap_u32(len));
 
 	assert(iovcnt <= XROW_IOVMAX);
 	return iovcnt;
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 10/10] xrow: use unaligned store operation in xrow_to_iovec()

[Timur Safin]

Indeed! LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 10/10] xrow: use unaligned store operation in
: xrow_to_iovec()
: 
: xrow_to_iovec() tried to save a uint32_t value by a not aligned
: address. The patch makes it use a special operation for that
: instead of regular assignment.
: 
: Part of #4609
: ---
:  src/box/xrow.c | 2 +-
:  1 file changed, 1 insertion(+), 1 deletion(-)
: 
: diff --git a/src/box/xrow.c b/src/box/xrow.c
: index 06473a8bc..bb64864b2 100644
: --- a/src/box/xrow.c
: +++ b/src/box/xrow.c
: @@ -892,7 +892,7 @@ xrow_to_iovec(const struct xrow_header *row, struct
: iovec *out)
:  	/* Encode length */
:  	char *data = (char *) out[0].iov_base;
:  	*(data++) = 0xce; /* MP_UINT32 */
: -	*(uint32_t *) data = mp_bswap_u32(len);
: +	store_u32(data, mp_bswap_u32(len));
: 
:  	assert(iovcnt <= XROW_IOVMAX);
:  	return iovcnt;
: --
: 2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 02/10] cmake: ignore warnings on alignof() and offsetof()

[Vladislav Shpilevoy]

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 56429dc20..ce3e7e506 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -276,11 +276,12 @@ 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
         add_compile_flags("CXX"
             "-Wno-invalid-offsetof"
         )
+        add_compile_flags("C;CXX" "-Wno-gnu-alignof-expression")
     endif()
 
     if (CMAKE_COMPILER_IS_GNUCC)
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 02/10] cmake: ignore warnings on alignof() and offsetof()

[Timur Safin]

Nice, LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 02/10] cmake: ignore warnings on alignof() and
: offsetof()
: 
: 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 | 3 ++-
:  1 file changed, 2 insertions(+), 1 deletion(-)
: 
: diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
: index 56429dc20..ce3e7e506 100644
: --- a/cmake/compiler.cmake
: +++ b/cmake/compiler.cmake
: @@ -276,11 +276,12 @@ 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
:          add_compile_flags("CXX"
:              "-Wno-invalid-offsetof"
:          )
: +        add_compile_flags("C;CXX" "-Wno-gnu-alignof-expression")
:      endif()
: 
:      if (CMAKE_COMPILER_IS_GNUCC)
: --
: 2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 02/10] cmake: ignore warnings on alignof() and offsetof()

[Kirill Yukhin]

Hello,

On 28 май 01:32, Vladislav Shpilevoy wrote:
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -276,11 +276,12 @@ 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
>          add_compile_flags("CXX"
>              "-Wno-invalid-offsetof"
>          )

So, you're disabling the warning for clang? According to comment it
was disabled for GCC because of bug in it. Update comment? Enable for
clang?

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 02/10] cmake: ignore warnings on alignof() and offsetof()

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 29/05/2020 08:24, Kirill Yukhin wrote:
> Hello,
> 
> On 28 май 01:32, Vladislav Shpilevoy wrote:
>> --- a/cmake/compiler.cmake
>> +++ b/cmake/compiler.cmake
>> @@ -276,11 +276,12 @@ 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
>>          add_compile_flags("CXX"
>>              "-Wno-invalid-offsetof"
>>          )
> 
> So, you're disabling the warning for clang? According to comment it
> was disabled for GCC because of bug in it. Update comment? Enable for
> clang?

I disable it for clang, because invalid-offsetof is not only
about the thing mentioned in the GCC bug (va_list, which is a
part of struct fiber, which uses offsetof via rlist). It is a
warning about offsetof used on a non-POD type. We use plenty
of non-POD offsetof, every time when use rlist and some other
data structures. Both clang++ and g++ complain about it.

Here is what I get for clang++:

	tarantool/src/box/alter.cc:1148:2: error: offset of on non-standard-layout type 'typeof (*(op))' (aka 'AlterSpaceOp') [-Werror,-Winvalid-offsetof]
	        rlist_foreach_entry(op, &alter->ops, link)

I added a more descriptive comment:

====================
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index ce3e7e506..9ad968d90 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -278,6 +278,11 @@ macro(enable_tnt_compile_flags)
 
     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"
         )

[Tarantool-patches] [PATCH v2 03/10] cmake: add option ENABLE_UB_SANITIZER

[Vladislav Shpilevoy]

Clang has a built-in sanitizer for undefined behaviour. Such as
wrong memory alignment, array boundaries violation, 0 division,
bool values with non standard content, etc.

The sanitizer emits runtime checks which lead to either crash, or
a trap, or a warning print, depending on what is chosen.

The patch makes it possible to turn the sanitizer on and catch
UBs. The only supported UB so far is alignment check. Other types
can be added gradually, along with fixing bugs which they find.

The UB sanitizer is activated for ASAN builds in CI.

Part of #4609
---
 .travis.mk           |  3 ++-
 cmake/compiler.cmake | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/.travis.mk b/.travis.mk
index 063537f25..748321f26 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -114,7 +114,8 @@ coverage_debian: deps_debian test_coverage_debian_no_deps
 
 build_asan_debian:
 	CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \
-		-DENABLE_WERROR=ON -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}
+		-DENABLE_WERROR=ON -DENABLE_ASAN=ON -DENABLE_UB_SANITIZER=ON \
+		${CMAKE_EXTRA_PARAMS}
 	make -j
 
 test_asan_debian_no_deps: build_asan_debian
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index ce3e7e506..373bcd3b0 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -238,6 +238,8 @@ endif()
 
 option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
 
+option(ENABLE_UB_SANITIZER "Make the compiler generate runtime code to perform undefined behaviour checks" OFF)
+
 macro(enable_tnt_compile_flags)
     # Tarantool code is written in GNU C dialect.
     # Additionally, compile it with more strict flags than the rest
@@ -263,6 +265,14 @@ macro(enable_tnt_compile_flags)
         "-Wno-strict-aliasing"
     )
 
+    if (ENABLE_UB_SANITIZER)
+        if (NOT CMAKE_COMPILER_IS_CLANG)
+            message(FATAL_ERROR "Undefined behaviour sanitizer only available for clang")
+        else()
+            add_compile_flags("C;CXX" "-fsanitize=alignment -fno-sanitize-recover=alignment")
+        endif()
+    endif()
+
     if (CMAKE_COMPILER_IS_CLANG AND CC_HAS_WNO_UNUSED_VALUE)
         # False-positive warnings for ({ xx = ...; x; }) macroses
         add_compile_flags("C;CXX" "-Wno-unused-value")
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 03/10] cmake: add option ENABLE_UB_SANITIZER

[Timur Safin]

Very nice!

LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 03/10] cmake: add option ENABLE_UB_SANITIZER
: 
: Clang has a built-in sanitizer for undefined behaviour. Such as
: wrong memory alignment, array boundaries violation, 0 division,
: bool values with non standard content, etc.
: 
: The sanitizer emits runtime checks which lead to either crash, or
: a trap, or a warning print, depending on what is chosen.
: 
: The patch makes it possible to turn the sanitizer on and catch
: UBs. The only supported UB so far is alignment check. Other types
: can be added gradually, along with fixing bugs which they find.
: 
: The UB sanitizer is activated for ASAN builds in CI.
: 
: Part of #4609
: ---
:  .travis.mk           |  3 ++-
:  cmake/compiler.cmake | 10 ++++++++++
:  2 files changed, 12 insertions(+), 1 deletion(-)
: 
: diff --git a/.travis.mk b/.travis.mk
: index 063537f25..748321f26 100644
: --- a/.travis.mk
: +++ b/.travis.mk
: @@ -114,7 +114,8 @@ coverage_debian: deps_debian
: test_coverage_debian_no_deps
: 
:  build_asan_debian:
:  	CC=clang-8 CXX=clang++-8 cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \
: -		-DENABLE_WERROR=ON -DENABLE_ASAN=ON ${CMAKE_EXTRA_PARAMS}
: +		-DENABLE_WERROR=ON -DENABLE_ASAN=ON -DENABLE_UB_SANITIZER=ON \
: +		${CMAKE_EXTRA_PARAMS}
:  	make -j
: 
:  test_asan_debian_no_deps: build_asan_debian
: diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
: index ce3e7e506..373bcd3b0 100644
: --- a/cmake/compiler.cmake
: +++ b/cmake/compiler.cmake
: @@ -238,6 +238,8 @@ endif()
: 
:  option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
: 
: +option(ENABLE_UB_SANITIZER "Make the compiler generate runtime code to
: perform undefined behaviour checks" OFF)
: +
:  macro(enable_tnt_compile_flags)
:      # Tarantool code is written in GNU C dialect.
:      # Additionally, compile it with more strict flags than the rest
: @@ -263,6 +265,14 @@ macro(enable_tnt_compile_flags)
:          "-Wno-strict-aliasing"
:      )
: 
: +    if (ENABLE_UB_SANITIZER)
: +        if (NOT CMAKE_COMPILER_IS_CLANG)
: +            message(FATAL_ERROR "Undefined behaviour sanitizer only
: available for clang")
: +        else()
: +            add_compile_flags("C;CXX" "-fsanitize=alignment -fno-
: sanitize-recover=alignment")
: +        endif()
: +    endif()
: +
:      if (CMAKE_COMPILER_IS_CLANG AND CC_HAS_WNO_UNUSED_VALUE)
:          # False-positive warnings for ({ xx = ...; x; }) macroses
:          add_compile_flags("C;CXX" "-Wno-unused-value")
: --
: 2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 03/10] cmake: add option ENABLE_UB_SANITIZER

[Sergey Bronnikov]

Vlad,

thanks for the patch!

On 01:32 Thu 28 May , Vladislav Shpilevoy wrote:

<snipped>

> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
> index ce3e7e506..373bcd3b0 100644
> --- a/cmake/compiler.cmake
> +++ b/cmake/compiler.cmake
> @@ -238,6 +238,8 @@ endif()
>  
>  option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
>  
> +option(ENABLE_UB_SANITIZER "Make the compiler generate runtime code to perform undefined behaviour checks" OFF)

It would be useful to add ENABLE_UB_SANITIZER option to a list "options"
in a root CMakeLists.txt. cmake shows options from this list and it's
status at the end of output.

<snipped>

Re: [Tarantool-patches] [PATCH v2 03/10] cmake: add option ENABLE_UB_SANITIZER

[Vladislav Shpilevoy]

Hi! Thanks for the comment!

On 29/05/2020 10:53, Sergey Bronnikov wrote:
> Vlad,
> 
> thanks for the patch!
> 
> On 01:32 Thu 28 May , Vladislav Shpilevoy wrote:
> 
> <snipped>
> 
>> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
>> index ce3e7e506..373bcd3b0 100644
>> --- a/cmake/compiler.cmake
>> +++ b/cmake/compiler.cmake
>> @@ -238,6 +238,8 @@ endif()
>>  
>>  option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
>>  
>> +option(ENABLE_UB_SANITIZER "Make the compiler generate runtime code to perform undefined behaviour checks" OFF)
> 
> It would be useful to add ENABLE_UB_SANITIZER option to a list "options"
> in a root CMakeLists.txt. cmake shows options from this list and it's
> status at the end of output.

I didn't even know that the option list existed, nice. Added:

====================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1d80b6806..e49317f8a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -571,7 +571,7 @@ set(PREFIX ${CMAKE_INSTALL_PREFIX})
 set(options PACKAGE VERSION BUILD C_COMPILER CXX_COMPILER C_FLAGS CXX_FLAGS
     PREFIX
     ENABLE_SSE2 ENABLE_AVX
-    ENABLE_GCOV ENABLE_GPROF ENABLE_VALGRIND ENABLE_ASAN
+    ENABLE_GCOV ENABLE_GPROF ENABLE_VALGRIND ENABLE_ASAN ENABLE_UB_SANITIZER
     ENABLE_BACKTRACE
     ENABLE_DOC
     ENABLE_DIST

[Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

[Vladislav Shpilevoy]

There is some assembly working with a byte array like with an
array of unsigned long values. That is incorrect, because the byte
array may be not aligned by 'unsigned long'.

The patch makes crc32 calculate the hash on the prefix
byte-by-byte, then word-by-word on aligned addresses, and again
byte-by-byte on a tail which is less than word.

Assuming that the word-by-word part is the longest, this should
reduce number of memory/cache loads in ~x2 times. Because in case
of a not aligned word load it was necessary to load 2 words, and
then merge them into one. When addresses are aligned, this is only
1 load.

Part of #4609
---
 src/cpu_feature.c        | 31 ++++++++++++-----
 test/unit/CMakeLists.txt |  3 ++
 test/unit/crc32.c        | 73 ++++++++++++++++++++++++++++++++++++++++
 test/unit/crc32.result   | 11 ++++++
 4 files changed, 110 insertions(+), 8 deletions(-)
 create mode 100644 test/unit/crc32.c
 create mode 100644 test/unit/crc32.result

diff --git a/src/cpu_feature.c b/src/cpu_feature.c
index 98567ccb3..9bf6223de 100644
--- a/src/cpu_feature.c
+++ b/src/cpu_feature.c
@@ -29,6 +29,7 @@
  * SUCH DAMAGE.
  */
 #include "trivia/config.h"
+#include "trivia/util.h"
 #include <sys/types.h>
 #include <errno.h>
 #include <stdlib.h>
@@ -50,7 +51,7 @@
 
 
 static uint32_t
-crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int length)
+crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
 {
 	while (length--) {
 		__asm__ __volatile__(
@@ -68,6 +69,26 @@ crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int length)
 uint32_t
 crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
 {
+	const int align = alignof(unsigned long);
+	unsigned long addr = (unsigned long)buf;
+	unsigned int not_aligned_prefix =
+		((addr - 1 + align) & ~(align - 1)) - addr;
+	/*
+	 * Calculate CRC32 for the prefix byte-by-byte so as to
+	 * then use aligned words to calculate the rest. This is
+	 * twice less loads, because every load takes exactly one
+	 * word from memory. Not 2 words, which would need to be
+	 * partially merged then.
+	 * But the main reason is that unaligned loads are just
+	 * unsafe, because this is an undefined behaviour.
+	 */
+	if (not_aligned_prefix < len) {
+		crc = crc32c_hw_byte(crc, buf, not_aligned_prefix);
+		buf += not_aligned_prefix;
+		len -= not_aligned_prefix;
+	} else {
+		return crc32c_hw_byte(crc, buf, len);
+	}
 	unsigned int iquotient = len / SCALE_F;
 	unsigned int iremainder = len % SCALE_F;
 	unsigned long *ptmp = (unsigned long *)buf;
@@ -80,13 +101,7 @@ crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
 		);
 		ptmp++;
 	}
-
-	if (iremainder) {
-		crc = crc32c_hw_byte(crc, (unsigned char const*)ptmp,
-							iremainder);
-	}
-
-	return crc;
+	return crc32c_hw_byte(crc, (const char *)ptmp, iremainder);
 }
 
 bool
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 3bdfc6bb1..672122118 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -138,6 +138,9 @@ target_link_libraries(scramble.test scramble)
 add_executable(guava.test guava.c)
 target_link_libraries(guava.test salad small)
 
+add_executable(crc32.test crc32.c)
+target_link_libraries(crc32.test unit crc32)
+
 add_executable(find_path.test find_path.c
     ${CMAKE_SOURCE_DIR}/src/find_path.c
 )
diff --git a/test/unit/crc32.c b/test/unit/crc32.c
new file mode 100644
index 000000000..7493bc0db
--- /dev/null
+++ b/test/unit/crc32.c
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#include "unit.h"
+#include "crc32.h"
+
+static void
+test_alignment(void)
+{
+	header();
+	plan(4);
+
+	unsigned long buf[1024];
+	char *str = (char *)buf;
+
+	strcpy(str, "1234567891234567");
+	uint32_t crc = crc32_calc(0, str, strlen(str));
+	is(crc, 3333896965, "aligned crc32 buffer without a tail");
+
+	strcpy(str, "12345678912345678");
+	crc = crc32_calc(0, str, strlen(str));
+	is(crc, 2400039513, "aligned crc32 buffer with a tail");
+
+	crc = crc32_calc(0, str + 2, strlen(str) - 2);
+	is(crc, 984331636, "not aligned crc32 buffer with a tail");
+
+	strcpy(str, "1234");
+	crc = crc32_calc(0, str + 2, strlen(str) - 2);
+	is(crc, 2211472564, "not aligned buffer less than a word");
+
+	check_plan();
+	footer();
+}
+
+int
+main(void)
+{
+	crc32_init();
+
+	header();
+	plan(1);
+	test_alignment();
+	int rc = check_plan();
+	footer();
+	return rc;
+}
diff --git a/test/unit/crc32.result b/test/unit/crc32.result
new file mode 100644
index 000000000..99b62c22a
--- /dev/null
+++ b/test/unit/crc32.result
@@ -0,0 +1,11 @@
+	*** main ***
+1..1
+	*** test_alignment ***
+    1..4
+    ok 1 - aligned crc32 buffer without a tail
+    ok 2 - aligned crc32 buffer with a tail
+    ok 3 - not aligned crc32 buffer with a tail
+    ok 4 - not aligned buffer less than a word
+ok 1 - subtests
+	*** test_alignment: done ***
+	*** main: done ***
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

[Timur Safin]

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: [PATCH v2 04/10] crc32: align memory access
: 
: 
: diff --git a/src/cpu_feature.c b/src/cpu_feature.c
: index 98567ccb3..9bf6223de 100644
: --- a/src/cpu_feature.c
: +++ b/src/cpu_feature.c
: @@ -50,7 +51,7 @@
: 
: 
:  static uint32_t
: -crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int
: length)
: +crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
:  {
:  	while (length--) {
:  		__asm__ __volatile__(
: @@ -68,6 +69,26 @@ crc32c_hw_byte(uint32_t crc, unsigned char const *data,
: unsigned int length)
:  uint32_t
:  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
:  {
: +	const int align = alignof(unsigned long);
: +	unsigned long addr = (unsigned long)buf;
: +	unsigned int not_aligned_prefix =
: +		((addr - 1 + align) & ~(align - 1)) - addr;

Hmm, hmm...

Isn't it simple `addr % align`? Or even `addr & (align - 1)` ?


: +	/*
: +	 * Calculate CRC32 for the prefix byte-by-byte so as to
: +	 * then use aligned words to calculate the rest. This is
: +	 * twice less loads, because every load takes exactly one
: +	 * word from memory. Not 2 words, which would need to be
: +	 * partially merged then.
: +	 * But the main reason is that unaligned loads are just
: +	 * unsafe, because this is an undefined behaviour.
: +	 */
: +	if (not_aligned_prefix < len) {
: +		crc = crc32c_hw_byte(crc, buf, not_aligned_prefix);
: +		buf += not_aligned_prefix;
: +		len -= not_aligned_prefix;
: +	} else {
: +		return crc32c_hw_byte(crc, buf, len);
: +	}
:  	unsigned int iquotient = len / SCALE_F;
:  	unsigned int iremainder = len % SCALE_F;
:  	unsigned long *ptmp = (unsigned long *)buf;

Re: [Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

[Vladislav Shpilevoy]

Thanks for the review!

On 28/05/2020 22:11, Timur Safin wrote:
> 
> 
> : -----Original Message-----
> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : Subject: [PATCH v2 04/10] crc32: align memory access
> : 
> : 
> : diff --git a/src/cpu_feature.c b/src/cpu_feature.c
> : index 98567ccb3..9bf6223de 100644
> : --- a/src/cpu_feature.c
> : +++ b/src/cpu_feature.c
> : @@ -50,7 +51,7 @@
> : 
> : 
> :  static uint32_t
> : -crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int
> : length)
> : +crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
> :  {
> :  	while (length--) {
> :  		__asm__ __volatile__(
> : @@ -68,6 +69,26 @@ crc32c_hw_byte(uint32_t crc, unsigned char const *data,
> : unsigned int length)
> :  uint32_t
> :  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
> :  {
> : +	const int align = alignof(unsigned long);
> : +	unsigned long addr = (unsigned long)buf;
> : +	unsigned int not_aligned_prefix =
> : +		((addr - 1 + align) & ~(align - 1)) - addr;
> 
> Hmm, hmm...
> 
> Isn't it simple `addr % align`? Or even `addr & (align - 1)` ?

Consider the example: addr = 14, align = 8.
Then not_aligned_prefix = 2. Need to read first 2
bytes one by one to get to 16, the closest aligned
address.

addr % align = 14 % 8 = 6 != 2
addr & (align - 1) = 14 & 7 = 1110 & 0111 = 110 = 6 != 2

But yeah, this could be done simpler: align - addr % align.
This will give how many bytes are needed to the next
aligned address. I wrote the solution above by blindly using
'aligned - not aligned' and the same schema as in
small_align().

Here is the diff:

====================
diff --git a/src/cpu_feature.c b/src/cpu_feature.c
index 9bf6223de..7b284fa98 100644
--- a/src/cpu_feature.c
+++ b/src/cpu_feature.c
@@ -69,10 +69,8 @@ crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
 uint32_t
 crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
 {
-	const int align = alignof(unsigned long);
-	unsigned long addr = (unsigned long)buf;
-	unsigned int not_aligned_prefix =
-		((addr - 1 + align) & ~(align - 1)) - addr;
+	const unsigned int align = alignof(unsigned long);
+	unsigned int not_aligned_prefix = align - (unsigned long)buf % align;
 	/*
 	 * Calculate CRC32 for the prefix byte-by-byte so as to
 	 * then use aligned words to calculate the rest. This is

Re: [Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

[Timur Safin]

Yup, this is better

LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Friday, May 29, 2020 2:23 AM
: To: Timur Safin <tsafin@tarantool.org>; tarantool-
: patches@dev.tarantool.org; alyapunov@tarantool.org; korablev@tarantool.org
: Subject: Re: [PATCH v2 04/10] crc32: align memory access
: 
: Thanks for the review!
: 
: On 28/05/2020 22:11, Timur Safin wrote:
: >
: >
: > : -----Original Message-----
: > : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: > : Subject: [PATCH v2 04/10] crc32: align memory access
: > :
: > :
: > : diff --git a/src/cpu_feature.c b/src/cpu_feature.c
: > : index 98567ccb3..9bf6223de 100644
: > : --- a/src/cpu_feature.c
: > : +++ b/src/cpu_feature.c
: > : @@ -50,7 +51,7 @@
: > :
: > :
: > :  static uint32_t
: > : -crc32c_hw_byte(uint32_t crc, unsigned char const *data, unsigned int
: > : length)
: > : +crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
: > :  {
: > :  	while (length--) {
: > :  		__asm__ __volatile__(
: > : @@ -68,6 +69,26 @@ crc32c_hw_byte(uint32_t crc, unsigned char const
: *data,
: > : unsigned int length)
: > :  uint32_t
: > :  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
: > :  {
: > : +	const int align = alignof(unsigned long);
: > : +	unsigned long addr = (unsigned long)buf;
: > : +	unsigned int not_aligned_prefix =
: > : +		((addr - 1 + align) & ~(align - 1)) - addr;
: >
: > Hmm, hmm...
: >
: > Isn't it simple `addr % align`? Or even `addr & (align - 1)` ?
: 
: Consider the example: addr = 14, align = 8.
: Then not_aligned_prefix = 2. Need to read first 2
: bytes one by one to get to 16, the closest aligned
: address.
: 
: addr % align = 14 % 8 = 6 != 2
: addr & (align - 1) = 14 & 7 = 1110 & 0111 = 110 = 6 != 2
: 
: But yeah, this could be done simpler: align - addr % align.
: This will give how many bytes are needed to the next
: aligned address. I wrote the solution above by blindly using
: 'aligned - not aligned' and the same schema as in
: small_align().
: 
: Here is the diff:
: 
: ====================
: diff --git a/src/cpu_feature.c b/src/cpu_feature.c
: index 9bf6223de..7b284fa98 100644
: --- a/src/cpu_feature.c
: +++ b/src/cpu_feature.c
: @@ -69,10 +69,8 @@ crc32c_hw_byte(uint32_t crc, char const *data, unsigned
: int length)
:  uint32_t
:  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
:  {
: -	const int align = alignof(unsigned long);
: -	unsigned long addr = (unsigned long)buf;
: -	unsigned int not_aligned_prefix =
: -		((addr - 1 + align) & ~(align - 1)) - addr;
: +	const unsigned int align = alignof(unsigned long);
: +	unsigned int not_aligned_prefix = align - (unsigned long)buf %
: align;
:  	/*
:  	 * Calculate CRC32 for the prefix byte-by-byte so as to
:  	 * then use aligned words to calculate the rest. This is

Re: [Tarantool-patches] [PATCH v2 04/10] crc32: align memory access

[Vladislav Shpilevoy]

> diff --git a/src/cpu_feature.c b/src/cpu_feature.c
> index 9bf6223de..7b284fa98 100644
> --- a/src/cpu_feature.c
> +++ b/src/cpu_feature.c
> @@ -69,10 +69,8 @@ crc32c_hw_byte(uint32_t crc, char const *data, unsigned int length)
>  uint32_t
>  crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
>  {
> -	const int align = alignof(unsigned long);
> -	unsigned long addr = (unsigned long)buf;
> -	unsigned int not_aligned_prefix =
> -		((addr - 1 + align) & ~(align - 1)) - addr;
> +	const unsigned int align = alignof(unsigned long);
> +	unsigned int not_aligned_prefix = align - (unsigned long)buf % align;

When the address is aligned, not_aligned_prefix becomes = align.
For 8 byte word it means we will do 8 operations instead of 1.

I fixed it this way:

====================
diff --git a/src/cpu_feature.c b/src/cpu_feature.c
index 7b284fa98..856f054c7 100644
--- a/src/cpu_feature.c
+++ b/src/cpu_feature.c
@@ -70,7 +70,8 @@ uint32_t
 crc32c_hw(uint32_t crc, const char *buf, unsigned int len)
 {
 	const unsigned int align = alignof(unsigned long);
-	unsigned int not_aligned_prefix = align - (unsigned long)buf % align;
+	unsigned int not_aligned_prefix =
+		(align - (unsigned long)buf % align) % align;
 	/*
 	 * Calculate CRC32 for the prefix byte-by-byte so as to
 	 * then use aligned words to calculate the rest. This is

====================
This is fast, because % align is transformed into & (align - 1)
in the assembly.

[Tarantool-patches] [PATCH v2 05/10] sql: make BtCursor's memory aligned

[Vladislav Shpilevoy]

Vdbe at runtime allocates VdbeCursor structure using
allocateCursor() function. Inside there is a pointer at BtCursor
structure. To make the allocation faster and improve cache
locality, both cursors are allocated in one memory block + some
extra memory for uint32_t array, where BtCursor followed
VdbeCursor and the array without any padding:

   VdbeCursor + uint32_t * N + BtCursor

The problem is that BtCursor needs 8 byte alignment. When it
followed VdbeCursor (aligned by 8) + some uint32_t values, its
actual alignment could become 4 bytes. That led to a crash when
alignment sanitizer is enabled in clang.

The patch makes BtCursor offset aligned by 8 bytes.

Part of #4609
---
 src/box/sql/vdbe.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 724bc188b..7a42602a2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -234,10 +234,9 @@ allocateCursor(
 	 */
 	Mem *pMem = iCur>0 ? &p->aMem[p->nMem-iCur] : p->aMem;
 
-	int nByte;
 	VdbeCursor *pCx = 0;
-	nByte =
-		ROUND8(sizeof(VdbeCursor)) + sizeof(u32)*nField +
+	int bt_offset = ROUND8(sizeof(VdbeCursor) + sizeof(uint32_t) * nField);
+	int nByte = bt_offset +
 		(eCurType==CURTYPE_TARANTOOL ? ROUND8(sizeof(BtCursor)) : 0);
 
 	assert(iCur>=0 && iCur<p->nCursor);
@@ -251,8 +250,7 @@ allocateCursor(
 		pCx->eCurType = eCurType;
 		pCx->nField = nField;
 		if (eCurType==CURTYPE_TARANTOOL) {
-			pCx->uc.pCursor = (BtCursor*)
-				&pMem->z[ROUND8(sizeof(VdbeCursor))+sizeof(u32)*nField];
+			pCx->uc.pCursor = (BtCursor*)&pMem->z[bt_offset];
 			sqlCursorZero(pCx->uc.pCursor);
 		}
 	}
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 05/10] sql: make BtCursor's memory aligned

[Timur Safin]

Quite tricky case, thanks!

LGTM!

Timur

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 05/10] sql: make BtCursor's memory aligned
: 
: Vdbe at runtime allocates VdbeCursor structure using
: allocateCursor() function. Inside there is a pointer at BtCursor
: structure. To make the allocation faster and improve cache
: locality, both cursors are allocated in one memory block + some
: extra memory for uint32_t array, where BtCursor followed
: VdbeCursor and the array without any padding:
: 
:    VdbeCursor + uint32_t * N + BtCursor
: 
: The problem is that BtCursor needs 8 byte alignment. When it
: followed VdbeCursor (aligned by 8) + some uint32_t values, its
: actual alignment could become 4 bytes. That led to a crash when
: alignment sanitizer is enabled in clang.
: 
: The patch makes BtCursor offset aligned by 8 bytes.
: 
: Part of #4609
: ---
:  src/box/sql/vdbe.c | 8 +++-----
:  1 file changed, 3 insertions(+), 5 deletions(-)
: 
: diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
: index 724bc188b..7a42602a2 100644
: --- a/src/box/sql/vdbe.c
: +++ b/src/box/sql/vdbe.c
: @@ -234,10 +234,9 @@ allocateCursor(
:  	 */
:  	Mem *pMem = iCur>0 ? &p->aMem[p->nMem-iCur] : p->aMem;
: 
: -	int nByte;
:  	VdbeCursor *pCx = 0;
: -	nByte =
: -		ROUND8(sizeof(VdbeCursor)) + sizeof(u32)*nField +
: +	int bt_offset = ROUND8(sizeof(VdbeCursor) + sizeof(uint32_t) *
: nField);
: +	int nByte = bt_offset +
:  		(eCurType==CURTYPE_TARANTOOL ? ROUND8(sizeof(BtCursor)) : 0);
: 
:  	assert(iCur>=0 && iCur<p->nCursor);
: @@ -251,8 +250,7 @@ allocateCursor(
:  		pCx->eCurType = eCurType;
:  		pCx->nField = nField;
:  		if (eCurType==CURTYPE_TARANTOOL) {
: -			pCx->uc.pCursor = (BtCursor*)
: -				&pMem-
: >z[ROUND8(sizeof(VdbeCursor))+sizeof(u32)*nField];
: +			pCx->uc.pCursor = (BtCursor*)&pMem->z[bt_offset];
:  			sqlCursorZero(pCx->uc.pCursor);
:  		}
:  	}
: --
: 2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 06/10] region: use aligned allocations where necessary

[Vladislav Shpilevoy]

Region is used for temporary allocations, usually bound to a
transaction, but not always. Keyword - temporary. It is usually
much faster to allocate something on the region than on the heap,
and much much faster to free the region, since it frees data in
whole slabs. Lots of objects at once.

Region is used both for byte arrays (MessagePack, strings, blobs),
and for objects like standard types, structs. Almost always for
allocations was used region_alloc() function, which returns a not
aligned address. It can be anything, even not multiple of 2.

That led to alignment violation for standard types and structs.
Usually it is harmless in terms of errors, but can be slower than
aligned access, and on some platforms may even crash. Also the
crash can be forced using clang undefined behaviour sanitizer,
which revealed all the not aligned accesses fixed in this patch.

Part of #4609
---
 src/box/alter.cc            | 39 ++++++++++++++----------
 src/box/applier.cc          | 22 +++++++-------
 src/box/bind.c              |  7 +++--
 src/box/ck_constraint.c     | 11 +++----
 src/box/field_map.c         | 17 ++++++-----
 src/box/fk_constraint.h     | 14 +++++----
 src/box/index_def.c         |  9 +++---
 src/box/key_def.c           |  9 +++---
 src/box/lua/execute.c       |  7 +++--
 src/box/lua/key_def.c       |  7 +++--
 src/box/lua/misc.cc         |  8 ++---
 src/box/memtx_tree.c        |  7 +++--
 src/box/space_def.c         |  7 +++--
 src/box/sql.c               | 20 ++++++++-----
 src/box/sql/build.c         | 60 ++++++++++++++++++++++---------------
 src/box/sql/func.c          |  7 +++--
 src/box/sql/select.c        | 14 +++++----
 src/box/sql/update.c        |  6 ++--
 src/box/sql/vdbe.c          |  8 ++---
 src/box/sql/wherecode.c     |  9 ++++--
 src/box/tuple_format.c      |  3 +-
 src/box/txn.c               | 12 ++++----
 src/box/user.cc             |  8 ++---
 src/box/vinyl.c             | 26 +++++++++-------
 src/box/vy_log.c            | 33 ++++++++++----------
 src/box/vy_point_lookup.c   |  9 +++---
 src/box/xrow_update_map.c   |  7 +++--
 src/box/xrow_update_route.c |  7 +++--
 src/lib/core/backtrace.cc   |  6 ++--
 src/lua/popen.c             | 20 +++++++------
 30 files changed, 240 insertions(+), 179 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index dbbbcbc44..bb4254878 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -537,11 +537,13 @@ space_format_decode(const char *data, uint32_t *out_count,
 		*fields = NULL;
 		return 0;
 	}
-	size_t size = count * sizeof(struct field_def);
+	size_t size;
 	struct field_def *region_defs =
-		(struct field_def *) region_alloc(region, size);
+		region_alloc_array(region, typeof(region_defs[0]), count,
+				   &size);
 	if (region_defs == NULL) {
-		diag_set(OutOfMemory, size, "region", "struct field_def");
+		diag_set(OutOfMemory, size, "region_alloc_array",
+			 "region_defs");
 		return -1;
 	}
 	/*
@@ -2452,10 +2454,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * comparators must be updated.
 		 */
 		struct key_def **keys;
-		size_t bsize = old_space->index_count * sizeof(keys[0]);
-		keys = (struct key_def **) region_alloc(&fiber()->gc, bsize);
+		size_t bsize;
+		keys = region_alloc_array(&fiber()->gc, typeof(keys[0]),
+					  old_space->index_count, &bsize);
 		if (keys == NULL) {
-			diag_set(OutOfMemory, bsize, "region", "new slab");
+			diag_set(OutOfMemory, bsize, "region_alloc_array",
+				 "keys");
 			return -1;
 		}
 		for (uint32_t i = 0; i < old_space->index_count; ++i)
@@ -2733,10 +2737,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 		 * index and a space format, defined by a user.
 		 */
 		struct key_def **keys;
-		size_t bsize = old_space->index_count * sizeof(keys[0]);
-		keys = (struct key_def **) region_alloc(&fiber()->gc, bsize);
+		size_t bsize;
+		keys = region_alloc_array(&fiber()->gc, typeof(keys[0]),
+					  old_space->index_count, &bsize);
 		if (keys == NULL) {
-			diag_set(OutOfMemory, bsize, "region", "new slab");
+			diag_set(OutOfMemory, bsize, "region_alloc_array",
+				 "keys");
 			return -1;
 		}
 		for (uint32_t i = 0, j = 0; i < old_space->index_count; ++i) {
@@ -5009,11 +5015,13 @@ decode_fk_links(struct tuple *tuple, uint32_t *out_count,
 		return NULL;
 	}
 	*out_count = count;
-	size_t size = count * sizeof(struct field_link);
+	size_t size;
 	struct field_link *region_links =
-		(struct field_link *)region_alloc(&fiber()->gc, size);
+		region_alloc_array(&fiber()->gc, typeof(region_links[0]), count,
+				   &size);
 	if (region_links == NULL) {
-		diag_set(OutOfMemory, size, "region", "struct field_link");
+		diag_set(OutOfMemory, size, "region_alloc_array",
+			 "region_links");
 		return NULL;
 	}
 	memset(region_links, 0, size);
@@ -5054,7 +5062,9 @@ fk_constraint_def_new_from_tuple(struct tuple *tuple, uint32_t errcode)
 						   name_len, errcode);
 	if (links == NULL)
 		return NULL;
-	size_t fk_def_sz = fk_constraint_def_sizeof(link_count, name_len);
+	uint32_t links_offset;
+	size_t fk_def_sz = fk_constraint_def_sizeof(link_count, name_len,
+						    &links_offset);
 	struct fk_constraint_def *fk_def =
 		(struct fk_constraint_def *) malloc(fk_def_sz);
 	if (fk_def == NULL) {
@@ -5065,8 +5075,7 @@ fk_constraint_def_new_from_tuple(struct tuple *tuple, uint32_t errcode)
 	auto def_guard = make_scoped_guard([=] { free(fk_def); });
 	memcpy(fk_def->name, name, name_len);
 	fk_def->name[name_len] = '\0';
-	fk_def->links = (struct field_link *)((char *)&fk_def->name +
-					      name_len + 1);
+	fk_def->links = (struct field_link *)((char *)fk_def + links_offset);
 	memcpy(fk_def->links, links, link_count * sizeof(struct field_link));
 	fk_def->field_count = link_count;
 	if (tuple_field_u32(tuple, BOX_FK_CONSTRAINT_FIELD_CHILD_ID,
diff --git a/src/box/applier.cc b/src/box/applier.cc
index c6deeff1b..df48b4796 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -588,13 +588,12 @@ applier_read_tx_row(struct applier *applier)
 {
 	struct ev_io *coio = &applier->io;
 	struct ibuf *ibuf = &applier->ibuf;
-
-	struct applier_tx_row *tx_row = (struct applier_tx_row *)
-		region_alloc(&fiber()->gc, sizeof(struct applier_tx_row));
+	size_t size;
+	struct applier_tx_row *tx_row =
+		region_alloc_object(&fiber()->gc, typeof(*tx_row), &size);
 
 	if (tx_row == NULL)
-		tnt_raise(OutOfMemory, sizeof(struct applier_tx_row),
-			  "region", "struct applier_tx_row");
+		tnt_raise(OutOfMemory, size, "region_alloc_object", "tx_row");
 
 	struct xrow_header *row = &tx_row->row;
 
@@ -809,13 +808,14 @@ applier_apply_tx(struct stailq *rows)
 
 	/* We are ready to submit txn to wal. */
 	struct trigger *on_rollback, *on_commit;
-	on_rollback = (struct trigger *)region_alloc(&txn->region,
-						     sizeof(struct trigger));
-	on_commit = (struct trigger *)region_alloc(&txn->region,
-						   sizeof(struct trigger));
+	size_t size;
+	on_rollback = region_alloc_object(&txn->region, typeof(*on_rollback),
+					  &size);
+	on_commit = region_alloc_object(&txn->region, typeof(*on_commit),
+					&size);
 	if (on_rollback == NULL || on_commit == NULL) {
-		diag_set(OutOfMemory, sizeof(struct trigger),
-			 "region_alloc", "on_rollback/on_commit");
+		diag_set(OutOfMemory, size, "region_alloc_object",
+			 "on_rollback/on_commit");
 		goto rollback;
 	}
 
diff --git a/src/box/bind.c b/src/box/bind.c
index bbc1f56df..d45a0f9a7 100644
--- a/src/box/bind.c
+++ b/src/box/bind.c
@@ -137,10 +137,11 @@ sql_bind_list_decode(const char *data, struct sql_bind **out_bind)
 	}
 	struct region *region = &fiber()->gc;
 	uint32_t used = region_used(region);
-	size_t size = sizeof(struct sql_bind) * bind_count;
-	struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
+	size_t size;
+	struct sql_bind *bind = region_alloc_array(region, typeof(bind[0]),
+						   bind_count, &size);
 	if (bind == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "struct sql_bind");
+		diag_set(OutOfMemory, size, "region_alloc_array", "bind");
 		return -1;
 	}
 	for (uint32_t i = 0; i < bind_count; ++i) {
diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index ff3f05587..b629a73eb 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -190,12 +190,13 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event)
 
 	struct space *space = stmt->space;
 	assert(space != NULL);
-	uint32_t field_ref_sz = sizeof(struct vdbe_field_ref) +
-				sizeof(uint32_t) * space->def->field_count;
-	struct vdbe_field_ref *field_ref =
-		region_alloc(&fiber()->gc, field_ref_sz);
+	struct vdbe_field_ref *field_ref;
+	size_t size = sizeof(field_ref->slots[0]) * space->def->field_count +
+		      sizeof(*field_ref);
+	field_ref = (struct vdbe_field_ref *)
+		region_aligned_alloc(&fiber()->gc, size, alignof(*field_ref));
 	if (field_ref == NULL) {
-		diag_set(OutOfMemory, field_ref_sz, "region_alloc",
+		diag_set(OutOfMemory, size, "region_aligned_alloc",
 			 "field_ref");
 		return -1;
 	}
diff --git a/src/box/field_map.c b/src/box/field_map.c
index 1876bdd95..5f4661941 100644
--- a/src/box/field_map.c
+++ b/src/box/field_map.c
@@ -43,10 +43,12 @@ field_map_builder_create(struct field_map_builder *builder,
 		builder->slots = NULL;
 		return 0;
 	}
-	uint32_t sz = builder->slot_count * sizeof(builder->slots[0]);
-	builder->slots = region_alloc(region, sz);
+	uint32_t sz;
+	builder->slots = region_alloc_array(region, typeof(builder->slots[0]),
+					    builder->slot_count, &sz);
 	if (builder->slots == NULL) {
-		diag_set(OutOfMemory, sz, "region_alloc", "field_map");
+		diag_set(OutOfMemory, sz, "region_alloc_array",
+			 "builder->slots");
 		return -1;
 	}
 	memset((char *)builder->slots, 0, sz);
@@ -61,11 +63,12 @@ field_map_builder_slot_extent_new(struct field_map_builder *builder,
 {
 	struct field_map_builder_slot_extent *extent;
 	assert(!builder->slots[offset_slot].has_extent);
-	uint32_t sz = sizeof(struct field_map_builder_slot_extent) +
-		      multikey_count * sizeof(uint32_t);
-	extent = region_alloc(region, sz);
+	uint32_t sz = sizeof(*extent) +
+		      multikey_count * sizeof(extent->offset[0]);
+	extent = (struct field_map_builder_slot_extent *)
+		region_aligned_alloc(region, sz, alignof(*extent));
 	if (extent == NULL) {
-		diag_set(OutOfMemory, sz, "region", "extent");
+		diag_set(OutOfMemory, sz, "region_aligned_alloc", "extent");
 		return NULL;
 	}
 	memset(extent, 0, sz);
diff --git a/src/box/fk_constraint.h b/src/box/fk_constraint.h
index b1e0cfb84..dcc5363c0 100644
--- a/src/box/fk_constraint.h
+++ b/src/box/fk_constraint.h
@@ -32,7 +32,8 @@
  */
 #include <stdbool.h>
 #include <stdint.h>
-
+#include "trivia/util.h"
+#include "small/slab_arena.h"
 #include "small/rlist.h"
 
 #if defined(__cplusplus)
@@ -125,15 +126,18 @@ struct fk_constraint {
  * |----------------------------------|
  * |             name + \0            |
  * |----------------------------------|
+ * |       memory align padding       |
+ * |----------------------------------|
  * |             links                |
  * +----------------------------------+
  */
 static inline size_t
-fk_constraint_def_sizeof(uint32_t link_count, uint32_t name_len)
+fk_constraint_def_sizeof(uint32_t link_count, uint32_t name_len,
+			 uint32_t *links_offset)
 {
-	return sizeof(struct fk_constraint_def) +
-		link_count * sizeof(struct field_link) +
-		name_len + 1;
+	*links_offset = small_align(sizeof(struct fk_constraint_def) +
+				    name_len + 1, alignof(struct field_link));
+	return *links_offset + link_count * sizeof(struct field_link);
 }
 
 static inline bool
diff --git a/src/box/index_def.c b/src/box/index_def.c
index 85128b1a5..98029612c 100644
--- a/src/box/index_def.c
+++ b/src/box/index_def.c
@@ -255,11 +255,12 @@ index_def_to_key_def(struct rlist *index_defs, int *size)
 	struct index_def *index_def;
 	rlist_foreach_entry(index_def, index_defs, link)
 		key_count++;
-	size_t sz = sizeof(struct key_def *) * key_count;
-	struct key_def **keys = (struct key_def **) region_alloc(&fiber()->gc,
-								 sz);
+	size_t bsize;
+	struct key_def **keys =
+		region_alloc_array(&fiber()->gc, typeof(keys[0]), key_count,
+				   &bsize);
 	if (keys == NULL) {
-		diag_set(OutOfMemory, sz, "region_alloc", "keys");
+		diag_set(OutOfMemory, bsize, "region_alloc_array", "keys");
 		return NULL;
 	}
 	*size = key_count;
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 3e3782163..18af44961 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -847,11 +847,12 @@ key_def_find_pk_in_cmp_def(const struct key_def *cmp_def,
 	size_t region_svp = region_used(region);
 
 	/* First, dump primary key parts as is. */
-	struct key_part_def *parts = region_alloc(region,
-			pk_def->part_count * sizeof(*parts));
+	size_t size;
+	struct key_part_def *parts =
+		region_alloc_array(region, typeof(parts[0]), pk_def->part_count,
+				   &size);
 	if (parts == NULL) {
-		diag_set(OutOfMemory, pk_def->part_count * sizeof(*parts),
-			 "region", "key def parts");
+		diag_set(OutOfMemory, size, "region_alloc_array", "parts");
 		goto out;
 	}
 	if (key_def_dump_parts(pk_def, parts, region) != 0)
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index b4c464af7..926a0a61c 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -404,15 +404,16 @@ lua_sql_bind_list_decode(struct lua_State *L, struct sql_bind **out_bind,
 	}
 	struct region *region = &fiber()->gc;
 	uint32_t used = region_used(region);
-	size_t size = sizeof(struct sql_bind) * bind_count;
+	size_t size;
 	/*
 	 * Memory allocated here will be freed in
 	 * sql_stmt_finalize() or in txn_commit()/txn_rollback() if
 	 * there is an active transaction.
 	 */
-	struct sql_bind *bind = (struct sql_bind *) region_alloc(region, size);
+	struct sql_bind *bind = region_alloc_array(region, typeof(bind[0]),
+						   bind_count, &size);
 	if (bind == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "bind");
+		diag_set(OutOfMemory, size, "region_alloc_array", "bind");
 		return -1;
 	}
 	for (uint32_t i = 0; i < bind_count; ++i) {
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index d8f96162d..1a99fab63 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -438,13 +438,14 @@ lbox_key_def_new(struct lua_State *L)
 				  "[, collation = <string>]}, ...}");
 
 	uint32_t part_count = lua_objlen(L, 1);
-	const ssize_t parts_size = sizeof(struct key_part_def) * part_count;
 
 	struct region *region = &fiber()->gc;
 	size_t region_svp = region_used(region);
-	struct key_part_def *parts = region_alloc(region, parts_size);
+	size_t size;
+	struct key_part_def *parts =
+		region_alloc_array(region, typeof(parts[0]), part_count, &size);
 	if (parts == NULL) {
-		diag_set(OutOfMemory, parts_size, "region", "parts");
+		diag_set(OutOfMemory, size, "region_alloc_array", "parts");
 		return luaT_error(L);
 	}
 	if (part_count == 0) {
diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index ae8fbb682..5da84b35a 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -184,13 +184,13 @@ lbox_tuple_format_new(struct lua_State *L)
 	uint32_t count = lua_objlen(L, 1);
 	if (count == 0)
 		return lbox_push_tuple_format(L, tuple_format_runtime);
-	size_t size = count * sizeof(struct field_def);
+	size_t size;
 	struct region *region = &fiber()->gc;
 	size_t region_svp = region_used(region);
-	struct field_def *fields =
-		(struct field_def *)region_alloc(region, size);
+	struct field_def *fields = region_alloc_array(region, typeof(fields[0]),
+						      count, &size);
 	if (fields == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "fields");
+		diag_set(OutOfMemory, size, "region_alloc_array", "fields");
 		return luaT_error(L);
 	}
 	for (uint32_t i = 0; i < count; ++i) {
diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c
index e155ecd65..76ff3fcd7 100644
--- a/src/box/memtx_tree.c
+++ b/src/box/memtx_tree.c
@@ -804,10 +804,11 @@ struct func_key_undo {
 struct func_key_undo *
 func_key_undo_new(struct region *region)
 {
-	struct func_key_undo *undo =
-		(struct func_key_undo *)region_alloc(region, sizeof(*undo));
+	size_t size;
+	struct func_key_undo *undo = region_alloc_object(region, typeof(*undo),
+							 &size);
 	if (undo == NULL) {
-		diag_set(OutOfMemory, sizeof(*undo), "region", "undo");
+		diag_set(OutOfMemory, size, "region_alloc_object", "undo");
 		return NULL;
 	}
 	return undo;
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 0ff51b9a7..efb1c8ee9 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -71,10 +71,11 @@ space_def_sizeof(uint32_t name_len, const struct field_def *fields,
 			}
 		}
 	}
-
-	*fields_offset = sizeof(struct space_def) + name_len + 1;
+	*fields_offset = small_align(sizeof(struct space_def) + name_len + 1,
+				     alignof(typeof(fields[0])));
 	*names_offset = *fields_offset + field_count * sizeof(struct field_def);
-	*def_expr_offset = *names_offset + field_strs_size;
+	*def_expr_offset = small_align(*names_offset + field_strs_size,
+				       alignof(uint64_t));
 	return *def_expr_offset + def_exprs_size;
 }
 
diff --git a/src/box/sql.c b/src/box/sql.c
index fc1386f52..555fcfd1f 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -341,13 +341,17 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	uint32_t name_len = strlen("_COLUMN_") + 11;
 	uint32_t size = field_count * (sizeof(struct field_def) + name_len) +
 			part_count * sizeof(struct key_part_def);
-	struct field_def *fields = region_alloc(region, size);
+	struct field_def *fields = region_aligned_alloc(region, size,
+							alignof(fields[0]));
 	if (fields == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "fields");
+		diag_set(OutOfMemory, size, "region_aligned_alloc", "fields");
 		return NULL;
 	}
 	struct key_part_def *ephemer_key_parts =
 		(void *)fields + field_count * sizeof(struct field_def);
+	static_assert(alignof(*fields) == alignof(*ephemer_key_parts),
+		      "allocated in one block, and should have the same "
+		      "alignment");
 	char *names = (char *)ephemer_key_parts +
 		       part_count * sizeof(struct key_part_def);
 	for (uint32_t i = 0; i < field_count; ++i) {
@@ -1234,9 +1238,10 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
 	uint32_t dummy;
 	size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy,
 				       &dummy);
-	def = (struct space_def *)region_alloc(&parser->region, size);
+	def = (struct space_def *)region_aligned_alloc(&parser->region, size,
+						       alignof(*def));
 	if (def == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc",
+		diag_set(OutOfMemory, size, "region_aligned_alloc",
 			 "sql_ephemeral_space_def_new");
 		parser->is_aborted = true;
 		return NULL;
@@ -1252,10 +1257,11 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name)
 struct space *
 sql_ephemeral_space_new(Parse *parser, const char *name)
 {
-	size_t sz = sizeof(struct space);
-	struct space *space = (struct space *) region_alloc(&parser->region, sz);
+	size_t sz;
+	struct space *space = region_alloc_object(&parser->region,
+						  typeof(*space), &sz);
 	if (space == NULL) {
-		diag_set(OutOfMemory, sz, "region", "space");
+		diag_set(OutOfMemory, sz, "region_alloc_object", "space");
 		parser->is_aborted = true;
 		return NULL;
 	}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b1f9fedb0..0b60d2ee7 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -260,12 +260,12 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id)
 		uint32_t columns_new = space_def->exact_field_count;
 		columns_new = (columns_new > 0) ? 2 * columns_new : 1;
 		struct region *region = &parser->region;
-		field = region_alloc(region, columns_new *
-				     sizeof(space_def->fields[0]));
+		size_t size;
+		field = region_alloc_array(region, typeof(field[0]),
+					   columns_new, &size);
 		if (field == NULL) {
-			diag_set(OutOfMemory, columns_new *
-				sizeof(space_def->fields[0]),
-				"region_alloc", "sql_field_retrieve");
+			diag_set(OutOfMemory, size, "region_alloc_array",
+				 "field");
 			parser->is_aborted = true;
 			return NULL;
 		}
@@ -609,10 +609,12 @@ sql_create_check_contraint(struct Parse *parser)
 	uint32_t expr_str_offset;
 	uint32_t ck_def_sz = ck_constraint_def_sizeof(name_len, expr_str_len,
 						      &expr_str_offset);
-	struct ck_constraint_parse *ck_parse =
-		region_alloc(region, sizeof(*ck_parse) + ck_def_sz);
+	struct ck_constraint_parse *ck_parse;
+	size_t total = sizeof(*ck_parse) + ck_def_sz;
+	ck_parse = (struct ck_constraint_parse *)
+		region_aligned_alloc(region, total, alignof(*ck_parse));
 	if (ck_parse == NULL) {
-		diag_set(OutOfMemory, sizeof(*ck_parse) + ck_def_sz, "region",
+		diag_set(OutOfMemory, total, "region_aligned_alloc",
 			 "ck_parse");
 		parser->is_aborted = true;
 		return;
@@ -620,6 +622,9 @@ sql_create_check_contraint(struct Parse *parser)
 	struct ck_constraint_def *ck_def =
 		(struct ck_constraint_def *)((char *)ck_parse +
 					     sizeof(*ck_parse));
+	static_assert(alignof(*ck_def) == alignof(*ck_parse),
+		      "allocated in one block and should have the same "
+		      "alignment");
 	ck_parse->ck_def = ck_def;
 	rlist_create(&ck_parse->link);
 
@@ -1869,11 +1874,13 @@ sql_create_foreign_key(struct Parse *parse_context)
 			goto tnt_error;
 		}
 	} else {
+		size_t size;
 		struct fk_constraint_parse *fk_parse =
-			region_alloc(&parse_context->region, sizeof(*fk_parse));
+			region_alloc_object(&parse_context->region,
+					    typeof(*fk_parse), &size);
 		if (fk_parse == NULL) {
-			diag_set(OutOfMemory, sizeof(*fk_parse), "region_alloc",
-				 "struct fk_constraint_parse");
+			diag_set(OutOfMemory, size, "region_alloc_object",
+				 "fk_parse");
 			goto tnt_error;
 		}
 		memset(fk_parse, 0, sizeof(*fk_parse));
@@ -1957,12 +1964,15 @@ sql_create_foreign_key(struct Parse *parse_context)
 		}
 	}
 	int name_len = strlen(constraint_name);
-	size_t fk_def_sz = fk_constraint_def_sizeof(child_cols_count, name_len);
-	struct fk_constraint_def *fk_def = region_alloc(&parse_context->region,
-							fk_def_sz);
+	uint32_t links_offset;
+	size_t fk_def_sz = fk_constraint_def_sizeof(child_cols_count, name_len,
+						    &links_offset);
+	struct fk_constraint_def *fk_def = (struct fk_constraint_def *)
+		region_aligned_alloc(&parse_context->region, fk_def_sz,
+				     alignof(*fk_def));
 	if (fk_def == NULL) {
-		diag_set(OutOfMemory, fk_def_sz, "region",
-			 "struct fk_constraint_def");
+		diag_set(OutOfMemory, fk_def_sz, "region_aligned_alloc",
+			 "fk_def");
 		goto tnt_error;
 	}
 	int actions = create_fk_def->actions;
@@ -1973,7 +1983,7 @@ sql_create_foreign_key(struct Parse *parse_context)
 	fk_def->match = (enum fk_constraint_match) (create_fk_def->match);
 	fk_def->on_update = (enum fk_constraint_action) ((actions >> 8) & 0xff);
 	fk_def->on_delete = (enum fk_constraint_action) (actions & 0xff);
-	fk_def->links = (struct field_link *) ((char *) fk_def->name + name_len + 1);
+	fk_def->links = (struct field_link *)((char *)fk_def + links_offset);
 	/* Fill links map. */
 	for (uint32_t i = 0; i < fk_def->field_count; ++i) {
 		if (!is_self_referenced && parent_cols == NULL) {
@@ -2260,11 +2270,13 @@ index_fill_def(struct Parse *parse, struct index *index,
 	int rc = -1;
 
 	struct key_def *key_def = NULL;
-	struct key_part_def *key_parts = region_alloc(&fiber()->gc,
-				sizeof(*key_parts) * expr_list->nExpr);
+	size_t size;
+	struct key_part_def *key_parts =
+		region_alloc_array(&fiber()->gc, typeof(key_parts[0]),
+				   expr_list->nExpr, &size);
 	if (key_parts == NULL) {
-		diag_set(OutOfMemory, sizeof(*key_parts) * expr_list->nExpr,
-			 "region", "key parts");
+		diag_set(OutOfMemory, size, "region_alloc_array",
+			 "key_parts");
 		goto tnt_error;
 	}
 	for (int i = 0; i < expr_list->nExpr; i++) {
@@ -2514,10 +2526,10 @@ sql_create_index(struct Parse *parse) {
 			parse->is_aborted = true;
 		}
 	}
-
-	index = (struct index *) region_alloc(&parse->region, sizeof(*index));
+	size_t size;
+	index = region_alloc_object(&parse->region, typeof(*index), &size);
 	if (index == NULL) {
-		diag_set(OutOfMemory, sizeof(*index), "region", "index");
+		diag_set(OutOfMemory, size, "region_alloc_object", "index");
 		parse->is_aborted = true;
 		goto exit_create_index;
 	}
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 2c510940b..4715ffabb 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -85,10 +85,11 @@ static struct Mem *
 vdbemem_alloc_on_region(uint32_t count)
 {
 	struct region *region = &fiber()->gc;
-	struct Mem *ret = region_alloc(region, count * sizeof(*ret));
+	size_t size;
+	struct Mem *ret = region_alloc_array(region, typeof(*ret), count,
+					     &size);
 	if (ret == NULL) {
-		diag_set(OutOfMemory, count * sizeof(*ret),
-			 "region_alloc", "ret");
+		diag_set(OutOfMemory, size, "region_alloc_array", "ret");
 		return NULL;
 	}
 	memset(ret, 0, count * sizeof(*ret));
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index f39484013..0b7358af4 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1773,11 +1773,12 @@ generate_column_metadata(struct Parse *pParse, struct SrcList *pTabList,
 	if (pParse->colNamesSet || db->mallocFailed)
 		return;
 	assert(v != 0);
-	size_t var_pos_sz =  pParse->nVar * sizeof(uint32_t);
-	uint32_t *var_pos = (uint32_t *) region_alloc(&pParse->region,
-						      var_pos_sz);
+	size_t size;
+	uint32_t *var_pos =
+		region_alloc_array(&pParse->region, typeof(var_pos[0]),
+				   pParse->nVar, &size);
 	if (var_pos == NULL) {
-		diag_set(OutOfMemory, var_pos_sz, "region", "var_pos");
+		diag_set(OutOfMemory, size, "region_alloc_array", "var_pos");
 		return;
 	}
 	assert(pTabList != 0);
@@ -1910,9 +1911,10 @@ sqlColumnsFromExprList(Parse * parse, ExprList * expr_list,
 	 */
 	assert(space_def->fields == NULL);
 	struct region *region = &parse->region;
+	size_t size;
 	space_def->fields =
-		region_alloc(region,
-			     column_count * sizeof(space_def->fields[0]));
+		region_alloc_array(region, typeof(space_def->fields[0]),
+				   column_count, &size);
 	if (space_def->fields == NULL) {
 		sqlOomFault(db);
 		goto cleanup;
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d25262c21..24c7cfa27 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -120,10 +120,10 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 	int pk_cursor = pParse->nTab++;
 	pTabList->a[0].iCursor = pk_cursor;
 	struct index *pPk = space_index(space, 0);
-	i = sizeof(int) * def->field_count;
-	aXRef = (int *) region_alloc(&pParse->region, i);
+	aXRef = region_alloc_array(&pParse->region, typeof(aXRef[0]),
+				   def->field_count, &i);
 	if (aXRef == NULL) {
-		diag_set(OutOfMemory, i, "region_alloc", "aXRef");
+		diag_set(OutOfMemory, i, "region_alloc_array", "aXRef");
 		goto update_cleanup;
 	}
 	memset(aXRef, -1, i);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a42602a2..5bc106b5d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -605,11 +605,11 @@ static int
 vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 {
 	assert(vdbe != NULL);
-	struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
-							 sizeof(*id_entry));
+	size_t size;
+	struct autoinc_id_entry *id_entry =
+		region_alloc_object(&fiber()->gc, typeof(*id_entry), &size);
 	if (id_entry == NULL) {
-		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
-			 "id_entry");
+		diag_set(OutOfMemory, size, "region_alloc_object", "id_entry");
 		return -1;
 	}
 	id_entry->id = id;
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 5bc27f134..6d8768865 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1084,10 +1084,13 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 		 * predicates, so we consider term as sequence
 		 * of AND'ed predicates.
 		 */
-		size_t addrs_sz = sizeof(int) * nEq;
-		int *seek_addrs = region_alloc(&pParse->region, addrs_sz);
+		size_t addrs_sz;
+		int *seek_addrs = region_alloc_array(&pParse->region,
+						     typeof(seek_addrs[0]), nEq,
+						     &addrs_sz);
 		if (seek_addrs == NULL) {
-			diag_set(OutOfMemory, addrs_sz, "region", "seek_addrs");
+			diag_set(OutOfMemory, addrs_sz, "region_alloc_array",
+				 "seek_addrs");
 			pParse->is_aborted = true;
 			return 0;
 		}
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index beaeb0fe7..68ec2a749 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -981,7 +981,8 @@ tuple_format_iterator_create(struct tuple_format_iterator *it,
 	if (validate)
 		it->required_fields_sz = bitmap_size(format->total_field_count);
 	uint32_t total_sz = frames_sz + 2 * it->required_fields_sz;
-	struct mp_frame *frames = region_alloc(region, total_sz);
+	struct mp_frame *frames = region_aligned_alloc(region, total_sz,
+						       alignof(frames[0]));
 	if (frames == NULL) {
 		diag_set(OutOfMemory, total_sz, "region",
 			 "tuple_format_iterator");
diff --git a/src/box/txn.c b/src/box/txn.c
index b81693c0a..123520166 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -802,13 +802,15 @@ struct txn_savepoint *
 txn_savepoint_new(struct txn *txn, const char *name)
 {
 	assert(txn == in_txn());
-	size_t svp_sz = sizeof(struct txn_savepoint);
 	int name_len = name != NULL ? strlen(name) : 0;
-	svp_sz += name_len;
-	struct txn_savepoint *svp =
-		(struct txn_savepoint *) region_alloc(&txn->region, svp_sz);
+	struct txn_savepoint *svp;
+	static_assert(sizeof(svp->name) == 1,
+		      "name member already has 1 byte for 0 termination");
+	size_t size = sizeof(*svp) + name_len;
+	svp = (struct txn_savepoint *)region_aligned_alloc(&txn->region, size,
+							   alignof(*svp));
 	if (svp == NULL) {
-		diag_set(OutOfMemory, svp_sz, "region", "svp");
+		diag_set(OutOfMemory, size, "region_aligned_alloc", "svp");
 		return NULL;
 	}
 	svp->stmt = stailq_last(&txn->stmts);
diff --git a/src/box/user.cc b/src/box/user.cc
index fe0555886..198bf828b 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -192,11 +192,11 @@ user_grant_priv(struct user *user, struct priv_def *def)
 {
 	struct priv_def *old = privset_search(&user->privs, def);
 	if (old == NULL) {
-		size_t size = sizeof(struct priv_def);
-		old = (struct priv_def *)
-			region_alloc(&user->pool, size);
+		size_t size;
+		old = region_alloc_object(&user->pool, typeof(*old), &size);
 		if (old == NULL) {
-			diag_set(OutOfMemory, size, "region", "struct priv_def");
+			diag_set(OutOfMemory, size, "region_alloc_object",
+				 "old");
 			return -1;
 		}
 		*old = *def;
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 3a582b6fe..e7669452e 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -603,10 +603,11 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def,
 	rlist_foreach_entry(index_def, key_list, link)
 		key_count++;
 	struct key_def **keys;
-	size_t size = sizeof(*keys) * key_count;
-	keys = region_alloc(&fiber()->gc, size);
+	size_t size;
+	keys = region_alloc_array(&fiber()->gc, typeof(keys[0]), key_count,
+				  &size);
 	if (keys == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "keys");
+		diag_set(OutOfMemory, size, "region_alloc_array", "keys");
 		free(space);
 		return NULL;
 	}
@@ -4439,19 +4440,22 @@ vy_deferred_delete_on_replace(struct trigger *trigger, void *event)
 		 * which will propagate the WAL row LSN to
 		 * the LSM tree.
 		 */
-		struct trigger *on_commit = region_alloc(&txn->region,
-							 sizeof(*on_commit));
+		size_t size;
+		struct trigger *on_commit =
+			region_alloc_object(&txn->region, typeof(*on_commit),
+					    &size);
 		if (on_commit == NULL) {
-			diag_set(OutOfMemory, sizeof(*on_commit),
-				 "region", "struct trigger");
+			diag_set(OutOfMemory, size, "region_alloc_object",
+				 "on_commit");
 			rc = -1;
 			break;
 		}
-		struct trigger *on_rollback = region_alloc(&txn->region,
-							   sizeof(*on_commit));
+		struct trigger *on_rollback =
+			region_alloc_object(&txn->region, typeof(*on_rollback),
+					    &size);
 		if (on_rollback == NULL) {
-			diag_set(OutOfMemory, sizeof(*on_commit),
-				 "region", "struct trigger");
+			diag_set(OutOfMemory, size, "region_alloc_object",
+				 "on_rollback");
 			rc = -1;
 			break;
 		}
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 9ead066af..311985c72 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -621,12 +621,13 @@ vy_log_record_decode(struct vy_log_record *record,
 		case VY_LOG_KEY_DEF: {
 			struct region *region = &fiber()->gc;
 			uint32_t part_count = mp_decode_array(&pos);
-			struct key_part_def *parts = region_alloc(region,
-						sizeof(*parts) * part_count);
+			size_t size;
+			struct key_part_def *parts =
+				region_alloc_array(region, typeof(parts[0]),
+						   part_count, &size);
 			if (parts == NULL) {
-				diag_set(OutOfMemory,
-					 sizeof(*parts) * part_count,
-					 "region", "struct key_part_def");
+				diag_set(OutOfMemory, size,
+					 "region_alloc_array", "parts");
 				return -1;
 			}
 			if (key_def_decode_parts(parts, part_count, &pos,
@@ -701,18 +702,18 @@ static struct vy_log_record *
 vy_log_record_dup(struct region *pool, const struct vy_log_record *src)
 {
 	size_t used = region_used(pool);
-
-	struct vy_log_record *dst = region_alloc(pool, sizeof(*dst));
+	size_t size;
+	struct vy_log_record *dst = region_alloc_object(pool, typeof(*dst),
+							&size);
 	if (dst == NULL) {
-		diag_set(OutOfMemory, sizeof(*dst),
-			 "region", "struct vy_log_record");
+		diag_set(OutOfMemory, size, "region_alloc_object", "dst");
 		goto err;
 	}
 	*dst = *src;
 	if (src->begin != NULL) {
 		const char *data = src->begin;
 		mp_next(&data);
-		size_t size = data - src->begin;
+		size = data - src->begin;
 		dst->begin = region_alloc(pool, size);
 		if (dst->begin == NULL) {
 			diag_set(OutOfMemory, size, "region",
@@ -724,7 +725,7 @@ vy_log_record_dup(struct region *pool, const struct vy_log_record *src)
 	if (src->end != NULL) {
 		const char *data = src->end;
 		mp_next(&data);
-		size_t size = data - src->end;
+		size = data - src->end;
 		dst->end = region_alloc(pool, size);
 		if (dst->end == NULL) {
 			diag_set(OutOfMemory, size, "region",
@@ -734,12 +735,12 @@ vy_log_record_dup(struct region *pool, const struct vy_log_record *src)
 		memcpy((char *)dst->end, src->end, size);
 	}
 	if (src->key_def != NULL) {
-		size_t size = src->key_def->part_count *
-				sizeof(struct key_part_def);
-		dst->key_parts = region_alloc(pool, size);
+		dst->key_parts =
+			region_alloc_array(pool, typeof(dst->key_parts[0]),
+					   src->key_def->part_count, &size);
 		if (dst->key_parts == NULL) {
-			diag_set(OutOfMemory, size, "region",
-				 "struct key_part_def");
+			diag_set(OutOfMemory, size, "region_alloc_array",
+				 "def->key_parts");
 			goto err;
 		}
 		if (key_def_dump_parts(src->key_def, dst->key_parts, pool) != 0)
diff --git a/src/box/vy_point_lookup.c b/src/box/vy_point_lookup.c
index ecdcde7db..80b5c5933 100644
--- a/src/box/vy_point_lookup.c
+++ b/src/box/vy_point_lookup.c
@@ -167,11 +167,12 @@ vy_point_lookup_scan_slices(struct vy_lsm *lsm, const struct vy_read_view **rv,
 							   ITER_EQ, key);
 	assert(range != NULL);
 	int slice_count = range->slice_count;
-	struct vy_slice **slices = (struct vy_slice **)
-		region_alloc(&fiber()->gc, slice_count * sizeof(*slices));
+	size_t size;
+	struct vy_slice **slices =
+		region_alloc_array(&fiber()->gc, typeof(slices[0]), slice_count,
+				   &size);
 	if (slices == NULL) {
-		diag_set(OutOfMemory, slice_count * sizeof(*slices),
-			 "region", "slices array");
+		diag_set(OutOfMemory, size, "region_alloc_array", "slices");
 		return -1;
 	}
 	int i = 0;
diff --git a/src/box/xrow_update_map.c b/src/box/xrow_update_map.c
index ff53a9ac4..57fb27f18 100644
--- a/src/box/xrow_update_map.c
+++ b/src/box/xrow_update_map.c
@@ -63,10 +63,11 @@ struct xrow_update_map_item {
 static inline struct xrow_update_map_item *
 xrow_update_map_item_alloc(void)
 {
-	struct xrow_update_map_item *item = (struct xrow_update_map_item *)
-		region_alloc(&fiber()->gc, sizeof(*item));
+	size_t size;
+	struct xrow_update_map_item *item =
+		region_alloc_object(&fiber()->gc, typeof(*item), &size);
 	if (item == NULL)
-		diag_set(OutOfMemory, sizeof(*item), "region_alloc", "item");
+		diag_set(OutOfMemory, size, "region_alloc_object", "item");
 	return item;
 }
 
diff --git a/src/box/xrow_update_route.c b/src/box/xrow_update_route.c
index 122f0329e..0352aec63 100644
--- a/src/box/xrow_update_route.c
+++ b/src/box/xrow_update_route.c
@@ -244,10 +244,11 @@ xrow_update_route_branch(struct xrow_update_field *field,
 	bool transform_root = (saved_old_offset == 0);
 	struct xrow_update_field *next_hop;
 	if (!transform_root) {
-		next_hop = (struct xrow_update_field *)
-			region_alloc(&fiber()->gc, sizeof(*next_hop));
+		size_t size;
+		next_hop = region_alloc_object(&fiber()->gc, typeof(*next_hop),
+					       &size);
 		if (next_hop == NULL) {
-			diag_set(OutOfMemory, sizeof(*next_hop), "region_alloc",
+			diag_set(OutOfMemory, size, "region_alloc_object",
 				 "next_hop");
 			return NULL;
 		}
diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc
index b5531a596..946420885 100644
--- a/src/lib/core/backtrace.cc
+++ b/src/lib/core/backtrace.cc
@@ -110,9 +110,9 @@ get_proc_name(unw_cursor_t *unw_cur, unw_word_t *offset, bool skip_cache)
 	}  else {
 		unw_get_proc_name(unw_cur, proc_name, sizeof(proc_name),
 				  offset);
-
-		entry = (struct proc_cache_entry *)
-			region_alloc(&cache_region, sizeof(struct proc_cache_entry));
+		size_t size;
+		entry = region_alloc_object(&cache_region, typeof(*entry),
+					    &size);
 		if (entry == NULL)
 			goto error;
 		node.key = ip;
diff --git a/src/lua/popen.c b/src/lua/popen.c
index 18c8282f1..354429f32 100644
--- a/src/lua/popen.c
+++ b/src/lua/popen.c
@@ -778,11 +778,12 @@ luaT_popen_parse_env(struct lua_State *L, int idx, struct region *region)
 		idx = lua_gettop(L) + idx + 1;
 
 	size_t capacity = POPEN_LUA_ENV_CAPACITY_DEFAULT;
-	size_t size = capacity * sizeof(char *);
 	size_t region_svp = region_used(region);
-	char **env = region_alloc(region, size);
+	size_t size;
+	char **env = region_alloc_array(region, typeof(env[0]), capacity,
+					&size);
 	if (env == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "env array");
+		diag_set(OutOfMemory, size, "region_alloc_array", "env");
 		return NULL;
 	}
 	size_t nr_env = 0;
@@ -829,10 +830,10 @@ luaT_popen_parse_env(struct lua_State *L, int idx, struct region *region)
 	 * the traverse again and fill `env` array.
 	 */
 	capacity = nr_env + 1;
-	size = capacity * sizeof(char *);
-	if ((env = region_alloc(region, size)) == NULL) {
+	env = region_alloc_array(region, typeof(env[0]), capacity, &size);
+	if (env == NULL) {
 		region_truncate(region, region_svp);
-		diag_set(OutOfMemory, size, "region_alloc", "env array");
+		diag_set(OutOfMemory, size, "region_alloc_array", "env");
 		return NULL;
 	}
 	nr_env = 0;
@@ -1039,10 +1040,11 @@ luaT_popen_parse_argv(struct lua_State *L, int idx, struct popen_opts *opts,
 	if (opts->flags & POPEN_FLAG_SHELL)
 		opts->nr_argv += 2;
 
-	size_t size = sizeof(char *) * opts->nr_argv;
-	opts->argv = region_alloc(region, size);
+	size_t size;
+	opts->argv = region_alloc_array(region, typeof(opts->argv[0]),
+					opts->nr_argv, &size);
 	if (opts->argv == NULL) {
-		diag_set(OutOfMemory, size, "region_alloc", "argv");
+		diag_set(OutOfMemory, size, "region_alloc_array", "opts->argv");
 		return -1;
 	}
 
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 06/10] region: use aligned allocations where necessary

[Timur Safin]

: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: [PATCH v2 06/10] region: use aligned allocations where necessary
: 


: diff --git a/src/box/alter.cc b/src/box/alter.cc
: index dbbbcbc44..bb4254878 100644
: --- a/src/box/alter.cc
: +++ b/src/box/alter.cc
: @@ -537,11 +537,13 @@ space_format_decode(const char *data, uint32_t
: *out_count,
:  		*fields = NULL;
:  		return 0;
:  	}
: -	size_t size = count * sizeof(struct field_def);
: +	size_t size;
:  	struct field_def *region_defs =
: -		(struct field_def *) region_alloc(region, size);
: +		region_alloc_array(region, typeof(region_defs[0]), count,
: +				   &size);

I'd say that `typeof(region_defs[0])` is very indirect way to write 
`struct field_def` :) Why you've chosen this way (and not 
simpler/clearer) way?


: @@ -2452,10 +2454,12 @@ on_replace_dd_space(struct trigger * /* trigger
: */, void *event)
:  		 * comparators must be updated.
:  		 */
:  		struct key_def **keys;
: -		size_t bsize = old_space->index_count * sizeof(keys[0]);
: -		keys = (struct key_def **) region_alloc(&fiber()->gc, bsize);
: +		size_t bsize;
: +		keys = region_alloc_array(&fiber()->gc, typeof(keys[0]),
: +					  old_space->index_count, &bsize);

Oh, now I see - for copy-paste sake :)
(I still think though the explicit `struct key_def*` would be 
clearer for the stranger passing by.)

The same question is applying multiple times below in the patch.

...
...

And joking aside, putting aside all debatable complains about 
simpler/clearer code which might be used (which you could ignore
in general), here is real complain - this is gcc preprocessor
extension, which might be not working under other C99/C11-compliant
compilers. Which we would rather avoid in a longer run (if standard 
C compatible code would not require much efforts from us, like in
these cases)

Regards,
Timur

Re: [Tarantool-patches] [PATCH v2 06/10] region: use aligned allocations where necessary

[Vladislav Shpilevoy]

Thanks for the review!

On 28/05/2020 22:35, Timur Safin wrote:
> 
> 
> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : Subject: [PATCH v2 06/10] region: use aligned allocations where necessary
> : 
> 
> 
> : diff --git a/src/box/alter.cc b/src/box/alter.cc
> : index dbbbcbc44..bb4254878 100644
> : --- a/src/box/alter.cc
> : +++ b/src/box/alter.cc
> : @@ -537,11 +537,13 @@ space_format_decode(const char *data, uint32_t
> : *out_count,
> :  		*fields = NULL;
> :  		return 0;
> :  	}
> : -	size_t size = count * sizeof(struct field_def);
> : +	size_t size;
> :  	struct field_def *region_defs =
> : -		(struct field_def *) region_alloc(region, size);
> : +		region_alloc_array(region, typeof(region_defs[0]), count,
> : +				   &size);
> 
> I'd say that `typeof(region_defs[0])` is very indirect way to write 
> `struct field_def` :) Why you've chosen this way (and not 
> simpler/clearer) way?

To avoid bugs like this:
https://github.com/tarantool/tarantool/commit/d1647590ec4263592d6408cd4c16c2c2d55a7847

Talking of simpler/clearer - I consider my way both simpler and clearer. And
safer. When type name is written only once, in variable declaration, there is
no chance in screwing it later, if you use sizeof(variable) and typeof(variable)
instead of duplicating type name.

> : @@ -2452,10 +2454,12 @@ on_replace_dd_space(struct trigger * /* trigger
> : */, void *event)
> :  		 * comparators must be updated.
> :  		 */
> :  		struct key_def **keys;
> : -		size_t bsize = old_space->index_count * sizeof(keys[0]);
> : -		keys = (struct key_def **) region_alloc(&fiber()->gc, bsize);
> : +		size_t bsize;
> : +		keys = region_alloc_array(&fiber()->gc, typeof(keys[0]),
> : +					  old_space->index_count, &bsize);
> 
> Oh, now I see - for copy-paste sake :)

No. I explained it above.

> And joking aside, putting aside all debatable complains about 
> simpler/clearer code which might be used (which you could ignore
> in general), here is real complain - this is gcc preprocessor
> extension, which might be not working under other C99/C11-compliant
> compilers. Which we would rather avoid in a longer run (if standard 
> C compatible code would not require much efforts from us, like in
> these cases)

We can't work without typeof(), because our generic data structures
heavily depend on it: rlist, stailq, heap. Tarantool uses them so
extensively, that it probably would be easier to implement them from
the scratch somehow, than get rid of them.

[Tarantool-patches] [PATCH v2 07/10] vinyl: align statements and bps tree extents

[Vladislav Shpilevoy]

Vinyl tuples (vy_stmt) in 0 level of LSM tree are stored in
lsregion. They were allocated using lsregion_alloc(), which does
not align its results, and is good only for byte arrays.

As a result, vy_stmt object addresses in 0 LSM level were not
aligned. Unaligned memory access is slower, and may even crash on
some platforms.

Besides, even aligned allocations couldn't help upserts in 0 level
of the LSM tree, because upsert vy_stmt objects had 1 byte prefix
to count merged upserts stored in this statement. This 1 byte
prefix ruined all the alignment. Now the upsert counter is also
aligned, the same as vy_stmt. Note, it does not consume
significantly more memory, since it used only for vinyl and only
for upserts, stored in 0 level of the LSM tree.

The same about BPS tree extents. LSM 0 level is a BPS tree, whose
blocks are allocated on lsregion. The extents are used as pointer
arrays inside the tree, so they need alignof(void *) alignment.

The mentioned unaligned accesses were revealed by clang undefined
behaviour sanitizer, and are fixed by this patch.

Part of #4609
---
 src/box/vy_mem.c                |  9 +++++----
 src/box/vy_stmt.c               | 13 ++++++++-----
 test/vinyl/quota.result         | 10 +++++-----
 test/vinyl/quota_timeout.result |  4 ++--
 test/vinyl/stat.result          |  4 ++--
 5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index b4d016a68..98027e784 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -75,11 +75,12 @@ vy_mem_tree_extent_alloc(void *ctx)
 {
 	struct vy_mem *mem = (struct vy_mem *) ctx;
 	struct vy_mem_env *env = mem->env;
-	void *ret = lsregion_alloc(&env->allocator, VY_MEM_TREE_EXTENT_SIZE,
-				   mem->generation);
+	void *ret = lsregion_aligned_alloc(&env->allocator,
+					   VY_MEM_TREE_EXTENT_SIZE,
+					   alignof(void *), mem->generation);
 	if (ret == NULL) {
-		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE, "lsregion_alloc",
-			 "ret");
+		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE,
+			 "lsregion_aligned_alloc", "ret");
 		return NULL;
 	}
 	mem->tree_extent_size += VY_MEM_TREE_EXTENT_SIZE;
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index adc3ba452..dc6960068 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -223,20 +223,23 @@ vy_stmt_dup_lsregion(struct tuple *stmt, struct lsregion *lsregion,
 	size_t size = tuple_size(stmt);
 	size_t alloc_size = size;
 	struct tuple *mem_stmt;
+	const size_t align = alignof(struct vy_stmt);
 
 	/* Reserve one byte for UPSERT counter. */
 	if (type == IPROTO_UPSERT)
-		alloc_size++;
+		alloc_size += align;
 
-	mem_stmt = lsregion_alloc(lsregion, alloc_size, alloc_id);
+	mem_stmt = lsregion_aligned_alloc(lsregion, alloc_size, align,
+					  alloc_id);
 	if (mem_stmt == NULL) {
-		diag_set(OutOfMemory, size, "lsregion_alloc", "mem_stmt");
+		diag_set(OutOfMemory, size, "lsregion_aligned_alloc",
+			 "mem_stmt");
 		return NULL;
 	}
 
 	if (type == IPROTO_UPSERT) {
-		*(uint8_t *)mem_stmt = 0;
-		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + 1);
+		memset(mem_stmt, 0, align);
+		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + align);
 	}
 
 	memcpy(mem_stmt, stmt, size);
diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
index d1b28ee51..940df4e49 100644
--- a/test/vinyl/quota.result
+++ b/test/vinyl/quota.result
@@ -31,7 +31,7 @@ space:insert({1, 1})
 ...
 box.stat.vinyl().memory.level0
 ---
-- 98343
+- 98344
 ...
 space:insert({1, 1})
 ---
@@ -39,7 +39,7 @@ space:insert({1, 1})
 ...
 box.stat.vinyl().memory.level0
 ---
-- 98343
+- 98344
 ...
 space:update({1}, {{'!', 1, 100}}) -- try to modify the primary key
 ---
@@ -47,7 +47,7 @@ space:update({1}, {{'!', 1, 100}}) -- try to modify the primary key
 ...
 box.stat.vinyl().memory.level0
 ---
-- 98343
+- 98344
 ...
 space:insert({2, 2})
 ---
@@ -63,7 +63,7 @@ space:insert({4, 4})
 ...
 box.stat.vinyl().memory.level0
 ---
-- 98460
+- 98463
 ...
 box.snapshot()
 ---
@@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 * 5)}
 ...
 box.stat.vinyl().memory.level0
 ---
-- 5292076
+- 5292080
 ...
 space:drop()
 ---
diff --git a/test/vinyl/quota_timeout.result b/test/vinyl/quota_timeout.result
index 7a71b29c6..31ca23670 100644
--- a/test/vinyl/quota_timeout.result
+++ b/test/vinyl/quota_timeout.result
@@ -49,7 +49,7 @@ s:count()
 ...
 box.stat.vinyl().memory.level0
 ---
-- 748241
+- 748248
 ...
 -- Since the following operation requires more memory than configured
 -- and dump is disabled, it should fail with ER_VY_QUOTA_TIMEOUT.
@@ -63,7 +63,7 @@ s:count()
 ...
 box.stat.vinyl().memory.level0
 ---
-- 748241
+- 748248
 ...
 --
 -- Check that increasing box.cfg.vinyl_memory wakes up fibers
diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
index d35def13d..a895528b9 100644
--- a/test/vinyl/stat.result
+++ b/test/vinyl/stat.result
@@ -761,7 +761,7 @@ put(1)
 ...
 stat_diff(gstat(), st, 'memory.level0')
 ---
-- 1061
+- 1064
 ...
 -- use cache
 st = gstat()
@@ -1130,7 +1130,7 @@ gstat()
   memory:
     tuple_cache: 14417
     tx: 0
-    level0: 262583
+    level0: 263210
     page_index: 1250
     bloom_filter: 140
   disk:
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 07/10] vinyl: align statements and bps tree extents

[Timur Safin]

Agreed! LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 07/10] vinyl: align statements and bps tree extents
: 
: Vinyl tuples (vy_stmt) in 0 level of LSM tree are stored in
: lsregion. They were allocated using lsregion_alloc(), which does
: not align its results, and is good only for byte arrays.
: 
: As a result, vy_stmt object addresses in 0 LSM level were not
: aligned. Unaligned memory access is slower, and may even crash on
: some platforms.
: 
: Besides, even aligned allocations couldn't help upserts in 0 level
: of the LSM tree, because upsert vy_stmt objects had 1 byte prefix
: to count merged upserts stored in this statement. This 1 byte
: prefix ruined all the alignment. Now the upsert counter is also
: aligned, the same as vy_stmt. Note, it does not consume
: significantly more memory, since it used only for vinyl and only
: for upserts, stored in 0 level of the LSM tree.
: 
: The same about BPS tree extents. LSM 0 level is a BPS tree, whose
: blocks are allocated on lsregion. The extents are used as pointer
: arrays inside the tree, so they need alignof(void *) alignment.
: 
: The mentioned unaligned accesses were revealed by clang undefined
: behaviour sanitizer, and are fixed by this patch.
: 
: Part of #4609
: ---
:  src/box/vy_mem.c                |  9 +++++----
:  src/box/vy_stmt.c               | 13 ++++++++-----
:  test/vinyl/quota.result         | 10 +++++-----
:  test/vinyl/quota_timeout.result |  4 ++--
:  test/vinyl/stat.result          |  4 ++--
:  5 files changed, 22 insertions(+), 18 deletions(-)
: 
: diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
: index b4d016a68..98027e784 100644
: --- a/src/box/vy_mem.c
: +++ b/src/box/vy_mem.c
: @@ -75,11 +75,12 @@ vy_mem_tree_extent_alloc(void *ctx)
:  {
:  	struct vy_mem *mem = (struct vy_mem *) ctx;
:  	struct vy_mem_env *env = mem->env;
: -	void *ret = lsregion_alloc(&env->allocator, VY_MEM_TREE_EXTENT_SIZE,
: -				   mem->generation);
: +	void *ret = lsregion_aligned_alloc(&env->allocator,
: +					   VY_MEM_TREE_EXTENT_SIZE,
: +					   alignof(void *), mem->generation);
:  	if (ret == NULL) {
: -		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE,
: "lsregion_alloc",
: -			 "ret");
: +		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE,
: +			 "lsregion_aligned_alloc", "ret");
:  		return NULL;
:  	}
:  	mem->tree_extent_size += VY_MEM_TREE_EXTENT_SIZE;
: diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
: index adc3ba452..dc6960068 100644
: --- a/src/box/vy_stmt.c
: +++ b/src/box/vy_stmt.c
: @@ -223,20 +223,23 @@ vy_stmt_dup_lsregion(struct tuple *stmt, struct
: lsregion *lsregion,
:  	size_t size = tuple_size(stmt);
:  	size_t alloc_size = size;
:  	struct tuple *mem_stmt;
: +	const size_t align = alignof(struct vy_stmt);
: 
:  	/* Reserve one byte for UPSERT counter. */
:  	if (type == IPROTO_UPSERT)
: -		alloc_size++;
: +		alloc_size += align;
: 
: -	mem_stmt = lsregion_alloc(lsregion, alloc_size, alloc_id);
: +	mem_stmt = lsregion_aligned_alloc(lsregion, alloc_size, align,
: +					  alloc_id);
:  	if (mem_stmt == NULL) {
: -		diag_set(OutOfMemory, size, "lsregion_alloc", "mem_stmt");
: +		diag_set(OutOfMemory, size, "lsregion_aligned_alloc",
: +			 "mem_stmt");
:  		return NULL;
:  	}
: 
:  	if (type == IPROTO_UPSERT) {
: -		*(uint8_t *)mem_stmt = 0;
: -		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + 1);
: +		memset(mem_stmt, 0, align);
: +		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + align);
:  	}
: 
:  	memcpy(mem_stmt, stmt, size);
: diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
: index d1b28ee51..940df4e49 100644
: --- a/test/vinyl/quota.result
: +++ b/test/vinyl/quota.result
: @@ -31,7 +31,7 @@ space:insert({1, 1})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:insert({1, 1})
:  ---
: @@ -39,7 +39,7 @@ space:insert({1, 1})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:update({1}, {{'!', 1, 100}}) -- try to modify the primary key
:  ---
: @@ -47,7 +47,7 @@ space:update({1}, {{'!', 1, 100}}) -- try to modify the
: primary key
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:insert({2, 2})
:  ---
: @@ -63,7 +63,7 @@ space:insert({4, 4})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98460
: +- 98463
:  ...
:  box.snapshot()
:  ---
: @@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 *
: 5)}
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 5292076
: +- 5292080
:  ...
:  space:drop()
:  ---
: diff --git a/test/vinyl/quota_timeout.result
: b/test/vinyl/quota_timeout.result
: index 7a71b29c6..31ca23670 100644
: --- a/test/vinyl/quota_timeout.result
: +++ b/test/vinyl/quota_timeout.result
: @@ -49,7 +49,7 @@ s:count()
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 748241
: +- 748248
:  ...
:  -- Since the following operation requires more memory than configured
:  -- and dump is disabled, it should fail with ER_VY_QUOTA_TIMEOUT.
: @@ -63,7 +63,7 @@ s:count()
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 748241
: +- 748248
:  ...
:  --
:  -- Check that increasing box.cfg.vinyl_memory wakes up fibers
: diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
: index d35def13d..a895528b9 100644
: --- a/test/vinyl/stat.result
: +++ b/test/vinyl/stat.result
: @@ -761,7 +761,7 @@ put(1)
:  ...
:  stat_diff(gstat(), st, 'memory.level0')
:  ---
: -- 1061
: +- 1064
:  ...
:  -- use cache
:  st = gstat()
: @@ -1130,7 +1130,7 @@ gstat()
:    memory:
:      tuple_cache: 14417
:      tx: 0
: -    level0: 262583
: +    level0: 263210
:      page_index: 1250
:      bloom_filter: 140
:    disk:
: --
: 2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 08/10] tuple: use unaligned store-load for field map

[Vladislav Shpilevoy]

A tuple can have a field map. It is an array of uint32_t values,
stored right after 'struct tuple' object in the same memory block.

'struct tuple' is 10 byte size, and is aligned by 4 bytes (even
though it is 'PACKED', so does not need an alignment). It means,
that field map is stored by an address like this: 4*N + 10.

This address is never aligned by 4, needed for uint32_t field map
array. So the array members should be accessed using operations
aware of unaligned nature of the addresses.

Unfortunately, it is impossible to make the field map aligned,
because that requires + 2 bytes for each tuple. It is unaffordable
luxury, since tuple count can be millions and even billions. Every
new byte may result in gigabytes of memory in a cluster.

The patch makes field map accessed using unaligned store-load
operations.

Part of #4609
---
 src/box/field_map.c | 13 +++++++++----
 src/box/field_map.h | 21 ++++++++++++---------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/box/field_map.c b/src/box/field_map.c
index 5f4661941..dc903115e 100644
--- a/src/box/field_map.c
+++ b/src/box/field_map.c
@@ -101,16 +101,21 @@ field_map_build(struct field_map_builder *builder, char *buffer)
 		(uint32_t *)(buffer + field_map_build_size(builder));
 	char *extent_wptr = buffer;
 	for (int32_t i = -1; i >= -(int32_t)builder->slot_count; i--) {
+		/*
+		 * Can not access field_map as a normal uint32
+		 * array because its alignment may be < 4 bytes.
+		 * Need to use unaligned store-load operations
+		 * explicitly.
+		 */
 		if (!builder->slots[i].has_extent) {
-			field_map[i] = builder->slots[i].offset;
+			store_u32(&field_map[i], builder->slots[i].offset);
 			continue;
 		}
 		struct field_map_builder_slot_extent *extent =
 						builder->slots[i].extent;
 		/** Retrive memory for the extent. */
-		field_map[i] =
-			(uint32_t)((char *)extent_wptr - (char *)field_map);
-		*(uint32_t *)extent_wptr = extent->size;
+		store_u32(&field_map[i], extent_wptr - (char *)field_map);
+		store_u32(extent_wptr, extent->size);
 		uint32_t extent_offset_sz = extent->size * sizeof(uint32_t);
 		memcpy(&((uint32_t *) extent_wptr)[1], extent->offset,
 			extent_offset_sz);
diff --git a/src/box/field_map.h b/src/box/field_map.h
index a1a5a9dba..d8ef726a1 100644
--- a/src/box/field_map.h
+++ b/src/box/field_map.h
@@ -33,6 +33,7 @@
 #include <assert.h>
 #include <stdint.h>
 #include <stddef.h>
+#include "bit/bit.h"
 
 struct region;
 struct field_map_builder_slot;
@@ -151,20 +152,22 @@ static inline uint32_t
 field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
 		     int multikey_idx)
 {
-	uint32_t offset;
-	if (multikey_idx != MULTIKEY_NONE &&
-	    (int32_t) field_map[offset_slot] < 0) {
+	/*
+	 * Can not access field_map as a normal uint32 array
+	 * because its alignment may be < 4 bytes. Need to use
+	 * unaligned store-load operations explicitly.
+	 */
+	uint32_t offset = load_u32(&field_map[offset_slot]);
+	if (multikey_idx != MULTIKEY_NONE && (int32_t)offset < 0) {
 		/**
 		 * The field_map extent has the following
 		 * structure: [size=N|slot1|slot2|..|slotN]
 		 */
-		uint32_t *extent = (uint32_t *)((char *)field_map +
-					(int32_t)field_map[offset_slot]);
-		if ((uint32_t)multikey_idx >= extent[0])
+		const uint32_t *extent = (const uint32_t *)
+			((const char *)field_map + (int32_t)offset);
+		if ((uint32_t)multikey_idx >= load_u32(&extent[0]))
 			return 0;
-		offset = extent[multikey_idx + 1];
-	} else {
-		offset = field_map[offset_slot];
+		offset = load_u32(&extent[multikey_idx + 1]);
 	}
 	return offset;
 }
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 08/10] tuple: use unaligned store-load for field map

[Timur Safin]

Yup!
LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 08/10] tuple: use unaligned store-load for field map
: 
: A tuple can have a field map. It is an array of uint32_t values,
: stored right after 'struct tuple' object in the same memory block.
: 
: 'struct tuple' is 10 byte size, and is aligned by 4 bytes (even
: though it is 'PACKED', so does not need an alignment). It means,
: that field map is stored by an address like this: 4*N + 10.
: 
: This address is never aligned by 4, needed for uint32_t field map
: array. So the array members should be accessed using operations
: aware of unaligned nature of the addresses.
: 
: Unfortunately, it is impossible to make the field map aligned,
: because that requires + 2 bytes for each tuple. It is unaffordable
: luxury, since tuple count can be millions and even billions. Every
: new byte may result in gigabytes of memory in a cluster.
: 
: The patch makes field map accessed using unaligned store-load
: operations.
: 
: Part of #4609
: ---
:  src/box/field_map.c | 13 +++++++++----
:  src/box/field_map.h | 21 ++++++++++++---------
:  2 files changed, 21 insertions(+), 13 deletions(-)
: 
: diff --git a/src/box/field_map.c b/src/box/field_map.c
: index 5f4661941..dc903115e 100644
: --- a/src/box/field_map.c
: +++ b/src/box/field_map.c
: @@ -101,16 +101,21 @@ field_map_build(struct field_map_builder *builder,
: char *buffer)
:  		(uint32_t *)(buffer + field_map_build_size(builder));
:  	char *extent_wptr = buffer;
:  	for (int32_t i = -1; i >= -(int32_t)builder->slot_count; i--) {
: +		/*
: +		 * Can not access field_map as a normal uint32
: +		 * array because its alignment may be < 4 bytes.
: +		 * Need to use unaligned store-load operations
: +		 * explicitly.
: +		 */
:  		if (!builder->slots[i].has_extent) {
: -			field_map[i] = builder->slots[i].offset;
: +			store_u32(&field_map[i], builder->slots[i].offset);
:  			continue;
:  		}
:  		struct field_map_builder_slot_extent *extent =
:  						builder->slots[i].extent;
:  		/** Retrive memory for the extent. */
: -		field_map[i] =
: -			(uint32_t)((char *)extent_wptr - (char *)field_map);
: -		*(uint32_t *)extent_wptr = extent->size;
: +		store_u32(&field_map[i], extent_wptr - (char *)field_map);
: +		store_u32(extent_wptr, extent->size);
:  		uint32_t extent_offset_sz = extent->size * sizeof(uint32_t);
:  		memcpy(&((uint32_t *) extent_wptr)[1], extent->offset,
:  			extent_offset_sz);
: diff --git a/src/box/field_map.h b/src/box/field_map.h
: index a1a5a9dba..d8ef726a1 100644
: --- a/src/box/field_map.h
: +++ b/src/box/field_map.h
: @@ -33,6 +33,7 @@
:  #include <assert.h>
:  #include <stdint.h>
:  #include <stddef.h>
: +#include "bit/bit.h"
: 
:  struct region;
:  struct field_map_builder_slot;
: @@ -151,20 +152,22 @@ static inline uint32_t
:  field_map_get_offset(const uint32_t *field_map, int32_t offset_slot,
:  		     int multikey_idx)
:  {
: -	uint32_t offset;
: -	if (multikey_idx != MULTIKEY_NONE &&
: -	    (int32_t) field_map[offset_slot] < 0) {
: +	/*
: +	 * Can not access field_map as a normal uint32 array
: +	 * because its alignment may be < 4 bytes. Need to use
: +	 * unaligned store-load operations explicitly.
: +	 */
: +	uint32_t offset = load_u32(&field_map[offset_slot]);
: +	if (multikey_idx != MULTIKEY_NONE && (int32_t)offset < 0) {
:  		/**
:  		 * The field_map extent has the following
:  		 * structure: [size=N|slot1|slot2|..|slotN]
:  		 */
: -		uint32_t *extent = (uint32_t *)((char *)field_map +
: -					(int32_t)field_map[offset_slot]);
: -		if ((uint32_t)multikey_idx >= extent[0])
: +		const uint32_t *extent = (const uint32_t *)
: +			((const char *)field_map + (int32_t)offset);
: +		if ((uint32_t)multikey_idx >= load_u32(&extent[0]))
:  			return 0;
: -		offset = extent[multikey_idx + 1];
: -	} else {
: -		offset = field_map[offset_slot];
: +		offset = load_u32(&extent[multikey_idx + 1]);
:  	}
:  	return offset;
:  }
: --
: 2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 09/10] port: make port_c_entry not PACKED

[Vladislav Shpilevoy]

PACKED structures don't have padding between their members and
after the structure (needed to be able to store them in an array).

Port_c_entry was PACKED, since it does not have padding between
its members anyway, and the padding in the end was not needed,
because these objects are never stored in an array. As a result,
sizeof(port_c_entry) was not aligned too.

Appeared, that mempool, used to allocate port_c_entry objects,
can't work correctly, when object size is not aligned at least by
8 bytes. Because mempool does not do any alignment internally, and
uses the free objects as a temporary storage for some metadata,
requiring 8 byte alignment.

The patch removes PACKED attribute from port_c_entry, so now its
size is aligned by 8 bytes, and mempool works fine.

Part of #4609
---
 src/box/lua/schema.lua |  2 +-
 src/box/port.h         | 10 +---------
 src/lib/core/port.h    |  2 +-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index cdfbb65f7..e6844b45f 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -79,7 +79,7 @@ ffi.cdef[[
     box_txn_savepoint_t *
     box_txn_savepoint();
 
-    struct __attribute__((packed)) port_c_entry {
+    struct port_c_entry {
         struct port_c_entry *next;
         union {
             struct tuple *tuple;
diff --git a/src/box/port.h b/src/box/port.h
index 76bb5ed1b..43d0f9deb 100644
--- a/src/box/port.h
+++ b/src/box/port.h
@@ -106,12 +106,7 @@ void
 port_vdbemem_create(struct port *base, struct sql_value *mem,
 		    uint32_t mem_count);
 
-/**
- * The struct is PACKED to avoid unnecessary 4 byte padding
- * after mp_size. These objects are never stored on stack, neither
- * allocated as an array, so the padding is not needed.
- */
-struct PACKED port_c_entry {
+struct port_c_entry {
 	struct port_c_entry *next;
 	union {
 		/** Valid if mp_size is 0. */
@@ -126,9 +121,6 @@ struct PACKED port_c_entry {
 	uint32_t mp_size;
 };
 
-static_assert(sizeof(struct port_c_entry) == 20,
-	      "port_c_entry is expected to be perfectly aligned and packed");
-
 /**
  * C port is used by C functions from the public API. They can
  * return tuples and arbitrary MessagePack.
diff --git a/src/lib/core/port.h b/src/lib/core/port.h
index bb7787679..5c51f76e1 100644
--- a/src/lib/core/port.h
+++ b/src/lib/core/port.h
@@ -122,7 +122,7 @@ struct port {
 	 * Implementation dependent content. Needed to declare
 	 * an abstract port instance on stack.
 	 */
-	char pad[52];
+	char pad[60];
 };
 
 /** Is not inlined just to be exported. */
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 09/10] port: make port_c_entry not PACKED

[Timur Safin]

LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 09/10] port: make port_c_entry not PACKED
: 
: PACKED structures don't have padding between their members and
: after the structure (needed to be able to store them in an array).
: 
: Port_c_entry was PACKED, since it does not have padding between
: its members anyway, and the padding in the end was not needed,
: because these objects are never stored in an array. As a result,
: sizeof(port_c_entry) was not aligned too.
: 
: Appeared, that mempool, used to allocate port_c_entry objects,
: can't work correctly, when object size is not aligned at least by
: 8 bytes. Because mempool does not do any alignment internally, and
: uses the free objects as a temporary storage for some metadata,
: requiring 8 byte alignment.
: 
: The patch removes PACKED attribute from port_c_entry, so now its
: size is aligned by 8 bytes, and mempool works fine.
: 
: Part of #4609
: ---
:  src/box/lua/schema.lua |  2 +-
:  src/box/port.h         | 10 +---------
:  src/lib/core/port.h    |  2 +-
:  3 files changed, 3 insertions(+), 11 deletions(-)
: 
: diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
: index cdfbb65f7..e6844b45f 100644
: --- a/src/box/lua/schema.lua
: +++ b/src/box/lua/schema.lua
: @@ -79,7 +79,7 @@ ffi.cdef[[
:      box_txn_savepoint_t *
:      box_txn_savepoint();
: 
: -    struct __attribute__((packed)) port_c_entry {
: +    struct port_c_entry {
:          struct port_c_entry *next;
:          union {
:              struct tuple *tuple;
: diff --git a/src/box/port.h b/src/box/port.h
: index 76bb5ed1b..43d0f9deb 100644
: --- a/src/box/port.h
: +++ b/src/box/port.h
: @@ -106,12 +106,7 @@ void
:  port_vdbemem_create(struct port *base, struct sql_value *mem,
:  		    uint32_t mem_count);
: 
: -/**
: - * The struct is PACKED to avoid unnecessary 4 byte padding
: - * after mp_size. These objects are never stored on stack, neither
: - * allocated as an array, so the padding is not needed.
: - */
: -struct PACKED port_c_entry {
: +struct port_c_entry {
:  	struct port_c_entry *next;
:  	union {
:  		/** Valid if mp_size is 0. */
: @@ -126,9 +121,6 @@ struct PACKED port_c_entry {
:  	uint32_t mp_size;
:  };
: 
: -static_assert(sizeof(struct port_c_entry) == 20,
: -	      "port_c_entry is expected to be perfectly aligned and
: packed");
: -
:  /**
:   * C port is used by C functions from the public API. They can
:   * return tuples and arbitrary MessagePack.
: diff --git a/src/lib/core/port.h b/src/lib/core/port.h
: index bb7787679..5c51f76e1 100644
: --- a/src/lib/core/port.h
: +++ b/src/lib/core/port.h
: @@ -122,7 +122,7 @@ struct port {
:  	 * Implementation dependent content. Needed to declare
:  	 * an abstract port instance on stack.
:  	 */
: -	char pad[52];
: +	char pad[60];
:  };
: 
:  /** Is not inlined just to be exported. */
: --
: 2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 00/10] Sanitize unaligned access

[Vladislav Shpilevoy]

Still need a second review here.

Re: [Tarantool-patches] [PATCH v2 00/10] Sanitize unaligned access

[Vladislav Shpilevoy]

Pushed to master.