[lldb] Fix {break,watch}point command function stopping behaviour

[mib]

Summary

In order to run a {break,watch}point command, lldb can resolve to the
script interpreter to run an arbitrary piece of code or call into a
user-provided function. To do so, we will generate a wrapping function,
where we first copy lldb's internal dictionary keys into the
interpreter's global dictionary, copied inline the user code before
resetting the global dictionary to its previous state.

However, {break,watch}point commands can optionally return a value that
would tell lldb whether we should stop or not. This feature was
only implemented for breakpoint commands and since we inlined the user
code directly into the wrapping function, introducing an early return,
that caused lldb to let the interpreter global dictionary tinted with the
internal dictionary keys.

This patch fixes that issue while also adding the stopping behaviour to
watchpoint commands.

To do so, this patch refactors the {break,watch}point command creation
method, to let the lldb wrapper function generator know if the user code is
a function call or a arbitrary expression.

Then the wrapper generator, if the user input was a function call, the
wrapper function will call the user function and save the return value into
a variable. If the user input was an arbitrary expression, the wrapper will
inline it into a nested function, call the nested function and save the
return value into the same variable. After resetting the interpreter global
dictionary to its previous state, the generated wrapper function will return
the varible containing the return value.

rdar://105461140

Signed-off-by: Med Ismail Bennani <[e-mail]>

[delcypher]

lldb/source/Commands/CommandObjectWatchpointCommand.cpp

425–426

@mib There's a reason I didn't do it this way when I tried to fix this locally.

The python stub function we generate would look something like this with your patch.

def lldb_autogen_python_wp_callback_func__0 (frame, wp, internal_dict):
     global_dict = globals()
     new_keys = internal_dict.keys()
     old_keys = global_dict.keys()
     global_dict.update (internal_dict)
     if True:
         return call_user_function(frame, wp, internal_dict)
     for key in new_keys:
         internal_dict[key] = global_dict[key]
         if key not in old_keys:
            del global_dict[key]

with your early return the logic for updating internal_dict and global_dict will not run. I'm not entirely sure what the purpose of this is but if its important then we need to implement this patch another way.

Here's another way we could do it. You could take the patch I originally wrote but change ScriptInterpreterPythonImpl::GenerateFunction(...) to take an additional parameter bool ReturnValue that is false by default. This parameter when true would do the work my patch did to make sure we use the return value from the last user statement evaluated. For the watchpoint evaluation we can pass a true value. For the other cases ReturnValue will be false so there will be no behavior change there.

lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py

159

This self.expect() fails when the bug hasn't been fixed, correct?

[mib]

lldb/source/Commands/CommandObjectWatchpointCommand.cpp

425–426

It's funny you're proposing this approach because we had the exact same idea when looking at this bug with @bulbazord.

I ended up going with this implementation because if you use and one-liner -o instead of a python function -F for your watchpoint callback, you'd also get the early return behavior.

I thought it would be better to stay consistant even if that implies leaving some internal_dict keys in the global_dict (because of the early return).

May be @jingham has some opinions about this.

lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py

159

Yup

[bulbazord]

I'm alright with this patch since it at least makes watchpoint commands consistent with breakpoint commands (w.r.t the -F flag).

lldb/source/Commands/CommandObjectWatchpointCommand.cpp

425–426

