Re: [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build

[Artem <artemreyt@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4582 bytes --]

Hi, thanks for review!

See my 2 comments below.


Branch: 
https://github.com/tarantool/tarantool/tree/artemreyt/gh-4966-gcc10-warns

Issue: https://github.com/tarantool/tarantool/issues/4966

21.11.2020 14:29, Aleksandr Lyapunov пишет:
> Hi! Thanks for the patch!
> I have the following thoughts:
> 1. It's not obvious for me that if a function does not allocate of 
> free memory
> then it can't have influence on memory access. For example, deletion 
> from tree
> can break tree structure and returned pages are not correct page anymore.
> I think we should dig more.

1. They may be not correct pages in a tree, but in this function 
(tt_bitset_set) it doesn't matter

for displayed warning.

The most important thing - is it enough memory was allocated for 
returned page AFTER

(uint8_t *) page + sizeof(tt_bitset_page). I will add a definition of 
tt_bitset_page for visibility:

struct tt_bitset_page {
    size_t first_pos; rb_node(struct tt_bitset_page)node; size_t cardinality; uint8_t data[0]; };

This is not trivial problem. I made a new ticket for this:

https://github.com/tarantool/tarantool/issues/5564

> 2. I'm sure we should not switch off the check for entire project 
> because of one file.
> At least we should use smth like set_source_file_properties
>
2. I agree with that, so i switched off the check only for bitset.c 
referred to the created ticket:

diff --git a/src/lib/bitset/CMakeLists.txt b/src/lib/bitset/CMakeLists.txt
index 1339a02ef..0aba49a47 100644
--- a/src/lib/bitset/CMakeLists.txt
+++ b/src/lib/bitset/CMakeLists.txt
@@ -6,6 +6,11 @@ set(lib_sources
      index.c
  )

+if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION 
VERSION_GREATER_EQUAL 10)
+    # gh-5564: bitset: writing 1 byte into a region of size 0 (gcc 10)

+ set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/bitset.c
+            PROPERTIES COMPILE_OPTIONS -Wno-stringop-overflow)
+endif()
  set_source_files_compile_flags(${lib_sources})
  add_library(bitset STATIC ${lib_sources})
  target_link_libraries(bitset bit)

> On 11/20/20 5:23 AM, Artem Starshov wrote:
>> GCC 10 reports: "warning: writing 1 byte into
>> a region of size 0 [-Wstringop-overflow=]"
>>
>> It's false positive. In order to fix build
>> i tried to add ignoring gcc -Wstringop-overflow via
>> preprocessor directive `pragma GCC ignored`.
>> But it doesn't work, so added -Wnostringop-overflow
>> flag in complier.cmake for gcc version equal or greather 10.
>>
>>
>> Explaining of false positive:
>> The problem is:
>> In file included from /root/ttar/src/lib/bitset/bitset.h:45,
>>                   from /root/ttar/src/lib/bitset/bitset.c:32:
>> In function ‘bit_set’,
>>      inlined from ‘tt_bitset_set’ at 
>> /root/ttar/src/lib/bitset/bitset.c:112:14:
>> /root/ttar/src/lib/bit/bit.h:230:15: warning: writing 1 byte into a 
>> region of size 0 [-Wstringop-overflow=]
>>    230 |  ldata[chunk] |= (1UL << offset);
>>        |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>>
>> I made a research and found out that if to explicitly init 
>> bitset->realloc = realloc
>> than a warning is still triggers, but if not to call rb_tree 
>> functions (tt_bitset_pages_search and
>> tt_bitset_pages_insert) it's ok. But these functions don't allocate 
>> or free a memory, so they can't
>> influence on memory region which `page` variable points to.
>>
>> Part-of #4966
>> ---
>>   cmake/compiler.cmake | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
>> index db2ae6227..5db636812 100644
>> --- a/cmake/compiler.cmake
>> +++ b/cmake/compiler.cmake
>> @@ -430,3 +430,17 @@ else()
>>       set(CMAKE_HOST_C_COMPILER ${CMAKE_C_COMPILER})
>>       set(CMAKE_HOST_CXX_COMPILER ${CMAKE_CXX_COMPILER})
>>   endif()
>> +
>> +if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION 
>> VERSION_GREATER_EQUAL 10)
>> +# In file included from src/lib/bitset/bitset.h:45,
>> +#                  from src/lib/bitset/bitset.c:32:
>> +# In function ‘bit_set’,
>> +#     inlined from ‘tt_bitset_set’ at src/lib/bitset/bitset.c:111:14:
>> +# src/lib/bit/bit.h:230:15: warning: writing 1 byte into a region of 
>> size 0 [-Wstringop-overflow=]
>> +#   230 |  ldata[chunk] |= (1UL << offset);
>> +#       |  ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>> +#
>> +# false positive for gcc version 10
>> +# macro 'GCC diagnostic ignored "-Wstringop-overflow"' doesn't help
>> +    add_compile_flags("C" "-Wno-stringop-overflow")
>> +endif()

[-- Attachment #2: Type: text/html, Size: 8139 bytes --]