-
Notifications
You must be signed in to change notification settings - Fork 472
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
Improve using of ASan and TSan in CMake build #599
Conversation
|
Thanks @PragmaTwice Cool, I want to this commit too long time.
Did TCL tests report errors? |
Seems like the server aborted by asan while running tcl tests, I will check its log.
|
Seems much less than unit tests? Full stderr:
|
But a weird thing is that the failed redis-server reported by tcl test has an empty stderr file. |
Those memory leaks Looks all came from Lua scripting, I found we didn't close Lua VM before stopping the server. I will have a try and fix it soon. |
seems like some of leaks from unittest is due to missing the dtor call (i.e. delete operator) after |
@PragmaTwice How can I reproduce the memory leaks in tcl tests? I have tried to build with |
Did these tcl tests on your side pass or fail? There are some suggestions you could try:
stderr from Clang:
|
Copy that, thanks @PragmaTwice. |
@PragmaTwice since #605 merged, is this PR ready for review (merge)? |
Leaks in unit tests are fixed, but these are still several leaks in TCL tests. |
This comment was marked as outdated.
This comment was marked as outdated.
#614 is resolved now 🎉 |
@PragmaTwice shall we merge master, running CI with nightly changes first, or current status is already ready for review? |
Yeah, I think this PR is ready for review now~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I love this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truly thank you @PragmaTwice I waited too long time for this commit.
But i still want to make sure these sanitizers work well, can they report errors if we leak memory or data race access designedly
@ShooterIT so far, sanitizers run daily and there will be failure reports in "Actions" page if daily CI failed. |
Yeah, I think these sanitizers work well. |
One problem though is that the TCL tests seem not to pay attention to the return value of the redis server (kvrocks) when it crashes, which may make the sanitizer's error difficult to observe by developers, I will investigate this issue later. |
This seems a separated issue. Merging this PR ... |
Close #590.