The behavior implemented in this patch makes the -F flag consistent with the way it behaves with breakpoint commands. If we want to make sure that internal_dict and global_dict are appropriately updated instead of doing an early return, then I think that should be done in a follow-up that would apply to both breakpoints and watchpoints (because they shouldn't really be behaving differently w.r.t. commands). A simple idea for that would be to store the result of the function call in a variable and return it at the very end.

lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py

1

I checked, the difference here is a byte order mark: https://en.wikipedia.org/wiki/Byte_order_mark

This just removes 3 bytes: ef bb bf

[mib]

lldb/source/Commands/CommandObjectWatchpointCommand.cpp

425–426

+1

[jingham]

IIUC, not removing elements from the internal dict will mean cross-contamination between the python states when you have multiple concurrent debuggers, which for instance Xcode does a lot. New variables would appear in the other session, but not linked to this debugger. That could cause subtle problems, which it's worth doing some extra work to avoid.

We should also make sure in the test that a watchpoint command that returns nothing still works. That's suppose to be the equivalent of returning True.

[mib]

In D144688#4151164, @jingham wrote:

IIUC, not removing elements from the internal dict will mean cross-contamination between the python states when you have multiple concurrent debuggers, which for instance Xcode does a lot. New variables would appear in the other session, but not linked to this debugger. That could cause subtle problems, which it's worth doing some extra work to avoid.

We've discuss offline with @jingham, and I'll fix this on a new diff to separate the 2 issues.

We should also make sure in the test that a watchpoint command that returns nothing still works. That's suppose to be the equivalent of returning True.

I'll update this PR to test this.

[mib]

Address @jingham & @delcypher feedbacks.

[bulbazord]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1310–1320

I feel like this is subtly wrong. For example, given the user provided function:

def func(frame, bp_loc, internal_dict):
    if frame.name == "main":
        return True
    return False

This will be transformed into:

def __user_callback():
    nonlocal __return_val
    if frame.name == "main":
        __return_val = True
    __return_val = False

Assume frame.name == "main": In the first one, we will return True. In the second one, __return_val will be False.

Maybe we can insert the input into __user_callback as-is and do __return_val = __user_callback()?

[bulbazord]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1310–1320

To be clear, I think this will work with -F because there's just one return statement inserted a few frames up.

This also may fail with -o if you concatenate multiple lines in one. For example, -o "print('foo'); return frame.name == 'main'". This is one line, so we'll never set __return_val to frame.name == 'main'. You might need to do further processing on the input to break it into multiple lines.

[mib]

The third implementation is the charm 😝:

• If the user input is a oneliner, we wrap it into a nested function.
• Then we call the function (either the nested function or the user provided python function) and capture the return value in a variable.
• Finally, after resetting the global dictionary to its previous state, we return the variable that contains the user's return value.

[delcypher]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1317

Why do we need to loop over multiple lines in this is a "oneliner"?

Is num_lines == 1 equivalent to is_oneliner?

If yes, then the is_oneliner parameter is not needed.
If no, then is_oneliner should probably be renamed to make sure this confusion cannot be made.

1331

Why are we erroring here? It looks like this is something that the old code supported. I would is expect is_oneliner to imply there are multiple lines in which case shouldn't we support that here?

[bulbazord]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1317

+1

1331

+1

[jingham]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1317

You can do:

(lldb) breakpoint command add -s python -o "first_python_line" -o "second_python_line"

which give you a "one-line" command with two lines.

[mib]

Rename is_oneliner flag to is_callback and swap the logic.

[delcypher]

@mib Thanks for working on this. LGTM (with very minor nits), but you should probably wait for someone who works on LLDB more frequently than me to give you the ok.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

1310–1340

Nit: the space between .update and (internal_dict) is probably an old mistake. We can probably tidy that up seeing as we're already touching the line.

1328

Nit: Might be worth leaving a comment here on why we're wrapping the code in a nested function.

[mib]

Address @delcypher comment.

[bulbazord]

LGTM thanks!

[JDevlieghere]

You're passing is_callback by value so the const is close to redundant. I think there's an "Effective C++" chapter dedicated to this. LLVM is pretty conservative in marking this as const and while LLDB uses it a bit more this isn't common at all. The current code now makes you wonder why this is different than has_extra_args. Even if that does get modified in some of the function's implementation, that's an implementation detail that shouldn't be exposed through the interface.

[kparzysz]

This broke Lua support:

In file included from /w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:9:
/w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h:95:68: error: non-virtual member function marked 'override' hides virtual member function
                                    const char *command_body_text) override;
                                                                   ^
/w/src/llvm.org/lldb/include/lldb/Interpreter/ScriptInterpreter.h:415:16: note: hidden overloaded virtual function 'lldb_private::ScriptInterpreter::SetWatchpointCommandCallback' declared here: different number of parameters (3 vs 2)
  virtual void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
               ^

[delcypher]

In D144688#4165144, @kparzysz wrote:

This broke Lua support:

In file included from /w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:9:
/w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h:95:68: error: non-virtual member function marked 'override' hides virtual member function
                                    const char *command_body_text) override;
                                                                   ^
/w/src/llvm.org/lldb/include/lldb/Interpreter/ScriptInterpreter.h:415:16: note: hidden overloaded virtual function 'lldb_private::ScriptInterpreter::SetWatchpointCommandCallback' declared here: different number of parameters (3 vs 2)
  virtual void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
               ^

@mib 👆

[mib]

In D144688#4165152, @delcypher wrote:

In D144688#4165144, @kparzysz wrote:

This broke Lua support:

In file included from /w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:9:
/w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h:95:68: error: non-virtual member function marked 'override' hides virtual member function
                                    const char *command_body_text) override;
                                                                   ^
/w/src/llvm.org/lldb/include/lldb/Interpreter/ScriptInterpreter.h:415:16: note: hidden overloaded virtual function 'lldb_private::ScriptInterpreter::SetWatchpointCommandCallback' declared here: different number of parameters (3 vs 2)
  virtual void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
               ^

@mib 👆

Yup, saw that. We don't have any bot that tests LUA so this went unnoticed ... I'm rebuilding lldb now with lua, will come up with a fix in a few :)

[mib]

@kparzysz I landed the fix in ab81fc29d9f7a8f8807293fc987f68f9ccf42259. Let me know if you still have some issues. Thanks!

[kparzysz]

Looks good. Thanks for the quick fix @mib!

lldb/include/lldb/Interpreter/ScriptInterpreter.h

public:
 
   virtual Status
   ExportFunctionDefinitionToInterpreter(StringList &function_def) {
     Status error;
     error.SetErrorString("not implemented");
     return error;
   }
 
-  virtual Status GenerateBreakpointCommandCallbackData(
-      StringList &input,
+  virtual Status GenerateBreakpointCommandCallbackData(StringList &input,
                                                        std::string &output,
-      bool has_extra_args) {
+                                                       bool has_extra_args,
+                                                       const bool is_callback) {
     Status error;
     error.SetErrorString("not implemented");
     return error;
   }
 
   virtual bool GenerateWatchpointCommandCallbackData(StringList &input,
-                                                     std::string &output) {
+                                                     std::string &output,
+                                                     const bool is_callback) {
     return false;
   }
 
   virtual bool GenerateTypeScriptFunction(const char *oneliner,
                                           std::string &output,
                                           const void *name_token = nullptr) {
     return false;
   }
public:
 
   virtual StructuredData::DictionarySP
   GetDynamicSettings(StructuredData::ObjectSP plugin_module_sp, Target *target,
                      const char *setting_name, lldb_private::Status &error) {
     return StructuredData::DictionarySP();
   }
 
   virtual Status GenerateFunction(const char *signature,
-                                  const StringList &input) {
+                                  const StringList &input,
+                                  const bool is_callback) {
     Status error;
     error.SetErrorString("unimplemented");
     return error;
   }
 
   virtual void CollectDataForBreakpointCommandCallback(
       std::vector<std::reference_wrapper<BreakpointOptions>> &options,
       CommandReturnObject &result);
 
   virtual void
   CollectDataForWatchpointCommandCallback(WatchpointOptions *wp_options,
                                           CommandReturnObject &result);
 
   /// Set the specified text as the callback for the breakpoint.
   Status SetBreakpointCommandCallback(
       std::vector<std::reference_wrapper<BreakpointOptions>> &bp_options_vec,
       const char *callback_text);
 
   virtual Status SetBreakpointCommandCallback(BreakpointOptions &bp_options,
-                                              const char *callback_text) {
+                                              const char *callback_text,
+                                              const bool is_callback) {
     Status error;
     error.SetErrorString("unimplemented");
     return error;
   }
 
   /// This one is for deserialization:
   virtual Status SetBreakpointCommandCallback(
       BreakpointOptions &bp_options,
SetBreakpointCommandCallbackFunction(BreakpointOptions &bp_options,
                                        StructuredData::ObjectSP extra_args_sp) {
     Status error;
     error.SetErrorString("unimplemented");
     return error;
   }
 
   /// Set a one-liner as the callback for the watchpoint.
   virtual void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
-                                            const char *oneliner) {}
+                                            const char *user_input,
+                                            const bool is_callback) {}
 
   virtual bool GetScriptedSummary(const char *function_name,
                                   lldb::ValueObjectSP valobj,
                                   StructuredData::ObjectSP &callee_wrapper_sp,
                                   const TypeSummaryOptions &options,
                                   std::string &retval) {
     return false;
   }

lldb/source/API/SBBreakpoint.cpp

