-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[NFC][LLDB][BoundsSatety] Add InstrumentationRuntime::MatchAllModules
#166001
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
[NFC][LLDB][BoundsSatety] Add InstrumentationRuntime::MatchAllModules
#166001
Conversation
This adds a virtual method that allows `InstrumentationRuntime` sub classes to match against all modules rather than just a library that matches a particular regex. When the implementation returns true `GetPatternForRuntimeLibrary()` is ignored and all modules are iterated over. The default implementation returns false which was the previous behavior which uses `GetPatternForRuntimeLibrary()` to only match a particular runtime library. The intended use case here is for implementing an `InstrumentationRuntime` where the runtime library of interest can have multiple implementations and whose name is not known ahead of time. The concrete use case here is for a `InstrumentationRuntime` plugin for implementations of the `-fbounds-safety` soft-trap runtime which can have multiple different implementations and so the module containing the runtime functions isn't known ahead of time. This plug-in will be upstreamed as part of the process of upstreaming `-fbounds-safety`. An alternative to this would be for the `GetPatternForRuntimeLibrary()` function to return a regex that matches everything. While that technically works this new API more clearly indicates in the intent. We probably also save a little perf by not executing the regex match for every loaded module but I have not measured this. rdar://163230807
|
@llvm/pr-subscribers-lldb Author: Dan Liew (delcypher) ChangesThis adds a virtual method that allows The intended use case here is for implementing an The concrete use case here is for a An alternative to this would be for the rdar://163230807 Full diff: https://github.com/llvm/llvm-project/pull/166001.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Target/InstrumentationRuntime.h b/lldb/include/lldb/Target/InstrumentationRuntime.h
index a6121c24b9560..d2499528e97ab 100644
--- a/lldb/include/lldb/Target/InstrumentationRuntime.h
+++ b/lldb/include/lldb/Target/InstrumentationRuntime.h
@@ -73,6 +73,13 @@ class InstrumentationRuntime
/// is guaranteed to be loaded.
virtual void Activate() = 0;
+ /// \return true if `CheckIfRuntimeIsValid` should be called on all modules.
+ /// In this case the return value of `GetPatternForRuntimeLibrary` will be
+ /// ignored. Return false if `CheckIfRuntimeIsValid` should only be called
+ /// for modules whose name matches `GetPatternForRuntimeLibrary`.
+ ///
+ virtual bool MatchAllModules() { return false; }
+
public:
static void ModulesDidLoad(lldb_private::ModuleList &module_list,
Process *process,
diff --git a/lldb/source/Target/InstrumentationRuntime.cpp b/lldb/source/Target/InstrumentationRuntime.cpp
index 7e58e8bf26cb1..d9800a8541f4e 100644
--- a/lldb/source/Target/InstrumentationRuntime.cpp
+++ b/lldb/source/Target/InstrumentationRuntime.cpp
@@ -55,7 +55,8 @@ void InstrumentationRuntime::ModulesDidLoad(
return IterationAction::Continue;
const RegularExpression &runtime_regex = GetPatternForRuntimeLibrary();
- if (runtime_regex.Execute(file_spec.GetFilename().GetCString()) ||
+ if (MatchAllModules() ||
+ runtime_regex.Execute(file_spec.GetFilename().GetCString()) ||
module_sp->IsExecutable()) {
if (CheckIfRuntimeIsValid(module_sp)) {
SetRuntimeModuleSP(module_sp);
|
medismailben
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
jimingham
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 too.
|
Thanks for the reviews. |
…s` (llvm#166001) This adds a virtual method that allows `InstrumentationRuntime` sub classes to match against all modules rather than just a library that matches a particular regex. When the implementation returns true `GetPatternForRuntimeLibrary()` is ignored and all modules are iterated over. The default implementation returns false which was the previous behavior which uses `GetPatternForRuntimeLibrary()` to only match a particular runtime library. The intended use case here is for implementing an `InstrumentationRuntime` where the runtime library of interest can have multiple implementations and whose name is not known ahead of time. The concrete use case here is for a `InstrumentationRuntime` plugin for implementations of the `-fbounds-safety` soft-trap runtime which can have multiple different implementations and so the module containing the runtime functions isn't known ahead of time. This plug-in will be upstreamed as part of the process of upstreaming `-fbounds-safety`. An alternative to this would be for the `GetPatternForRuntimeLibrary()` function to return a regex that matches everything. While that technically works this new API more clearly indicates in the intent. We probably also save a little perf by not executing the regex match for every loaded module but I have not measured this. rdar://163230807 (cherry picked from commit a8de649)
…s` (llvm#166001) This adds a virtual method that allows `InstrumentationRuntime` sub classes to match against all modules rather than just a library that matches a particular regex. When the implementation returns true `GetPatternForRuntimeLibrary()` is ignored and all modules are iterated over. The default implementation returns false which was the previous behavior which uses `GetPatternForRuntimeLibrary()` to only match a particular runtime library. The intended use case here is for implementing an `InstrumentationRuntime` where the runtime library of interest can have multiple implementations and whose name is not known ahead of time. The concrete use case here is for a `InstrumentationRuntime` plugin for implementations of the `-fbounds-safety` soft-trap runtime which can have multiple different implementations and so the module containing the runtime functions isn't known ahead of time. This plug-in will be upstreamed as part of the process of upstreaming `-fbounds-safety`. An alternative to this would be for the `GetPatternForRuntimeLibrary()` function to return a regex that matches everything. While that technically works this new API more clearly indicates in the intent. We probably also save a little perf by not executing the regex match for every loaded module but I have not measured this. rdar://163230807 (cherry picked from commit a8de649)
This adds a virtual method that allows
InstrumentationRuntimesub classes to match against all modules rather than just a library that matches a particular regex. When the implementation returns trueGetPatternForRuntimeLibrary()is ignored and all modules are iterated over. The default implementation returns false which was the previous behavior which usesGetPatternForRuntimeLibrary()to only match a particular runtime library.The intended use case here is for implementing an
InstrumentationRuntimewhere the runtime library of interest can have multiple implementations and whose name is not known ahead of time.The concrete use case here is for a
InstrumentationRuntimeplugin for implementations of the-fbounds-safetysoft-trap runtime which can have multiple different implementations and so the module containing the runtime functions isn't known ahead of time. This plug-in will be upstreamed as part of the process of upstreaming-fbounds-safety.An alternative to this would be for the
GetPatternForRuntimeLibrary()function to return a regex that matches everything. While that technically works this new API more clearly indicates in the intent. We probably also save a little perf by not executing the regex match for every loaded module but I have not measured this.rdar://163230807