Add AddressSanitizer and valgrind support#61
Conversation
|
LGTM |
|
This looks great! I definitely would like to see ease-of-use scripts (#1) and integration with CI (#3) in a future PR. I think it's important to get it started now before we slide. I also think keeping around memcheck and AddressSanitizer is a good idea for now until we can kick the tires a bit to see which is better. I know memcheck can be painfully slow on big problems, so I'm tempted to go with Asan. |
jamiebramwell
left a comment
jamiebramwell
left a comment
There was a problem hiding this comment.
Thanks for looking at this!
| .. ## | ||
| .. ## SPDX-License-Identifier: (BSD-3-Clause) | ||
|
|
||
| =============== |
|
|
||
| // mesh | ||
| const char *mesh_file = "../../data/beam-hex.mesh"; | ||
| std::string base_mesh_file = std::string(SERAC_SRC_DIR) + "/data/beam-hex.mesh"; |
There was a problem hiding this comment.
I assume SERAC_SRC_DIR is in the config header. Should we change the tests do it this way instead?
There was a problem hiding this comment.
Would you like me to remove the command line path? I could push that to this PR if you want. It is one less thing to remember / copy&paste.
There was a problem hiding this comment.
I wouldn't worry about it now. I can add it to my next PR.
Also Asan is kinda the new hotness, there is usually value in going with what the community is converging on even if its a bit behind. |
samuelpmish
left a comment
samuelpmish
left a comment
There was a problem hiding this comment.
Looks great, can't wait to give it a shot!
3 participants
First shot at enabling memory checking. I added two options: AddressSanitizer (seems to be the new hotness) and valgrind (tried and true, but cumbersome and slow).
I added documentation here:
https://serac.readthedocs.io/en/feature-white238-memcheck/memory_checking.html
The next step is to make it easier to use. Could be in another PR though.
Improvement ideas:
Thoughts?