SBError SBBreakpoint::SetScriptCallbackBody(const char *callback_body_text) {
   if (bkpt_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         bkpt_sp->GetTarget().GetAPIMutex());
     BreakpointOptions &bp_options = bkpt_sp->GetOptions();
     Status error =
         bkpt_sp->GetTarget()
             .GetDebugger()
             .GetScriptInterpreter()
-            ->SetBreakpointCommandCallback(bp_options, callback_body_text);
+            ->SetBreakpointCommandCallback(bp_options, callback_body_text,
+                                           /*is_callback=*/false);
     sb_error.SetError(error);
   } else
     sb_error.SetErrorString("invalid breakpoint");
 
   return sb_error;
 }
 
 bool SBBreakpoint::AddName(const char *new_name) {

lldb/source/API/SBBreakpointLocation.cpp

if (loc_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         loc_sp->GetTarget().GetAPIMutex());
     BreakpointOptions &bp_options = loc_sp->GetLocationOptions();
     Status error =
         loc_sp->GetBreakpoint()
             .GetTarget()
             .GetDebugger()
             .GetScriptInterpreter()
-            ->SetBreakpointCommandCallback(bp_options, callback_body_text);
+            ->SetBreakpointCommandCallback(bp_options, callback_body_text,
+                                           /*is_callback=*/false);
     sb_error.SetError(error);
   } else
     sb_error.SetErrorString("invalid breakpoint");
 
   return sb_error;
 }
 
 void SBBreakpointLocation::SetCommandLineCommands(SBStringList &commands) {

lldb/source/API/SBBreakpointName.cpp

SBBreakpointName::SetScriptCallbackBody(const char *callback_body_text) {
   BreakpointName *bp_name = GetBreakpointName();
   if (!bp_name)
     return sb_error;
 
   std::lock_guard<std::recursive_mutex> guard(
         m_impl_up->GetTarget()->GetAPIMutex());
 
   BreakpointOptions &bp_options = bp_name->GetOptions();
-  Status error =
-      m_impl_up->GetTarget()
+  Status error = m_impl_up->GetTarget()
                      ->GetDebugger()
                      .GetScriptInterpreter()
-          ->SetBreakpointCommandCallback(bp_options, callback_body_text);
+                     ->SetBreakpointCommandCallback(
+                         bp_options, callback_body_text, /*is_callback=*/false);
   sb_error.SetError(error);
   if (!sb_error.Fail())
     UpdateName(*bp_name);
 
   return sb_error;
 }
 
 bool SBBreakpointName::GetAllowList() const {

lldb/source/Commands/CommandObjectWatchpointCommand.cpp

for (size_t i = 0; i < count; ++i) {
         // to set or collect command callback.  Otherwise, call the methods
         // associated with this object.
         if (m_options.m_use_script_language) {
           ScriptInterpreter *script_interp = GetDebugger().GetScriptInterpreter(
               /*can_create=*/true, m_options.m_script_language);
           // Special handling for one-liner specified inline.
           if (m_options.m_use_one_liner) {
             script_interp->SetWatchpointCommandCallback(
-                wp_options, m_options.m_one_liner.c_str());
+                wp_options, m_options.m_one_liner.c_str(),
+                /*is_callback=*/false);
           }
           // Special handling for using a Python function by name instead of
           // extending the watchpoint callback data structures, we just
           // automatize what the user would do manually: make their watchpoint
           // command be a function call
           else if (!m_options.m_function_name.empty()) {
-            std::string oneliner(m_options.m_function_name);
-            oneliner += "(frame, wp, internal_dict)";
+            std::string function_signature = m_options.m_function_name;
+            function_signature += "(frame, wp, internal_dict)";
             script_interp->SetWatchpointCommandCallback(
-                wp_options, oneliner.c_str());
+                wp_options, function_signature.c_str(), /*is_callback=*/true);
           } else {
             script_interp->CollectDataForWatchpointCommandCallback(wp_options,
                                                                    result);
           }
         } else {
           // Special handling for one-liner specified inline.
           if (m_options.m_use_one_liner)
             SetWatchpointCommandCallback(wp_options,

lldb/source/Interpreter/ScriptInterpreter.cpp

ScriptInterpreter::StringToLanguage(const llvm::StringRef &language) {
   return eScriptLanguageUnknown;
 }
 
 Status ScriptInterpreter::SetBreakpointCommandCallback(
     std::vector<std::reference_wrapper<BreakpointOptions>> &bp_options_vec,
     const char *callback_text) {
   Status error;
   for (BreakpointOptions &bp_options : bp_options_vec) {
-    error = SetBreakpointCommandCallback(bp_options, callback_text);
+    error = SetBreakpointCommandCallback(bp_options, callback_text,
+                                         /*is_callback=*/false);
     if (!error.Success())
       break;
   }
   return error;
 }
 
 Status ScriptInterpreter::SetBreakpointCommandCallbackFunction(
     std::vector<std::reference_wrapper<BreakpointOptions>> &bp_options_vec,

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h

void CollectDataForBreakpointCommandCallback(
       std::vector<std::reference_wrapper<BreakpointOptions>> &bp_options_vec,
       CommandReturnObject &result) override;
 
   void
   CollectDataForWatchpointCommandCallback(WatchpointOptions *wp_options,
                                           CommandReturnObject &result) override;
 
   Status SetBreakpointCommandCallback(BreakpointOptions &bp_options,
-                                      const char *command_body_text) override;
+                                      const char *command_body_text,
+                                      const bool is_callback) override;
 
   void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
                                     const char *command_body_text) override;
 
   Status SetBreakpointCommandCallbackFunction(
       BreakpointOptions &bp_options, const char *function_name,
       StructuredData::ObjectSP extra_args_sp) override;
 

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

for (BreakpointOptions &bp_options : *bp_options_vec) {
       auto data_up = std::make_unique<CommandDataPython>();
       if (!data_up)
         break;
       data_up->user_source.SplitIntoLines(data);
 
       StructuredData::ObjectSP empty_args_sp;
       if (GenerateBreakpointCommandCallbackData(data_up->user_source,
                                                 data_up->script_source,
-                                                false)
+                                                /*has_extra_args=*/false,
+                                                /*is_callback=*/false)
               .Success()) {
         auto baton_sp = std::make_shared<BreakpointOptions::CommandBaton>(
             std::move(data_up));
         bp_options.SetCallback(
             ScriptInterpreterPythonImpl::BreakpointCallbackFunction, baton_sp);
       } else if (!batch_mode) {
         StreamFileSP error_sp = io_handler.GetErrorStreamFileSP();
         if (error_sp) {
           error_sp->Printf("Warning: No command attached to breakpoint.\n");
           error_sp->Flush();
         }
       }
     }
     m_active_io_handler = eIOHandlerNone;
   } break;
   case eIOHandlerWatchpoint: {
     WatchpointOptions *wp_options =
         (WatchpointOptions *)io_handler.GetUserData();
     auto data_up = std::make_unique<WatchpointOptions::CommandData>();
     data_up->user_source.SplitIntoLines(data);
 
     if (GenerateWatchpointCommandCallbackData(data_up->user_source,
-                                              data_up->script_source)) {
+                                              data_up->script_source,
+                                              /*is_callback=*/false)) {
       auto baton_sp =
           std::make_shared<WatchpointOptions::CommandBaton>(std::move(data_up));
       wp_options->SetCallback(
           ScriptInterpreterPythonImpl::WatchpointCallbackFunction, baton_sp);
     } else if (!batch_mode) {
       StreamFileSP error_sp = io_handler.GetErrorStreamFileSP();
       if (error_sp) {
         error_sp->Printf("Warning: No command attached to breakpoint.\n");
m_debugger.GetCommandInterpreter().GetPythonCommandsFromIOHandler(
       "    ", *this, wp_options);
 }
 
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallbackFunction(
     BreakpointOptions &bp_options, const char *function_name,
     StructuredData::ObjectSP extra_args_sp) {
   Status error;
   // For now just cons up a oneliner that calls the provided function.
-  std::string oneliner("return ");
-  oneliner += function_name;
+  std::string function_signature = function_name;
 
   llvm::Expected<unsigned> maybe_args =
       GetMaxPositionalArgumentsForCallable(function_name);
   if (!maybe_args) {
     error.SetErrorStringWithFormat(
         "could not get num args: %s",
         llvm::toString(maybe_args.takeError()).c_str());
     return error;
   }
   size_t max_args = *maybe_args;
 
   bool uses_extra_args = false;
   if (max_args >= 4) {
     uses_extra_args = true;
-    oneliner += "(frame, bp_loc, extra_args, internal_dict)";
+    function_signature += "(frame, bp_loc, extra_args, internal_dict)";
   } else if (max_args >= 3) {
     if (extra_args_sp) {
       error.SetErrorString("cannot pass extra_args to a three argument callback"
                           );
       return error;
     }
     uses_extra_args = false;
-    oneliner += "(frame, bp_loc, internal_dict)";
+    function_signature += "(frame, bp_loc, internal_dict)";
   } else {
     error.SetErrorStringWithFormat("expected 3 or 4 argument "
                                    "function, %s can only take %zu",
                                    function_name, max_args);
     return error;
   }
 
-  SetBreakpointCommandCallback(bp_options, oneliner.c_str(), extra_args_sp,
-                               uses_extra_args);
+  SetBreakpointCommandCallback(bp_options, function_signature.c_str(),
+                               extra_args_sp, uses_extra_args,
+                               /*is_callback=*/true);
   return error;
 }
 
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallback(
     BreakpointOptions &bp_options,
     std::unique_ptr<BreakpointOptions::CommandData> &cmd_data_up) {
   Status error;
   error = GenerateBreakpointCommandCallbackData(cmd_data_up->user_source,
                                                 cmd_data_up->script_source,
-                                                false);
+                                                /*has_extra_args=*/false,
+                                                /*is_callback=*/false);
   if (error.Fail()) {
     return error;
   }
   auto baton_sp =
       std::make_shared<BreakpointOptions::CommandBaton>(std::move(cmd_data_up));
   bp_options.SetCallback(
       ScriptInterpreterPythonImpl::BreakpointCallbackFunction, baton_sp);
   return error;
 }
 
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallback(
-    BreakpointOptions &bp_options, const char *command_body_text) {
-  return SetBreakpointCommandCallback(bp_options, command_body_text, {},false);
+    BreakpointOptions &bp_options, const char *command_body_text,
+    const bool is_callback) {
+  return SetBreakpointCommandCallback(bp_options, command_body_text, {},
+                                      /*uses_extra_args=*/false, is_callback);
 }
 
 // Set a Python one-liner as the callback for the breakpoint.
 Status ScriptInterpreterPythonImpl::SetBreakpointCommandCallback(
     BreakpointOptions &bp_options, const char *command_body_text,
-    StructuredData::ObjectSP extra_args_sp, bool uses_extra_args) {
+    StructuredData::ObjectSP extra_args_sp, bool uses_extra_args,
+    const bool is_callback) {
   auto data_up = std::make_unique<CommandDataPython>(extra_args_sp);
   // Split the command_body_text into lines, and pass that to
   // GenerateBreakpointCommandCallbackData.  That will wrap the body in an
   // auto-generated function, and return the function name in script_source.
   // That is what the callback will actually invoke.
 
   data_up->user_source.SplitIntoLines(command_body_text);
-  Status error = GenerateBreakpointCommandCallbackData(data_up->user_source,
-                                                       data_up->script_source,
-                                                       uses_extra_args);
+  Status error = GenerateBreakpointCommandCallbackData(
+      data_up->user_source, data_up->script_source, uses_extra_args,
+      is_callback);
   if (error.Success()) {
     auto baton_sp =
         std::make_shared<BreakpointOptions::CommandBaton>(std::move(data_up));
     bp_options.SetCallback(
         ScriptInterpreterPythonImpl::BreakpointCallbackFunction, baton_sp);
     return error;
   }
   return error;
 }
 
 // Set a Python one-liner as the callback for the watchpoint.
 void ScriptInterpreterPythonImpl::SetWatchpointCommandCallback(
-    WatchpointOptions *wp_options, const char *oneliner) {
+    WatchpointOptions *wp_options, const char *user_input,
+    const bool is_callback) {
   auto data_up = std::make_unique<WatchpointOptions::CommandData>();
 
   // It's necessary to set both user_source and script_source to the oneliner.
   // The former is used to generate callback description (as in watchpoint
   // command list) while the latter is used for Python to interpret during the
   // actual callback.
 
-  data_up->user_source.AppendString(oneliner);
-  data_up->script_source.assign(oneliner);
+  data_up->user_source.AppendString(user_input);
+  data_up->script_source.assign(user_input);
 
-  if (GenerateWatchpointCommandCallbackData(data_up->user_source,
-                                            data_up->script_source)) {
+  if (GenerateWatchpointCommandCallbackData(
+          data_up->user_source, data_up->script_source, is_callback)) {
     auto baton_sp =
         std::make_shared<WatchpointOptions::CommandBaton>(std::move(data_up));
     wp_options->SetCallback(
         ScriptInterpreterPythonImpl::WatchpointCallbackFunction, baton_sp);
   }
 }
 
 Status ScriptInterpreterPythonImpl::ExportFunctionDefinitionToInterpreter(
     StringList &function_def) {
   // Convert StringList to one long, newline delimited, const char *.
   std::string function_def_string(function_def.CopyList());
 
   Status error = ExecuteMultipleLines(
       function_def_string.c_str(),
       ExecuteScriptOptions().SetEnableIO(false));
   return error;
 }
 
 Status ScriptInterpreterPythonImpl::GenerateFunction(const char *signature,
-                                                     const StringList &input) {
+                                                     const StringList &input,
+                                                     const bool is_callback) {
   Status error;
   int num_lines = input.GetSize();
   if (num_lines == 0) {
     error.SetErrorString("No input data.");
     return error;
   }
 
   if (!signature || *signature == 0) {
     error.SetErrorString("No output function name.");
     return error;
   }
 
   StreamString sstr;
   StringList auto_generated_function;
   auto_generated_function.AppendString(signature);
   auto_generated_function.AppendString(
-      "     global_dict = globals()"); // Grab the global dictionary
+      "    global_dict = globals()"); // Grab the global dictionary
   auto_generated_function.AppendString(
-      "     new_keys = internal_dict.keys()"); // Make a list of keys in the
-                                               // session dict
+      "    new_keys = internal_dict.keys()"); // Make a list of keys in the
   auto_generated_function.AppendString(
-      "     old_keys = global_dict.keys()"); // Save list of keys in global dict
+      "    old_keys = global_dict.keys()"); // Save list of keys in global dict
   auto_generated_function.AppendString(
-      "     global_dict.update (internal_dict)"); // Add the session dictionary
-                                                  // to the
-  // global dictionary.
-
-  // Wrap everything up inside the function, increasing the indentation.
+      "    global_dict.update(internal_dict)"); // Add the session dictionary
+                                                // to the global dictionary.
 
-  auto_generated_function.AppendString("     if True:");
+  if (is_callback) {
+    // If the user input is a callback to a python function, make sure the input
+    // is only 1 line, otherwise appending the user input would break the
+    // generated wrapped function
+    if (num_lines == 1) {
+      sstr.Clear();
+      sstr.Printf("    __return_val = %s", input.GetStringAtIndex(0));
+      auto_generated_function.AppendString(sstr.GetData());
+    } else {
+      return Status("ScriptInterpreterPythonImpl::GenerateFunction(is_callback="
+                    "true) = ERROR: python function is multiline.");
+    }
+  } else {
+    auto_generated_function.AppendString(
+        "    __return_val = None"); // Initialize user callback return value.
+    auto_generated_function.AppendString(
+        "    def __user_code():"); // Create a nested function that will wrap
+                                   // the user input. This is necessary to
+                                   // capture the return value of the user input
+                                   // and prevent early returns.
     for (int i = 0; i < num_lines; ++i) {
       sstr.Clear();
-    sstr.Printf("       %s", input.GetStringAtIndex(i));
+      sstr.Printf("      %s", input.GetStringAtIndex(i));
       auto_generated_function.AppendString(sstr.GetData());
     }
     auto_generated_function.AppendString(
-      "     for key in new_keys:"); // Iterate over all the keys from session
-                                    // dict
+        "    __return_val = __user_code()"); //  Call user code and capture
+                                             //  return value
+  }
+  auto_generated_function.AppendString(
+      "    for key in new_keys:"); // Iterate over all the keys from session
   auto_generated_function.AppendString(
-      "         internal_dict[key] = global_dict[key]"); // Update session dict
-                                                         // values
+      "        internal_dict[key] = global_dict[key]"); // Update session dict
   auto_generated_function.AppendString(
-      "         if key not in old_keys:"); // If key was not originally in
-                                           // global dict
+      "        if key not in old_keys:"); // If key was not originally in
   auto_generated_function.AppendString(
-      "             del global_dict[key]"); //  ...then remove key/value from
-                                            //  global dict
+      "            del global_dict[key]"); //  ...then remove key/value from
+  auto_generated_function.AppendString(
+      "    return __return_val"); //  Return the user callback return value.
 
   // Verify that the results are valid Python.
