[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315037: Implement interactive command interruption (authored 
by lemo).

Changed prior to commit:
  https://reviews.llvm.org/D37923?vs=117900=117936#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37923

Files:
  lldb/trunk/include/lldb/API/SBCommandInterpreter.h
  lldb/trunk/include/lldb/Core/IOHandler.h
  lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
  lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
  lldb/trunk/scripts/interface/SBCommandInterpreter.i
  lldb/trunk/source/API/SBCommandInterpreter.cpp
  lldb/trunk/source/Commands/CommandObjectTarget.cpp
  lldb/trunk/source/Core/Debugger.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp

Index: lldb/trunk/include/lldb/Core/IOHandler.h
===
--- lldb/trunk/include/lldb/Core/IOHandler.h
+++ lldb/trunk/include/lldb/Core/IOHandler.h
@@ -195,7 +195,7 @@
   enum class Completion { None, LLDBCommand, Expression };
 
   IOHandlerDelegate(Completion completion = Completion::None)
-  : m_completion(completion), m_io_handler_done(false) {}
+  : m_completion(completion) {}
 
   virtual ~IOHandlerDelegate() = default;
 
@@ -296,7 +296,6 @@
 
 protected:
   Completion m_completion; // Support for common builtin completions
-  bool m_io_handler_done;
 };
 
 //--
Index: lldb/trunk/include/lldb/API/SBCommandInterpreter.h
===
--- lldb/trunk/include/lldb/API/SBCommandInterpreter.h
+++ lldb/trunk/include/lldb/API/SBCommandInterpreter.h
@@ -165,6 +165,8 @@
int match_start_point, int max_return_elements,
lldb::SBStringList );
 
+  bool WasInterrupted() const;
+
   // Catch commands before they execute by registering a callback that will
   // get called when the command gets executed. This allows GUI or command
   // line interfaces to intercept a command and stop it from happening
Index: lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
===
--- lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
+++ lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
@@ -242,6 +242,8 @@
  bool repeat_on_empty_command = true,
  bool no_context_switching = false);
 
+  bool WasInterrupted() const;
+
   //--
   /// Execute a list of commands in sequence.
   ///
@@ -522,6 +524,25 @@
   StringList _help,
   CommandObject::CommandMap _map);
 
+  // An interruptible wrapper around the stream output
+  void PrintCommandOutput(Stream , llvm::StringRef str);
+
+  // A very simple state machine which models the command handling transitions
+  enum class CommandHandlingState {
+eIdle,
+eInProgress,
+eInterrupted,
+  };
+
+  std::atomic m_command_state{
+  CommandHandlingState::eIdle};
+
+  int m_iohandler_nesting_level = 0;
+
+  void StartHandlingCommand();
+  void FinishHandlingCommand();
+  bool InterruptCommand();
+
   Debugger _debugger; // The debugger session that this interpreter is
 // associated with
   ExecutionContextRef m_exe_ctx_ref; // The current execution context to use
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/.lldb
@@ -1 +1,2 @@
-script import my
+# one more level of indirection to stress the command interpreter reentrancy
+command source commands.txt
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
@@ -0,0 +1,2 @@
+script import my
+p 1 + 1
Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -546,7 +546,7 @@
   char buffer[1024];
   int num_printed =
   snprintf(buffer, 1024, "%s %s", break_regexes[i][1], "-o");
-  assert(num_printed < 1024);
+  lldbassert(num_printed < 1024);
   UNUSED_IF_ASSERT_DISABLED(num_printed);
   success =
   

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

LGTM


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

That looks fine.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 117900.
lemo edited the summary of this revision.
lemo added a comment.

Updating the original changes to handle reentrant calls to 
CommandInterpreter::IOHandlerInputComplete().

- This fixes bug #34758
- Also enhanced the existing test case for "command source" to cover this 
regression
- I considered changing SBCommandReturnObject::PutOutput() to handle output 
interruptions too, but CommandReturnObjects are decoupled from 
CommandInterpreter instances, so there's no easy way to query for interruption 
in the current scheme.


https://reviews.llvm.org/D37923

Files:
  include/lldb/API/SBCommandInterpreter.h
  include/lldb/Core/IOHandler.h
  include/lldb/Interpreter/CommandInterpreter.h
  packages/Python/lldbsuite/test/functionalities/command_source/.lldb
  packages/Python/lldbsuite/test/functionalities/command_source/commands.txt
  scripts/interface/SBCommandInterpreter.i
  source/API/SBCommandInterpreter.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/Debugger.cpp
  source/Interpreter/CommandInterpreter.cpp

Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -546,7 +546,7 @@
   char buffer[1024];
   int num_printed =
   snprintf(buffer, 1024, "%s %s", break_regexes[i][1], "-o");
-  assert(num_printed < 1024);
+  lldbassert(num_printed < 1024);
   UNUSED_IF_ASSERT_DISABLED(num_printed);
   success =
   tbreak_regex_cmd_ap->AddRegexCommand(break_regexes[i][0], buffer);
@@ -891,8 +891,8 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   if (name.empty())
 return false;
