bulbazord accepted this revision.
bulbazord added a comment.
This revision is now accepted and ready to land.
I'm alright with this patch since it at least makes watchpoint commands
consistent with breakpoint commands (w.r.t the `-F` flag).
================
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;
----------------
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.
================
Comment at:
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py:1
-"""
+"""
Test 'watchpoint command'.
----------------
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144688/new/
https://reviews.llvm.org/D144688
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
