[LLDB] Don't cache module sp when Activate() fails. #95586
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, the instrumentation runtime is caching a library the first time it sees it in the module list. However, in some rare cases on Darwin, the cached pre-run unloaded modules are different from the runtime module that is loaded at runtime. This patch removes the cached module if the plugin fails to activate, ensuring that on subsequent calls we don't try to activate using the unloaded cached module. There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should have a stronger check to ensure the module is loaded and can be activated. Further investigation in UpdateSpecialBinariesFromNewImageInfos calling ModulesDidLoad when the module list may have unloaded modules. I have not included a test for the following reasons: 1. This is an incredibly rare occurance and is only observed in a specific circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not commonly encountered. 2. It is difficult to reproduce -- this bug requires precise conditions on darwin and it is unclear how we'd reproduce that in a controlled testing environment. rdar://128971453
|
@llvm/pr-subscribers-lldb Author: None (thetruestblue) ChangesCurrently, the instrumentation runtime is caching a library the first time it sees it in the module list. However, in some rare cases on Darwin, the cached pre-run unloaded modules are different from the runtime module that is loaded at runtime. This patch removes the cached module if the plugin fails to activate, ensuring that on subsequent calls we don't try to activate using the unloaded cached module. There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should have a stronger check to ensure the module is loaded and can be activated. Further investigation in UpdateSpecialBinariesFromNewImageInfos calling ModulesDidLoad when the module list may have unloaded modules. I have not included a test for the following reasons:
rdar://128971453 Full diff: https://github.com/llvm/llvm-project/pull/95586.diff 1 Files Affected:
diff --git a/lldb/source/Target/InstrumentationRuntime.cpp b/lldb/source/Target/InstrumentationRuntime.cpp
index 9f22a1be20ccb..94f4ca353d7ef 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -60,6 +60,8 @@ void InstrumentationRuntime::ModulesDidLoad(
if (CheckIfRuntimeIsValid(module_sp)) {
SetRuntimeModuleSP(module_sp);
Activate();
+ if (IsActive())
+ SetRuntimeModuleSP({}); // Don't cache module if activation failed.
return false; // Stop iterating, we're done.
}
}
|
jimingham
approved these changes
Jun 14, 2024
Comment on lines
63
to
64
| if (IsActive()) | ||
| SetRuntimeModuleSP({}); // Don't cache module if activation failed. |
There was a problem hiding this comment.
Should this be if (!IsActive())?
There was a problem hiding this comment.
Thanks Jonas. 100%.
Fixed.
thetruestblue
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 18, 2024
Currently, the instrumentation runtime is caching a library the first time it sees it in the module list. However, in some rare cases on Darwin, the cached pre-run unloaded modules are different from the runtime module that is loaded at runtime. This patch removes the cached module if the plugin fails to activate, ensuring that on subsequent calls we don't try to activate using the unloaded cached module. There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should have a stronger check to ensure the module is loaded and can be activated. Further investigation in UpdateSpecialBinariesFromNewImageInfos calling ModulesDidLoad when the module list may have unloaded modules. I have not included a test for the following reasons: 1. This is an incredibly rare occurance and is only observed in a specific circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not commonly encountered. 2. It is difficult to reproduce -- this bug requires precise conditions on darwin and it is unclear how we'd reproduce that in a controlled testing environment. rdar://128971453
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Jun 18, 2024
[LLDB] Don't cache module sp when Activate() fails. (llvm#95586)
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Currently, the instrumentation runtime is caching a library the first time it sees it in the module list. However, in some rare cases on Darwin, the cached pre-run unloaded modules are different from the runtime module that is loaded at runtime. This patch removes the cached module if the plugin fails to activate, ensuring that on subsequent calls we don't try to activate using the unloaded cached module. There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should have a stronger check to ensure the module is loaded and can be activated. Further investigation in UpdateSpecialBinariesFromNewImageInfos calling ModulesDidLoad when the module list may have unloaded modules. I have not included a test for the following reasons: 1. This is an incredibly rare occurance and is only observed in a specific circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not commonly encountered. 2. It is difficult to reproduce -- this bug requires precise conditions on darwin and it is unclear how we'd reproduce that in a controlled testing environment. rdar://128971453
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the instrumentation runtime is caching a library the first time it sees it in the module list. However, in some rare cases on Darwin, the cached pre-run unloaded modules are different from the runtime module that is loaded at runtime. This patch removes the cached module if the plugin fails to activate, ensuring that on subsequent calls we don't try to activate using the unloaded cached module.
There are a few related bugs to fix in a follow up: CheckIfRuntimeValid should have a stronger check to ensure the module is loaded and can be activated. Further investigation in UpdateSpecialBinariesFromNewImageInfos calling ModulesDidLoad when the module list may have unloaded modules.
I have not included a test for the following reasons:
This is an incredibly rare occurance and is only observed in a specific circumstance on Darwin. It is tied to behavior in the DynamicLoader thai is not commonly encountered.
It is difficult to reproduce -- this bug requires precise conditions on darwin and it is unclear how we'd reproduce that in a controlled testing environment.
rdar://128971453