@@ -913,8 +913,8 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   if (!name.empty()) {
 // do not allow replacement of internal commands
@@ -1062,8 +1062,8 @@
  lldb::CommandObjectSP _obj_sp,
  llvm::StringRef args_string) {
   if (command_obj_sp.get())
-assert((this == _obj_sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _obj_sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   std::unique_ptr command_alias_up(
   new CommandAlias(*this, command_obj_sp, args_string, alias_name));
@@ -1541,6 +1541,12 @@
   if (!no_context_switching)
 UpdateExecutionContext(override_context);
 
+  if (WasInterrupted()) {
+result.AppendError("interrupted");
+result.SetStatus(eReturnStatusFailed);
+return false;
+  }
+
   bool add_to_history;
   if (lazy_add_to_history == eLazyBoolCalculate)
 add_to_history = (m_command_source_depth == 0);
@@ -1839,7 +1845,7 @@
   matches.Clear();
 
   // Only max_return_elements == -1 is supported at present:
-  assert(max_return_elements == -1);
+  lldbassert(max_return_elements == -1);
   bool word_complete;
   num_command_matches = HandleCompletionMatches(
   parsed_line, cursor_index, cursor_char_position, match_start_point,
@@ -2210,7 +2216,7 @@
 m_debugger.SetAsyncExecution(false);
   }
 
-  for (size_t idx = 0; idx < num_lines; idx++) {
+  for (size_t idx = 0; idx < num_lines && !WasInterrupted(); idx++) {
 const char *cmd = commands.GetStringAtIndex(idx);
 if (cmd[0] == '\0')
   continue;
@@ -2677,8 +2683,67 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto idle_state = CommandHandlingState::eIdle;
+  if (m_command_state.compare_exchange_strong(
+  idle_state, CommandHandlingState::eInProgress))
+lldbassert(m_iohandler_nesting_level == 0);
+  else
+lldbassert(m_iohandler_nesting_level > 0);
+  ++m_iohandler_nesting_level;
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  lldbassert(m_iohandler_nesting_level > 0);
+  if (--m_iohandler_nesting_level == 0) {
+auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+lldbassert(prev_state != CommandHandlingState::eIdle);
+  }
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return 

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Adrian McCarthy via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL313904: [LLDB] Implement interactive command interruption 
(authored by amccarth).

Changed prior to commit:
  https://reviews.llvm.org/D37923?vs=116074=116247#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37923

Files:
  lldb/trunk/include/lldb/API/SBCommandInterpreter.h
  lldb/trunk/include/lldb/Core/IOHandler.h
  lldb/trunk/include/lldb/Interpreter/CommandInterpreter.h
  lldb/trunk/scripts/interface/SBCommandInterpreter.i
  lldb/trunk/source/API/SBCommandInterpreter.cpp
  lldb/trunk/source/Commands/CommandObjectTarget.cpp
  lldb/trunk/source/Core/Debugger.cpp
  lldb/trunk/source/Interpreter/CommandInterpreter.cpp

Index: lldb/trunk/source/Interpreter/CommandInterpreter.cpp
===
--- lldb/trunk/source/Interpreter/CommandInterpreter.cpp
+++ lldb/trunk/source/Interpreter/CommandInterpreter.cpp
@@ -546,7 +546,7 @@
   char buffer[1024];
   int num_printed =
   snprintf(buffer, 1024, "%s %s", break_regexes[i][1], "-o");
-  assert(num_printed < 1024);
+  lldbassert(num_printed < 1024);
   UNUSED_IF_ASSERT_DISABLED(num_printed);
   success =
   tbreak_regex_cmd_ap->AddRegexCommand(break_regexes[i][0], buffer);
@@ -891,8 +891,8 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   if (name.empty())
 return false;
@@ -913,8 +913,8 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   if (!name.empty()) {
 // do not allow replacement of internal commands
@@ -1062,8 +1062,8 @@
  lldb::CommandObjectSP _obj_sp,
  llvm::StringRef args_string) {
   if (command_obj_sp.get())
-assert((this == _obj_sp->GetCommandInterpreter()) &&
-   "tried to add a CommandObject from a different interpreter");
+lldbassert((this == _obj_sp->GetCommandInterpreter()) &&
+   "tried to add a CommandObject from a different interpreter");
 
   std::unique_ptr command_alias_up(
   new CommandAlias(*this, command_obj_sp, args_string, alias_name));
@@ -1839,7 +1839,7 @@
   matches.Clear();
 
   // Only max_return_elements == -1 is supported at present:
-  assert(max_return_elements == -1);
+  lldbassert(max_return_elements == -1);
   bool word_complete;
   num_command_matches = HandleCompletionMatches(
   parsed_line, cursor_index, cursor_char_position, match_start_point,
@@ -2677,6 +2677,57 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  lldbassert(prev_state == CommandHandlingState::eIdle);
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  lldbassert(prev_state != CommandHandlingState::eIdle);
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return m_command_state.compare_exchange_strong(
+  in_progress, CommandHandlingState::eInterrupted);
+}
+
+bool CommandInterpreter::WasInterrupted() const {
+  return m_command_state == CommandHandlingState::eInterrupted;
+}
+
+void CommandInterpreter::PrintCommandOutput(Stream , llvm::StringRef str,
+bool interruptible) {
+  if (str.empty())
+return;
+
+  if (interruptible) {
+// Split the output into lines and poll for interrupt requests
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
+  size_t chunk_size = 0;
+  for (; chunk_size < size; ++chunk_size) {
+lldbassert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {
+  ++chunk_size;
+  break;
+}
+  }
+  chunk_size = stream.Write(data, chunk_size);
+  lldbassert(size >= chunk_size);
+  data += chunk_size;
+  size -= chunk_size;
+}
+if (size > 0) {
+  stream.Printf("\n... Interrupted.\n");
+}
+  } else {
+stream.PutCString(str);
+  }
+}
+
 void CommandInterpreter::IOHandlerInputComplete(IOHandler _handler,
 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

> On Sep 20, 2017, at 4:16 PM, Leonard Mosescu  wrote:
> 
> I don't quite understand the comment about signals adding indeterminacy. No 
> signal delivery is required to test this part.  The lldb driver has a sigint 
> handler that calls SBDebugger::DispatchInputInterrupt.  But since you aren't 
> testing whether SIGINT actually calls DispatchInputInterrupt, you can just 
> call it directly.  
> 
> Agreed.
> 
> I was suggesting executing a command (with 
> SBCommandInterpreter::ExecuteCommand) for a Python command that cycles 
> calling WasInterrupted.  Then you would make another thread in Python and 
> have that thread wait a bit then call DispatchInputInterrupt,  If "wait a 
> bit" bothers you then you could have the python based command you are 
> interrupting signal the interrupting thread before going into its loop.  I 
> don't see how this would result in indeterminacy,  And it would be testing 
> exactly what you want to  test: does calling DispatchInputInterrupt cause a 
> command in flight to be interrupted.
> 
> Once you have a second thread you end up with the non-determinism I hinted to 
> (this is true regardless if you hardcode a wait, use an event or keep 
> interrupting at a fixed rate). Now this is not a deal breaker in itself, 
> after all if you go after testing async behavior it's part of the deal.
> 
> But in this case it gets a bit more complicated as far as I can tell: first, 
> DispatchInputInterrupt() is only passed on the top IO Handler, if any. So 
> DispatchInputInterrupt() through SBDebugger is not exactly the same as a real 
> input interrupt.
> 
> Second, if we want to allow the interruption of commands that are executed 
> through SBDebugger::HandleCommand() the command state machine is not a simple 
> idle -> executing (->interrupted) -> idle since you get reentrancy (the 
> command can invoke other commands, etc...). Note that in the current version, 
> the states are only tracking in CommandInterpreter::IOHandlerInputComplete() 
> which should not lead to reentrancy (I did manual testing for this - if 
> anything I'd love a way to automate testing _this_ part btw)
> 
> I got pretty far in dancing around all this, but it become clear that I was 
> not really testing the real path and I was just introducing more artificial 
> complexity. If I'm missing anything I'd be happy to be revisit this and to be 
> proven wrong.

It been a couple of years since I dug into this code - Greg’s mostly been 
maintaining it.  So I’m entirely willing to believe it’s changed in a way that 
makes my memory of how it works unhelpful, but I’ll have to do some reading up 
to see.  I’ll do that when I get a chance.

Jim

> 
> On Wed, Sep 20, 2017 at 3:51 PM, Jim Ingham via Phabricator 
> > wrote:
> jingham accepted this revision.
> jingham added a comment.
> 
> I don't quite understand the comment about signals adding indeterminacy. No 
> signal delivery is required to test this part.  The lldb driver has a sigint 
> handler that calls SBDebugger::DispatchInputInterrupt.  But since you aren't 
> testing whether SIGINT actually calls DispatchInputInterrupt, you can just 
> call it directly.  I was suggesting executing a command (with 
> SBCommandInterpreter::ExecuteCommand) for a Python command that cycles 
> calling WasInterrupted.  Then you would make another thread in Python and 
> have that thread wait a bit then call DispatchInputInterrupt,  If "wait a 
> bit" bothers you then you could have the python based command you are 
> interrupting signal the interrupting thread before going into its loop.  I 
> don't see how this would result in indeterminacy,  And it would be testing 
> exactly what you want to  test: does calling DispatchInputInterrupt cause a 
> command in flight to be interrupted.
> 
> But this change is fine.  Check it in and I'll add a test when I get a chance.
> 
> 
> https://reviews.llvm.org/D37923 
> 
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Yes, using line endings as split points is somewhat arbitrary, anything
that's a reasonable compromise between interruption responsiveness and low
interrupt polling overhead would do. I feel that the lines are somewhat
nicer since arbitrary splitting may lead to confusion and/or formatting
ugliness.

On Wed, Sep 20, 2017 at 4:24 PM, Adrian McCarthy via Phabricator <
revi...@reviews.llvm.org> wrote:

> amccarth accepted this revision.
> amccarth added a comment.
>
> LGTM.
>
> But just a thought:  Is it worth doing all the work to scan for line
> endings for the interruption points?  What if, instead, it just printed the
> next _n_ characters on each iteration until the entire buffer has been
> printed.  Sure, sometimes an interruption will split a line, and sometimes
> it won't.  I'm not sure that's important for interactive usage.  It would
> mean less fiddly code, faster output (because you don't have to scan every
> character), and a zillion short lines will print as fast as a smaller
> number of longer lines that represents the same volume of text.
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.

LGTM.

But just a thought:  Is it worth doing all the work to scan for line endings 
for the interruption points?  What if, instead, it just printed the next _n_ 
characters on each iteration until the entire buffer has been printed.  Sure, 
sometimes an interruption will split a line, and sometimes it won't.  I'm not 
sure that's important for interactive usage.  It would mean less fiddly code, 
faster output (because you don't have to scan every character), and a zillion 
short lines will print as fast as a smaller number of longer lines that 
represents the same volume of text.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

I don't quite understand the comment about signals adding indeterminacy. No 
signal delivery is required to test this part.  The lldb driver has a sigint 
handler that calls SBDebugger::DispatchInputInterrupt.  But since you aren't 
testing whether SIGINT actually calls DispatchInputInterrupt, you can just call 
it directly.  I was suggesting executing a command (with 
SBCommandInterpreter::ExecuteCommand) for a Python command that cycles calling 
WasInterrupted.  Then you would make another thread in Python and have that 
thread wait a bit then call DispatchInputInterrupt,  If "wait a bit" bothers 
you then you could have the python based command you are interrupting signal 
the interrupting thread before going into its loop.  I don't see how this would 
result in indeterminacy,  And it would be testing exactly what you want to  
test: does calling DispatchInputInterrupt cause a command in flight to be 
interrupted.

But this change is fine.  Check it in and I'll add a test when I get a chance.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via Phabricator via lldb-commits]

zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

seems fine to me, I agree with the point about non-determinism, due to the 
nature of signals and the fact this is only intended to provide a best-effort 
interruption.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 116074.
lemo added a comment.

1. Added SB APIs (SBCommandInterpreter::WasInterrupted()). This allows python 
commands to query for interrupt requests.

2. I talked offline with Zach and decided that the line_iterator would require 
more tinkering to get it to work and it's not worth blocking the change for it.

3. I looked into adding a test for the SB APIs, but it would not really test 
much of the real path - which targets the _interactive_ interruption. Also, 
while I appreciate the ideal of testing everything I'm afraid that the net 
value of a test here might be negative (testing for async interruption would 
introduce some level of non determinism in the test suite and would require 
extra logic that's only used for testing). If anyone has any specific guidance 
on how to create a good test please let me know!




https://reviews.llvm.org/D37923

Files:
  include/lldb/API/SBCommandInterpreter.h
  include/lldb/Core/IOHandler.h
  include/lldb/Interpreter/CommandInterpreter.h
  scripts/interface/SBCommandInterpreter.i
  source/API/SBCommandInterpreter.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/Debugger.cpp
  source/Interpreter/CommandInterpreter.cpp

Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -546,7 +546,7 @@
   char buffer[1024];
   int num_printed =
   snprintf(buffer, 1024, "%s %s", break_regexes[i][1], "-o");
-  assert(num_printed < 1024);
+  lldbassert(num_printed < 1024);
   UNUSED_IF_ASSERT_DISABLED(num_printed);
   success =
   tbreak_regex_cmd_ap->AddRegexCommand(break_regexes[i][0], buffer);
@@ -891,7 +891,7 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
+lldbassert((this == _sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   if (name.empty())
@@ -913,7 +913,7 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
+lldbassert((this == _sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   if (!name.empty()) {
@@ -1062,7 +1062,7 @@
  lldb::CommandObjectSP _obj_sp,
  llvm::StringRef args_string) {
   if (command_obj_sp.get())
-assert((this == _obj_sp->GetCommandInterpreter()) &&
+lldbassert((this == _obj_sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   std::unique_ptr command_alias_up(
@@ -1839,7 +1839,7 @@
   matches.Clear();
 
   // Only max_return_elements == -1 is supported at present:
-  assert(max_return_elements == -1);
+  lldbassert(max_return_elements == -1);
   bool word_complete;
   num_command_matches = HandleCompletionMatches(
   parsed_line, cursor_index, cursor_char_position, match_start_point,
@@ -2677,6 +2677,57 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  lldbassert(prev_state == CommandHandlingState::eIdle);
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  lldbassert(prev_state != CommandHandlingState::eIdle);
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return m_command_state.compare_exchange_strong(
+  in_progress, CommandHandlingState::eInterrupted);
+}
+
+bool CommandInterpreter::WasInterrupted() const {
+  return m_command_state == CommandHandlingState::eInterrupted;
+}
+
+void CommandInterpreter::PrintCommandOutput(Stream , llvm::StringRef str,
+bool interruptible) {
+  if (str.empty())
+return;
+
+  if (interruptible) {
+// Split the output into lines and poll for interrupt requests
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
+  size_t chunk_size = 0;
+  for (; chunk_size < size; ++chunk_size) {
+lldbassert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {
+  ++chunk_size;
+  break;
+}
+  }
+  chunk_size = stream.Write(data, chunk_size);
+  lldbassert(size >= chunk_size);
+  data += chunk_size;
+  size -= chunk_size;
+}
+if (size > 0) {
+  stream.Printf("\n... Interrupted.\n");
+}
+  } else {
+stream.PutCString(str);
+  }
+}
+
 void 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

> On Sep 20, 2017, at 11:25 AM, Zachary Turner  wrote:
> 
> 
> 
> On Wed, Sep 20, 2017 at 11:14 AM Jim Ingham  > wrote:
> 
> The amount of test coverage lldb has at present has much more to do with the 
> very aggressive schedules lldb has been driven by since its inception than 
> any difficulties with writing tests IMHBSEO.  
> 
> This probably applies to the folks at Apple, but I think there's a large 
> disconnect between what you guys experience and what the rest of the LLVM 
> community (and external contributors) experience. 
> 
> I think this is the opposite of true for everyone else.  I've heard it time 
> and time again from long-time LLVM developers as well as would-be 
> contributors.


W.r.t. external contributors writing tests, I think the barrier is one of 
education not difficulty.  Yeah, okay you gotta learn a little Python but I 
don’t think e is a significant barrier to serious developers, and the 
alternative would be to figure out how to do the same thing with the 
lldb_private API’s - which is always going to be harder than the SB API’s - or 
with some DSL we invent which is unlikely to be that much more straightforward, 
and which unlike learning the SB API’s will benefit you in the rest of your 
life as a developer not at all.

When I explain how to write tests to folks I get more of enthusiasm for an 
opportunity to get familiar with a practical programmatic interface to the 
debugger than complaint about the difficulty of writing the test. Admittedly, 
that’s in part because that’s how I pitch it.  But in the same vein, there 
might be some bias in your impressions if I’m right in imagining that someone 
coming to ask you how to write a Python test gets more of an earful about how 
this testing methodology sucks than helpful advice on how to write one!  

Anyway, up till about 6 months ago there was no good template for writing new 
tests.  That was because most of us working on lldb already knew how to write 
tests.  I fixed that by adding the sample test templates a little while ago.  
If you can think of a way to make these easier to use, please do so.  But in my 
experience folks who haven’t written tests have been able to grab one or the 
other of them and get a test done pretty easily.  The other barrier is learning 
how to read a test failure.  That isn’t hard but the test output is noisy and 
its easy to overlook the part of the output that tells you directly where to 
look for the failure.  I have it on my list of tasks to write up a description 
of this process but I have many things on my list of tasks.  If somebody else 
wants to add their experiences to the testing README, please do!

Jim

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

I write lots of tests.  I don’t really think that the SB API’s are really a 
barrier to writing tests.  The lldbinline tests are trivial to write and can 
even be made just as a transcription of an lldb command line session, so the 
barrier to entry is trivial.  That is surely the easiest way to write tests for 
behavior that is available at the Command line or SB levels.  The SB tests are 
not much harder to write.  The fact that we didn’t have easy templates for 
writing tests for a while made figuring out how to get started hard.  But the 
sample_tests make it easy to get yourself to a debugger with a loaded target 
that you can poke at.  I just can’t see how a developer who can write good C++ 
code could have any difficulty write an SB API test. 

Moreover, for anything that requires complex setup, the python tests are much 
easier to write than lldb_private API tests because the SB API’s are 
specifically meant to be a streamlined way to drive the debugger.  So for 
somebody coming in to make a surgical fix who is not an LLDB expert, making 
them learn the lldb_private API’s for driving a debug session is even more work 
that the SB API’s.

The amount of test coverage lldb has at present has much more to do with the 
very aggressive schedules lldb has been driven by since its inception than any 
difficulties with writing tests IMHBSEO.  

Jim


> On Sep 20, 2017, at 10:50 AM, Zachary Turner  wrote:
> 
> On Wed, Sep 20, 2017 at 10:46 AM Jim Ingham  > wrote:
> Directly WRT testing.  I’m not against ALSO adding gtests when you add some 
> functionality.  But when your change is actually adding behaviors to lldb, 
> one of the things you need to ask yourself is if this is functionality that 
> extension developers for lldb will benefit from.  If it is, then adding 
> affordances in the SB API should be considered an essential part of a 
> complete implementation of the feature.  And I assert from experience that 
> writing tests for that feature at the SB API layer is a really good way to 
> ensure that the API’s you add make sense and actually get at the things you 
> intended to make available.
> 
> I also assert from experience that unfortunately it's also a good way to make 
> sure that the test doesn't get written in the first place :(
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Wed, Sep 20, 2017 at 10:46 AM Jim Ingham  wrote:

> Directly WRT testing.  I’m not against ALSO adding gtests when you add
> some functionality.  But when your change is actually adding behaviors to
> lldb, one of the things you need to ask yourself is if this is
> functionality that extension developers for lldb will benefit from.  If it
> is, then adding affordances in the SB API should be considered an essential
> part of a complete implementation of the feature.  And I assert from
> experience that writing tests for that feature at the SB API layer is a
> really good way to ensure that the API’s you add make sense and actually
> get at the things you intended to make available.
>

I also assert from experience that unfortunately it's also a good way to
make sure that the test doesn't get written in the first place :(
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

Directly WRT testing.  I’m not against ALSO adding gtests when you add some 
functionality.  But when your change is actually adding behaviors to lldb, one 
of the things you need to ask yourself is if this is functionality that 
extension developers for lldb will benefit from.  If it is, then adding 
affordances in the SB API should be considered an essential part of a complete 
implementation of the feature.  And I assert from experience that writing tests 
for that feature at the SB API layer is a really good way to ensure that the 
API’s you add make sense and actually get at the things you intended to make 
available.

So for anything like the change we are directly discussing, you should first 
think about how to make an SB affordance for it.  Then test that to make sure 
it makes sense.  Then if you feel like you also want to add gtests because 
there was something you couldn’t easily get through the SB API’s, please do so!

Jim

> On Sep 20, 2017, at 10:41 AM, Zachary Turner  wrote:
> 
> 
> 
> On Wed, Sep 20, 2017 at 10:33 AM Jim Ingham  > wrote:
> One of the fundamental goals of lldb is that it be an extensible debugger.  
> The extension mechanism for command line lldb all runs through the SB API 
> through either Python or C++ (though most folks choose to use Python).  We 
> know first hand that many people take advantage this mechanism to add custom 
> commands specific to their workflows.  Again, look at the xnu macros for an 
> example if you remain unconvinced of this.  Facebook also made a nice set of 
> extension commands for introspecting UI elements when debugging iOS apps.  
> And every WWDC folks come with questions about how to implement some command 
> or other.  If this doesn’t convince you also check out the effort the gdb 
> folks have made and continue to make to support the Python extensions to what 
> is purely a command-line debugger.
> 
> Not to mention that most of the powerful things you can do with breakpoint 
> callbacks are available through the SB API's, and most sophisticated data 
> formatters are written in Python.  And again based on questions and bug 
> reports we get there are many people using this facility.
> 
> I’ve said this many times before, but the notion that the SB API’s are not a 
> crucial part of the command-line lldb experience shows a fundamental 
> misperception of the design of lldb.
> 
> Jim
> 
> I don't disagree with this, only with the notion that every test is best 
> written by involving the extensibility layer in the test.
> 
> A fundamental and universal principle of good test design is writing tests 
> that test the minimal amount of code necessary to verify a piece of 
> functionality.  You don't *need* an extensibility layer to check if you can 
> stop at a breakpoint.
> 
> The fact that the extensibility mechanism provides a way of stopping at 
> breakpoints just means you should *also* have a test that says "did the 
> extensibility mechanism properly tell the debugger to stop at a breakpoint?".
> 
> And, of course you can (and should) also have full integration tests that 
> check the extensibility mechanism end-to-end.
> 
> But they just should not be the first and only line of defense.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Wed, Sep 20, 2017 at 10:33 AM Jim Ingham  wrote:

> One of the fundamental goals of lldb is that it be an extensible
> debugger.  The extension mechanism for command line lldb all runs through
> the SB API through either Python or C++ (though most folks choose to use
> Python).  We know first hand that many people take advantage this mechanism
> to add custom commands specific to their workflows.  Again, look at the xnu
> macros for an example if you remain unconvinced of this.  Facebook also
> made a nice set of extension commands for introspecting UI elements when
> debugging iOS apps.  And every WWDC folks come with questions about how to
> implement some command or other.  If this doesn’t convince you also check
> out the effort the gdb folks have made and continue to make to support the
> Python extensions to what is purely a command-line debugger.
>
> Not to mention that most of the powerful things you can do with breakpoint
> callbacks are available through the SB API's, and most sophisticated data
> formatters are written in Python.  And again based on questions and bug
> reports we get there are many people using this facility.
>
> I’ve said this many times before, but the notion that the SB API’s are not
> a crucial part of the command-line lldb experience shows a fundamental
> misperception of the design of lldb.
>
> Jim
>

I don't disagree with this, only with the notion that every test is best
written by involving the extensibility layer in the test.

A fundamental and universal principle of good test design is writing tests
that test the minimal amount of code necessary to verify a piece of
functionality.  You don't *need* an extensibility layer to check if you can
stop at a breakpoint.

The fact that the extensibility mechanism provides a way of stopping at
breakpoints just means you should *also* have a test that says "did the
extensibility mechanism properly tell the debugger to stop at a
breakpoint?".

And, of course you can (and should) also have full integration tests that
check the extensibility mechanism end-to-end.

But they just should not be the first and only line of defense.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

One of the fundamental goals of lldb is that it be an extensible debugger.  The 
extension mechanism for command line lldb all runs through the SB API through 
either Python or C++ (though most folks choose to use Python).  We know first 
hand that many people take advantage this mechanism to add custom commands 
specific to their workflows.  Again, look at the xnu macros for an example if 
you remain unconvinced of this.  Facebook also made a nice set of extension 
commands for introspecting UI elements when debugging iOS apps.  And every WWDC 
folks come with questions about how to implement some command or other.  If 
this doesn’t convince you also check out the effort the gdb folks have made and 
continue to make to support the Python extensions to what is purely a 
command-line debugger.

Not to mention that most of the powerful things you can do with breakpoint 
callbacks are available through the SB API's, and most sophisticated data 
formatters are written in Python.  And again based on questions and bug reports 
we get there are many people using this facility.

I’ve said this many times before, but the notion that the SB API’s are not a 
crucial part of the command-line lldb experience shows a fundamental 
misperception of the design of lldb.

Jim

> On Sep 20, 2017, at 7:37 AM, Zachary Turner  wrote:
> 
> 
> 
> On Tue, Sep 19, 2017 at 7:12 PM Jason Molenda  > wrote:
> 
> 
> > On Sep 19, 2017, at 6:57 PM, Zachary Turner via lldb-commits 
> > > wrote:
> >
> >
> >
> > After some additional thought, I stilled think testing below the sb api 
> > layer is more appropriate.
> >   While Xcode might be going through the SB api, users of upstream lldb 
> > (which is what we supposed to be testing upstream) are not.
> 
> 
> One thing to keep in mind is that while Xcode is the IDE using lldb that 
> Apple directly supports, there are many other IDEs that are using lldb today. 
>  I don't know for certain that all these are using SB API, but a partial list 
> of additional IDEs in addition to Xcode includes CLion ("A Cross-Platform IDE 
> for C and C+ by JetBrains"), Nuclide (atom based IDE), Android Studio, Visual 
> Code Studio, and Eclipse (I know this one is done via lldb-mi).
> 
> Command line lldb is the interface I personally tend to prefer, but it is not 
> the interface that the vast, vast majority of our users use when debugging 
> with lldb.  SB API is.  The lldb driver is more akin to one front-end among 
> many, and an odd one, given that it doesn't go through the SB API itself.
> 
> Definitely, but that's also one reason I think it's so important to split the 
> tests up.  Right now there is almost no coverage of not going through the SB 
> API, and yet that is the interface that we, as the upstream open-source 
> project, provide to users.  And while it may not be the interface that the 
> vast majority of users *currently* use, I think if we compared the number of 
> people using GDB to the number of people using LLDB, it paints a different 
> picture.  Namely, a picture of how many users *could* be using LLDB.  
> 
> I don't want to break any existing users of LLDB obviously, including those 
> that go through the SB API, but at the same time the primary product that 
> comes out of the open-source project is, in fact, a command line debugger, 
> and we should strive to make that experience as good as possible.
> 
> I believe if the tests are split into private tests and public API tests you 
> get the best of both worlds.  I would even go farther to say that the whole 
> is greater than the sum of parts, because it makes it easy to write tests for 
> any new api that comes along, and vastly reduces the barrier to entry to 
> writing new tests, which I still believe is one of the most critical bugs 
> facing the project today.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Tue, Sep 19, 2017 at 11:51 AM Zachary Turner  wrote:

> On Tue, Sep 19, 2017 at 11:40 AM Jim Ingham  wrote:
>
>> I must be missing something.
>>
>> DisaptchInputInterrupt knows how to interrupt a running process, and
>> because of Enrico's Python interruption stuff it will interrupt script
>> execution at some point but it wasn't very reliable last time I tried it.
>>
>> But the whole point of this patch is that in general commands don't
>> listen to that interrupt if they are just running in a loop doing work.
>> Sending the interrupt is not enough.
>>
>> Jim
>>
>
> I guess you are saying they installed a custom IOHandler?  IF that's the
> case, then yea I guess it won't work.
>

After some additional thought, I stilled think testing below the sb api
layer is more appropriate.

>   While Xcode might be going through the SB api, users of upstream lldb
(which is what we supposed to be testing upstream) are not.

In order to achieve maximum test coverage and minimal flakiness, there
should be 2 tests:

 the first test is for core functionality and tests that if you pass call
tge interrupt function, it gets interrupted and the output is as expected.

The second test test checks that if you call the Python api, it calls the
function that has been tested by step 1.

If you want a third function that tests both, you can certainly do that,
but this way the creation of a test is not blocked on the addition of a
Python api.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

Here's a short description of the coding rules and the general idea of how to 
add API's:

http://lldb.llvm.org/SB-api-coding-rules.html

If you come across something you wished would have been in this document while 
you are implementing this, please add it.  It's sometimes hard to see some 
crucial step you've internalized so that it isn't even conscious anymore...

Jim


> On Sep 19, 2017, at 1:04 PM, Leonard Mosescu  wrote:
> 
> Any pointers on the steps required to make changes to the SB APIs? Is it 
> documented anywhere? Thanks!
> 
> On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham  wrote:
> 
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu  wrote:
> >
> > These are all great suggestions, thanks everyone!
> >
> > > We should have a test. The test would need to use pexpect or something 
> > > similar. If anyone has any pointer on how to make a test for this, please 
> > > chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm missing 
> > something obvious - are you talking about splitting a string into chunks? 
> > The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a 
> > significant expansion of the original goal which is to improve the 
> > interactive LLDB debugger.
> 
> This is sort of a side point, but one of the goals of lldb is to make the 
> command set extensible by adding commands implemented with the SB API's 
> either from Python or from C++ (Python is more common but both are possible.) 
>  And there are a bunch of groups here at Apple that have all sorts of special 
> purpose LLDB commands implemented this way.  The xnu.py and Co. that gets 
> distributed with the kernel development kit has a wide array of examples of 
> this that are publicly available if you want to have a look.
> 
> So the SB API's are in fact an intrinsic part of the interactive LLDB 
> debugger.  We aren't there yet (there isn't an SB API to define command 
> options & arguments so SB added commands don't parse like lldb commands do) 
> but the end goal is that it you can't tell whether commands are from SB API's 
> or are builtin.
> 
> BTW, I think the fact that we use lldb_private API's is an unfortunate 
> artifact of how lldb was implemented.  Having to use our vended API's to 
> implement the commands would make them better implemented - since we would 
> really be on the hook for them, and would reduce a lot of code duplication 
> where we do things one way in the commands and slightly differently in the 
> equivalent SB API's, and reduce the testing surface considerably.  I'm not 
> sure this is a project we will ever actually get to, but it is a good 
> aspirational goal to keep in mind.
> 
> We didn't do it that way originally because we needed something we could poke 
> at early on in the development of lldb, at a point where it would have been 
> premature to design the SB API's.  Then lldb went from skunkworks project to 
> mission critical debugger too quickly for us to be able to revise the 
> decision.
> 
> Jim
> 
> 
> 
> > It may be nice looking at SB APIs later on, but I'm afraid that trying to 
> > pile up everything in one change is just going to spiral the complexity out 
> > of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner  wrote:
> > That would work, but I think it's adding many more pieces to the test.  Now 
> > there's threads, a Python layer, and multiprocess dotest infrastructure in 
> > the equation.  Each providing an additional source of flakiness and 
> > instability.
> >
> > If all it is is a pass-through, then just calling the function it passes 
> > through to is a strictly better test, since it's focusing the test on the 
> > underlying piece of functionality and removing additional sources of 
> > failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also 
> > need commands made through the SB API to be interruptible, and we should 
> > test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the 
> > questions you should ask yourself is whether this is useful at the SB API 
> > layer.  In this case it obviously is (actually I'm a little embarrassed I 
> > didn't think of this requirement).  If so, then it's simplest to test it at 
> > that layer.  If the SB API is anything more than a pass-through, then you 
> > might also want to write a unit test for the underlying C++ API's.  But in 
> > this case the SB function will just call the underlying CommandInterpreter 
> > one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> > >  wrote:
> > >
> 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Any pointers on the steps required to make changes to the SB APIs? Is it
documented anywhere? Thanks!

On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham  wrote:

>
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu 
> wrote:
> >
> > These are all great suggestions, thanks everyone!
> >
> > > We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm
> missing something obvious - are you talking about splitting a string into
> chunks? The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a
> significant expansion of the original goal which is to improve the
> interactive LLDB debugger.
>
> This is sort of a side point, but one of the goals of lldb is to make the
> command set extensible by adding commands implemented with the SB API's
> either from Python or from C++ (Python is more common but both are
> possible.)  And there are a bunch of groups here at Apple that have all
> sorts of special purpose LLDB commands implemented this way.  The xnu.py
> and Co. that gets distributed with the kernel development kit has a wide
> array of examples of this that are publicly available if you want to have a
> look.
>
> So the SB API's are in fact an intrinsic part of the interactive LLDB
> debugger.  We aren't there yet (there isn't an SB API to define command
> options & arguments so SB added commands don't parse like lldb commands do)
> but the end goal is that it you can't tell whether commands are from SB
> API's or are builtin.
>
> BTW, I think the fact that we use lldb_private API's is an unfortunate
> artifact of how lldb was implemented.  Having to use our vended API's to
> implement the commands would make them better implemented - since we would
> really be on the hook for them, and would reduce a lot of code duplication
> where we do things one way in the commands and slightly differently in the
> equivalent SB API's, and reduce the testing surface considerably.  I'm not
> sure this is a project we will ever actually get to, but it is a good
> aspirational goal to keep in mind.
>
> We didn't do it that way originally because we needed something we could
> poke at early on in the development of lldb, at a point where it would have
> been premature to design the SB API's.  Then lldb went from skunkworks
> project to mission critical debugger too quickly for us to be able to
> revise the decision.
>
> Jim
>
>
>
> > It may be nice looking at SB APIs later on, but I'm afraid that trying
> to pile up everything in one change is just going to spiral the complexity
> out of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Thanks for the context.

On Tue, Sep 19, 2017 at 11:58 AM, Jim Ingham  wrote:

>
> > On Sep 19, 2017, at 11:25 AM, Leonard Mosescu 
> wrote:
> >
> > These are all great suggestions, thanks everyone!
> >
> > > We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
> > I love tests but what exactly do we want to test here? Sorry if I'm
> missing something obvious - are you talking about splitting a string into
> chunks? The actual interruption? ...
> >
> > Also, I like the idea of exposing this though the SB APIs, but that's a
> significant expansion of the original goal which is to improve the
> interactive LLDB debugger.
>
> This is sort of a side point, but one of the goals of lldb is to make the
> command set extensible by adding commands implemented with the SB API's
> either from Python or from C++ (Python is more common but both are
> possible.)  And there are a bunch of groups here at Apple that have all
> sorts of special purpose LLDB commands implemented this way.  The xnu.py
> and Co. that gets distributed with the kernel development kit has a wide
> array of examples of this that are publicly available if you want to have a
> look.
>
> So the SB API's are in fact an intrinsic part of the interactive LLDB
> debugger.  We aren't there yet (there isn't an SB API to define command
> options & arguments so SB added commands don't parse like lldb commands do)
> but the end goal is that it you can't tell whether commands are from SB
> API's or are builtin.
>
> BTW, I think the fact that we use lldb_private API's is an unfortunate
> artifact of how lldb was implemented.  Having to use our vended API's to
> implement the commands would make them better implemented - since we would
> really be on the hook for them, and would reduce a lot of code duplication
> where we do things one way in the commands and slightly differently in the
> equivalent SB API's, and reduce the testing surface considerably.  I'm not
> sure this is a project we will ever actually get to, but it is a good
> aspirational goal to keep in mind.
>
> We didn't do it that way originally because we needed something we could
> poke at early on in the development of lldb, at a point where it would have
> been premature to design the SB API's.  Then lldb went from skunkworks
> project to mission critical debugger too quickly for us to be able to
> revise the decision.
>
> Jim
>
>
>
> > It may be nice looking at SB APIs later on, but I'm afraid that trying
> to pile up everything in one change is just going to spiral the complexity
> out of control.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > >  : CommandInterpreter(Interpreter),
> 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

> On Sep 19, 2017, at 11:25 AM, Leonard Mosescu  wrote:
> 
> These are all great suggestions, thanks everyone!
> 
> > We should have a test. The test would need to use pexpect or something 
> > similar. If anyone has any pointer on how to make a test for this, please 
> > chime in. I was thinking just a pexpect based test.
> I love tests but what exactly do we want to test here? Sorry if I'm missing 
> something obvious - are you talking about splitting a string into chunks? The 
> actual interruption? ...
> 
> Also, I like the idea of exposing this though the SB APIs, but that's a 
> significant expansion of the original goal which is to improve the 
> interactive LLDB debugger.

This is sort of a side point, but one of the goals of lldb is to make the 
command set extensible by adding commands implemented with the SB API's either 
from Python or from C++ (Python is more common but both are possible.)  And 
there are a bunch of groups here at Apple that have all sorts of special 
purpose LLDB commands implemented this way.  The xnu.py and Co. that gets 
distributed with the kernel development kit has a wide array of examples of 
this that are publicly available if you want to have a look.

So the SB API's are in fact an intrinsic part of the interactive LLDB debugger. 
 We aren't there yet (there isn't an SB API to define command options & 
arguments so SB added commands don't parse like lldb commands do) but the end 
goal is that it you can't tell whether commands are from SB API's or are 
builtin.  

BTW, I think the fact that we use lldb_private API's is an unfortunate artifact 
of how lldb was implemented.  Having to use our vended API's to implement the 
commands would make them better implemented - since we would really be on the 
hook for them, and would reduce a lot of code duplication where we do things 
one way in the commands and slightly differently in the equivalent SB API's, 
and reduce the testing surface considerably.  I'm not sure this is a project we 
will ever actually get to, but it is a good aspirational goal to keep in mind.

We didn't do it that way originally because we needed something we could poke 
at early on in the development of lldb, at a point where it would have been 
premature to design the SB API's.  Then lldb went from skunkworks project to 
mission critical debugger too quickly for us to be able to revise the decision.

Jim



> It may be nice looking at SB APIs later on, but I'm afraid that trying to 
> pile up everything in one change is just going to spiral the complexity out 
> of control.
> 
> On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner  wrote:
> That would work, but I think it's adding many more pieces to the test.  Now 
> there's threads, a Python layer, and multiprocess dotest infrastructure in 
> the equation.  Each providing an additional source of flakiness and 
> instability.
> 
> If all it is is a pass-through, then just calling the function it passes 
> through to is a strictly better test, since it's focusing the test on the 
> underlying piece of functionality and removing additional sources of failure 
> and instability from the test.
> 
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> I'd really prefer to do this as/along with an SB API test since we also need 
> commands made through the SB API to be interruptible, and we should test that 
> that also works.  So this kills two birds with one stone.
> 
> In general, when developing any high-level features in lldb one of the 
> questions you should ask yourself is whether this is useful at the SB API 
> layer.  In this case it obviously is (actually I'm a little embarrassed I 
> didn't think of this requirement).  If so, then it's simplest to test it at 
> that layer.  If the SB API is anything more than a pass-through, then you 
> might also want to write a unit test for the underlying C++ API's.  But in 
> this case the SB function will just call the underlying CommandInterpreter 
> one, so testing at the SB layer is sufficient.
> 
> Jim
> 
> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> >  wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> >
> >> We should have a test. The test would need to use pexpect or something 
> >> similar. If anyone has any pointer on how to make a test for this, please 
> >> chime in. I was thinking just a pexpect based test.
> >
> >
> > This kind of thing is perfect for a unit test, but I'm not sure how easy 
> > that would be with the current design.  You'd basically do something like:
> >
> >  struct MockStream {
> >explicit MockStream(CommandInterpreter , int InterruptAfter)
> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> >
> >CommandInterpreter 
> >const int InterruptAfter;
> >int Lines = 0;
> >std::string Buffer;
> >
> >void 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Tue, Sep 19, 2017 at 11:40 AM Jim Ingham  wrote:

> I must be missing something.
>
> DisaptchInputInterrupt knows how to interrupt a running process, and
> because of Enrico's Python interruption stuff it will interrupt script
> execution at some point but it wasn't very reliable last time I tried it.
>
> But the whole point of this patch is that in general commands don't listen
> to that interrupt if they are just running in a loop doing work.  Sending
> the interrupt is not enough.
>
> Jim
>

I guess you are saying they installed a custom IOHandler?  IF that's the
case, then yea I guess it won't work.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

That seems fine to me.

Jim

> On Sep 19, 2017, at 11:39 AM, Leonard Mosescu  wrote:
> 
> So, how about I look into exposing WasInterrupted() through SB APIs in a 
> follow up change?
> 
> On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham  wrote:
> Xcode does, I don't know about other UI's.
> 
> Jim
> 
> > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu  wrote:
> >
> > I agree Jim. I'd like like to build thing incrementally - checking in the 
> > current change as is does not preclude adding the SB APIs while it does 
> > provide the foundation.
> >
> > I think that going after the scenarios you mention is a significant 
> > increase in scope. Are people even hooking up DispatchInterrupt() from 
> > whatever interactive driver they use?
> >
> > On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:
> > We agreed to forwards compatibility because people write big scripts that 
> > use the SB API, implement GUI's on top of them (more than just Xcode) etc.  
> > So we try not to jerk those folks around.  That adds a little more 
> > responsibility on our part to think carefully about what we add, but the 
> > notion that we should refrain from making useful functionality available 
> > because we'd rather not be beholden to our decisions seems really 
> > wrong-headed to me.
> >
> > And in this case there's a clear use for this. For instance the xnu macros 
> > have a bunch of Python based commands that spew out pages and pages of 
> > output.  Those guys would love to make their commands interruptible.  To do 
> > that they would need to call WasInterrupted.  So this is 100% something 
> > that should be available at the SB API layer.
> >
> > Jim
> >
> >
> >
> > > On Sep 19, 2017, at 11:18 AM, Zachary Turner  wrote:
> > >
> > > Also, it avoids polluting the SB interface with another function that 
> > > probably nobody is ever going to use outside of testing.  Adding to the 
> > > SB API should take an act of God, given that once it gets added it has to 
> > > stay until the end of time.
> > >
> > > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner  
> > > wrote:
> > > That would work, but I think it's adding many more pieces to the test.  
> > > Now there's threads, a Python layer, and multiprocess dotest 
> > > infrastructure in the equation.  Each providing an additional source of 
> > > flakiness and instability.
> > >
> > > If all it is is a pass-through, then just calling the function it passes 
> > > through to is a strictly better test, since it's focusing the test on the 
> > > underlying piece of functionality and removing additional sources of 
> > > failure and instability from the test.
> > >
> > > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > > I'd really prefer to do this as/along with an SB API test since we also 
> > > need commands made through the SB API to be interruptible, and we should 
> > > test that that also works.  So this kills two birds with one stone.
> > >
> > > In general, when developing any high-level features in lldb one of the 
> > > questions you should ask yourself is whether this is useful at the SB API 
> > > layer.  In this case it obviously is (actually I'm a little embarrassed I 
> > > didn't think of this requirement).  If so, then it's simplest to test it 
> > > at that layer.  If the SB API is anything more than a pass-through, then 
> > > you might also want to write a unit test for the underlying C++ API's.  
> > > But in this case the SB function will just call the underlying 
> > > CommandInterpreter one, so testing at the SB layer is sufficient.
> > >
> > > Jim
> > >
> > > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> > > >  wrote:
> > > >
> > > > zturner added a comment.
> > > >
> > > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > > >
> > > >> We should have a test. The test would need to use pexpect or something 
> > > >> similar. If anyone has any pointer on how to make a test for this, 
> > > >> please chime in. I was thinking just a pexpect based test.
> > > >
> > > >
> > > > This kind of thing is perfect for a unit test, but I'm not sure how 
> > > > easy that would be with the current design.  You'd basically do 
> > > > something like:
> > > >
> > > >  struct MockStream {
> > > >explicit MockStream(CommandInterpreter , int 
> > > > InterruptAfter)
> > > >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) 
> > > > {}
> > > >
> > > >CommandInterpreter 
> > > >const int InterruptAfter;
> > > >int Lines = 0;
> > > >std::string Buffer;
> > > >
> > > >void Write(StringRef S) {
> > > >  ++Lines;
> > > >  if (Lines >= InterruptAfter) {
> > > >Interpreter.Interrupt();
> > > >return;
> > > >  }
> > > >  Buffer += S;
> > > >}
> > > >  };
> > > >
> > > >  TEST_F(CommandInterruption) {

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

I must be missing something.  

DisaptchInputInterrupt knows how to interrupt a running process, and because of 
Enrico's Python interruption stuff it will interrupt script execution at some 
point but it wasn't very reliable last time I tried it.  

But the whole point of this patch is that in general commands don't listen to 
that interrupt if they are just running in a loop doing work.  Sending the 
interrupt is not enough.

Jim



> On Sep 19, 2017, at 11:36 AM, Zachary Turner  wrote:
> 
> 
> 
> On Tue, Sep 19, 2017 at 11:34 AM Jim Ingham  wrote:
> 
> > On Sep 19, 2017, at 11:30 AM, Zachary Turner  wrote:
> >
> >
> >
> > On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham  wrote:
> > We agreed to forwards compatibility because people write big scripts that 
> > use the SB API, implement GUI's on top of them (more than just Xcode) etc.  
> > So we try not to jerk those folks around.  That adds a little more 
> > responsibility on our part to think carefully about what we add, but the 
> > notion that we should refrain from making useful functionality available 
> > because we'd rather not be beholden to our decisions seems really 
> > wrong-headed to me.
> >
> > And in this case there's a clear use for this. For instance the xnu macros 
> > have a bunch of Python based commands that spew out pages and pages of 
> > output.  Those guys would love to make their commands interruptible.  To do 
> > that they would need to call WasInterrupted.  So this is 100% something 
> > that should be available at the SB API layer.
> >
> >
> > Couldn't it just return eCommandFinishedNoResult?  Or a new value, 
> > eCommandFinishedPartialResult?
> 
> I don't follow.  How would it know the user asked it to stop?
> 
> 
> The user already has a way to interrupt a command via DispatchInputInterrupt. 
>  If the command is then interrupted and output is lost as a result, the 
> private api returns the appropriate value, which the user can check for.

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

So, how about I look into exposing WasInterrupted() through SB APIs in a
follow up change?

On Tue, Sep 19, 2017 at 11:36 AM, Jim Ingham  wrote:

> Xcode does, I don't know about other UI's.
>
> Jim
>
> > On Sep 19, 2017, at 11:35 AM, Leonard Mosescu 
> wrote:
> >
> > I agree Jim. I'd like like to build thing incrementally - checking in
> the current change as is does not preclude adding the SB APIs while it does
> provide the foundation.
> >
> > I think that going after the scenarios you mention is a significant
> increase in scope. Are people even hooking up DispatchInterrupt() from
> whatever interactive driver they use?
> >
> > On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:
> > We agreed to forwards compatibility because people write big scripts
> that use the SB API, implement GUI's on top of them (more than just Xcode)
> etc.  So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
> >
> > And in this case there's a clear use for this. For instance the xnu
> macros have a bunch of Python based commands that spew out pages and pages
> of output.  Those guys would love to make their commands interruptible.  To
> do that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
> >
> > Jim
> >
> >
> >
> > > On Sep 19, 2017, at 11:18 AM, Zachary Turner 
> wrote:
> > >
> > > Also, it avoids polluting the SB interface with another function that
> probably nobody is ever going to use outside of testing.  Adding to the SB
> API should take an act of God, given that once it gets added it has to stay
> until the end of time.
> > >
> > > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner 
> wrote:
> > > That would work, but I think it's adding many more pieces to the
> test.  Now there's threads, a Python layer, and multiprocess dotest
> infrastructure in the equation.  Each providing an additional source of
> flakiness and instability.
> > >
> > > If all it is is a pass-through, then just calling the function it
> passes through to is a strictly better test, since it's focusing the test
> on the underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> > >
> > > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > > I'd really prefer to do this as/along with an SB API test since we
> also need commands made through the SB API to be interruptible, and we
> should test that that also works.  So this kills two birds with one stone.
> > >
> > > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> > >
> > > Jim
> > >
> > > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > > >
> > > > zturner added a comment.
> > > >
> > > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > > >
> > > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > > >
> > > >
> > > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > > >
> > > >  struct MockStream {
> > > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > > >  : CommandInterpreter(Interpreter),
> InterruptAfter(InterruptAfter) {}
> > > >
> > > >CommandInterpreter 
> > > >const int InterruptAfter;
> > > >int Lines = 0;
> > > >std::string Buffer;
> > > >
> > > >void Write(StringRef S) {
> > > >  ++Lines;
> > > >  if (Lines >= InterruptAfter) {
> > > >Interpreter.Interrupt();
> > > >return;
> > > >  }
> > > >  Buffer += S;
> > > >}
> > > >  };
> > > >
> > > >  TEST_F(CommandInterruption) {
> > > >CommandInterpreter Interpreter;
> > > >MockStream Stream(Interpreter, 3);
> > > >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > > >EXPECT_EQ(Stream.Lines == 3);
> > > >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > > >  }
> > > >
> > > >
> > > > 

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Tue, Sep 19, 2017 at 11:34 AM Jim Ingham  wrote:

>
> > On Sep 19, 2017, at 11:30 AM, Zachary Turner  wrote:
> >
> >
> >
> > On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham  wrote:
> > We agreed to forwards compatibility because people write big scripts
> that use the SB API, implement GUI's on top of them (more than just Xcode)
> etc.  So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
> >
> > And in this case there's a clear use for this. For instance the xnu
> macros have a bunch of Python based commands that spew out pages and pages
> of output.  Those guys would love to make their commands interruptible.  To
> do that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
> >
> >
> > Couldn't it just return eCommandFinishedNoResult?  Or a new value,
> eCommandFinishedPartialResult?
>
> I don't follow.  How would it know the user asked it to stop?
>
>
The user already has a way to interrupt a command via
DispatchInputInterrupt.  If the command is then interrupted and output is
lost as a result, the private api returns the appropriate value, which the
user can check for.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

Xcode does, I don't know about other UI's.

Jim

> On Sep 19, 2017, at 11:35 AM, Leonard Mosescu  wrote:
> 
> I agree Jim. I'd like like to build thing incrementally - checking in the 
> current change as is does not preclude adding the SB APIs while it does 
> provide the foundation.
> 
> I think that going after the scenarios you mention is a significant increase 
> in scope. Are people even hooking up DispatchInterrupt() from whatever 
> interactive driver they use?
> 
> On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:
> We agreed to forwards compatibility because people write big scripts that use 
> the SB API, implement GUI's on top of them (more than just Xcode) etc.  So we 
> try not to jerk those folks around.  That adds a little more responsibility 
> on our part to think carefully about what we add, but the notion that we 
> should refrain from making useful functionality available because we'd rather 
> not be beholden to our decisions seems really wrong-headed to me.
> 
> And in this case there's a clear use for this. For instance the xnu macros 
> have a bunch of Python based commands that spew out pages and pages of 
> output.  Those guys would love to make their commands interruptible.  To do 
> that they would need to call WasInterrupted.  So this is 100% something that 
> should be available at the SB API layer.
> 
> Jim
> 
> 
> 
> > On Sep 19, 2017, at 11:18 AM, Zachary Turner  wrote:
> >
> > Also, it avoids polluting the SB interface with another function that 
> > probably nobody is ever going to use outside of testing.  Adding to the SB 
> > API should take an act of God, given that once it gets added it has to stay 
> > until the end of time.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner  wrote:
> > That would work, but I think it's adding many more pieces to the test.  Now 
> > there's threads, a Python layer, and multiprocess dotest infrastructure in 
> > the equation.  Each providing an additional source of flakiness and 
> > instability.
> >
> > If all it is is a pass-through, then just calling the function it passes 
> > through to is a strictly better test, since it's focusing the test on the 
> > underlying piece of functionality and removing additional sources of 
> > failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also 
> > need commands made through the SB API to be interruptible, and we should 
> > test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the 
> > questions you should ask yourself is whether this is useful at the SB API 
> > layer.  In this case it obviously is (actually I'm a little embarrassed I 
> > didn't think of this requirement).  If so, then it's simplest to test it at 
> > that layer.  If the SB API is anything more than a pass-through, then you 
> > might also want to write a unit test for the underlying C++ API's.  But in 
> > this case the SB function will just call the underlying CommandInterpreter 
> > one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> > >  wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or something 
> > >> similar. If anyone has any pointer on how to make a test for this, 
> > >> please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how easy 
> > > that would be with the current design.  You'd basically do something like:
> > >
> > >  struct MockStream {
> > >explicit MockStream(CommandInterpreter , int 
> > > InterruptAfter)
> > >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> > >
> > >CommandInterpreter 
> > >const int InterruptAfter;
> > >int Lines = 0;
> > >std::string Buffer;
> > >
> > >void Write(StringRef S) {
> > >  ++Lines;
> > >  if (Lines >= InterruptAfter) {
> > >Interpreter.Interrupt();
> > >return;
> > >  }
> > >  Buffer += S;
> > >}
> > >  };
> > >
> > >  TEST_F(CommandInterruption) {
> > >CommandInterpreter Interpreter;
> > >MockStream Stream(Interpreter, 3);
> > >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > >EXPECT_EQ(Stream.Lines == 3);
> > >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > >  }
> > >
> > >
> > > https://reviews.llvm.org/D37923
> > >
> > >
> > >
> >
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

IIRC Enrico put in something where we would tell Python to interrupt at points 
where Python checks for interruptibility, but that is pretty herky-jerky.  It 
would be much better to have the commands control this.

Jim

> On Sep 19, 2017, at 11:34 AM, Jim Ingham  wrote:
> 
> 
>> On Sep 19, 2017, at 11:30 AM, Zachary Turner  wrote:
>> 
>> 
>> 
>> On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham  wrote:
>> We agreed to forwards compatibility because people write big scripts that 
>> use the SB API, implement GUI's on top of them (more than just Xcode) etc.  
>> So we try not to jerk those folks around.  That adds a little more 
>> responsibility on our part to think carefully about what we add, but the 
>> notion that we should refrain from making useful functionality available 
>> because we'd rather not be beholden to our decisions seems really 
>> wrong-headed to me.
>> 
>> And in this case there's a clear use for this. For instance the xnu macros 
>> have a bunch of Python based commands that spew out pages and pages of 
>> output.  Those guys would love to make their commands interruptible.  To do 
>> that they would need to call WasInterrupted.  So this is 100% something that 
>> should be available at the SB API layer.
>> 
>> 
>> Couldn't it just return eCommandFinishedNoResult?  Or a new value, 
>> eCommandFinishedPartialResult? 
> 
> I don't follow.  How would it know the user asked it to stop?
> 
> Jim
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

I agree Jim. I'd like like to build thing incrementally - checking in the
current change as is does not preclude adding the SB APIs while it does
provide the foundation.

I think that going after the scenarios you mention is a significant
increase in scope. Are people even hooking up DispatchInterrupt() from
whatever interactive driver they use?

On Tue, Sep 19, 2017 at 11:27 AM, Jim Ingham  wrote:

> We agreed to forwards compatibility because people write big scripts that
> use the SB API, implement GUI's on top of them (more than just Xcode) etc.
> So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
>
> And in this case there's a clear use for this. For instance the xnu macros
> have a bunch of Python based commands that spew out pages and pages of
> output.  Those guys would love to make their commands interruptible.  To do
> that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
>
> Jim
>
>
>
> > On Sep 19, 2017, at 11:18 AM, Zachary Turner  wrote:
> >
> > Also, it avoids polluting the SB interface with another function that
> probably nobody is ever going to use outside of testing.  Adding to the SB
> API should take an act of God, given that once it gets added it has to stay
> until the end of time.
> >
> > On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner 
> wrote:
> > That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
> >
> > If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
> >
> > On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> > I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
> >
> > In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
> >
> > Jim
> >
> > > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> > >
> > > zturner added a comment.
> > >
> > > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> > >
> > >> We should have a test. The test would need to use pexpect or
> something similar. If anyone has any pointer on how to make a test for
> this, please chime in. I was thinking just a pexpect based test.
> > >
> > >
> > > This kind of thing is perfect for a unit test, but I'm not sure how
> easy that would be with the current design.  You'd basically do something
> like:
> > >
> > >  struct MockStream {
> > >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> > >  : CommandInterpreter(Interpreter),
> InterruptAfter(InterruptAfter) {}
> > >
> > >CommandInterpreter 
> > >const int InterruptAfter;
> > >int Lines = 0;
> > >std::string Buffer;
> > >
> > >void Write(StringRef S) {
> > >  ++Lines;
> > >  if (Lines >= InterruptAfter) {
> > >Interpreter.Interrupt();
> > >return;
> > >  }
> > >  Buffer += S;
> > >}
> > >  };
> > >
> > >  TEST_F(CommandInterruption) {
> > >CommandInterpreter Interpreter;
> > >MockStream Stream(Interpreter, 3);
> > >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> > >EXPECT_EQ(Stream.Lines == 3);
> > >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> > >  }
> > >
> > >
> > > https://reviews.llvm.org/D37923
> > >
> > >
> > >
> >
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

> On Sep 19, 2017, at 11:30 AM, Zachary Turner  wrote:
> 
> 
> 
> On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham  wrote:
> We agreed to forwards compatibility because people write big scripts that use 
> the SB API, implement GUI's on top of them (more than just Xcode) etc.  So we 
> try not to jerk those folks around.  That adds a little more responsibility 
> on our part to think carefully about what we add, but the notion that we 
> should refrain from making useful functionality available because we'd rather 
> not be beholden to our decisions seems really wrong-headed to me.
> 
> And in this case there's a clear use for this. For instance the xnu macros 
> have a bunch of Python based commands that spew out pages and pages of 
> output.  Those guys would love to make their commands interruptible.  To do 
> that they would need to call WasInterrupted.  So this is 100% something that 
> should be available at the SB API layer.
> 
> 
> Couldn't it just return eCommandFinishedNoResult?  Or a new value, 
> eCommandFinishedPartialResult? 

I don't follow.  How would it know the user asked it to stop?

Jim


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Tue, Sep 19, 2017 at 11:27 AM Jim Ingham  wrote:

> We agreed to forwards compatibility because people write big scripts that
> use the SB API, implement GUI's on top of them (more than just Xcode) etc.
> So we try not to jerk those folks around.  That adds a little more
> responsibility on our part to think carefully about what we add, but the
> notion that we should refrain from making useful functionality available
> because we'd rather not be beholden to our decisions seems really
> wrong-headed to me.
>
> And in this case there's a clear use for this. For instance the xnu macros
> have a bunch of Python based commands that spew out pages and pages of
> output.  Those guys would love to make their commands interruptible.  To do
> that they would need to call WasInterrupted.  So this is 100% something
> that should be available at the SB API layer.
>
>
Couldn't it just return eCommandFinishedNoResult?  Or a new value,
eCommandFinishedPartialResult?
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

Right, I can fix that.  Give me a few minutes though.

On Tue, Sep 19, 2017 at 11:28 AM Leonard Mosescu  wrote:

> This looks beautiful indeed. The problem is that it doesn't quite work
> with the current MemoryBuffer and the line_iterator : for one thing there's
> no way to construct a MemoryBuffer from a StringRef, or to use the
> line_iterator directly with a StringRef.
>
>
> On Tue, Sep 19, 2017 at 10:39 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
>> zturner added a comment.
>>
>> Give me a few more hours, if there's a way to make this work with
>> `line_iterator` I'd really prefer that since I think it improves
>> readability.  Can you confirm that if you were able to write:
>>
>>   auto begin = line_iterator(str, /* skip_empty_lines =*/ false);
>>   auto end = line_iterator();
>>   while (begin != end && !WasInterrupted()) {
>> stream.Write(*begin);
>> if (++begin != end)
>>   stream.Write("\n");
>>   }
>>
>> That this would be equivalent?
>>
>>
>> https://reviews.llvm.org/D37923
>>
>>
>>
>>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

This looks beautiful indeed. The problem is that it doesn't quite work with
the current MemoryBuffer and the line_iterator : for one thing there's no
way to construct a MemoryBuffer from a StringRef, or to use the
line_iterator directly with a StringRef.


On Tue, Sep 19, 2017 at 10:39 AM, Zachary Turner via Phabricator <
revi...@reviews.llvm.org> wrote:

> zturner added a comment.
>
> Give me a few more hours, if there's a way to make this work with
> `line_iterator` I'd really prefer that since I think it improves
> readability.  Can you confirm that if you were able to write:
>
>   auto begin = line_iterator(str, /* skip_empty_lines =*/ false);
>   auto end = line_iterator();
>   while (begin != end && !WasInterrupted()) {
> stream.Write(*begin);
> if (++begin != end)
>   stream.Write("\n");
>   }
>
> That this would be equivalent?
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

We agreed to forwards compatibility because people write big scripts that use 
the SB API, implement GUI's on top of them (more than just Xcode) etc.  So we 
try not to jerk those folks around.  That adds a little more responsibility on 
our part to think carefully about what we add, but the notion that we should 
refrain from making useful functionality available because we'd rather not be 
beholden to our decisions seems really wrong-headed to me.

And in this case there's a clear use for this. For instance the xnu macros have 
a bunch of Python based commands that spew out pages and pages of output.  
Those guys would love to make their commands interruptible.  To do that they 
would need to call WasInterrupted.  So this is 100% something that should be 
available at the SB API layer.

Jim



> On Sep 19, 2017, at 11:18 AM, Zachary Turner  wrote:
> 
> Also, it avoids polluting the SB interface with another function that 
> probably nobody is ever going to use outside of testing.  Adding to the SB 
> API should take an act of God, given that once it gets added it has to stay 
> until the end of time.
> 
> On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner  wrote:
> That would work, but I think it's adding many more pieces to the test.  Now 
> there's threads, a Python layer, and multiprocess dotest infrastructure in 
> the equation.  Each providing an additional source of flakiness and 
> instability.
> 
> If all it is is a pass-through, then just calling the function it passes 
> through to is a strictly better test, since it's focusing the test on the 
> underlying piece of functionality and removing additional sources of failure 
> and instability from the test.
> 
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> I'd really prefer to do this as/along with an SB API test since we also need 
> commands made through the SB API to be interruptible, and we should test that 
> that also works.  So this kills two birds with one stone.
> 
> In general, when developing any high-level features in lldb one of the 
> questions you should ask yourself is whether this is useful at the SB API 
> layer.  In this case it obviously is (actually I'm a little embarrassed I 
> didn't think of this requirement).  If so, then it's simplest to test it at 
> that layer.  If the SB API is anything more than a pass-through, then you 
> might also want to write a unit test for the underlying C++ API's.  But in 
> this case the SB function will just call the underlying CommandInterpreter 
> one, so testing at the SB layer is sufficient.
> 
> Jim
> 
> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> >  wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> >
> >> We should have a test. The test would need to use pexpect or something 
> >> similar. If anyone has any pointer on how to make a test for this, please 
> >> chime in. I was thinking just a pexpect based test.
> >
> >
> > This kind of thing is perfect for a unit test, but I'm not sure how easy 
> > that would be with the current design.  You'd basically do something like:
> >
> >  struct MockStream {
> >explicit MockStream(CommandInterpreter , int InterruptAfter)
> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> >
> >CommandInterpreter 
> >const int InterruptAfter;
> >int Lines = 0;
> >std::string Buffer;
> >
> >void Write(StringRef S) {
> >  ++Lines;
> >  if (Lines >= InterruptAfter) {
> >Interpreter.Interrupt();
> >return;
> >  }
> >  Buffer += S;
> >}
> >  };
> >
> >  TEST_F(CommandInterruption) {
> >CommandInterpreter Interpreter;
> >MockStream Stream(Interpreter, 3);
> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> >EXPECT_EQ(Stream.Lines == 3);
> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> >  }
> >
> >
> > https://reviews.llvm.org/D37923
> >
> >
> >
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

These are all great suggestions, thanks everyone!

> We should have a test. The test would need to use pexpect or something
similar. If anyone has any pointer on how to make a test for this, please
chime in. I was thinking just a pexpect based test.
I love tests but what exactly do we want to test here? Sorry if I'm missing
something obvious - are you talking about splitting a string into chunks?
The actual interruption? ...

Also, I like the idea of exposing this though the SB APIs, but that's a
significant expansion of the original goal which is to improve the
interactive LLDB debugger. It may be nice looking at SB APIs later on, but
I'm afraid that trying to pile up everything in one change is just going to
spiral the complexity out of control.

On Tue, Sep 19, 2017 at 11:15 AM, Zachary Turner  wrote:

> That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
>
> If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
>
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
>
>> I'd really prefer to do this as/along with an SB API test since we also
>> need commands made through the SB API to be interruptible, and we should
>> test that that also works.  So this kills two birds with one stone.
>>
>> In general, when developing any high-level features in lldb one of the
>> questions you should ask yourself is whether this is useful at the SB API
>> layer.  In this case it obviously is (actually I'm a little embarrassed I
>> didn't think of this requirement).  If so, then it's simplest to test it at
>> that layer.  If the SB API is anything more than a pass-through, then you
>> might also want to write a unit test for the underlying C++ API's.  But in
>> this case the SB function will just call the underlying CommandInterpreter
>> one, so testing at the SB layer is sufficient.
>>
>> Jim
>>
>> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>> >
>> > zturner added a comment.
>> >
>> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
>> >
>> >> We should have a test. The test would need to use pexpect or something
>> similar. If anyone has any pointer on how to make a test for this, please
>> chime in. I was thinking just a pexpect based test.
>> >
>> >
>> > This kind of thing is perfect for a unit test, but I'm not sure how
>> easy that would be with the current design.  You'd basically do something
>> like:
>> >
>> >  struct MockStream {
>> >explicit MockStream(CommandInterpreter , int
>> InterruptAfter)
>> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter)
>> {}
>> >
>> >CommandInterpreter 
>> >const int InterruptAfter;
>> >int Lines = 0;
>> >std::string Buffer;
>> >
>> >void Write(StringRef S) {
>> >  ++Lines;
>> >  if (Lines >= InterruptAfter) {
>> >Interpreter.Interrupt();
>> >return;
>> >  }
>> >  Buffer += S;
>> >}
>> >  };
>> >
>> >  TEST_F(CommandInterruption) {
>> >CommandInterpreter Interpreter;
>> >MockStream Stream(Interpreter, 3);
>> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
>> >EXPECT_EQ(Stream.Lines == 3);
>> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
>> >  }
>> >
>> >
>> > https://reviews.llvm.org/D37923
>> >
>> >
>> >
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

I disagree.  The things we care about are (a) that this works in C++ 
implemented commands and (b) that it works in Python commands.  This doesn't 
seem to test either of those things.

Also, I find writing tests for new functionality to be a great way to inform 
you about what you need to add to the SB API's to support the functionality you 
are adding.  So if you avoid that step when thinking about testing you are 
likely to miss adding access that you will need or not designing it well 
because you didn't really try to use it.  If you also want to add unit tests to 
test closer to the metal that's also fine.

Jim

> On Sep 19, 2017, at 11:15 AM, Zachary Turner  wrote:
> 
> That would work, but I think it's adding many more pieces to the test.  Now 
> there's threads, a Python layer, and multiprocess dotest infrastructure in 
> the equation.  Each providing an additional source of flakiness and 
> instability.
> 
> If all it is is a pass-through, then just calling the function it passes 
> through to is a strictly better test, since it's focusing the test on the 
> underlying piece of functionality and removing additional sources of failure 
> and instability from the test.
> 
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
> I'd really prefer to do this as/along with an SB API test since we also need 
> commands made through the SB API to be interruptible, and we should test that 
> that also works.  So this kills two birds with one stone.
> 
> In general, when developing any high-level features in lldb one of the 
> questions you should ask yourself is whether this is useful at the SB API 
> layer.  In this case it obviously is (actually I'm a little embarrassed I 
> didn't think of this requirement).  If so, then it's simplest to test it at 
> that layer.  If the SB API is anything more than a pass-through, then you 
> might also want to write a unit test for the underlying C++ API's.  But in 
> this case the SB function will just call the underlying CommandInterpreter 
> one, so testing at the SB layer is sufficient.
> 
> Jim
> 
> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
> >  wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> >
> >> We should have a test. The test would need to use pexpect or something 
> >> similar. If anyone has any pointer on how to make a test for this, please 
> >> chime in. I was thinking just a pexpect based test.
> >
> >
> > This kind of thing is perfect for a unit test, but I'm not sure how easy 
> > that would be with the current design.  You'd basically do something like:
> >
> >  struct MockStream {
> >explicit MockStream(CommandInterpreter , int InterruptAfter)
> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> >
> >CommandInterpreter 
> >const int InterruptAfter;
> >int Lines = 0;
> >std::string Buffer;
> >
> >void Write(StringRef S) {
> >  ++Lines;
> >  if (Lines >= InterruptAfter) {
> >Interpreter.Interrupt();
> >return;
> >  }
> >  Buffer += S;
> >}
> >  };
> >
> >  TEST_F(CommandInterruption) {
> >CommandInterpreter Interpreter;
> >MockStream Stream(Interpreter, 3);
> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> >EXPECT_EQ(Stream.Lines == 3);
> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> >  }
> >
> >
> > https://reviews.llvm.org/D37923
> >
> >
> >
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

Also, it avoids polluting the SB interface with another function that
probably nobody is ever going to use outside of testing.  Adding to the SB
API should take an act of God, given that once it gets added it has to stay
until the end of time.

On Tue, Sep 19, 2017 at 11:15 AM Zachary Turner  wrote:

> That would work, but I think it's adding many more pieces to the test.
> Now there's threads, a Python layer, and multiprocess dotest infrastructure
> in the equation.  Each providing an additional source of flakiness and
> instability.
>
> If all it is is a pass-through, then just calling the function it passes
> through to is a strictly better test, since it's focusing the test on the
> underlying piece of functionality and removing additional sources of
> failure and instability from the test.
>
> On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:
>
>> I'd really prefer to do this as/along with an SB API test since we also
>> need commands made through the SB API to be interruptible, and we should
>> test that that also works.  So this kills two birds with one stone.
>>
>> In general, when developing any high-level features in lldb one of the
>> questions you should ask yourself is whether this is useful at the SB API
>> layer.  In this case it obviously is (actually I'm a little embarrassed I
>> didn't think of this requirement).  If so, then it's simplest to test it at
>> that layer.  If the SB API is anything more than a pass-through, then you
>> might also want to write a unit test for the underlying C++ API's.  But in
>> this case the SB function will just call the underlying CommandInterpreter
>> one, so testing at the SB layer is sufficient.
>>
>> Jim
>>
>> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
>> revi...@reviews.llvm.org> wrote:
>> >
>> > zturner added a comment.
>> >
>> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
>> >
>> >> We should have a test. The test would need to use pexpect or something
>> similar. If anyone has any pointer on how to make a test for this, please
>> chime in. I was thinking just a pexpect based test.
>> >
>> >
>> > This kind of thing is perfect for a unit test, but I'm not sure how
>> easy that would be with the current design.  You'd basically do something
>> like:
>> >
>> >  struct MockStream {
>> >explicit MockStream(CommandInterpreter , int
>> InterruptAfter)
>> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter)
>> {}
>> >
>> >CommandInterpreter 
>> >const int InterruptAfter;
>> >int Lines = 0;
>> >std::string Buffer;
>> >
>> >void Write(StringRef S) {
>> >  ++Lines;
>> >  if (Lines >= InterruptAfter) {
>> >Interpreter.Interrupt();
>> >return;
>> >  }
>> >  Buffer += S;
>> >}
>> >  };
>> >
>> >  TEST_F(CommandInterruption) {
>> >CommandInterpreter Interpreter;
>> >MockStream Stream(Interpreter, 3);
>> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
>> >EXPECT_EQ(Stream.Lines == 3);
>> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
>> >  }
>> >
>> >
>> > https://reviews.llvm.org/D37923
>> >
>> >
>> >
>>
>>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

That would work, but I think it's adding many more pieces to the test.  Now
there's threads, a Python layer, and multiprocess dotest infrastructure in
the equation.  Each providing an additional source of flakiness and
instability.

If all it is is a pass-through, then just calling the function it passes
through to is a strictly better test, since it's focusing the test on the
underlying piece of functionality and removing additional sources of
failure and instability from the test.

On Tue, Sep 19, 2017 at 11:02 AM Jim Ingham  wrote:

> I'd really prefer to do this as/along with an SB API test since we also
> need commands made through the SB API to be interruptible, and we should
> test that that also works.  So this kills two birds with one stone.
>
> In general, when developing any high-level features in lldb one of the
> questions you should ask yourself is whether this is useful at the SB API
> layer.  In this case it obviously is (actually I'm a little embarrassed I
> didn't think of this requirement).  If so, then it's simplest to test it at
> that layer.  If the SB API is anything more than a pass-through, then you
> might also want to write a unit test for the underlying C++ API's.  But in
> this case the SB function will just call the underlying CommandInterpreter
> one, so testing at the SB layer is sufficient.
>
> Jim
>
> > On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > zturner added a comment.
> >
> > In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> >
> >> We should have a test. The test would need to use pexpect or something
> similar. If anyone has any pointer on how to make a test for this, please
> chime in. I was thinking just a pexpect based test.
> >
> >
> > This kind of thing is perfect for a unit test, but I'm not sure how easy
> that would be with the current design.  You'd basically do something like:
> >
> >  struct MockStream {
> >explicit MockStream(CommandInterpreter , int
> InterruptAfter)
> >  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> >
> >CommandInterpreter 
> >const int InterruptAfter;
> >int Lines = 0;
> >std::string Buffer;
> >
> >void Write(StringRef S) {
> >  ++Lines;
> >  if (Lines >= InterruptAfter) {
> >Interpreter.Interrupt();
> >return;
> >  }
> >  Buffer += S;
> >}
> >  };
> >
> >  TEST_F(CommandInterruption) {
> >CommandInterpreter Interpreter;
> >MockStream Stream(Interpreter, 3);
> >Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
> >EXPECT_EQ(Stream.Lines == 3);
> >EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
> >  }
> >
> >
> > https://reviews.llvm.org/D37923
> >
> >
> >
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

I'd really prefer to do this as/along with an SB API test since we also need 
commands made through the SB API to be interruptible, and we should test that 
that also works.  So this kills two birds with one stone.

In general, when developing any high-level features in lldb one of the 
questions you should ask yourself is whether this is useful at the SB API 
layer.  In this case it obviously is (actually I'm a little embarrassed I 
didn't think of this requirement).  If so, then it's simplest to test it at 
that layer.  If the SB API is anything more than a pass-through, then you might 
also want to write a unit test for the underlying C++ API's.  But in this case 
the SB function will just call the underlying CommandInterpreter one, so 
testing at the SB layer is sufficient.

Jim

> On Sep 19, 2017, at 10:45 AM, Zachary Turner via Phabricator 
>  wrote:
> 
> zturner added a comment.
> 
> In https://reviews.llvm.org/D37923#875322, @clayborg wrote:
> 
>> We should have a test. The test would need to use pexpect or something 
>> similar. If anyone has any pointer on how to make a test for this, please 
>> chime in. I was thinking just a pexpect based test.
> 
> 
> This kind of thing is perfect for a unit test, but I'm not sure how easy that 
> would be with the current design.  You'd basically do something like:
> 
>  struct MockStream {
>explicit MockStream(CommandInterpreter , int InterruptAfter) 
>  : CommandInterpreter(Interpreter), InterruptAfter(InterruptAfter) {}
> 
>CommandInterpreter 
>const int InterruptAfter;
>int Lines = 0;
>std::string Buffer;
> 
>void Write(StringRef S) {
>  ++Lines;
>  if (Lines >= InterruptAfter) {
>Interpreter.Interrupt();
>return;
>  }
>  Buffer += S;
>}
>  };
> 
>  TEST_F(CommandInterruption) {
>CommandInterpreter Interpreter;
>MockStream Stream(Interpreter, 3);
>Interpreter.PrintCommandOutput(Stream, "a\nb\nc\nd\ne\nf\n");
>EXPECT_EQ(Stream.Lines == 3);
>EXPECT_EQ(Stream.Buffer == "a\nb\nc\n");
>  }
> 
> 
> https://reviews.llvm.org/D37923
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via lldb-commits]

Python based commands will need to be able to call WasInterrupted if they are 
going to do their jobs.  So it would make sense to add an 
SBCommandInterpreter::WasInterrupted API and make sure that works.  And you can 
already send the interrupt with DispatchInputInterrupt.  So it should be pretty 
straight-forward to make a Python based command that runs forever, and start up 
another Python thread that just calls DispatchInputInterrupt.  The test would 
just be that the command returns.  That would be more direct, and wouldn't 
involve pexpect which has proven to be pretty fragile for anything complex.

Jim

> On Sep 19, 2017, at 10:37 AM, Greg Clayton via Phabricator 
>  wrote:
> 
> clayborg added a comment.
> 
> We should have a test. The test would need to use pexpect or something 
> similar. If anyone has any pointer on how to make a test for this, please 
> chime in. I was thinking just a pexpect based test.
> 
> 
> https://reviews.llvm.org/D37923
> 
> 
> 

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Give me a few more hours, if there's a way to make this work with 
`line_iterator` I'd really prefer that since I think it improves readability.  
Can you confirm that if you were able to write:

  auto begin = line_iterator(str, /* skip_empty_lines =*/ false);
  auto end = line_iterator();
  while (begin != end && !WasInterrupted()) {
stream.Write(*begin);
if (++begin != end)
  stream.Write("\n");
  }

That this would be equivalent?


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

We should have a test. The test would need to use pexpect or something similar. 
If anyone has any pointer on how to make a test for this, please chime in. I 
was thinking just a pexpect based test.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Ping? Are there any more open questions or unresolved comments or is this good 
to be checked in?

Thanks!


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Hi Greg, are you referring to the manual line splitting vs. using
StringRef::split()? This is how it's documented:

/// Split into two substrings around the first occurrence of a separator
/// character.
///
/// If \p Separator is in the string, then the result is a pair (LHS, RHS)
/// such that (*this == LHS + Separator + RHS) is true and RHS is
/// maximal. If \p Separator is not in the string, then the result is a
/// pair (LHS, RHS) where (*this == LHS) and (RHS == "").

Yes, RHS == "" happens to be be implemented as a StringRef with nullptr
data, but this seems to make the implementation even more depended on the
subtle details. My point was that second == "" in two cases: separator is
not in the string AND separator is _last_ character in the string.

Did I get the question right?

On Mon, Sep 18, 2017 at 3:51 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> ok. Then back to the "can we use StringRef"? We might be able to check if
> the second.data() is NULL? It might be NULL if the string doesn't end with
> a newline. It it does, it might be empty but contain a pointer to the '\0'
> byte?
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

Looking at line_iterator, it seems to be designed to work with
MemoryBuffer, which in turn seems specialized for dealing with file content
(so while it may be possible to force init a MemoryBuffer from a StringRef
it seems a bit awkward to me). Also, line_iterator has extra stuff which we
don't need here yet requires extra care (skipping empty lines and comments,
tracking line numbers...)


On Mon, Sep 18, 2017 at 3:18 PM, Zachary Turner  wrote:

>
>
> On Mon, Sep 18, 2017 at 3:13 PM Leonard Mosescu 
> wrote:
>
>> It's a good question - here's a two part answer:
>>
>> 1. The actual printing of the output is the longest blocking in many
>> cases (as mentioned in the description: the actual data gathering for
>> "target module dump symtab" can take 1-2sec, but printing it can take
>> 20min. For quick experiment, try dis -p -c 1).
>> 2. This change provides the scaffolding for cooperative interruption that
>> can be used were appropriate, not just the printing part. I did this for
>> "target" commands (see the changes in CommandObjectTarget.cpp), and it's
>> very easy to do the same in other places as needed.
>>
>>
> Makes sense.  Can you try `llvm::line_iterator` then instead of the
> hand-splitting?  See `llvm/Support/LineIterator.h`
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

ok. Then back to the "can we use StringRef"? We might be able to check if the 
second.data() is NULL? It might be NULL if the string doesn't end with a 
newline. It it does, it might be empty but contain a pointer to the '\0' byte?


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

And in general, interruption should be as responsive as you can make it.  When 
I tell lldb to "Stop right now" it really should do as close as possible to 
"stop right now".  Not, :-"excuse me I have 10 more pages of output to dump".  
It's not uncommon to dump some info, see what you want and not need to see the 
rest.  Interrupt in that case is expressing that "don't show me any more" so we 
should honor that.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via lldb-commits]

On Mon, Sep 18, 2017 at 3:13 PM Leonard Mosescu  wrote:

> It's a good question - here's a two part answer:
>
> 1. The actual printing of the output is the longest blocking in many cases
> (as mentioned in the description: the actual data gathering for "target
> module dump symtab" can take 1-2sec, but printing it can take 20min. For
> quick experiment, try dis -p -c 1).
> 2. This change provides the scaffolding for cooperative interruption that
> can be used were appropriate, not just the printing part. I did this for
> "target" commands (see the changes in CommandObjectTarget.cpp), and it's
> very easy to do the same in other places as needed.
>
>
Makes sense.  Can you try `llvm::line_iterator` then instead of the
hand-splitting?  See `llvm/Support/LineIterator.h`
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In the original discussion of this feature (it was on the mailing list), it 
sounded like the specific cast that  motivated Leonard in adding this feature 
was when there's a command that's in the process of generating tons of output, 
and you want to interrupt the tons of output.  Not the work, just the output.  
So the fact that is discards output was part of the design.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via lldb-commits]

It's a good question - here's a two part answer:

1. The actual printing of the output is the longest blocking in many cases
(as mentioned in the description: the actual data gathering for "target
module dump symtab" can take 1-2sec, but printing it can take 20min. For
quick experiment, try dis -p -c 1).
2. This change provides the scaffolding for cooperative interruption that
can be used were appropriate, not just the printing part. I did this for
"target" commands (see the changes in CommandObjectTarget.cpp), and it's
very easy to do the same in other places as needed.




On Mon, Sep 18, 2017 at 3:05 PM, Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg added a comment.
>
> I think Zach might be correct. If we already gathered the data, why not
> just output it all? Lets answer this first before returning to which
> implementation to use when parsing it up into lines and dumping it. Seems a
> shame to throw any data away.
>
>
> https://reviews.llvm.org/D37923
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.



Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {

lemo wrote:
> lemo wrote:
> > lemo wrote:
> > > clayborg wrote:
> > > > Since we are using "llvm::StringRef" here, why not use its split 
> > > > functionality? Something like:
> > > > ```
> > > > bool done = false;
> > > > while (!done) {
> > > >   auto pair = str.split('\n');
> > > >   auto len = pair.first.size();
> > > >   done = pair.second.empty();
> > > >   // Include newline if we are not done
> > > >   if (!done)
> > > > ++len; 
> > > >   stream.Write(pair.first.data(), 
> > > > }
> > > > ```
> > > > 
> > > > This approach also avoids the issue amccarth mentioned below about not 
> > > > ending with a newline. It is also quite a bit simpler to follow.
> > > I'll give it a try, thanks for the suggestion.
> > I forgot to reply to this: I tried using str.split() as suggested, with a 
> > small tweak to avoid reading past the end of the first slice (++len to 
> > include the '\n'):
> > 
> > while (!str.empty() && !WasInterrupted()) {
> >   auto pair = str.split('\n');
> >   stream.Write(pair.first.data(), pair.first.size());
> >   if (str.size() != pair.first.size())
> > stream.PutChar('\n');
> >   str = pair.second;
> > }
> > if (!str.empty()) {
> >   stream.Printf("\n... Interrupted.\n");
> > }
> > 
> > While this version is shorter there I see a number of small problems with 
> > it:
> > 1. It requires a close look at the StringRef::split() interface (ex. done = 
> > pair.second.empty() is actually not the correct check since it will eat the 
> > final \n)
> > 2. While conceptually straightforward, the StringRef::split() interface is 
> > not ideal in this case - see #1
> > 3. This version assumes that stream.Write() actually writes everything (ie. 
> > not handling the return size)
> > 4. In order to avoid reading past the end of the first slice we need to do 
> > a separate PutChar() for each \n
> > 
> > So in the end I prefer the original version, which although a bit more 
> > verbose, tracks the invariants clearly, is potentially faster and handles 
> > strings regardless if they are terminated with \n or not.
> Thoughts?
I haven't followed the rest of this too closely, so forgive me if this is a 
dumb question, but it looks like by the time we reach this function, the 
command is already complete.  Why would you want to interrupt it if it's 
already done?  Couldn't you just print the entire output in one call to 
`stream.Write()` at this point?


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {

lemo wrote:
> lemo wrote:
> > clayborg wrote:
> > > Since we are using "llvm::StringRef" here, why not use its split 
> > > functionality? Something like:
> > > ```
> > > bool done = false;
> > > while (!done) {
> > >   auto pair = str.split('\n');
> > >   auto len = pair.first.size();
> > >   done = pair.second.empty();
> > >   // Include newline if we are not done
> > >   if (!done)
> > > ++len; 
> > >   stream.Write(pair.first.data(), 
> > > }
> > > ```
> > > 
> > > This approach also avoids the issue amccarth mentioned below about not 
> > > ending with a newline. It is also quite a bit simpler to follow.
> > I'll give it a try, thanks for the suggestion.
> I forgot to reply to this: I tried using str.split() as suggested, with a 
> small tweak to avoid reading past the end of the first slice (++len to 
> include the '\n'):
> 
> while (!str.empty() && !WasInterrupted()) {
>   auto pair = str.split('\n');
>   stream.Write(pair.first.data(), pair.first.size());
>   if (str.size() != pair.first.size())
> stream.PutChar('\n');
>   str = pair.second;
> }
> if (!str.empty()) {
>   stream.Printf("\n... Interrupted.\n");
> }
> 
> While this version is shorter there I see a number of small problems with it:
> 1. It requires a close look at the StringRef::split() interface (ex. done = 
> pair.second.empty() is actually not the correct check since it will eat the 
> final \n)
> 2. While conceptually straightforward, the StringRef::split() interface is 
> not ideal in this case - see #1
> 3. This version assumes that stream.Write() actually writes everything (ie. 
> not handling the return size)
> 4. In order to avoid reading past the end of the first slice we need to do a 
> separate PutChar() for each \n
> 
> So in the end I prefer the original version, which although a bit more 
> verbose, tracks the invariants clearly, is potentially faster and handles 
> strings regardless if they are terminated with \n or not.
Thoughts?


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {

lemo wrote:
> clayborg wrote:
> > Since we are using "llvm::StringRef" here, why not use its split 
> > functionality? Something like:
> > ```
> > bool done = false;
> > while (!done) {
> >   auto pair = str.split('\n');
> >   auto len = pair.first.size();
> >   done = pair.second.empty();
> >   // Include newline if we are not done
> >   if (!done)
> > ++len; 
> >   stream.Write(pair.first.data(), 
> > }
> > ```
> > 
> > This approach also avoids the issue amccarth mentioned below about not 
> > ending with a newline. It is also quite a bit simpler to follow.
> I'll give it a try, thanks for the suggestion.
I forgot to reply to this: I tried using str.split() as suggested, with a small 
tweak to avoid reading past the end of the first slice (++len to include the 
'\n'):

while (!str.empty() && !WasInterrupted()) {
  auto pair = str.split('\n');
  stream.Write(pair.first.data(), pair.first.size());
  if (str.size() != pair.first.size())
stream.PutChar('\n');
  str = pair.second;
}
if (!str.empty()) {
  stream.Printf("\n... Interrupted.\n");
}

While this version is shorter there I see a number of small problems with it:
1. It requires a close look at the StringRef::split() interface (ex. done = 
pair.second.empty() is actually not the correct check since it will eat the 
final \n)
2. While conceptually straightforward, the StringRef::split() interface is not 
ideal in this case - see #1
3. This version assumes that stream.Write() actually writes everything (ie. not 
handling the return size)
4. In order to avoid reading past the end of the first slice we need to do a 
separate PutChar() for each \n

So in the end I prefer the original version, which although a bit more verbose, 
tracks the invariants clearly, is potentially faster and handles strings 
regardless if they are terminated with \n or not.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 115516.
lemo edited the summary of this revision.
lemo added a comment.

Incorporating CR feedback.


https://reviews.llvm.org/D37923

Files:
  include/lldb/Core/IOHandler.h
  include/lldb/Interpreter/CommandInterpreter.h
  source/Commands/CommandObjectTarget.cpp
  source/Interpreter/CommandInterpreter.cpp

Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -546,7 +546,7 @@
   char buffer[1024];
   int num_printed =
   snprintf(buffer, 1024, "%s %s", break_regexes[i][1], "-o");
-  assert(num_printed < 1024);
+  lldbassert(num_printed < 1024);
   UNUSED_IF_ASSERT_DISABLED(num_printed);
   success =
   tbreak_regex_cmd_ap->AddRegexCommand(break_regexes[i][0], buffer);
@@ -891,7 +891,7 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
+lldbassert((this == _sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   if (name.empty())
@@ -913,7 +913,7 @@
 const lldb::CommandObjectSP _sp,
 bool can_replace) {
   if (cmd_sp.get())
-assert((this == _sp->GetCommandInterpreter()) &&
+lldbassert((this == _sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   if (!name.empty()) {
@@ -1062,7 +1062,7 @@
  lldb::CommandObjectSP _obj_sp,
  llvm::StringRef args_string) {
   if (command_obj_sp.get())
-assert((this == _obj_sp->GetCommandInterpreter()) &&
+lldbassert((this == _obj_sp->GetCommandInterpreter()) &&
"tried to add a CommandObject from a different interpreter");
 
   std::unique_ptr command_alias_up(
@@ -1839,7 +1839,7 @@
   matches.Clear();
 
   // Only max_return_elements == -1 is supported at present:
-  assert(max_return_elements == -1);
+  lldbassert(max_return_elements == -1);
   bool word_complete;
   num_command_matches = HandleCompletionMatches(
   parsed_line, cursor_index, cursor_char_position, match_start_point,
@@ -2677,6 +2677,57 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  lldbassert(prev_state == CommandHandlingState::eIdle);
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  lldbassert(prev_state != CommandHandlingState::eIdle);
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return m_command_state.compare_exchange_strong(
+  in_progress, CommandHandlingState::eInterrupted);
+}
+
+bool CommandInterpreter::WasInterrupted() const {
+  return m_command_state == CommandHandlingState::eInterrupted;
+}
+
+void CommandInterpreter::PrintCommandOutput(Stream , llvm::StringRef str,
+bool interruptible) {
+  if (str.empty())
+return;
+
+  if (interruptible) {
+// Split the output into lines and poll for interrupt requests
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
+  size_t chunk_size = 0;
+  for (; chunk_size < size; ++chunk_size) {
+lldbassert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {
+  ++chunk_size;
+  break;
+}
+  }
+  chunk_size = stream.Write(data, chunk_size);
+  lldbassert(size >= chunk_size);
+  data += chunk_size;
+  size -= chunk_size;
+}
+if (size > 0) {
+  stream.Printf("\n... Interrupted.\n");
+}
+  } else {
+stream.PutCString(str);
+  }
+}
+
 void CommandInterpreter::IOHandlerInputComplete(IOHandler _handler,
 std::string ) {
   const bool is_interactive = io_handler.GetIsInteractive();
@@ -2700,6 +2751,8 @@
line.c_str());
   }
 
+  StartHandlingCommand();
+
   lldb_private::CommandReturnObject result;
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
 
@@ -2710,18 +2763,20 @@
 
 if (!result.GetImmediateOutputStream()) {
   llvm::StringRef output = result.GetOutputData();
-  if (!output.empty())
-io_handler.GetOutputStreamFile()->PutCString(output);
+  PrintCommandOutput(*io_handler.GetOutputStreamFile(), output,
+ is_interactive);
 }
 
 // Now emit the command error text from the command we just executed
 if (!result.GetImmediateErrorStream()) {
   llvm::StringRef error = 

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Interpreter/CommandInterpreter.cpp:2713
+  for (; chunk_size < size; ++chunk_size) {
+assert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {

amccarth wrote:
> Should we be that trusting of a caller?  In a non-debug build, if a caller 
> doesn't end the (non-empty) string with '\n', this'll just run past the end 
> of the buffer.  Did I miss something that guarantees the caller won't make a 
> mistake?  Would it be too expensive to play it safe?
There's no assumption that the string ends with \n, see the loop condition: 
chunk_size < size. The assert is just to validate that we don't have embedded 
NULs.



Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {

clayborg wrote:
> Since we are using "llvm::StringRef" here, why not use its split 
> functionality? Something like:
> ```
> bool done = false;
> while (!done) {
>   auto pair = str.split('\n');
>   auto len = pair.first.size();
>   done = pair.second.empty();
>   // Include newline if we are not done
>   if (!done)
> ++len; 
>   stream.Write(pair.first.data(), 
> }
> ```
> 
> This approach also avoids the issue amccarth mentioned below about not ending 
> with a newline. It is also quite a bit simpler to follow.
I'll give it a try, thanks for the suggestion.



Comment at: source/Interpreter/CommandInterpreter.cpp:2728
+  } else {
+stream.PutCString(str);
+  }

clayborg wrote:
> llvm::StringRef can contain NULLs right? Maybe use
> 
> ```
> stream.Write(data, size);
> ```
The original code (line 2714) was using PutCString(), so this path is just 
preserving the original functionality (which also suggests that the output is 
not expected to have embedded nuls)


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:2056-2058
+if (m_interpreter.WasInterrupted()) {
+  break;
+}

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2087-2089
+if (m_interpreter.WasInterrupted()) {
+  break;
+}

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2155-2157
+if (m_interpreter.WasInterrupted()) {
+  break;
+}

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2179-2181
+  if (m_interpreter.WasInterrupted()) {
+break;
+  }

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2254-2256
+if (m_interpreter.WasInterrupted()) {
+  break;
+}

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2278-2280
+  if (m_interpreter.WasInterrupted()) {
+break;
+  }

Remove braces



Comment at: source/Commands/CommandObjectTarget.cpp:2348-2350
+if (m_interpreter.WasInterrupted()) {
+  break;
+}

Remove braces



Comment at: source/Interpreter/CommandInterpreter.cpp:2682
+  auto prev_state = 
m_command_state.exchange(CommandHandlingState::eInProgress);
+  assert(prev_state == CommandHandlingState::eIdle);
+}

lldb_assert



Comment at: source/Interpreter/CommandInterpreter.cpp:2687
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  assert(prev_state != CommandHandlingState::eIdle);
+}

lldb_assert



Comment at: source/Interpreter/CommandInterpreter.cpp:2708-2710
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {

Since we are using "llvm::StringRef" here, why not use its split functionality? 
Something like:
```
bool done = false;
while (!done) {
  auto pair = str.split('\n');
  auto len = pair.first.size();
  done = pair.second.empty();
  // Include newline if we are not done
  if (!done)
++len; 
  stream.Write(pair.first.data(), 
}
```

This approach also avoids the issue amccarth mentioned below about not ending 
with a newline. It is also quite a bit simpler to follow.



Comment at: source/Interpreter/CommandInterpreter.cpp:2728
+  } else {
+stream.PutCString(str);
+  }

llvm::StringRef can contain NULLs right? Maybe use

```
stream.Write(data, size);
```


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:2058
+  break;
+}
 num_dumped++;

amccarth wrote:
> Many LLVM developers prefer to omit the braces when the body of the 
> control-flow statement is a single statement.
So do the hackers: https://blog.codecentric.de/en/2014/02/curly-braces :) I too 
prefer to omit braces in small test snippets, but in production code it's not 
worth the risk of making a silly mistake.



Comment at: source/Interpreter/CommandInterpreter.cpp:2720
+  chunk_size = stream.Write(data, chunk_size);
+  assert(size >= chunk_size);
+  data += chunk_size;

amccarth wrote:
> This assert should precede the line before it.
Pedantically, it should be both before and after (and for ultimate paranoid 
mode, asserting that Write returns <= than the passed in value)

But the asserts looks for the really nasty case where "size -= chunk_size" 
overflows.



Comment at: tools/driver/Driver.cpp:1189
 
-  exit(signo);
+  _exit(signo);
 }

amccarth wrote:
> Can you add a comment explaining why this uses `_exit` rather than `exit`?  
> It's not obvious to me.
Explained in the SIGINT patch: exit() is not signal-safe 
(http://pubs.opengroup.org/onlinepubs/95399/functions/exit.html)

Calling it from a signal handler can result in all kind of nasty issues, in 
particular exit() does call a lot of stuff, both runtime and user code (ex. 
atexit functions) 




Comment at: tools/lldb-mi/MIDriverMain.cpp:71
 //--
 void sigint_handler(int vSigno) {
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows

amccarth wrote:
> I think this concurrency fix for SIGINT would be better in a separate patch.  
> I understand how it's related to the rest of this patch, but LLVM folks tend 
> to prefer small, incremental patches.
Agreed, I already split this change into separate patches (I wasn't sure if 
people prefer to review two small changes vs a single one with more context)


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added inline comments.



Comment at: source/Commands/CommandObjectTarget.cpp:2058
+  break;
+}
 num_dumped++;

Many LLVM developers prefer to omit the braces when the body of the 
control-flow statement is a single statement.



Comment at: source/Interpreter/CommandInterpreter.cpp:2713
+  for (; chunk_size < size; ++chunk_size) {
+assert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {

Should we be that trusting of a caller?  In a non-debug build, if a caller 
doesn't end the (non-empty) string with '\n', this'll just run past the end of 
the buffer.  Did I miss something that guarantees the caller won't make a 
mistake?  Would it be too expensive to play it safe?



Comment at: source/Interpreter/CommandInterpreter.cpp:2720
+  chunk_size = stream.Write(data, chunk_size);
+  assert(size >= chunk_size);
+  data += chunk_size;

This assert should precede the line before it.



Comment at: tools/driver/Driver.cpp:1182
   if (g_driver) {
-if (!g_interrupt_sent) {
-  g_interrupt_sent = true;
+if (!g_interrupt_sent.test_and_set()) {
   g_driver->GetDebugger().DispatchInputInterrupt();

I'm not sure why these two ifs aren't one, as in:

```
if (g_driver && !g_interrupt_sent.test_and_set())
```



Comment at: tools/driver/Driver.cpp:1189
 
-  exit(signo);
+  _exit(signo);
 }

Can you add a comment explaining why this uses `_exit` rather than `exit`?  
It's not obvious to me.



Comment at: tools/lldb-mi/MIDriverMain.cpp:71
 //--
 void sigint_handler(int vSigno) {
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows

I think this concurrency fix for SIGINT would be better in a separate patch.  I 
understand how it's related to the rest of this patch, but LLVM folks tend to 
prefer small, incremental patches.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 115476.
lemo added a comment.

Split the SIGINT handles fixes into a stanalone patch


https://reviews.llvm.org/D37923

Files:
  include/lldb/Core/IOHandler.h
  include/lldb/Interpreter/CommandInterpreter.h
  source/Commands/CommandObjectTarget.cpp
  source/Interpreter/CommandInterpreter.cpp

Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -2677,6 +2677,58 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  assert(prev_state == CommandHandlingState::eIdle);
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  assert(prev_state != CommandHandlingState::eIdle);
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return m_command_state.compare_exchange_strong(
+  in_progress, CommandHandlingState::eInterrupted);
+}
+
+bool CommandInterpreter::WasInterrupted() const {
+  return m_command_state == CommandHandlingState::eInterrupted;
+}
+
+void CommandInterpreter::PrintCommandOutput(Stream , llvm::StringRef str,
+bool interruptible) {
+  if (str.empty()) {
+return;
+  }
+
+  if (interruptible) {
+// Split the output into lines and poll for interrupt requests
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
+  size_t chunk_size = 0;
+  for (; chunk_size < size; ++chunk_size) {
+assert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {
+  ++chunk_size;
+  break;
+}
+  }
+  chunk_size = stream.Write(data, chunk_size);
+  assert(size >= chunk_size);
+  data += chunk_size;
+  size -= chunk_size;
+}
+if (size > 0) {
+  stream.Printf("\n... Interrupted.\n");
+}
+  } else {
+stream.PutCString(str);
+  }
+}
+
 void CommandInterpreter::IOHandlerInputComplete(IOHandler _handler,
 std::string ) {
   const bool is_interactive = io_handler.GetIsInteractive();
@@ -2700,6 +2752,8 @@
line.c_str());
   }
 
+  StartHandlingCommand();
+
   lldb_private::CommandReturnObject result;
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
 
@@ -2710,18 +2764,20 @@
 
 if (!result.GetImmediateOutputStream()) {
   llvm::StringRef output = result.GetOutputData();
-  if (!output.empty())
-io_handler.GetOutputStreamFile()->PutCString(output);
+  PrintCommandOutput(*io_handler.GetOutputStreamFile(), output,
+ is_interactive);
 }
 
 // Now emit the command error text from the command we just executed
 if (!result.GetImmediateErrorStream()) {
   llvm::StringRef error = result.GetErrorData();
-  if (!error.empty())
-io_handler.GetErrorStreamFile()->PutCString(error);
+  PrintCommandOutput(*io_handler.GetErrorStreamFile(), error,
+ is_interactive);
 }
   }
 
+  FinishHandlingCommand();
+
   switch (result.GetStatus()) {
   case eReturnStatusInvalid:
   case eReturnStatusSuccessFinishNoResult:
@@ -2777,6 +2833,10 @@
   ExecutionContext exe_ctx(GetExecutionContext());
   Process *process = exe_ctx.GetProcessPtr();
 
+  if (InterruptCommand()) {
+return true;
+  }
+
   if (process) {
 StateType state = process->GetState();
 if (StateIsRunningState(state)) {
Index: source/Commands/CommandObjectTarget.cpp
===
--- source/Commands/CommandObjectTarget.cpp
+++ source/Commands/CommandObjectTarget.cpp
@@ -2053,6 +2053,9 @@
   result.GetOutputStream().EOL();
   result.GetOutputStream().EOL();
 }
+if (m_interpreter.WasInterrupted()) {
+  break;
+}
 num_dumped++;
 DumpModuleSymtab(
 m_interpreter, result.GetOutputStream(),
@@ -2081,6 +2084,9 @@
   result.GetOutputStream().EOL();
   result.GetOutputStream().EOL();
 }
+if (m_interpreter.WasInterrupted()) {
+  break;
+}
 num_dumped++;
 DumpModuleSymtab(m_interpreter, result.GetOutputStream(),
  module, m_options.m_sort_order);
@@ -2146,6 +2152,9 @@
   " modules.\n",
   (uint64_t)num_modules);
   for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
+if (m_interpreter.WasInterrupted()) {
+

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

I haven't looked at the whole patch yet, but it seems the SIGINT fix is well 
isolated.  That should probably be in a separate patch.


https://reviews.llvm.org/D37923



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
Herald added a subscriber: ki.stfu.

The core of this change is the new CommandInterpreter::m_command_state, which 
models the state transitions for interactive commands, including an 
"interrupted" state transition.

In general, command interruption requires cooperation from the code executing 
the command, which needs to poll for interruption requests through 
CommandInterpreter::WasInterrupted().

CommandInterpreter::PrintCommandOutput() implements an optionally interruptible 
printing of the command output, which for large outputs was likely the longest 
blocking part

  (ex. target modules dump symtab on a complex binary could take 10+ minutes)

Also included are a few fixes in the handlers for SIGINT (thread safety and 
using the signal-safe _exit() instead of exit())


https://reviews.llvm.org/D37923

Files:
  include/lldb/Core/IOHandler.h
  include/lldb/Interpreter/CommandInterpreter.h
  source/Commands/CommandObjectTarget.cpp
  source/Interpreter/CommandInterpreter.cpp
  tools/driver/Driver.cpp
  tools/lldb-mi/MIDriverMain.cpp

Index: tools/lldb-mi/MIDriverMain.cpp
===
--- tools/lldb-mi/MIDriverMain.cpp
+++ tools/lldb-mi/MIDriverMain.cpp
@@ -72,14 +72,13 @@
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows
   signal(SIGINT, sigint_handler);
 #endif
-  static bool g_interrupt_sent = false;
+  static std::atomic_flag g_interrupt_sent = ATOMIC_FLAG_INIT;
   CMIDriverMgr  = CMIDriverMgr::Instance();
   lldb::SBDebugger *pDebugger = rDriverMgr.DriverGetTheDebugger();
   if (pDebugger != nullptr) {
-if (!g_interrupt_sent) {
-  g_interrupt_sent = true;
+if (!g_interrupt_sent.test_and_set()) {
   pDebugger->DispatchInputInterrupt();
-  g_interrupt_sent = false;
+  g_interrupt_sent.clear();
 }
   }
 
Index: tools/driver/Driver.cpp
===
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -1177,17 +1177,16 @@
 }
 
 void sigint_handler(int signo) {
-  static bool g_interrupt_sent = false;
+  static std::atomic_flag g_interrupt_sent = ATOMIC_FLAG_INIT;
   if (g_driver) {
-if (!g_interrupt_sent) {
-  g_interrupt_sent = true;
+if (!g_interrupt_sent.test_and_set()) {
   g_driver->GetDebugger().DispatchInputInterrupt();
-  g_interrupt_sent = false;
+  g_interrupt_sent.clear();
   return;
 }
   }
 
-  exit(signo);
+  _exit(signo);
 }
 
 void sigtstp_handler(int signo) {
Index: source/Interpreter/CommandInterpreter.cpp
===
--- source/Interpreter/CommandInterpreter.cpp
+++ source/Interpreter/CommandInterpreter.cpp
@@ -2677,6 +2677,58 @@
   return total_bytes;
 }
 
+void CommandInterpreter::StartHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eInProgress);
+  assert(prev_state == CommandHandlingState::eIdle);
+}
+
+void CommandInterpreter::FinishHandlingCommand() {
+  auto prev_state = m_command_state.exchange(CommandHandlingState::eIdle);
+  assert(prev_state != CommandHandlingState::eIdle);
+}
+
+bool CommandInterpreter::InterruptCommand() {
+  auto in_progress = CommandHandlingState::eInProgress;
+  return m_command_state.compare_exchange_strong(
+  in_progress, CommandHandlingState::eInterrupted);
+}
+
+bool CommandInterpreter::WasInterrupted() const {
+  return m_command_state == CommandHandlingState::eInterrupted;
+}
+
+void CommandInterpreter::PrintCommandOutput(Stream , llvm::StringRef str,
+bool interruptible) {
+  if (str.empty()) {
+return;
+  }
+
+  if (interruptible) {
+// Split the output into lines and poll for interrupt requests
+const char *data = str.data();
+size_t size = str.size();
+while (size > 0 && !WasInterrupted()) {
+  size_t chunk_size = 0;
+  for (; chunk_size < size; ++chunk_size) {
+assert(data[chunk_size] != '\0');
+if (data[chunk_size] == '\n') {
+  ++chunk_size;
+  break;
+}
+  }
+  chunk_size = stream.Write(data, chunk_size);
+  assert(size >= chunk_size);
+  data += chunk_size;
+  size -= chunk_size;
+}
+if (size > 0) {
+  stream.Printf("\n... Interrupted.\n");
+}
+  } else {
+stream.PutCString(str);
+  }
+}
+
 void CommandInterpreter::IOHandlerInputComplete(IOHandler _handler,
 std::string ) {
   const bool is_interactive = io_handler.GetIsInteractive();
@@ -2700,6 +2752,8 @@
line.c_str());
   }
 
+  StartHandlingCommand();
+
   lldb_private::CommandReturnObject result;
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
 
@@ -2710,18 +2764,20 @@
 
 if (!result.GetImmediateOutputStream()) {
   llvm::StringRef output = result.GetOutputData();
-  if (!output.empty())
-