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

clean MSAN warnings #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jianguan210
Copy link

  1. Changed to "calloc" to create and initialize array.
  2. Initialized info before being passed into blas package.

1. Changed to "calloc" to create and initialize array.
2. Initialized `info` before being passed into blas package.
@stevengj
Copy link
Collaborator

stevengj commented Jan 28, 2021

It's a little frustrating that MSan complains about passing a pointer to uninitialized data, even when it is an output buffer for the routine you are calling, as it is here. I'm worried that using calloc everywhere will potentially hide bugs.

In particular, suppose that you malloc uninitialized memory and then you subsequently read from that memory. A memory sanitizer like valgrind will then (correctly) give you an error or warning at the site of the read, allowing you to inspect the code and fix the logic. If instead you use calloc everywhere, the same code will silently succeed even if zero is not the correct initialization.

Maybe instead define a CHK_MALLOC_ZERO macro that we can call explicitly when we want to initialize the memory to zero, e.g. to silence MSan warnings for LAPACK output buffers.

@jianguan210
Copy link
Author

Thanks for the suggestion. Got your point. The explicit initialization is good to me. The only concern is that we have to manually add CHK_MALLOC_ZERO to the places where MSan complains. Hopefully it will not be lot. Let me try it first and reach back to you. Thank you.

@stevengj
Copy link
Collaborator

stevengj commented Feb 1, 2021

The only concern is that we have to manually add CHK_MALLOC_ZERO to the places where MSan complains.

Yes, that's the point — when a memory sanitizer complains, this is an opportunity to manually check what is the correct initialization at that point in the code. The goal is not to silence the sanitizer, but to find and fix bugs.

To avoid the warning of use-of-uninitialized-value from MSan and minimize the change to the original code, we changed`calloc` back to `malloc`, and initilize an array using `memset` at the place MSan complains.
@jianguan210
Copy link
Author

Hi Prof. Johnson, please take another look, and let me know if this revision is good to you. Thanks.

@@ -323,6 +324,7 @@ void sqmatrix_sqrt(sqmatrix Usqrt, sqmatrix U, sqmatrix W)
CHECK(Usqrt.p == U.p && U.p == W.p, "matrices not conformant");

CHK_MALLOC(eigenvals, real, U.p);
memset(eigenvals, 0, sizeof(real) * U.p);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be done in sqmatrix_eigensolve, i.e. won't similar warnings come up on other calls to sqmatrix_eigensolve if Msan doesn't understand LAPACK output variables?

Also should have a comment that that this initialization is not actually necessary, but is just to silence Msan.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that this is an incomplete solution to silence the MSan based on the unit tests provided from MEEP. It would give additional warnings if other similar functions are hit, e.g. sqmatrix_eigensolve. It is quite hard to find all the MSan warnings without additional testing cases. Now maybe it is not necessary to silence warnings of use-of-uninitialized-value if you think calloc is not a good solution. Please feel free to close this one and the other one if you agree to leave as it is. Thanks.

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