Analyse Valgrind report #104
Comments
|
I remember doing this analysis a few months ago. I think the ones in |
|
There’s a few involving StringPieces (now absl::string_views) which makes me wonder if there are some lifetime issues where the view is outliving the string or vice versa. I definitely want to take another look at how a Scanner contains a pointer to a StringPiece… |
|
@peterzhu2118 suggested the source of the Scanner leak in rewind where I overwrite the input without deleting the old: https://ruby.social/@peterzhu2118/111108837791465609 |
|
Oh, right, that makes sense. |
|
Following @peterzhu2118's advice, I've been trying to focus on minimal test cases that trigger the Valgrind reports: for the leak in it "raises an error if given an inappropriate type" do
expect { RE2::Regexp.new(nil) }.to raise_error(TypeError)
end |
|
It seems to be fixed by adding a |
|
This one has me currently stumped: Lines 1571 to 1573 in 23fab86 |
|
My valgrind trace shows: Is valgrind flagging I applied this diff to see the symbols in abseil and libre2: diff --git a/ext/re2/extconf.rb b/ext/re2/extconf.rb
index f8fd706..c8c6681 100644
--- a/ext/re2/extconf.rb
+++ b/ext/re2/extconf.rb
@@ -119,8 +119,8 @@ end
def build_extension(static_p = false)
# Enable optional warnings but disable deprecated register warning for Ruby 2.6 support
- $CFLAGS << " -Wall -Wextra -funroll-loops"
- $CPPFLAGS << " -Wno-register"
+ $CFLAGS << " -Wall -Wextra -funroll-loops -ggdb3"
+ $CPPFLAGS << " -Wno-register -ggdb3"
# Pass -x c++ to force gcc to compile the test program
# as C++ (as it will end in .c by default).
@@ -395,11 +395,11 @@ def build_with_vendored_libraries
abseil_recipe, re2_recipe = load_recipes
process_recipe(abseil_recipe) do |recipe|
- recipe.configure_options += ['-DABSL_PROPAGATE_CXX_STD=ON', '-DCMAKE_CXX_VISIBILITY_PRESET=hidden']
+ recipe.configure_options += ['-DABSL_PROPAGATE_CXX_STD=ON', '-DCMAKE_CXX_VISIBILITY_PRESET=hidden', '-DCMAKE_CXXFLAGS=-ggdb3']
end
process_recipe(re2_recipe) do |recipe|
- recipe.configure_options += ["-DCMAKE_PREFIX_PATH=#{abseil_recipe.path}", '-DCMAKE_CXX_FLAGS=-DNDEBUG',
+ recipe.configure_options += ["-DCMAKE_PREFIX_PATH=#{abseil_recipe.path}", '-DCMAKE_CXX_FLAGS=-DNDEBUG', '-DCMAKE_CXX_FLAGS=-ggdb3',
'-DCMAKE_CXX_VISIBILITY_PRESET=hidden']
end |
|
I've been talking to Peter on ruby.social about this while I look into it: https://ruby.social/@mudge/111110365227989022 In short, the leak disappears if I remove the |
|
As https://valgrind.org/docs/manual/faq.html#faq.reports mentions:
I tried to make |
See #104 Specifically, ensure we delete the previous input inside an RE2::Scanner before replacing it and check whether inputs are strings as early as possible to avoid raising an exception after allocating memory.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed so we need to free it ourselves manually. However, we also need a copy of what is inside it to return to the user so we turn that into a C string first. The maximum message size of 100 is taken from the length of the prefix of the message (33 characters) and the longest error message currently in RE2 (35 characters) plus a little extra in case new releases of RE2 add longer messages.
|
I think I've now fixed all the leaks in this report in #105 |
See #104 Specifically, ensure we delete the previous input inside an RE2::Scanner before replacing it and check whether inputs are strings as early as possible to avoid raising an exception after allocating memory. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed so we need to free it ourselves manually. However, we also need a copy of what is inside it to return to the user so we turn that into a C string first. The maximum message size of 100 is taken from the length of the prefix of the message (33 characters) and the longest error message currently in RE2 (35 characters) plus a little extra in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed so we need to free it ourselves manually. However, we also need a copy of what is inside it to return to the user so we turn that into a C string first. The maximum message size of 100 is taken from the length of the prefix of the message (33 characters) and the longest error message currently in RE2 (35 characters) plus a little extra in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed so we need to free it ourselves manually. However, we also need a copy of what is inside it to return to the user so we turn that into a C string first. The maximum message size of 100 is taken from the length of the prefix of the message (33 characters) and the longest error message currently in RE2 (35 characters) plus a little extra in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed. Fix this by ensuring it goes out of scope before we call rb_raise. However, we also need a copy of what is inside it to return to the user so we take a copy of its contents as a C string first. The current longest error message inside RE2 is 35 characters long so 100 characters gives us some headroom in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
See #104 When we raise an exception in re2_set_add, the memory used by the std::string used to store the error message is never freed. Fix this by ensuring it goes out of scope before we call rb_raise. However, we also need a copy of what is inside it to return to the user so we take a copy of its contents as a C string first. The current longest error message inside RE2 is 35 characters long so 100 characters gives us some headroom in case new releases of RE2 add longer messages. Thanks to @peterzhu2118 for both authoring ruby_memcheck and helping find the source of these leaks.
Running ruby_memcheck has produced the following possible leaks from Valgrind:
The text was updated successfully, but these errors were encountered: