mib added a subscriber: jingham.
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;
----------------
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.
================
Comment at:
lldb/test/API/commands/watchpoints/watchpoint_commands/command/TestWatchpointCommandPython.py:159
+
+ self.expect("thread backtrace", STOPPED_DUE_TO_BREAKPOINT,
+ substrs=['stop reason = breakpoint'])
----------------
delcypher wrote:
> This `self.expect()` fails when the bug hasn't been fixed, correct?
Yup
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
