[Lldb-commits] [PATCH] D144688: [lldb] Fix watchpoint command function stopping behavior

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectWatchpointCommand.cpp:425
           else if (!m_options.m_function_name.empty()) {
-            std::string oneliner(m_options.m_function_name);
+            std::string oneliner("return ");
+            oneliner += m_options.m_function_name;
----------------
bulbazord wrote:
> mib wrote:
> > delcypher wrote:
> > > @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.
> > > 
> > > ```lang=python
> > > 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.
> > 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.
> 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.
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144688/new/

https://reviews.llvm.org/D144688