From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EFE04469710 for ; Wed, 25 Nov 2020 14:29:31 +0300 (MSK) References: <848eefec50be4135d9ae3a50f3816d7e334fbc02.1605828734.git.artemreyt@tarantool.org> <25943aa1-3cb1-0a89-8099-b56c332ff04d@tarantool.org> From: Artem Message-ID: <9a4cdffe-9746-e050-6d6f-570c21e27616@tarantool.org> Date: Wed, 25 Nov 2020 14:29:29 +0300 MIME-Version: 1.0 In-Reply-To: <25943aa1-3cb1-0a89-8099-b56c332ff04d@tarantool.org> Content-Type: multipart/alternative; boundary="------------F57C802C18B996CF9BC6880B" Content-Language: ru Subject: Re: [Tarantool-patches] [PATCH 2/2] bitset: fix GCC 10 build List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , Alexander Turenko Cc: tarantool-patches@dev.tarantool.org This is a multi-part message in MIME format. --------------F57C802C18B996CF9BC6880B Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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() --------------F57C802C18B996CF9BC6880B Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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()
--------------F57C802C18B996CF9BC6880B--