[lldb][ClangUserExpression][NFCI] Pass the most specific ExecutionContextScope possible into ClangExpressionParser #87657
Conversation
…textScope possible into ClangExpressionParser
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThe However, This patch simplifies Full diff: https://github.com/llvm/llvm-project/pull/87657.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 30bc81c9ed8c19..5776b1e94e0721 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -553,9 +553,9 @@ bool ClangUserExpression::PrepareForParsing(
}
bool ClangUserExpression::TryParse(
- DiagnosticManager &diagnostic_manager, ExecutionContextScope *exe_scope,
- ExecutionContext &exe_ctx, lldb_private::ExecutionPolicy execution_policy,
- bool keep_result_in_memory, bool generate_debug_info) {
+ DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx,
+ lldb_private::ExecutionPolicy execution_policy, bool keep_result_in_memory,
+ bool generate_debug_info) {
m_materializer_up = std::make_unique<Materializer>();
ResetDeclMap(exe_ctx, m_result_delegate, keep_result_in_memory);
@@ -574,7 +574,8 @@ bool ClangUserExpression::TryParse(
}
m_parser = std::make_unique<ClangExpressionParser>(
- exe_scope, *this, generate_debug_info, m_include_directories, m_filename);
+ exe_ctx.GetBestExecutionContextScope(), *this, generate_debug_info,
+ m_include_directories, m_filename);
unsigned num_errors = m_parser->Parse(diagnostic_manager);
@@ -669,15 +670,8 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
// Parse the expression
//
- Process *process = exe_ctx.GetProcessPtr();
- ExecutionContextScope *exe_scope = process;
-
- if (!exe_scope)
- exe_scope = exe_ctx.GetTargetPtr();
-
- bool parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
- execution_policy, keep_result_in_memory,
- generate_debug_info);
+ bool parse_success = TryParse(diagnostic_manager, exe_ctx, execution_policy,
+ keep_result_in_memory, generate_debug_info);
// If the expression failed to parse, check if retrying parsing with a loaded
// C++ module is possible.
if (!parse_success && shouldRetryWithCppModule(*target, execution_policy)) {
@@ -695,9 +689,8 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
// so recreate those.
CreateSourceCode(retry_manager, exe_ctx, m_imported_cpp_modules,
/*for_completion*/ false);
- parse_success = TryParse(retry_manager, exe_scope, exe_ctx,
- execution_policy, keep_result_in_memory,
- generate_debug_info);
+ parse_success = TryParse(retry_manager, exe_ctx, execution_policy,
+ keep_result_in_memory, generate_debug_info);
// Return the parse diagnostics if we were successful.
if (parse_success)
diagnostic_manager = std::move(retry_manager);
@@ -759,6 +752,7 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
}
}
+ Process *process = exe_ctx.GetProcessPtr();
if (process && m_jit_start_addr != LLDB_INVALID_ADDRESS)
m_jit_process_wp = lldb::ProcessWP(process->shared_from_this());
return true;
@@ -840,13 +834,8 @@ bool ClangUserExpression::Complete(ExecutionContext &exe_ctx,
DeclMap()->SetLookupsEnabled(true);
}
- Process *process = exe_ctx.GetProcessPtr();
- ExecutionContextScope *exe_scope = process;
-
- if (!exe_scope)
- exe_scope = exe_ctx.GetTargetPtr();
-
- ClangExpressionParser parser(exe_scope, *this, false);
+ ClangExpressionParser parser(exe_ctx.GetBestExecutionContextScope(), *this,
+ false);
// We have to find the source code location where the user text is inside
// the transformed expression code. When creating the transformed text, we
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
index 7a8c095f61183b..bc07cbcf9e646d 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -185,9 +185,9 @@ class ClangUserExpression : public LLVMUserExpression {
/// The parameter have the same meaning as in ClangUserExpression::Parse.
/// \see ClangUserExpression::Parse
bool TryParse(DiagnosticManager &diagnostic_manager,
- ExecutionContextScope *exe_scope, ExecutionContext &exe_ctx,
- lldb_private::ExecutionPolicy execution_policy, bool keep_result_in_memory,
- bool generate_debug_info);
+ ExecutionContext &exe_ctx,
+ lldb_private::ExecutionPolicy execution_policy,
+ bool keep_result_in_memory, bool generate_debug_info);
void SetupCppModuleImports(ExecutionContext &exe_ctx);
|
There was a problem hiding this comment.
Made suggestions to keep the number of changes down on this patch. Seems like we might be able to get away with always passing exe_ctx.GetBestExecutionContextScope() into places that used to take an ExecutionContextScope *. If there is a reason to upgrade to a full ExecutionContext object I am fine with doing so if needed, but wanted to suggest this just in case.
| DiagnosticManager &diagnostic_manager, ExecutionContextScope *exe_scope, | ||
| ExecutionContext &exe_ctx, lldb_private::ExecutionPolicy execution_policy, | ||
| bool keep_result_in_memory, bool generate_debug_info) { | ||
| DiagnosticManager &diagnostic_manager, ExecutionContext &exe_ctx, | ||
| lldb_private::ExecutionPolicy execution_policy, bool keep_result_in_memory, | ||
| bool generate_debug_info) { |
There was a problem hiding this comment.
This seems like it doesn't need to change. We can just always pass exe_ctx.GetBestExecutionContextScope() in at the call sites?
There was a problem hiding this comment.
My understanding is that passing ExecutionContextScope * is an inferior API since we don't care about the implementation detail that it is an object that inherits this base class. I was actually going to put up a patch the refactors more methods in this regard soonish.
@jimingham do you agree that ExecutionContext &exe_ctx is the more appropriate parameter for an API?
There was a problem hiding this comment.
Jim made comments below. ExecutionContextScope * is just as good as an ExecutionContext, but it is actually more specific as a Target, Process, Thread and StackFrame are all ExecutionContextScope * objects. They just know how to re-create any items needed. So a StackFrame can use ExecutionContextScope APIs (which is virtually overrides) to get the thread, process and target. The Thread can get the process and target. The Process can get the target. So it is no better or worse, it just specifies one thing would be the same as having an ExecutionContext filled out as much as needed. We do need to be careful to pass in the right thing as Jim said, where if we only want the Target to be used, we shouldn't pass in a StackFrame as the ExecutionContextScope *, but this will be exactly the same as passing a ExecutionContext that has the stack frame filled out when we only want to consult the target. So no real difference, and using ExecutionContextScope * is better since we can control what we pass as the root of where to start looking whereas if we pass in a fully filled out ExecutionContext, we will always consult the deepest item (Stack, then frame, then thread, then process/target) if it is filled out.
So if the ExecutionContext is always filled out correctly to the right depth, then calling the exe_ctx.GetBestExecutionContextScope() suggestion will work. If not, then we need to pass in the right level, or modify the ExecutionContext and remove items from it.
There was a problem hiding this comment.
I removed this parameter because it seemed redundant as it was only used for the ClangExpressionParser, which wants access to all of Process/Target/StackFrame. I didn't see the point in passing exe_scope into TryParse only to pass it down the the parser if we can get that from the exe_ctx anyway. Maybe I'm misunderstanding but my impression was that this is a simplification and wouldn't cause any functional change. This is purely about making the ClangExpressionParser get the StackFrame calculation right, which it doesn't even currently use apart from some logging
There was a problem hiding this comment.
I was also mostly moved by the fact that there were two things specifying what should be one thing. I was hoping that this was just redundant, not someone being overly clever, and Michael showed that it wasn't a used distinction, which is good. As far as which type to use, that's really more convenience, right? If the methods you want to call immediately from TryParse take ExecutionContextScopes, use that, if ExecutionContext's, use that. I don't think it makes any other difference.
| exe_scope, *this, generate_debug_info, m_include_directories, m_filename); | ||
| exe_ctx.GetBestExecutionContextScope(), *this, generate_debug_info, | ||
| m_include_directories, m_filename); | ||
|
|
| bool parse_success = TryParse(diagnostic_manager, exe_ctx, execution_policy, | ||
| keep_result_in_memory, generate_debug_info); |
There was a problem hiding this comment.
Per above comment this would just become:
bool parse_success = TryParse(diagnostic_manager, exe_ctx.GetBestExecutionContextScope(), execution_policy, keep_result_in_memory, generate_debug_info);
There was a problem hiding this comment.
Removing exe_ctx in favour of exe_scope is trickier because we store away the exe_ctx for use in ClangExpressionDeclMap. I'd prefer sticking with this approach unless people are strongly opposed to it
There was a problem hiding this comment.
At some point we should go through and rationalize the use of ExecutionContext vrs. ExecutionContextScope, but this patch doesn't seem the place to do that. Instead, going with whatever is locally most convenient seems the right choice here.
There was a problem hiding this comment.
@clayborg Any remaining concerns with landing this as-is? It seems like we want to have a separate discussion around which one of ExecutionContextScope or ExecutionContext to use in APIs, orthogonal to this patch.
| parse_success = TryParse(retry_manager, exe_ctx, execution_policy, | ||
| keep_result_in_memory, generate_debug_info); |
There was a problem hiding this comment.
Per initial comments above this would become:
parse_success = TryParse(retry_manager, exe_ctx.GetBestExecutionContextScope(), execution_policy,
keep_result_in_memory, generate_debug_info);
| bool TryParse(DiagnosticManager &diagnostic_manager, | ||
| ExecutionContextScope *exe_scope, ExecutionContext &exe_ctx, | ||
| lldb_private::ExecutionPolicy execution_policy, bool keep_result_in_memory, | ||
| bool generate_debug_info); | ||
| ExecutionContext &exe_ctx, | ||
| lldb_private::ExecutionPolicy execution_policy, | ||
| bool keep_result_in_memory, bool generate_debug_info); |
There was a problem hiding this comment.
Can we revert this and always just pass in "exe_ctx.GetBestExecutionContextScope()"?
You have to be careful about this. The scope we use determines how we do name lookup. That's done when we pass the exe_ctx to ResetDeclMap in ClangUserExpression::TryParse, for instance. If I want the expression parsing to use a target level name lookup (not consulting locals in a particular stack frame) that's signified by passing in an exe_ctx that doesn't have a thread or stackframe. But if you use GetBestExecutionContextScope, that will end up adding in a frame and defeating our lookup scope. |
Oh, wait. GetBestExecutionContextScope just fills in the most specific thing in the ExecutionContext. I was mistaking it for the one that fills in all the selected entities that weren't specified in the ExecutionContext. That should be equivalent to using the incoming ExecutionContext. |
…port when evaluating C++ expressions This patch attempts to decouple C++ expression evaluation from Objective-C support. We've previously enabled it by default (if a runtime existed), but that meant we're opting into extra work we only need to do for Objective-C, which complicates/slows down C++ expression evaluation. Of course there's a valid use-case for this, which is calling Objective-C APIs when stopped in C++ frames (which Objective-C++ developers might want to do). In those case we should really prompt the user to add the `expr --language objc++` flag. To accomodate a likely frequent use-case where a user breaks in a system C++ library (without debug-symbols) but their application is actually an Objective-C app, we allow Objective-C support in C++ expressions if the current frame doesn't have debug-info. This fixes llvm#75443 and allows us to add more `LangOpts.ObjC` guards around the expression evaluator in the future (e.g., we could avoid looking into the Objective-C runtime during C++ expression evaluation, which we currently do unconditionally). Depends on llvm#87657
…port when evaluating C++ expressions (#87767) This patch attempts to decouple C++ expression evaluation from Objective-C support. We've previously enabled it by default (if a runtime existed), but that meant we're opting into extra work we only need to do for Objective-C, which complicates/slows down C++ expression evaluation. Of course there's a valid use-case for this, which is calling Objective-C APIs when stopped in C++ frames (which Objective-C++ developers might want to do). In those cases we should really prompt the user to add the `expr --language objc++` flag. To accomodate a likely frequent use-case where a user breaks in a system C++ library (without debug-symbols) but their application is actually an Objective-C app, we allow Objective-C support in C++ expressions if the current frame doesn't have debug-info. This fixes #75443 and allows us to add more `LangOpts.ObjC` guards around the expression evaluator in the future (e.g., we could avoid looking into the Objective-C runtime during C++ expression evaluation, which we currently do unconditionally). Depends on #87657
The
ClangExpressionParsertakes anExecutionContextScopewhich it uses to query theProcess/Target/StackFrameto set various compiler options in preparation for parsing an expression.However,
TryParseconstructs the parser with aProcessorTarget, never aStackFrame. So when the parser tries to retrieve the currentStackFramefrom theexe_scope, it doesn't succeed. In future patches we want to query theStackFramefrom within theClangExpressionParserconstructor.This patch simplifies
TryParse, by removing the redundantexe_scopeparameter, and instead uses theexe_ctxto derive the most fittingexe_scopeto pass intoClangExpressionParser.Not entirely sure how to test this. This patch is a prerequisite to get subsequent patches that set
LangOptsbased on the currentStackFrameto work.