<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, thanks for review!</p>
<p>See my 2 comments below.</p>
<p><br>
</p>
<p><font face="monospace">Branch:
<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/tree/artemreyt/gh-4966-gcc10-warns">https://github.com/tarantool/tarantool/tree/artemreyt/gh-4966-gcc10-warns</a></font></p>
<p><font face="monospace">Issue:
<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/4966">https://github.com/tarantool/tarantool/issues/4966</a></font><br>
</p>
<div class="moz-cite-prefix">21.11.2020 14:29, Aleksandr Lyapunov
пишет:<br>
</div>
<blockquote type="cite"
cite="mid:25943aa1-3cb1-0a89-8099-b56c332ff04d@tarantool.org">Hi!
Thanks for the patch!
<br>
I have the following thoughts:
<br>
1. It's not obvious for me that if a function does not allocate of
free memory
<br>
then it can't have influence on memory access. For example,
deletion from tree
<br>
can break tree structure and returned pages are not correct page
anymore.
<br>
I think we should dig more.
<br>
</blockquote>
<p>1. They may be not correct pages in a tree, but in this function
(<font face="monospace">tt_bitset_set</font>) it doesn't matter</p>
<p>for displayed warning.</p>
<p>The most important thing - is it enough memory was allocated for
returned page AFTER <br>
</p>
<p><font face="monospace">(uint8_t *) page + sizeof(tt_bitset_page)</font>.
I will add a definition of <font face="monospace">tt_bitset_page</font>
for visibility:<br>
</p>
<pre style="background-color:#2b2b2b;color:#a9b7c6;font-family:'JetBrains Mono',monospace;font-size:9,8pt;"><span style="color:#cc7832;">struct </span><span style="color:#b5b6e3;">tt_bitset_page </span>{
<span style="color:#b9bcd1;">size_t </span><span style="color:#9373a5;">first_pos</span><span style="color:#cc7832;">;
</span><span style="color:#cc7832;"> </span>rb_node(<span style="color:#cc7832;">struct </span><span style="color:#b5b6e3;">tt_bitset_page</span>) <span style="color:#9373a5;">node</span><span style="color:#cc7832;">;
</span><span style="color:#cc7832;"> </span><span style="color:#b9bcd1;">size_t </span><span style="color:#9373a5;">cardinality</span><span style="color:#cc7832;">;
</span><span style="color:#cc7832;"> </span><span style="color:#b9bcd1;">uint8_t </span><span style="color:#9373a5;">data</span>[<span style="color:#6897bb;">0</span>]<span style="color:#cc7832;">;
</span>}<span style="color:#cc7832;">;
</span></pre>
<p>This is not trivial problem. I made a new ticket for this:</p>
<p><a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/5564">https://github.com/tarantool/tarantool/issues/5564</a><br>
</p>
<blockquote type="cite"
cite="mid:25943aa1-3cb1-0a89-8099-b56c332ff04d@tarantool.org">2.
I'm sure we should not switch off the check for entire project
because of one file.
<br>
At least we should use smth like set_source_file_properties
<br>
<br>
</blockquote>
<p>2. I agree with that, so i switched off the check only for <font
face="monospace">bitset.c </font>referred to the created ticket<font
face="monospace">:</font></p>
<p><font face="monospace">diff --git a/src/lib/bitset/CMakeLists.txt
b/src/lib/bitset/CMakeLists.txt<br>
index 1339a02ef..0aba49a47 100644<br>
--- a/src/lib/bitset/CMakeLists.txt<br>
+++ b/src/lib/bitset/CMakeLists.txt<br>
@@ -6,6 +6,11 @@ set(lib_sources<br>
index.c<br>
)<br>
<br>
+if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION
VERSION_GREATER_EQUAL 10)<br>
+ # gh-5564: bitset: writing 1 byte into a region of size 0
(gcc 10)</font></p>
<p><font face="monospace">+
set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/bitset.c<br>
+ PROPERTIES COMPILE_OPTIONS -Wno-stringop-overflow)<br>
+endif()<br>
set_source_files_compile_flags(${lib_sources})<br>
add_library(bitset STATIC ${lib_sources})<br>
target_link_libraries(bitset bit)<br>
<br>
</font></p>
<blockquote type="cite"
cite="mid:25943aa1-3cb1-0a89-8099-b56c332ff04d@tarantool.org">On
11/20/20 5:23 AM, Artem Starshov wrote:
<br>
<blockquote type="cite">GCC 10 reports: "warning: writing 1 byte
into
<br>
a region of size 0 [-Wstringop-overflow=]"
<br>
<br>
It's false positive. In order to fix build
<br>
i tried to add ignoring gcc -Wstringop-overflow via
<br>
preprocessor directive `pragma GCC ignored`.
<br>
But it doesn't work, so added -Wnostringop-overflow
<br>
flag in complier.cmake for gcc version equal or greather 10.
<br>
<br>
<br>
Explaining of false positive:
<br>
The problem is:
<br>
In file included from /root/ttar/src/lib/bitset/bitset.h:45,
<br>
from /root/ttar/src/lib/bitset/bitset.c:32:
<br>
In function ‘bit_set’,
<br>
inlined from ‘tt_bitset_set’ at
/root/ttar/src/lib/bitset/bitset.c:112:14:
<br>
/root/ttar/src/lib/bit/bit.h:230:15: warning: writing 1 byte
into a region of size 0 [-Wstringop-overflow=]
<br>
230 | ldata[chunk] |= (1UL << offset);
<br>
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
<br>
<br>
I made a research and found out that if to explicitly init
bitset->realloc = realloc
<br>
than a warning is still triggers, but if not to call rb_tree
functions (tt_bitset_pages_search and
<br>
tt_bitset_pages_insert) it's ok. But these functions don't
allocate or free a memory, so they can't
<br>
influence on memory region which `page` variable points to.
<br>
<br>
Part-of #4966
<br>
---
<br>
cmake/compiler.cmake | 14 ++++++++++++++
<br>
1 file changed, 14 insertions(+)
<br>
<br>
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
<br>
index db2ae6227..5db636812 100644
<br>
--- a/cmake/compiler.cmake
<br>
+++ b/cmake/compiler.cmake
<br>
@@ -430,3 +430,17 @@ else()
<br>
set(CMAKE_HOST_C_COMPILER ${CMAKE_C_COMPILER})
<br>
set(CMAKE_HOST_CXX_COMPILER ${CMAKE_CXX_COMPILER})
<br>
endif()
<br>
+
<br>
+if (CMAKE_COMPILER_IS_GNUCC AND CMAKE_C_COMPILER_VERSION
VERSION_GREATER_EQUAL 10)
<br>
+# In file included from src/lib/bitset/bitset.h:45,
<br>
+# from src/lib/bitset/bitset.c:32:
<br>
+# In function ‘bit_set’,
<br>
+# inlined from ‘tt_bitset_set’ at
src/lib/bitset/bitset.c:111:14:
<br>
+# src/lib/bit/bit.h:230:15: warning: writing 1 byte into a
region of size 0 [-Wstringop-overflow=]
<br>
+# 230 | ldata[chunk] |= (1UL << offset);
<br>
+# | ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
<br>
+#
<br>
+# false positive for gcc version 10
<br>
+# macro 'GCC diagnostic ignored "-Wstringop-overflow"' doesn't
help
<br>
+ add_compile_flags("C" "-Wno-stringop-overflow")
<br>
+endif()
<br>
</blockquote>
</blockquote>
</body>
</html>