Skip to content

[lldb] Add interface to check if UserExpression::Parse() is cacheable #66826

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

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

augusto2112
Copy link
Contributor

When setting conditional breakpoints, we currently assume that a call to UserExpression::Parse() can be cached and resued multiple times. This may not be true for every user expression. Add a new method so subclasses of UserExpression can customize if they are parseable or not.

Copy link
Collaborator

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

Presumably this will do something for Swift expressions? I don't think there's any reason that the expression for a location in C/C++/ObjC code would not be cacheable, so the fact that this is a no-op in all of llvm.org lldb is expected.

@DavidSpickett
Copy link
Collaborator

This got forgotten it seems.

Could you add a comment in the header to address Jim's question?

So folks upstream know whether to care about overriding this for whatever new thing they might be building (there are lots of mysterious methods over in llvm and I end up ignoring them and hoping they're not important).

@augusto2112
Copy link
Contributor Author

@DavidSpickett thanks for reminding me. I'll add a comment describing the situation.

Right now, for conditional breakpoints, we assume that the condition expression is cacheable, since it's being run over and over again in the exact same context. This works for C/C++, but not for Swift, because Swift generics aren't monomorphized (the same problem would happen for other languages where generics aren't monomorphized either, so a more general escape hatch seemed appropriate to me).

To illustrate what I mean, a function such as the following (in C++ syntax but Swift semantics):

template <class T>
void use(T t) {}

// In main
use(5);
use(std::string())

Is lowered only once as a truly generic function (unlike C++ which will generate one version per instantiation), so it isn't safe to cache the expression parse, as the argument type may be different in every invocation.

@llvmbot
Copy link
Member

llvmbot commented Nov 16, 2023

@llvm/pr-subscribers-lldb

Author: Augusto Noronha (augusto2112)

Changes

When setting conditional breakpoints, we currently assume that a call to UserExpression::Parse() can be cached and resued multiple times. This may not be true for every user expression. Add a new method so subclasses of UserExpression can customize if they are parseable or not.


Full diff: https://github.com/llvm/llvm-project/pull/66826.diff

2 Files Affected:

  • (modified) lldb/include/lldb/Expression/UserExpression.h (+8)
  • (modified) lldb/source/Breakpoint/BreakpointLocation.cpp (+1)
diff --git a/lldb/include/lldb/Expression/UserExpression.h b/lldb/include/lldb/Expression/UserExpression.h
index df7a76664f6d5b6..b6cfeec7e899330 100644
--- a/lldb/include/lldb/Expression/UserExpression.h
+++ b/lldb/include/lldb/Expression/UserExpression.h
@@ -192,6 +192,14 @@ class UserExpression : public Expression {
   /// expression.  Text() should contain the definition of this function.
   const char *FunctionName() override { return "$__lldb_expr"; }
 
+  /// Returns whether the call to Parse on this user expression is cacheable.
+  /// This function exists to provide an escape hatch for supporting languages
+  /// where parsing an expression in the exact same context is unsafe. For
+  /// example, languages where generic functions aren't monomorphized, but
+  /// implement some other mechanism to represent generic values, may be unsafe
+  /// to cache, as the concrete type substitution may be different in every
+  /// expression evaluation.
+  virtual bool IsParseCacheable() { return true; }
   /// Return the language that should be used when parsing.  To use the
   /// default, return eLanguageTypeUnknown.
   lldb::LanguageType Language() const override { return m_language; }
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 0fcefe5c63be749..2d2e00883f32ef7 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -250,6 +250,7 @@ bool BreakpointLocation::ConditionSaysStop(ExecutionContext &exe_ctx,
   DiagnosticManager diagnostics;
 
   if (condition_hash != m_condition_hash || !m_user_expression_sp ||
+      !m_user_expression_sp->IsParseCacheable() ||
       !m_user_expression_sp->MatchesContext(exe_ctx)) {
     LanguageType language = eLanguageTypeUnknown;
     // See if we can figure out the language from the frame, otherwise use the

Copy link

github-actions bot commented Nov 16, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

When setting conditional breakpoints, we currently assume that a call to
UserExpression::Parse() can be cached and resued multiple times. This
may not be true for every user expression. Add a new method so
subclasses of UserExpression can customize if they are parseable or not.
@augusto2112 augusto2112 merged commit 4639610 into llvm:main Nov 16, 2023
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
…llvm#66826)

When setting conditional breakpoints, we currently assume that a call to
UserExpression::Parse() can be cached and resued multiple times. This
may not be true for every user expression. Add a new method so
subclasses of UserExpression can customize if they are parseable or not.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…llvm#66826)

When setting conditional breakpoints, we currently assume that a call to
UserExpression::Parse() can be cached and resued multiple times. This
may not be true for every user expression. Add a new method so
subclasses of UserExpression can customize if they are parseable or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants