[lldb] Add expression command options in dwim-print

[kastiglione]

Summary

Adopt expression's options in dwim-print.

This is primarily added to support the --language/-l flag.

[kastiglione]

Some expression flags force the use of expression evaluation.

[kastiglione]

Use expression's "description verbosity"

[aprantl]

Conceptually, I think this looks good to me.

lldb/source/Commands/CommandObjectDWIMPrint.cpp

77

Maybe comment here that these are the only options that only make sense in the expression evaluator?

lldb/test/API/commands/dwim-print/TestDWIMPrint.py

119

why is this needed?

[kastiglione]

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.

[kastiglione]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

77

As we discussed offline, instead of having flags that force expression evaluation, those flags are no longer exposed. If users need to force expression evaluation, they can use the expression command directly.

lldb/test/API/commands/dwim-print/TestDWIMPrint.py

119

the test this comment referred to has since been deleted, on account of no longer having flags that force expression evaluation.

[aprantl]

Yeah, this is better.

[jingham]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

118

There's a version of Append that lets you exclude and remap option sets. This seems like a finer-grained version of the same thing where you just want to exclude particular options by name but not mess with the option sets. That seems a generally useful thing to do, so it would be better to make another flavor of Append that takes an array of short character options and just excludes them, than do it as a one-off here.

lldb/source/Commands/CommandObjectExpression.h

38

Maybe this should instead take an OptionGroupValueObjectDisplay& instead? I think it's clearer to say "some of the EvaluateExpressionOptions are governed by how you want to display the result ValueObject you're going to produce" than "I'm passing a couple separate ValueObjectDisplay options under the table." Plus, that way if you end up needing another display option you won't have to keep adding parameters to this.

[kastiglione]

Add and adopt new variation of OptionGroupOptions::Append
Pass OptionGroupValueObjectDisplay to GetEvalateExpressionOptions

[kastiglione]

• Fix bug in new OptionGroupOptions::Append
• Fix typo in GetEvaluateExpressionOptions

[jingham]

LGTM

lldb/source/Commands/CommandObjectDWIMPrint.cpp

118

That would also make this code easier to read, since the exclusion would happen where you added it rather than way down below.

[kastiglione]

lldb/source/Commands/CommandObjectDWIMPrint.cpp

118

This approach is much better. I appreciate the helpful review!

lldb/include/lldb/Interpreter/Options.h

 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/CompletionRequest.h"
 #include "lldb/Utility/OptionDefinition.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
 
 namespace lldb_private {
 
 struct Option;
 
 typedef std::vector<std::tuple<std::string, int, std::string>> OptionArgVector;
 typedef std::shared_ptr<OptionArgVector> OptionArgVectorSP;
 
public:
   ///     you to specify the same options from \a group multiple times
   ///     for different option sets.
   ///
   /// \param[in] dst_mask
   ///     Set the usage mask for any copied options to \a dst_mask after
   ///     copying the option definition.
   void Append(OptionGroup *group, uint32_t src_mask, uint32_t dst_mask);
 
+  /// Append selected options from a OptionGroup class.
+  ///
+  /// Append the subset of options from \a group, where the "long_option" value
+  /// is _not_ in \a exclude_long_options.
+  ///
+  /// \param[in] group
+  ///     A group of options to take option values from and copy their
+  ///     definitions into this class.
+  ///
+  /// \param[in] exclude_long_options
+  ///     A set of long option strings which indicate which option values values
+  ///     to limit from \a group.
+  void Append(OptionGroup *group,
+              llvm::ArrayRef<llvm::StringRef> exclude_long_options);
+
   void Finalize();
 
   bool DidFinalize() { return m_did_finalize; }
 
   Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                         ExecutionContext *execution_context) override;
 
   void OptionParsingStarting(ExecutionContext *execution_context) override;

lldb/source/Commands/CommandObjectDWIMPrint.h

 //===-- CommandObjectDWIMPrint.h --------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "lldb/Interpreter/OptionValueFormat.h"
 
 namespace lldb_private {
 
 /// Implements `dwim-print`, a printing command that chooses the most direct,
public:
   Options *GetOptions() override;
 
 private:
   bool DoExecute(llvm::StringRef command, CommandReturnObject &result) override;
 
   OptionGroupOptions m_option_group;
   OptionGroupFormat m_format_options = lldb::eFormatDefault;
   OptionGroupValueObjectDisplay m_varobj_options;
+  CommandObjectExpression::CommandOptions m_expr_options;
 };
 
 } // namespace lldb_private
 
 #endif

lldb/source/Commands/CommandObjectDWIMPrint.cpp

CommandObjectDWIMPrint::CommandObjectDWIMPrint(CommandInterpreter &interpreter)
     : CommandObjectRaw(interpreter, "dwim-print",
                        "Print a variable or expression.",
                        "dwim-print [<variable-name> | <expression>]",
                        eCommandProcessMustBePaused | eCommandTryTargetAPILock) {
   m_option_group.Append(&m_format_options,
                         OptionGroupFormat::OPTION_GROUP_FORMAT |
                             OptionGroupFormat::OPTION_GROUP_GDB_FMT,
                         LLDB_OPT_SET_1);
+  StringRef exclude_expr_options[] = {"debug", "top-level"};
+  m_option_group.Append(&m_expr_options, exclude_expr_options);
   m_option_group.Append(&m_varobj_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_1);
   m_option_group.Finalize();
 }
 
 Options *CommandObjectDWIMPrint::GetOptions() { return &m_option_group; }
 
 bool CommandObjectDWIMPrint::DoExecute(StringRef command,
                                        CommandReturnObject &result) {
   m_option_group.NotifyOptionParsingStarting(&m_exe_ctx);
   OptionsWithRaw args{command};
   StringRef expr = args.GetRawPart();
 
   if (args.HasArgs()) {
     if (!ParseOptionsAndNotify(args.GetArgs(), result, m_option_group,
                                m_exe_ctx))
       return false;
   } else if (command.empty()) {
     result.AppendErrorWithFormatv("'{0}' takes a variable or expression",
                                   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()) {
     auto valobj_sp = frame->FindVariable(ConstString(expr));
     if (valobj_sp && valobj_sp->GetError().Success()) {
       if (verbosity == eDWIMPrintVerbosityFull) {
         StringRef flags;
         if (args.HasArgs())
           flags = args.GetArgString();
         result.AppendMessageWithFormatv("note: ran `frame variable {0}{1}`",
                                         flags, expr);
       }
       valobj_sp->Dump(result.GetOutputStream(), dump_options);
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
   }
 
   // Second, also lastly, try `expr` as a source expression to evaluate.
   {
     Target *target_ptr = m_exe_ctx.GetTargetPtr();
     // Fallback to the dummy target, which can allow for expression evaluation.
     Target &target = target_ptr ? *target_ptr : GetDummyTarget();
 
     auto *exe_scope = m_exe_ctx.GetBestExecutionContextScope();
+    const EvaluateExpressionOptions eval_options =
+        m_expr_options.GetEvaluateExpressionOptions(target, m_varobj_options);
     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())
           flags = args.GetArgStringWithDelimiter();
         result.AppendMessageWithFormatv("note: ran `expression {0}{1}`", flags,
                                         expr);
       }
       valobj_sp->Dump(result.GetOutputStream(), dump_options);
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     } else {
       if (valobj_sp)
         result.SetError(valobj_sp->GetError());
       else
         result.AppendErrorWithFormatv(
             "unknown error evaluating expression `{0}`", expr);
       return false;
     }
   }
 }

lldb/source/Commands/CommandObjectExpression.h

public:
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                           ExecutionContext *execution_context) override;
 
     void OptionParsingStarting(ExecutionContext *execution_context) override;
 
+    /// Return the appropriate expression options used for evaluating the
+    /// expression in the given target.
+    EvaluateExpressionOptions GetEvaluateExpressionOptions(
+        const Target &target,
+        const OptionGroupValueObjectDisplay &display_opts);
+
     bool top_level;
     bool unwind_on_error;
     bool ignore_breakpoints;
     bool allow_jit;
     bool show_types;
     bool show_summary;
     bool debug;
     uint32_t timeout;
protected:
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &line) override;
 
   bool IOHandlerIsInputComplete(IOHandler &io_handler,
                                 StringList &lines) override;
 
   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.
   /// \param error_stream Contains error messages that should be displayed to
   ///                     the user in case the evaluation fails.
   /// \param result A CommandReturnObject which status will be set to the
   ///               appropriate value depending on evaluation success and
   ///               whether the expression produced any result.

lldb/source/Commands/CommandObjectExpression.cpp

void CommandObjectExpression::CommandOptions::OptionParsingStarting(
   allow_jit = true;
 }
 
 llvm::ArrayRef<OptionDefinition>
 CommandObjectExpression::CommandOptions::GetDefinitions() {
   return llvm::ArrayRef(g_expression_options);
 }
 
+EvaluateExpressionOptions
+CommandObjectExpression::CommandOptions::GetEvaluateExpressionOptions(
+    const Target &target, const OptionGroupValueObjectDisplay &display_opts) {
+  EvaluateExpressionOptions options;
+  options.SetCoerceToId(display_opts.use_objc);
+  if (m_verbosity == eLanguageRuntimeDescriptionDisplayVerbosityCompact)
+    options.SetSuppressPersistentResult(display_opts.use_objc);
+  options.SetUnwindOnError(unwind_on_error);
+  options.SetIgnoreBreakpoints(ignore_breakpoints);
+  options.SetKeepInMemory(true);
+  options.SetUseDynamic(display_opts.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",
                        "Evaluate an expression on the current "
                        "thread.  Displays any returned value "
                        "with LLDB's default formatting.",
                        "",
                        eCommandProcessMustBePaused | eCommandTryTargetAPILock),
CanBeUsedForElementCountPrinting(ValueObject &valobj) {
   CompilerType pointee;
   if (!type.IsPointerType(&pointee))
     return Status("as it does not refer to a pointer");
   if (pointee.IsVoidType())
     return Status("as it refers to a pointer to void");
   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,
                                                  CommandReturnObject &result) {
   // Don't use m_exe_ctx as this might be called asynchronously after the
   // command object DoExecute has finished when doing multi-line expression
   // that use an input reader...
   ExecutionContext exe_ctx(m_interpreter.GetExecutionContext());
   Target *exe_target = exe_ctx.GetTargetPtr();
   Target &target = exe_target ? *exe_target : GetDummyTarget();
 
   lldb::ValueObjectSP result_valobj_sp;
   StackFrame *frame = exe_ctx.GetFramePtr();
 
   if (m_command_options.top_level && !m_command_options.allow_jit) {
     result.AppendErrorWithFormat(
         "Can't disable JIT compilation for top-level expressions.\n");
     return false;
   }
 
-  const EvaluateExpressionOptions options = GetEvalOptions(target);
+  const EvaluateExpressionOptions options =
+      m_command_options.GetEvaluateExpressionOptions(target, m_varobj_options);
   ExpressionResults success = target.EvaluateExpression(
       expr, frame, result_valobj_sp, options, &m_fixed_expression);
 
   // We only tell you about the FixIt if we applied it.  The compiler errors
   // will suggest the FixIt if it parsed.
   if (!m_fixed_expression.empty() && target.GetEnableNotifyAboutFixIts()) {
     error_stream.Printf("  Fix-it applied, fixed expression was: \n    %s\n",
                         m_fixed_expression.c_str());

lldb/source/Interpreter/Options.cpp

for (uint32_t i = 0; i < group_option_defs.size(); ++i) {
     if (group_option_defs[i].usage_mask & src_mask) {
       m_option_infos.push_back(OptionInfo(group, i));
       m_option_defs.push_back(group_option_defs[i]);
       m_option_defs.back().usage_mask = dst_mask;
     }
   }
 }
 
+void OptionGroupOptions::Append(
+    OptionGroup *group, llvm::ArrayRef<llvm::StringRef> exclude_long_options) {
+  auto group_option_defs = group->GetDefinitions();
+  for (uint32_t i = 0; i < group_option_defs.size(); ++i) {
+    const auto &definition = group_option_defs[i];
+    if (llvm::is_contained(exclude_long_options, definition.long_option))
+      continue;
+
+    m_option_infos.push_back(OptionInfo(group, i));
+    m_option_defs.push_back(definition);
+  }
+}
+
 void OptionGroupOptions::Finalize() {
   m_did_finalize = true;
 }
 
 Status OptionGroupOptions::SetOptionValue(uint32_t option_idx,
                                           llvm::StringRef option_value,
                                           ExecutionContext *execution_context) {
   // After calling OptionGroupOptions::Append(...), you must finalize the

lldb/test/API/commands/dwim-print/TestDWIMPrint.py

def test_format_flags(self):
         self._expect_cmd(f"dwim-print -fx -- argc", "frame variable")
         self._expect_cmd(f"dwim-print -fx -- argc + 1", "expression")
 
     def test_display_flags(self):
         self.build()
         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")