-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[Support] Fix memory leak induced by sys::RemoveFileOnSignal
#159984
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
Conversation
Before this PR, `FilesToRemove` was constructed but never deleted.
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-llvm-support Author: Alexandre Ganea (aganea) ChangesBefore this PR, Full diff: https://github.com/llvm/llvm-project/pull/159984.diff 1 Files Affected:
diff --git a/llvm/lib/Support/Windows/Signals.inc b/llvm/lib/Support/Windows/Signals.inc
index dad0fa3066868..db6d2eeb4169c 100644
--- a/llvm/lib/Support/Windows/Signals.inc
+++ b/llvm/lib/Support/Windows/Signals.inc
@@ -421,8 +421,13 @@ bool sys::RemoveFileOnSignal(StringRef Filename, std::string *ErrMsg) {
return true;
}
- if (FilesToRemove == NULL)
+ if (FilesToRemove == NULL) {
FilesToRemove = new std::vector<std::string>;
+ std::atexit([]() {
+ delete FilesToRemove;
+ FilesToRemove = NULL;
+ });
+ }
FilesToRemove->push_back(std::string(Filename));
|
| } | ||
|
|
||
| if (FilesToRemove == NULL) | ||
| if (FilesToRemove == NULL) { |
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.
I think we can change FilesToRemove from static std::vector<std::string> *FilesToRemove = NULL; to static std::vector<std::string> FilesToRemove;, and change all pices where FilesToRemove == NULL to FilesToRemove.empty(), What do you think?
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.
I was thinking about that. However if we make it a non-pointer, and it is destroyed during global destruction; if any other subsequent global variable destructor code tries to access FilesToRemove after its destruction, that might crash in unexpected ways. Whereas here, as a pointer, it is clearly set to NULL and we already check if (FilesToRemove != NULL) in all the APIs.
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.
Hmm, yes, it's may depends on globals destruct order.
yronglin
left a comment
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!
…159984) Before this PR, `FilesToRemove` was constructed but never deleted.
…159984) Before this PR, `FilesToRemove` was constructed but never deleted.
Before this PR,
FilesToRemovewas constructed but never deleted.