Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some buffers having uninitialized contents after resizing #51

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

daleonov
Copy link
Contributor

@daleonov daleonov commented Jun 9, 2023

I used valgrind to test my audio plugin, and it spat out a bunch of warnings like this one:

==61792== Conditional jump or move depends on uninitialised value(s)
==61792==    at 0x4D4CB77: tanhf (s_tanhf.c:36)
==61792==    by 0x461359: std::tanh(float) (cmath:502)
==61792==    by 0xACCF70: activations::ActivationTanh::apply(float*, long) (activations.h:70)
==61792==    by 0xACCEF9: activations::Activation::apply(Eigen::Block<Eigen::Matrix<float, -1, -1, 0, -1, -1>, -1, -1, true>) (activations.h:50)
==61792==    by 0xAD086B: convnet::ConvNetBlock::process_(Eigen::Matrix<float, -1, -1, 0, -1, -1> const&, Eigen::Matrix<float, -1, -1, 0, -1, -1>&, long, long) const (convnet.cpp:71)
==61792==    by 0xAD10A9: convnet::ConvNet::_process_core_() (convnet.cpp:129)
==61792==    by 0xAD6E69: DSP::process(double**, double**, int, int, double, double, std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, double, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, double> > > const&) (dsp.cpp:40)
(...)

Apparently a lot of times after matrix or vector gets resized, the memory in those buffers isn't initialized before getting referenced. Which could cause garbage data in audio and ML buffers... This patch fixes it.

Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this! My Linux box has been down for ages and I haven't found time to figure out how to get it resurrected. So I've been missing valgrind dearly on this project.

I think it's worth checking out setZero instead of introducing init_matrix. Mind having a look and either getting back witha revision or letting me know if I'm off-base?

Thanks again!

NAM/util.cpp Outdated Show resolved Hide resolved
@daleonov daleonov requested a review from sdatkinson June 11, 2023 12:00
@sdatkinson
Copy link
Owner

Thanks for your patience. I need a moment to sit down with this and to make sure that the .setZeros are only called when necessary: IIRC, .resize doesn't do anything to the memory for Eigen if the requested size matches, so it's nice and fast. .setZero following such cases would be bad for speed's sake.

@daleonov
Copy link
Contributor Author

No problem! But every time you resize the matrix, regardless of whether it needs actual memory allocation or not, the contents of this matrix are no longer valid, right? So before getting referenced (and they are getting referenced after resizing) we need to put something in there. One thing I've ignored is that some matrices might need be initialized with different values, like identity matrix or something pre-determined. And there were some matrices that got filled with something after resizing right away, so obviously I didn't mess with those.

Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍🏻

@sdatkinson sdatkinson merged commit ca5b3a2 into sdatkinson:main Sep 5, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants