Break cycle lldb/Commands [3->] lldb/Expression [1->] lldb/Commands

[JDevlieghere]

Summary

Inspired by Zachary's mail on lldb-dev, this seemed like low hanging fruit. This patch breaks the circular dependency between commands and expression.

[zturner]

lldb/include/lldb/Commands/CommandObjectBreakpoint.h

36 ↗

(On Diff #189935)

I think the clang-format got messed up. It appears to be formatting your entire tree instead of only the files that were changed in this patch. Can you try to re-run clang-format only on the changed files?

[JDevlieghere]

lldb/include/lldb/Commands/CommandObjectBreakpoint.h

36 ↗

(On Diff #189935)

I did. I use git clang-format, which probably considers the whole file to be modified when you move it. It rings a bell but I don't remember the specifics.

The other files look fine though, so I can just reset this one.

[zturner]

I think maybe part of the problem is that this patch looks like actually 2 things. 1) A move of the include files from lldb/source/Commands to lldb/Include/lldb/Commands, and 2) The dependency changes. So it makes it hard to see what changes are actually needed for breaking the dependency.

Would it be possible to first move the header files as an independent change (which probably doesn't even need to be clang-formatted), and then after that fix the dependency issues?

lldb/source/Expression/REPL.cpp

10 ↗

(On Diff #189935)

AFAICT, this doesn't really appear to break the dependency does it? Because right here, Expression will still cause a link dependency against Commands.

[JDevlieghere]

lldb/source/Expression/REPL.cpp

10 ↗

(On Diff #189935)

Yeah you're totally right, I'm doing too many things at the same time.

[JDevlieghere]

format

[zturner]

Interesting, I had looked at fixing this one once before but I didn't realize we had a class named EvaluateExpressionOptions that contained just the right set of information, and it felt gross to invent a new one just for this purpose (It's a good thing I didn't too, since we already had another one).

Can you also update the CMake files? You'll need to update Expression/CMakeLists.txt to not pull in lldbTarget. I was going to say that you would also need to update unittests/Expression/CMakeLists.txt, but it's already not listed, so I guess it's just been relying on linking it transitively.

Do you have the ability to test the build with CMake? If so, could you try ninja ExpressionTests and make sure everything still works after removing the link dependcy in Expression/CMakeLists.txt?

[JDevlieghere]

In D59158#1423483, @zturner wrote:

Interesting, I had looked at fixing this one once before but I didn't realize we had a class named EvaluateExpressionOptions that contained just the right set of information, and it felt gross to invent a new one just for this purpose (It's a good thing I didn't too, since we already had another one).

Can you also update the CMake files? You'll need to update Expression/CMakeLists.txt to not pull in lldbTarget. I was going to say that you would also need to update unittests/Expression/CMakeLists.txt, but it's already not listed, so I guess it's just been relying on linking it transitively.

You mean lldbCommand, right? It's already not listed there, and neither in the unittest.

Do you have the ability to test the build with CMake? If so, could you try ninja ExpressionTests and make sure everything still works after removing the link dependcy in Expression/CMakeLists.txt?

[zturner]

Ahh yea, sorry. Got confused for a second :)

[jingham]

It's a little weird that the CommandObjectExpression is telling the REPL what its options should be, it would be more natural to go the other way. Maybe have the repl_sp provide a properly set up set of EvaluateExpressionOptions and then copy them over to this CommandObjectExpression execution?

Also, as is GetExprOptions is not a very descriptive name for something that's specifically setting up Command & EvaluateExpressionOptions for the REPL.

lldb/trunk/include/lldb/Expression/REPL.h

 //===-- REPL.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_REPL_h
 #define lldb_REPL_h
 
 #include <string>
 
-#include "lldb/../../source/Commands/CommandObjectExpression.h"
+#include "lldb/Core/IOHandler.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
+#include "lldb/Target/Target.h"
 
 namespace lldb_private {
 
 class REPL : public IOHandlerDelegate {
 public:
   //----------------------------------------------------------------------
   // See TypeSystem.h for how to add subclasses to this.
   //----------------------------------------------------------------------
void SetFormatOptions(const OptionGroupFormat &options) {
     m_format_options = options;
   }
 
   void
   SetValueObjectDisplayOptions(const OptionGroupValueObjectDisplay &options) {
     m_varobj_options = options;
   }
 
-  void
-  SetCommandOptions(const CommandObjectExpression::CommandOptions &options) {
-    m_command_options = options;
+  void SetEvaluateOptions(const EvaluateExpressionOptions &options) {
+    m_expr_options = options;
   }
 
   void SetCompilerOptions(const char *options) {
     if (options)
       m_compiler_options = options;
   }
 
   lldb::IOHandlerSP GetIOHandler();
virtual bool PrintOneVariable(Debugger &debugger,
                                 lldb::ValueObjectSP &valobj_sp,
                                 ExpressionVariable *var = nullptr) = 0;
 
   virtual int CompleteCode(const std::string &current_code,
                            StringList &matches) = 0;
 
   OptionGroupFormat m_format_options = OptionGroupFormat(lldb::eFormatDefault);
   OptionGroupValueObjectDisplay m_varobj_options;
-  CommandObjectExpression::CommandOptions m_command_options;
+  EvaluateExpressionOptions m_expr_options;
   std::string m_compiler_options;
 
   bool m_enable_auto_indent = true;
   std::string m_indent_str; // Use this string for each level of indentation
   std::string m_current_indent_str;
   uint32_t m_current_indent_level = 0;
 
   std::string m_repl_source_path;

lldb/trunk/source/Commands/CommandObjectExpression.cpp

void CommandObjectExpression::GetMultilineExpression() {
   if (output_sp) {
     output_sp->PutCString(
         "Enter expressions, then terminate with an empty line to evaluate:\n");
     output_sp->Flush();
   }
   debugger.PushIOHandler(io_handler_sp);
 }
 
+static EvaluateExpressionOptions
+GetExprOptions(ExecutionContext &ctx,
+               CommandObjectExpression::CommandOptions command_options) {
+  command_options.OptionParsingStarting(&ctx);
+
+  // Default certain settings for REPL regardless of the global settings.
+  command_options.unwind_on_error = false;
+  command_options.ignore_breakpoints = false;
+  command_options.debug = false;
+
+  EvaluateExpressionOptions expr_options;
+  expr_options.SetUnwindOnError(command_options.unwind_on_error);
+  expr_options.SetIgnoreBreakpoints(command_options.ignore_breakpoints);
+  expr_options.SetTryAllThreads(command_options.try_all_threads);
+
+  if (command_options.timeout > 0)
+    expr_options.SetTimeout(std::chrono::microseconds(command_options.timeout));
+  else
+    expr_options.SetTimeout(llvm::None);
+
+  return expr_options;
+}
+
 bool CommandObjectExpression::DoExecute(llvm::StringRef command,
                                         CommandReturnObject &result) {
   m_fixed_expression.clear();
   auto exe_ctx = GetCommandInterpreter().GetExecutionContext();
   m_option_group.NotifyOptionParsingStarting(&exe_ctx);
 
   if (command.empty()) {
     GetMultilineExpression();
if (m_repl_option.GetOptionValue().GetCurrentValue()) {
             if (!repl_error.Success()) {
               result.SetError(repl_error);
               return result.Succeeded();
             }
           }
 
           if (repl_sp) {
             if (initialize) {
-              repl_sp->SetCommandOptions(m_command_options);
+              repl_sp->SetEvaluateOptions(
+                  GetExprOptions(exe_ctx, m_command_options));
               repl_sp->SetFormatOptions(m_format_options);
               repl_sp->SetValueObjectDisplayOptions(m_varobj_options);
             }
 
             IOHandlerSP io_handler_sp(repl_sp->GetIOHandler());
 
             io_handler_sp->SetIsDone(false);
 

lldb/trunk/source/Expression/REPL.cpp

 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
-#include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/AnsiTerminal.h"
 
 #include <memory>
 
 using namespace lldb_private;
 
 REPL::REPL(LLVMCastKind kind, Target &target) : m_target(target), m_kind(kind) {
   // Make sure all option values have sane defaults
   Debugger &debugger = m_target.GetDebugger();
   auto exe_ctx = debugger.GetCommandInterpreter().GetExecutionContext();
   m_format_options.OptionParsingStarting(&exe_ctx);
   m_varobj_options.OptionParsingStarting(&exe_ctx);
-  m_command_options.OptionParsingStarting(&exe_ctx);
-
-  // Default certain settings for REPL regardless of the global settings.
-  m_command_options.unwind_on_error = false;
-  m_command_options.ignore_breakpoints = false;
-  m_command_options.debug = false;
 }
 
 REPL::~REPL() = default;
 
 lldb::REPLSP REPL::Create(Status &err, lldb::LanguageType language,
                           Debugger *debugger, Target *target,
                           const char *repl_options) {
   uint32_t idx = 0;
if (code[0] == ':') {
         if (thread && thread->UnwindInnermostExpression().Success()) {
           thread->SetSelectedFrameByIndex(0, false);
           exe_ctx.SetFrameSP(thread->GetSelectedFrame());
         }
       }
 
       const bool colorize_err = error_sp->GetFile().GetIsTerminalWithColors();
 
-      EvaluateExpressionOptions expr_options;
+      EvaluateExpressionOptions expr_options = m_expr_options;
       expr_options.SetCoerceToId(m_varobj_options.use_objc);
-      expr_options.SetUnwindOnError(m_command_options.unwind_on_error);
-      expr_options.SetIgnoreBreakpoints(m_command_options.ignore_breakpoints);
       expr_options.SetKeepInMemory(true);
       expr_options.SetUseDynamic(m_varobj_options.use_dynamic);
-      expr_options.SetTryAllThreads(m_command_options.try_all_threads);
       expr_options.SetGenerateDebugInfo(true);
       expr_options.SetREPLEnabled(true);
       expr_options.SetColorizeErrors(colorize_err);
       expr_options.SetPoundLine(m_repl_source_path.c_str(),
                                 m_code.GetSize() + 1);
-      if (m_command_options.timeout > 0)
-        expr_options.SetTimeout(std::chrono::microseconds(m_command_options.timeout));
-      else
-        expr_options.SetTimeout(llvm::None);
 
       expr_options.SetLanguage(GetLanguage());
 
       PersistentExpressionState *persistent_state =
           m_target.GetPersistentExpressionStateForLanguage(GetLanguage());
 
       const size_t var_count_before = persistent_state->GetSize();