[lldb][ExpressionParser][NFCI] Add new DoPrepareForExecution interface to be implemented by language plugins #96290

Michael137 · 2024-06-21T09:50:48Z

This patch adds a new DoPrepareForExecution API, which can be implemented by the Clang and Swift language plugins. This also moves RunStaticInitializers into ExpressionParser::PrepareForExecution, so we call it consistently between language plugins.

This should be mostly NFC (the static initializers will still only run after we finished parsing). We've been living on this patch downstream for sometime now.

rdar://130267058

llvmbot · 2024-06-21T09:51:24Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch adds a new DoPrepareForExecution API, which can be implemented by the Clang and Swift language plugins. This also moves RunStaticInitializers into ExpressionParser::PrepareForExecution, so we call it consistently between language plugins.

This should be mostly NFC (the static initializers will still only run after we finished parsing). We've been living on this patch downstream for sometime now.

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

6 Files Affected:

(modified) lldb/include/lldb/Expression/ExpressionParser.h (+23-2)
(modified) lldb/source/Expression/CMakeLists.txt (+1)
(added) lldb/source/Expression/ExpressionParser.cpp (+73)
(modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+1-45)
(modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h (+5-18)
(modified) lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp (-15)

diff --git a/lldb/include/lldb/Expression/ExpressionParser.h b/lldb/include/lldb/Expression/ExpressionParser.h
index ab5223c915530..2ef7e036909c7 100644
--- a/lldb/include/lldb/Expression/ExpressionParser.h
+++ b/lldb/include/lldb/Expression/ExpressionParser.h
@@ -119,14 +119,35 @@ class ExpressionParser {
   /// \return
   ///     An error code indicating the success or failure of the operation.
   ///     Test with Success().
-  virtual Status
+  Status
   PrepareForExecution(lldb::addr_t &func_addr, lldb::addr_t &func_end,
                       std::shared_ptr<IRExecutionUnit> &execution_unit_sp,
                       ExecutionContext &exe_ctx, bool &can_interpret,
-                      lldb_private::ExecutionPolicy execution_policy) = 0;
+                      lldb_private::ExecutionPolicy execution_policy);
 
   bool GetGenerateDebugInfo() const { return m_generate_debug_info; }
 
+protected:
+  virtual Status
+  DoPrepareForExecution(lldb::addr_t &func_addr, lldb::addr_t &func_end,
+                        std::shared_ptr<IRExecutionUnit> &execution_unit_sp,
+                        ExecutionContext &exe_ctx, bool &can_interpret,
+                        lldb_private::ExecutionPolicy execution_policy) = 0;
+
+private:
+  /// Run all static initializers for an execution unit.
+  ///
+  /// \param[in] execution_unit_sp
+  ///     The execution unit.
+  ///
+  /// \param[in] exe_ctx
+  ///     The execution context to use when running them.  Thread can't be null.
+  ///
+  /// \return
+  ///     The error code indicating the
+  Status RunStaticInitializers(lldb::IRExecutionUnitSP &execution_unit_sp,
+                               ExecutionContext &exe_ctx);
+
 protected:
   Expression &m_expr; ///< The expression to be parsed
   bool m_generate_debug_info;
diff --git a/lldb/source/Expression/CMakeLists.txt b/lldb/source/Expression/CMakeLists.txt
index 9ba5fefc09b6a..be1e132f7aaad 100644
--- a/lldb/source/Expression/CMakeLists.txt
+++ b/lldb/source/Expression/CMakeLists.txt
@@ -3,6 +3,7 @@ add_lldb_library(lldbExpression NO_PLUGIN_DEPENDENCIES
   DWARFExpression.cpp
   DWARFExpressionList.cpp
   Expression.cpp
+  ExpressionParser.cpp
   ExpressionTypeSystemHelper.cpp
   ExpressionVariable.cpp
   FunctionCaller.cpp
diff --git a/lldb/source/Expression/ExpressionParser.cpp b/lldb/source/Expression/ExpressionParser.cpp
new file mode 100644
index 0000000000000..ac727be78e8d3
--- /dev/null
+++ b/lldb/source/Expression/ExpressionParser.cpp
@@ -0,0 +1,73 @@
+//===-- ExpressionParser.cpp ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Expression/ExpressionParser.h"
+#include "lldb/Expression/DiagnosticManager.h"
+#include "lldb/Expression/IRExecutionUnit.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/Target/ThreadPlanCallFunction.h"
+
+using namespace lldb_private;
+
+Status ExpressionParser::PrepareForExecution(
+    lldb::addr_t &func_addr, lldb::addr_t &func_end,
+    std::shared_ptr<IRExecutionUnit> &execution_unit_sp,
+    ExecutionContext &exe_ctx, bool &can_interpret,
+    lldb_private::ExecutionPolicy execution_policy) {
+  Status status =
+      DoPrepareForExecution(func_addr, func_end, execution_unit_sp, exe_ctx,
+                            can_interpret, execution_policy);
+  if (status.Success() && exe_ctx.GetProcessPtr() && exe_ctx.HasThreadScope()) {
+    status = RunStaticInitializers(execution_unit_sp, exe_ctx);
+  }
+  return status;
+}
+
+Status ExpressionParser::RunStaticInitializers(
+    lldb::IRExecutionUnitSP &execution_unit_sp, ExecutionContext &exe_ctx) {
+  lldb_private::Status err;
+
+  lldbassert(execution_unit_sp.get());
+  lldbassert(exe_ctx.HasThreadScope());
+
+  if (!execution_unit_sp.get()) {
+    err.SetErrorString(
+        "can't run static initializers for a NULL execution unit");
+    return err;
+  }
+
+  if (!exe_ctx.HasThreadScope()) {
+    err.SetErrorString("can't run static initializers without a thread");
+    return err;
+  }
+
+  std::vector<lldb::addr_t> static_initializers;
+
+  execution_unit_sp->GetStaticInitializers(static_initializers);
+
+  for (lldb::addr_t static_initializer : static_initializers) {
+    EvaluateExpressionOptions options;
+
+    lldb::ThreadPlanSP call_static_initializer(new ThreadPlanCallFunction(
+        exe_ctx.GetThreadRef(), Address(static_initializer), CompilerType(),
+        llvm::ArrayRef<lldb::addr_t>(), options));
+
+    DiagnosticManager execution_errors;
+    lldb::ExpressionResults results =
+        exe_ctx.GetThreadRef().GetProcess()->RunThreadPlan(
+            exe_ctx, call_static_initializer, options, execution_errors);
+
+    if (results != lldb::eExpressionCompleted) {
+      err.SetErrorStringWithFormat("couldn't run static initializer: %s",
+                                   execution_errors.GetString().c_str());
+      return err;
+    }
+  }
+
+  return err;
+}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 1dd98567f8d69..303e88feea20b 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1296,7 +1296,7 @@ static bool FindFunctionInModule(ConstString &mangled_name,
   return false;
 }
 
-lldb_private::Status ClangExpressionParser::PrepareForExecution(
+lldb_private::Status ClangExpressionParser::DoPrepareForExecution(
     lldb::addr_t &func_addr, lldb::addr_t &func_end,
     lldb::IRExecutionUnitSP &execution_unit_sp, ExecutionContext &exe_ctx,
     bool &can_interpret, ExecutionPolicy execution_policy) {
@@ -1472,47 +1472,3 @@ lldb_private::Status ClangExpressionParser::PrepareForExecution(
 
   return err;
 }
-
-lldb_private::Status ClangExpressionParser::RunStaticInitializers(
-    lldb::IRExecutionUnitSP &execution_unit_sp, ExecutionContext &exe_ctx) {
-  lldb_private::Status err;
-
-  lldbassert(execution_unit_sp.get());
-  lldbassert(exe_ctx.HasThreadScope());
-
-  if (!execution_unit_sp.get()) {
-    err.SetErrorString(
-        "can't run static initializers for a NULL execution unit");
-    return err;
-  }
-
-  if (!exe_ctx.HasThreadScope()) {
-    err.SetErrorString("can't run static initializers without a thread");
-    return err;
-  }
-
-  std::vector<lldb::addr_t> static_initializers;
-
-  execution_unit_sp->GetStaticInitializers(static_initializers);
-
-  for (lldb::addr_t static_initializer : static_initializers) {
-    EvaluateExpressionOptions options;
-
-    lldb::ThreadPlanSP call_static_initializer(new ThreadPlanCallFunction(
-        exe_ctx.GetThreadRef(), Address(static_initializer), CompilerType(),
-        llvm::ArrayRef<lldb::addr_t>(), options));
-
-    DiagnosticManager execution_errors;
-    lldb::ExpressionResults results =
-        exe_ctx.GetThreadRef().GetProcess()->RunThreadPlan(
-            exe_ctx, call_static_initializer, options, execution_errors);
-
-    if (results != lldb::eExpressionCompleted) {
-      err.SetErrorStringWithFormat("couldn't run static initializer: %s",
-                                   execution_errors.GetString().c_str());
-      return err;
-    }
-  }
-
-  return err;
-}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
index 185a5a381f23c..0852e928a9d42 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
@@ -113,24 +113,11 @@ class ClangExpressionParser : public ExpressionParser {
   /// \return
   ///     An error code indicating the success or failure of the operation.
   ///     Test with Success().
-  Status
-  PrepareForExecution(lldb::addr_t &func_addr, lldb::addr_t &func_end,
-                      lldb::IRExecutionUnitSP &execution_unit_sp,
-                      ExecutionContext &exe_ctx, bool &can_interpret,
-                      lldb_private::ExecutionPolicy execution_policy) override;
-
-  /// Run all static initializers for an execution unit.
-  ///
-  /// \param[in] execution_unit_sp
-  ///     The execution unit.
-  ///
-  /// \param[in] exe_ctx
-  ///     The execution context to use when running them.  Thread can't be null.
-  ///
-  /// \return
-  ///     The error code indicating the
-  Status RunStaticInitializers(lldb::IRExecutionUnitSP &execution_unit_sp,
-                               ExecutionContext &exe_ctx);
+  Status DoPrepareForExecution(
+      lldb::addr_t &func_addr, lldb::addr_t &func_end,
+      lldb::IRExecutionUnitSP &execution_unit_sp, ExecutionContext &exe_ctx,
+      bool &can_interpret,
+      lldb_private::ExecutionPolicy execution_policy) override;
 
   /// Returns a string representing current ABI.
   ///
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index c7e98d12590d9..35038a56440d5 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -698,21 +698,6 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
   if (!parse_success)
     return false;
 
-  if (exe_ctx.GetProcessPtr() && execution_policy == eExecutionPolicyTopLevel) {
-    Status static_init_error =
-        m_parser->RunStaticInitializers(m_execution_unit_sp, exe_ctx);
-
-    if (!static_init_error.Success()) {
-      const char *error_cstr = static_init_error.AsCString();
-      if (error_cstr && error_cstr[0])
-        diagnostic_manager.Printf(lldb::eSeverityError, "%s\n", error_cstr);
-      else
-        diagnostic_manager.PutString(lldb::eSeverityError,
-                                     "couldn't run static initializers\n");
-      return false;
-    }
-  }
-
   if (m_execution_unit_sp) {
     bool register_execution_unit = false;

JDevlieghere · 2024-06-21T14:49:54Z

lldb/source/Expression/ExpressionParser.cpp

+  if (status.Success() && exe_ctx.GetProcessPtr() && exe_ctx.HasThreadScope()) {
+    status = RunStaticInitializers(execution_unit_sp, exe_ctx);
+  }


Nit: no braces.

JDevlieghere · 2024-06-21T14:51:46Z

lldb/source/Expression/ExpressionParser.cpp

+
+Status ExpressionParser::RunStaticInitializers(
+    lldb::IRExecutionUnitSP &execution_unit_sp, ExecutionContext &exe_ctx) {
+  lldb_private::Status err;


Nit: Drop lldb_private::

JDevlieghere · 2024-06-21T14:52:59Z

lldb/source/Expression/ExpressionParser.cpp

+  lldbassert(execution_unit_sp.get());
+  lldbassert(exe_ctx.HasThreadScope());
+
+  if (!execution_unit_sp.get()) {
+    err.SetErrorString(
+        "can't run static initializers for a NULL execution unit");
+    return err;
+  }
+
+  if (!exe_ctx.HasThreadScope()) {
+    err.SetErrorString("can't run static initializers without a thread");
+    return err;
+  }


If we're going to handle the error graciously, then I don't see the point of the lldbassert above. If we want to avoid this situation in the test suite we can make them regular asserts, or just omit them altogether.

JDevlieghere · 2024-06-21T14:53:30Z

lldb/source/Expression/ExpressionParser.cpp

+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/Target/ThreadPlanCallFunction.h"
+
+using namespace lldb_private;


If you add using namespace lldb you could drop some lldb:: prefixes.

asserts

…efactor-to-next [lldb][SwiftExpressionParser] Adapt SwiftExpressionParser to new DoPrepareForExecution API Adapts Swift plugin to the API refactoring in llvm#96290

…e to be implemented by language plugins (llvm#96290) This patch adds a new `DoPrepareForExecution` API, which can be implemented by the Clang and Swift language plugins. This also moves `RunStaticInitializers` into `ExpressionParser::PrepareForExecution`, so we call it consistently between language plugins. This *should* be mostly NFC (the static initializers will still only run after we finished parsing). We've been living on this patch downstream for sometime now. rdar://130267058 (cherry picked from commit bfd263a)

…e to be implemented by language plugins (llvm#96290) This patch adds a new `DoPrepareForExecution` API, which can be implemented by the Clang and Swift language plugins. This also moves `RunStaticInitializers` into `ExpressionParser::PrepareForExecution`, so we call it consistently between language plugins. This *should* be mostly NFC (the static initializers will still only run after we finished parsing). We've been living on this patch downstream for sometime now. rdar://130267058

[lldb][ExpressionParser] Add DoPrepareForExecution API

67d8bab

Michael137 requested review from adrian-prantl and jasonmolenda June 21, 2024 09:50

Michael137 requested a review from JDevlieghere as a code owner June 21, 2024 09:50

llvmbot added the lldb label Jun 21, 2024

Michael137 mentioned this pull request Jun 21, 2024

[lldb][SwiftExpressionParser] Adapt SwiftExpressionParser to new DoPrepareForExecution API swiftlang/llvm-project#8922

Merged

JDevlieghere reviewed Jun 21, 2024

View reviewed changes

adrian-prantl approved these changes Jun 21, 2024

View reviewed changes

Michael137 added 2 commits June 21, 2024 19:40

fixup! remove lldb_private/lldb namespace prefixes; remove redundant

73879ad

asserts

fixup! clang-format

387f2cd

Michael137 merged commit bfd263a into llvm:main Jun 22, 2024
6 checks passed

Michael137 deleted the lldb/prepare_for_execution-refactor branch June 22, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][ExpressionParser][NFCI] Add new DoPrepareForExecution interface to be implemented by language plugins #96290

[lldb][ExpressionParser][NFCI] Add new DoPrepareForExecution interface to be implemented by language plugins #96290

Uh oh!

Michael137 commented Jun 21, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jun 21, 2024

Uh oh!

JDevlieghere Jun 21, 2024

Uh oh!

JDevlieghere Jun 21, 2024

Uh oh!

JDevlieghere Jun 21, 2024

Uh oh!

JDevlieghere Jun 21, 2024

Uh oh!

Uh oh!

Uh oh!

[lldb][ExpressionParser][NFCI] Add new DoPrepareForExecution interface to be implemented by language plugins #96290

[lldb][ExpressionParser][NFCI] Add new DoPrepareForExecution interface to be implemented by language plugins #96290

Uh oh!

Conversation

Michael137 commented Jun 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 21, 2024

Uh oh!

JDevlieghere Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Jun 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Michael137 commented Jun 21, 2024 •

edited

Loading