Re: [Tarantool-patches] [PATCH small 0/2] Aligned lsregion

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 16/05/2020 21:09, Aleksandr Lyapunov wrote:
> 
> On 5/16/20 2:22 AM, Vladislav Shpilevoy wrote:
>>
>>> lsregion will allocate aligned addresses as long as you request only aligned sizes,
>>> up to sizeof(uintptr_t) (see lslab_sizeof()).
>>> This property is not documented (maybe it should be!). It is also similar to mempool
>>> property, see mempool_create description.
>> Yeah, here are 3 problems:
>> - lsregion is used not only for tuples, but for bps tree extents too. We can't
>>    rely on their alignment being the same. Sanitizer revealed, that both extents
>>    and tuples are not aligned;
> Since you decided to implement _aligned_alloc anyway, it's not very important, but
> I want to find the truth. It could help us one day.
> Extents' sizes are also (very) aligned, all you need is to align tuple sizes, as I proposed.
> Try it and you will see: if you align sizes (up to 8) - you'll get aligned addresses.

This is based on an assumption, that extent and tuples need the same alignment.
I don't like making such obscure assumptions, because they tend to break, when
any tiny detail changes.

>> - sanitizer revealed, that mempool_alloc returns unaligned addresses too. I am
>>    going to look into that after lsregion;
> I checked the code and I confirm that if mempool item size is aligned, the addresses
> will be also aligned. Could you suggest me how to get those sanitizer results?
> I also see that small_alloc aligns all allocations with 8 bytes.

If mempool item size is not aligned, this won't work. I got the unaligned
access crash somewhere in box/port.c around port_c_entry structure. I didn't
check where removal of 'PACKED' from this structure helps yet. But anyway
this looks like a mempool bug. It should not have problems with any object
size, regardless of its alignment. Because the crash was inside of mempool,
when it tried to write something into a block passed to mempool_free(). So
mempool assumes, that object size is aligned at least like mempool internal
entries. And if one day we will add uint64 into a mempool entry, but will
use the pool to allocate 4 byte aligned objects, this will break too.

You can try the sanitizer, if you have clang, with this diff:

====================
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")

====================

>> - slab first position can be aligned not enough. So we can't assume anything
>>    about first address in a slab. And therefore about all the other addresses
>>    too.
> Yes, that position is aligned with 8 bytes now. Are there any cases when we need bigger
> align factor?

As I said, I don't like making such assumptions.

>>> I like one-line change more.
>>>
>>> If you still want _reserve and _aligned_alloc method for further purposes, please
>>> read my by-commit comments.
>> I want lsregion be consistent with other allocators, and provide basic
>> API for aligned allocations. Like region does.
> Ok, that's a great idea, doesn't it earn a special ticket on github?

No. I didn't create an issue for that, since I consider it a part of 4609.