kastiglione updated this revision to Diff 498054.
kastiglione added a comment.

Use only a subset of expression flags.

Specifically, don't expose `--debug`/`-g` or `--top-level`/`-p`. These, unlike
the other `expression` flags, the semantics of these flags would seem to force
`expression` evaluation. In which case, user should use `expression` directly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144114/new/

https://reviews.llvm.org/D144114

Files:
  lldb/source/Commands/CommandObjectDWIMPrint.cpp
  lldb/source/Commands/CommandObjectDWIMPrint.h
  lldb/source/Commands/CommandObjectExpression.cpp
  lldb/source/Commands/CommandObjectExpression.h
  lldb/test/API/commands/dwim-print/TestDWIMPrint.py

Index: lldb/test/API/commands/dwim-print/TestDWIMPrint.py
===================================================================
--- lldb/test/API/commands/dwim-print/TestDWIMPrint.py
+++ lldb/test/API/commands/dwim-print/TestDWIMPrint.py
@@ -103,3 +103,10 @@
         lldbutil.run_to_name_breakpoint(self, "main")
         self._expect_cmd(f"dwim-print -T -- argc", "frame variable")
         self._expect_cmd(f"dwim-print -T -- argc + 1", "expression")
+
+    def test_expression_language(self):
+        """Test that the language flag doesn't affect the choice of command."""
+        self.build()
+        lldbutil.run_to_name_breakpoint(self, "main")
+        self._expect_cmd(f"dwim-print -l c++ -- argc", "frame variable")
+        self._expect_cmd(f"dwim-print -l c++ -- argc + 1", "expression")
Index: lldb/source/Commands/CommandObjectExpression.h
===================================================================
--- lldb/source/Commands/CommandObjectExpression.h
+++ lldb/source/Commands/CommandObjectExpression.h
@@ -35,6 +35,12 @@
 
     void OptionParsingStarting(ExecutionContext *execution_context) override;
 
+    /// Return the appropriate expression options used for evaluating the
+    /// expression in the given target.
+    EvaluateExpressionOptions
+    GetEvalateExpressionOptions(const Target &target, bool use_objc,
+                                lldb::DynamicValueType use_dynamic);
+
     bool top_level;
     bool unwind_on_error;
     bool ignore_breakpoints;
@@ -67,10 +73,6 @@
 
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
 
-  /// Return the appropriate expression options used for evaluating the
-  /// expression in the given target.
-  EvaluateExpressionOptions GetEvalOptions(const Target &target);
-
   /// Evaluates the given expression.
   /// \param output_stream The stream to which the evaluation result will be
   ///                      printed.
Index: lldb/source/Commands/CommandObjectExpression.cpp
===================================================================
--- lldb/source/Commands/CommandObjectExpression.cpp
+++ lldb/source/Commands/CommandObjectExpression.cpp
@@ -181,6 +181,48 @@
   return llvm::ArrayRef(g_expression_options);
 }
 
+EvaluateExpressionOptions
+CommandObjectExpression::CommandOptions::GetEvalateExpressionOptions(
+    const Target &target, bool use_objc, lldb::DynamicValueType use_dynamic) {
+  EvaluateExpressionOptions options;
+  options.SetCoerceToId(use_objc);
+  if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
+    options.SetSuppressPersistentResult(use_objc);
+  options.SetUnwindOnError(unwind_on_error);
+  options.SetIgnoreBreakpoints(ignore_breakpoints);
+  options.SetKeepInMemory(true);
+  options.SetUseDynamic(use_dynamic);
+  options.SetTryAllThreads(try_all_threads);
+  options.SetDebug(debug);
+  options.SetLanguage(language);
+  options.SetExecutionPolicy(
+      allow_jit ? EvaluateExpressionOptions::default_execution_policy
+                : lldb_private::eExecutionPolicyNever);
+
+  bool auto_apply_fixits;
+  if (this->auto_apply_fixits == eLazyBoolCalculate)
+    auto_apply_fixits = target.GetEnableAutoApplyFixIts();
+  else
+    auto_apply_fixits = this->auto_apply_fixits == eLazyBoolYes;
+
+  options.SetAutoApplyFixIts(auto_apply_fixits);
+  options.SetRetriesWithFixIts(target.GetNumberOfRetriesWithFixits());
+
+  if (top_level)
+    options.SetExecutionPolicy(eExecutionPolicyTopLevel);
+
+  // If there is any chance we are going to stop and want to see what went
+  // wrong with our expression, we should generate debug info
+  if (!ignore_breakpoints || !unwind_on_error)
+    options.SetGenerateDebugInfo(true);
+
+  if (timeout > 0)
+    options.SetTimeout(std::chrono::microseconds(timeout));
+  else
+    options.SetTimeout(std::nullopt);
+  return options;
+}
+
 CommandObjectExpression::CommandObjectExpression(
     CommandInterpreter &interpreter)
     : CommandObjectRaw(interpreter, "expression",
@@ -343,50 +385,6 @@
   return Status();
 }
 
-EvaluateExpressionOptions
-CommandObjectExpression::GetEvalOptions(const Target &target) {
-  EvaluateExpressionOptions options;
-  options.SetCoerceToId(m_varobj_options.use_objc);
-  if (m_command_options.m_verbosity ==
-      eLanguageRuntimeDescriptionDisplayVerbosityCompact)
-    options.SetSuppressPersistentResult(m_varobj_options.use_objc);
-  options.SetUnwindOnError(m_command_options.unwind_on_error);
-  options.SetIgnoreBreakpoints(m_command_options.ignore_breakpoints);
-  options.SetKeepInMemory(true);
-  options.SetUseDynamic(m_varobj_options.use_dynamic);
-  options.SetTryAllThreads(m_command_options.try_all_threads);
-  options.SetDebug(m_command_options.debug);
-  options.SetLanguage(m_command_options.language);
-  options.SetExecutionPolicy(
-      m_command_options.allow_jit
-          ? EvaluateExpressionOptions::default_execution_policy
-          : lldb_private::eExecutionPolicyNever);
-
-  bool auto_apply_fixits;
-  if (m_command_options.auto_apply_fixits == eLazyBoolCalculate)
-    auto_apply_fixits = target.GetEnableAutoApplyFixIts();
-  else
-    auto_apply_fixits = m_command_options.auto_apply_fixits == eLazyBoolYes;
-
-  options.SetAutoApplyFixIts(auto_apply_fixits);
-  options.SetRetriesWithFixIts(target.GetNumberOfRetriesWithFixits());
-
-  if (m_command_options.top_level)
-    options.SetExecutionPolicy(eExecutionPolicyTopLevel);
-
-  // If there is any chance we are going to stop and want to see what went
-  // wrong with our expression, we should generate debug info
-  if (!m_command_options.ignore_breakpoints ||
-      !m_command_options.unwind_on_error)
-    options.SetGenerateDebugInfo(true);
-
-  if (m_command_options.timeout > 0)
-    options.SetTimeout(std::chrono::microseconds(m_command_options.timeout));
-  else
-    options.SetTimeout(std::nullopt);
-  return options;
-}
-
 bool CommandObjectExpression::EvaluateExpression(llvm::StringRef expr,
                                                  Stream &output_stream,
                                                  Stream &error_stream,
@@ -407,7 +405,9 @@
     return false;
   }
 
-  const EvaluateExpressionOptions options = GetEvalOptions(target);
+  const EvaluateExpressionOptions options =
+      m_command_options.GetEvalateExpressionOptions(
+          target, m_varobj_options.use_objc, m_varobj_options.use_dynamic);
   ExpressionResults success = target.EvaluateExpression(
       expr, frame, result_valobj_sp, options, &m_fixed_expression);
 
Index: lldb/source/Commands/CommandObjectDWIMPrint.h
===================================================================
--- lldb/source/Commands/CommandObjectDWIMPrint.h
+++ lldb/source/Commands/CommandObjectDWIMPrint.h
@@ -9,6 +9,7 @@
 #ifndef LLDB_SOURCE_COMMANDS_COMMANDOBJECTDWIMPRINT_H
 #define LLDB_SOURCE_COMMANDS_COMMANDOBJECTDWIMPRINT_H
 
+#include "CommandObjectExpression.h"
 #include "lldb/Interpreter/CommandObject.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
@@ -37,11 +38,17 @@
   Options *GetOptions() override;
 
 private:
+  class CommandOptions : public CommandObjectExpression::CommandOptions {
+  public:
+    llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
+  };
+
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
 
   OptionGroupOptions m_option_group;
   OptionGroupFormat m_format_options = lldb::eFormatDefault;
   OptionGroupValueObjectDisplay m_varobj_options;
+  CommandOptions m_expr_options;
 };
 
 } // namespace lldb_private
Index: lldb/source/Commands/CommandObjectDWIMPrint.cpp
===================================================================
--- lldb/source/Commands/CommandObjectDWIMPrint.cpp
+++ lldb/source/Commands/CommandObjectDWIMPrint.cpp
@@ -22,6 +22,7 @@
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/Threading.h"
 
 using namespace llvm;
 using namespace lldb;
@@ -36,6 +37,7 @@
                         OptionGroupFormat::OPTION_GROUP_FORMAT |
                             OptionGroupFormat::OPTION_GROUP_GDB_FMT,
                         LLDB_OPT_SET_1);
+  m_option_group.Append(&m_expr_options);
   m_option_group.Append(&m_varobj_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
   m_option_group.Finalize();
 }
@@ -57,11 +59,11 @@
                                   m_cmd_name);
     return false;
   }
+
   auto verbosity = GetDebugger().GetDWIMPrintVerbosity();
 
   DumpValueObjectOptions dump_options = m_varobj_options.GetAsDumpOptions(
-      eLanguageRuntimeDescriptionDisplayVerbosityFull,
-      m_format_options.GetFormat());
+      m_expr_options.m_verbosity, m_format_options.GetFormat());
 
   // First, try `expr` as the name of a frame variable.
   if (StackFrame *frame = m_exe_ctx.GetFramePtr()) {
@@ -87,9 +89,13 @@
     Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
     auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
+    const EvaluateExpressionOptions eval_options =
+        m_expr_options.GetEvalateExpressionOptions(
+            target, m_varobj_options.use_objc, m_varobj_options.use_dynamic);
     ValueObjectSP valobj_sp;
-    if (target.EvaluateExpression(expr, exe_scope, valobj_sp) ==
-        eExpressionCompleted) {
+    ExpressionResults expr_result =
+        target.EvaluateExpression(expr, exe_scope, valobj_sp, eval_options);
+    if (expr_result == eExpressionCompleted) {
       if (verbosity != eDWIMPrintVerbosityNone) {
         StringRef flags;
         if (args.HasArgs())
@@ -110,3 +116,22 @@
     }
   }
 }
+
+llvm::ArrayRef<OptionDefinition>
+CommandObjectDWIMPrint::CommandOptions::GetDefinitions() {
+  static std::vector<OptionDefinition> g_dwim_print_definitions;
+
+  static llvm::once_flag once_flag;
+  llvm::call_once(once_flag, [this]() {
+    auto expression_definitions =
+        CommandObjectExpression::CommandOptions::GetDefinitions();
+    for (auto &&definition : expression_definitions) {
+      // Filter out --debug/-g and --top-level/-p
+      if (definition.short_option != 'g' && definition.short_option != 'p') {
+        g_dwim_print_definitions.push_back(definition);
+      }
+    }
+  });
+
+  return g_dwim_print_definitions;
+}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to