-
   error = ExportFunctionDefinitionToInterpreter(auto_generated_function);
 
   return error;
 }
 
 bool ScriptInterpreterPythonImpl::GenerateTypeScriptFunction(
     StringList &user_input, std::string &output, const void *name_token) {
   static uint32_t num_created_functions = 0;
   user_input.RemoveBlankLines();
   StreamString sstr;
 
   // Check to see if we have any data; if not, just return.
   if (user_input.GetSize() == 0)
     return false;
 
   // Take what the user wrote, wrap it all up inside one big auto-generated
   // Python function, passing in the ValueObject as parameter to the function.
 
   std::string auto_generated_function_name(
       GenerateUniqueName("lldb_autogen_python_type_print_func",
                          num_created_functions, name_token));
   sstr.Printf("def %s (valobj, internal_dict):",
               auto_generated_function_name.c_str());
 
-  if (!GenerateFunction(sstr.GetData(), user_input).Success())
+  if (!GenerateFunction(sstr.GetData(), user_input, /*is_callback=*/false)
+           .Success())
     return false;
 
   // Store the name of the auto-generated function to be called.
   output.assign(auto_generated_function_name);
   return true;
 }
 
 bool ScriptInterpreterPythonImpl::GenerateScriptAliasFunction(
     StringList &user_input, std::string &output) {
   static uint32_t num_created_functions = 0;
   user_input.RemoveBlankLines();
   StreamString sstr;
 
   // Check to see if we have any data; if not, just return.
   if (user_input.GetSize() == 0)
     return false;
 
   std::string auto_generated_function_name(GenerateUniqueName(
       "lldb_autogen_python_cmd_alias_func", num_created_functions));
 
   sstr.Printf("def %s (debugger, args, exe_ctx, result, internal_dict):",
               auto_generated_function_name.c_str());
 
-  if (!GenerateFunction(sstr.GetData(), user_input).Success())
+  if (!GenerateFunction(sstr.GetData(), user_input, /*is_callback=*/true)
+           .Success())
     return false;
 
   // Store the name of the auto-generated function to be called.
   output.assign(auto_generated_function_name);
   return true;
 }
 
 bool ScriptInterpreterPythonImpl::GenerateTypeSynthClass(

 bool ScriptInterpreterPythonImpl::GenerateTypeSynthClass(
     const char *oneliner, std::string &output, const void *name_token) {
   StringList input;
   input.SplitIntoLines(oneliner, strlen(oneliner));
   return GenerateTypeSynthClass(input, output, name_token);
 }
 
 Status ScriptInterpreterPythonImpl::GenerateBreakpointCommandCallbackData(
-    StringList &user_input, std::string &output,
-    bool has_extra_args) {
+    StringList &user_input, std::string &output, bool has_extra_args,
+    const bool is_callback) {
   static uint32_t num_created_functions = 0;
   user_input.RemoveBlankLines();
   StreamString sstr;
   Status error;
   if (user_input.GetSize() == 0) {
     error.SetErrorString("No input data.");
     return error;
   }
 
   std::string auto_generated_function_name(GenerateUniqueName(
       "lldb_autogen_python_bp_callback_func_", num_created_functions));
   if (has_extra_args)
     sstr.Printf("def %s (frame, bp_loc, extra_args, internal_dict):",
                 auto_generated_function_name.c_str());
   else
     sstr.Printf("def %s (frame, bp_loc, internal_dict):",
                 auto_generated_function_name.c_str());
 
-  error = GenerateFunction(sstr.GetData(), user_input);
+  error = GenerateFunction(sstr.GetData(), user_input, is_callback);
   if (!error.Success())
     return error;
 
   // Store the name of the auto-generated function to be called.
   output.assign(auto_generated_function_name);
   return error;
 }
 
 bool ScriptInterpreterPythonImpl::GenerateWatchpointCommandCallbackData(
-    StringList &user_input, std::string &output) {
+    StringList &user_input, std::string &output, const bool is_callback) {
   static uint32_t num_created_functions = 0;
   user_input.RemoveBlankLines();
   StreamString sstr;
 
   if (user_input.GetSize() == 0)
     return false;
 
   std::string auto_generated_function_name(GenerateUniqueName(
       "lldb_autogen_python_wp_callback_func_", num_created_functions));
   sstr.Printf("def %s (frame, wp, internal_dict):",
               auto_generated_function_name.c_str());
 
-  if (!GenerateFunction(sstr.GetData(), user_input).Success())
+  if (!GenerateFunction(sstr.GetData(), user_input, is_callback).Success())
     return false;
 
   // Store the name of the auto-generated function to be called.
   output.assign(auto_generated_function_name);
   return true;
 }
 
 bool ScriptInterpreterPythonImpl::GetScriptedSummary(

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

RunScriptBasedCommand(const char *impl_function, llvm::StringRef args,
                         const lldb_private::ExecutionContext &exe_ctx) override;
 
   bool RunScriptBasedCommand(
       StructuredData::GenericSP impl_obj_sp, llvm::StringRef args,
       ScriptedCommandSynchronicity synchronicity,
       lldb_private::CommandReturnObject &cmd_retobj, Status &error,
       const lldb_private::ExecutionContext &exe_ctx) override;
 
-  Status GenerateFunction(const char *signature,
-                          const StringList &input) override;
+  Status GenerateFunction(const char *signature, const StringList &input,
+                          const bool is_callback) override;
 
-  Status GenerateBreakpointCommandCallbackData(
-      StringList &input,
+  Status GenerateBreakpointCommandCallbackData(StringList &input,
                                                std::string &output,
-      bool has_extra_args) override;
+                                               bool has_extra_args,
+                                               const bool is_callback) override;
 
   bool GenerateWatchpointCommandCallbackData(StringList &input,
-                                             std::string &output) override;
+                                             std::string &output,
+                                             const bool is_callback) override;
 
   bool GetScriptedSummary(const char *function_name, lldb::ValueObjectSP valobj,
                           StructuredData::ObjectSP &callee_wrapper_sp,
                           const TypeSummaryOptions &options,
                           std::string &retval) override;
 
   bool FormatterCallbackFunction(const char *function_name,
                                  lldb::TypeImplSP type_impl_sp) override;
void CollectDataForBreakpointCommandCallback(
       CommandReturnObject &result) override;
 
   void
   CollectDataForWatchpointCommandCallback(WatchpointOptions *wp_options,
                                           CommandReturnObject &result) override;
 
   /// Set the callback body text into the callback for the breakpoint.
   Status SetBreakpointCommandCallback(BreakpointOptions &bp_options,
-                                      const char *callback_body) override;
+                                      const char *callback_body,
+                                      const bool is_callback) override;
 
   Status SetBreakpointCommandCallbackFunction(
       BreakpointOptions &bp_options, const char *function_name,
       StructuredData::ObjectSP extra_args_sp) override;
 
   /// This one is for deserialization:
   Status SetBreakpointCommandCallback(
       BreakpointOptions &bp_options,
       std::unique_ptr<BreakpointOptions::CommandData> &data_up) override;
 
   Status SetBreakpointCommandCallback(BreakpointOptions &bp_options,
                                       const char *command_body_text,
                                       StructuredData::ObjectSP extra_args_sp,
-                                      bool uses_extra_args);
+                                      bool uses_extra_args,
+                                      const bool is_callback);
 
   /// Set a one-liner as the callback for the watchpoint.
   void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
