Skip to content

[lldb][ClangUserExpression][NFCI] Pass the most specific ExecutionContextScope possible into ClangExpressionParser #87657

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 11, 2024
Merged

[lldb][ClangUserExpression][NFCI] Pass the most specific ExecutionContextScope possible into ClangExpressionParser #87657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 12 additions & 23 deletions lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Comment on lines -556 to +558
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it doesn't need to change. We can just always pass exe_ctx.GetBestExecutionContextScope() in at the call sites?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@Michael137 Michael137 Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m_materializer_up = std::make_unique<Materializer>();

ResetDeclMap(exe_ctx, m_result_delegate, keep_result_in_memory);
Expand All @@ -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);

Comment on lines -577 to 579
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

unsigned num_errors = m_parser->Parse(diagnostic_manager);

Expand Down Expand Up @@ -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);
Comment on lines +673 to +674
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayborg ping

// If the expression failed to parse, check if retrying parsing with a loaded
// C++ module is possible.
if (!parse_success && shouldRetryWithCppModule(*target, execution_policy)) {
Expand All @@ -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);
Comment on lines +692 to +693
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per initial comments above this would become:

parse_success = TryParse(retry_manager, exe_ctx.GetBestExecutionContextScope(), 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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines 187 to +190
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert this and always just pass in "exe_ctx.GetBestExecutionContextScope()"?


void SetupCppModuleImports(ExecutionContext &exe_ctx);

Expand Down