Break dependency from Expression -> Commands

[zturner]

Summary

The REPL (which lives in Expression) was making use of the command options for the expression command. It's arguable whether REPL should even live in Expression to begin with, but it makes more sense for Command to depend on REPL than the other way around. The command library should be thought of as a UI library, low level engine stuff shouldn't depend on it, but the other way around makes perfect sense.

Anyway, only about 3-4 of the fields of this structure were even used and the rest were ignored, so we just give REPL its own custom structure and have the command object fill this out and pass it in. This way REPL doesn't need to reference one from a higher level library.

This breaks the cycle from Expression -> Commands -> Expression, reducing the distinct cycle count to 36.

Note that no CMakeLists.txt file needs to be updated here, because Expression was (incorrectly) not including Commands in its link list to begin with.

[davide]

Looks fine to me, but let's wait for Pavel. Are you back working on lldb ? :)

[jingham]

The REPL is just a mode of the expression command. You invoke it by saying:

(lldb) expr --repl --

or you can add any other options to it that you want, including flags like -i or -u.

So it seems odd to handle the passing of one subset of those options from the expression command to this expression mode differently from all the other options you pass to it (e.g. the variable object - i.e. result printing - options.)

[jingham]

Perhaps a better way to handle this is to think of REPL.cpp/h as adjuncts of CommandObjectExpression.cpp/h - they mostly define the fancy IOHandler that gets pushed when you run the command in this mode. I think it would be fine to move them to Command, at which point using all the OptionGroups would be expected.

[labath]

Both of the solutions sound plausible to me (extra struct vs. moving REPL to Command). Maybe that's because I don't know enough about the REPL to have formed an opinion on it.

lldb/include/lldb/Expression/REPL.h

31

If you stick with this solution, please make this Timeout<std::micro>

[jingham]

The expression command had two modes before introducing the REPL. You can do:

(lldb) expr -- some C expression

or you can do:

(lldb) expr
Enter expressions, then terminate with an empty line to evaluate:

1: first C expression
2: Second C Expression
3:

The second only differs from the first in that it pushes an IOHandler to deal with gathering the entered text that the user typed up to the point where they type an empty expression.

The repl adds a third mode:

(lldb) expr -r --

1> let a = 10

a: Int = 10

2> print(a)

10

3>

Which works the same as "expr --toplevel" with the addition of a fancier IOHandler that looks for complete expressions and evaluates each completed expression as it sees it. I'm glossing over a few details here (like the fact that ":" will switch you to the lldb command interpreter), but for the most part that's all it is.

Note, there's another "repl" that lldb provides:

> lldb --repl

Which packages up "make a target out of a little victim program we've stashed away in LLDB.framework, set a breakpoint in it, run to the breakpoint and then execute "expr -r --". So there's also a little bit to support that. But I don't think that materially changes how you should understand the REPL. And then most people actually access this for swift by invoking "swift" with no arguments, but the swift driver with no arguments just turns around and exec's "lldb --repl" so that doesn't add much besides convenience.

Jim

[zturner]

In D47232#1108865, @jingham wrote:

Perhaps a better way to handle this is to think of REPL.cpp/h as adjuncts of CommandObjectExpression.cpp/h - they mostly define the fancy IOHandler that gets pushed when you run the command in this mode. I think it would be fine to move them to Command, at which point using all the OptionGroups would be expected.

Core doesn't currently have a dependency on Commands (few things do actually), and I would prefer not to add one. But several things include REPL.h (notably Debugger.cpp and Target.cpp. So if I move this file to Commands it would introduce 2 extra edges in the dependency graph both of which would introduce a new layering violation.

[zturner]

FWIW REPL only uses 4 of the 10+ fields of CommandObjectExpression::Options. If you don't like the separate struct idea, I can just pass them directly as as independent arguments to REPL::SetCommandOptions.

[jingham]

I worry when concerns of layering which seem a little artificial to me would make us add a shadow class for something that is already well expressed as it is. If you pass them as arguments, and then I add another useful one and want to use it in both the regular and the REPL versions of the expression parser, it would be ugly to have to add another argument to this function when I should have been able to share the structure.

Seems like the thing you are actually objecting to is the use of CommandObjectExpression::CommandOptions in REPL.h. REPL.h also use OptionGroupValueObjectDisplay or OptionGroupFormat. They live in Interpreter, which this and other files in Expression have to use for other reasons as well as to get these option groups. Their usage doesn't change your graph. So another way to address your concerns would be to make an OptionGroupExpressionOptions in source/Interpreter and have CommandObjectExpression and REPL.h use that. There's no reason other than convenience that the option group for expression specific options live in the CommandObjectExpression class.

That would be fine, and then you could leave REPL.cpp where it is.

lldb/include/lldb/Expression/REPL.h

 #define lldb_REPL_h
 
 // C Includes
 // C++ Includes
 #include <string>
 
 // Other libraries and framework includes
 // Project includes
-#include "lldb/../../source/Commands/CommandObjectExpression.h"
+#include "lldb/Core/IOHandler.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 
 namespace lldb_private {
 
 class REPL : public IOHandlerDelegate {
 public:
+  struct CommandOptions {
+    bool UnwindOnError = false;
+    bool IgnoreBreakpoints = false;
+    bool TryAllThreads = true;
+    uint32_t Timeout = 0;
+  };
   //----------------------------------------------------------------------
   // See TypeSystem.h for how to add subclasses to this.
   //----------------------------------------------------------------------
   enum LLVMCastKind { eKindClang, eKindSwift, eKindGo, kNumKinds };
 
   LLVMCastKind getKind() const { return m_kind; }
 
   REPL(LLVMCastKind kind, Target &target);
void SetFormatOptions(const OptionGroupFormat &options) {
     m_format_options = options;
   }
 
   void
   SetValueObjectDisplayOptions(const OptionGroupValueObjectDisplay &options) {
     m_varobj_options = options;
   }
 
-  void
-  SetCommandOptions(const CommandObjectExpression::CommandOptions &options) {
+  void SetCommandOptions(const CommandOptions &options) {
     m_command_options = options;
   }
 
   void SetCompilerOptions(const char *options) {
     if (options)
       m_compiler_options = options;
   }
 
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;
+  CommandOptions m_command_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/source/Commands/CommandObjectExpression.cpp

if (end_options) {
               if (!repl_error.Success()) {
                 result.SetError(repl_error);
                 return result.Succeeded();
               }
             }
 
             if (repl_sp) {
               if (initialize) {
-                repl_sp->SetCommandOptions(m_command_options);
+                REPL::CommandOptions Options;
+                Options.IgnoreBreakpoints =
+                    m_command_options.ignore_breakpoints;
+                Options.Timeout = m_command_options.timeout;
+                Options.TryAllThreads = m_command_options.try_all_threads;
+                Options.UnwindOnError = m_command_options.unwind_on_error;
+                repl_sp->SetCommandOptions(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/source/Expression/REPL.cpp

 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] == ':') {
           exe_ctx.SetFrameSP(thread->GetSelectedFrame());
         }
       }
 
       const bool colorize_err = error_sp->GetFile().GetIsTerminalWithColors();
 
       EvaluateExpressionOptions 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.SetUnwindOnError(m_command_options.UnwindOnError);
+      expr_options.SetIgnoreBreakpoints(m_command_options.IgnoreBreakpoints);
       expr_options.SetKeepInMemory(true);
       expr_options.SetUseDynamic(m_varobj_options.use_dynamic);
-      expr_options.SetTryAllThreads(m_command_options.try_all_threads);
+      expr_options.SetTryAllThreads(m_command_options.TryAllThreads);
       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));
+      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());