-                                    const char *oneliner) override;
+                                    const char *user_input,
+                                    const bool is_callback) override;
 
   const char *GetDictionaryName() { return m_dictionary_name.c_str(); }
 
   PyThreadState *GetThreadState() { return m_command_thread_state; }
 
   void SetThreadState(PyThreadState *s) {
     if (s)
       m_command_thread_state = s;

lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py

-"""
+"""
 Test 'watchpoint command'.
 """
 
 
 
 import os
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class WatchpointPythonCommandTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Our simple source filename.
         self.source = 'main.cpp'
         # Find the line number to break inside main().
         self.line = line_number(
             self.source, '// Set break point at this line.')
+        self.second_line = line_number(
+            self.source, '// Set another breakpoint here.')
         # And the watchpoint variable declaration line number.
         self.decl = line_number(self.source,
                                 '// Watchpoint variable declaration.')
         # Build dictionary to have unique executable names for each test
         # method.
         self.exe_name = self.testMethodName
         self.d = {'CXX_SOURCES': self.source, 'EXE': self.exe_name}
 
def test_continue_in_watchpoint_command(self):
 
         self.runCmd("process continue")
 
         # We should be stopped again due to the watchpoint (write type).
         # The stop reason of the thread should be watchpoint.
         self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
                     substrs=['stop reason = watchpoint'])
 
+        # We should have hit the watchpoint once, set cookie to 888, since the
+        # user callback returned True.
+        self.expect("frame variable --show-globals cookie",
+                    substrs=['(int32_t)', 'cookie = 888'])
+
+        self.runCmd("process continue")
+
+        # We should be stopped again due to the watchpoint (write type).
+        # The stop reason of the thread should be watchpoint.
+        self.expect("thread backtrace", STOPPED_DUE_TO_WATCHPOINT,
+                    substrs=['stop reason = watchpoint'])
+
+        # We should have hit the watchpoint a second time, set cookie to 666,
+        # even if the user callback didn't return anything and then continue.
+        self.expect("frame variable --show-globals cookie",
+                    substrs=['(int32_t)', 'cookie = 666'])
+
+        # Add a breakpoint to set a watchpoint when stopped on the breakpoint.
+        lldbutil.run_break_set_by_file_and_line(
+            self, None, self.second_line, num_expected_locations=1)
+
+        self.runCmd("process continue")
+
+        self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stop reason = breakpoint'])
+
         # We should have hit the watchpoint once, set cookie to 888, then continued to the
         # second hit and set it to 999
         self.expect("frame variable --show-globals cookie",
                     substrs=['(int32_t)', 'cookie = 999'])
 

lldb/test/API/commands/watchpoints/watchpoint_commands/command/main.cpp

 int main(int argc, char** argv) {
     int local = 0;
     printf("&global=%p\n", &global);
     printf("about to write to 'global'...\n"); // Set break point at this line.
     for (int i = 0; i < 10; ++i)
         modify(global);
 
     printf("global=%d\n", global);
-    printf("cookie=%d\n", cookie);
+    printf("cookie=%d\n", cookie); // Set another breakpoint here.
 }

lldb/test/API/commands/watchpoints/watchpoint_commands/command/watchpoint_command.py

 import lldb
 
 num_hits = 0
 
 
 def watchpoint_command(frame, wp, dict):
     global num_hits
     if num_hits == 0:
         print ("I stopped the first time")
         frame.EvaluateExpression("cookie = 888")
         num_hits += 1
-        frame.thread.process.Continue()
+        return True
+    if num_hits == 1:
+        print ("I stopped the second time, but with no return")
+        frame.EvaluateExpression("cookie = 666")
+        num_hits += 1
     else:
         print ("I stopped the %d time" % (num_hits))
         frame.EvaluateExpression("cookie = 999")
+        return False # This cause the process to continue.