WIP: #107: Debugger "watchpoints": breakpoints that trigger when a command… #108

singalen · 2020-05-28T19:00:58Z

… executed matches a particular regex.

Got a permission from my company to contribute.

Here's what worked for me. It's rather an invitation to review, there are some things to clean up here.

rocky · 2020-05-28T19:48:38Z

Thanks. I'll be trying it out soon.

singalen · 2020-06-11T06:15:16Z

So what do you think?

rocky

First, thanks for undertaking to do this and the incredible amount of work put in so far.

There are hopefully a small number of additional items that I think would greatly improve useablility.

Also, please add info watch to doc/readthedocs/debugger/commands/info.rst and a doc/readthedocs/debugger/commands/info./watch.rst so that this appears in the readthedocs documentation.

Thanks!

rocky · 2020-06-11T09:34:20Z

libdebugger/command/help/watch.h

+ *
+ */
+#define watch_HELP_TEXT							\
+  "Add a "command watch" breakpoint that triggers when a command executed matches the given regex.\n"			\


I needed to escape the embedded quotes to get this to compile. E.g.

index c6d6de55..5c6d506d 100644 --- a/libdebugger/command/help/watch.h +++ b/libdebugger/command/help/watch.h @@ -4,5 +4,5 @@ * */ #define watch_HELP_TEXT \ - "Add a "command watch" breakpoint that triggers when a command executed matches the given regex.\n" \ + "Add a \"command watch\" breakpoint that triggers when a command executed matches the given regex.\n" \ "An argument is the regex."

Also: "command executed" -> "is about to be executed" since this happens before the command gets run.

rocky · 2020-06-11T09:54:08Z

libdebugger/cmd.c

@@ -274,6 +276,7 @@ debug_return_t enter_debugger (target_stack_node_t *p,
      case DEBUG_BRKPT_AFTER_CMD:
      case DEBUG_BRKPT_BEFORE_PREREQ:
      case DEBUG_BRKPT_AFTER_PREREQ:
+      case DEBUG_WATCHPOINT: // FIXME: I have about no idea what I'm doing here. Should this reason be "cleared"?


gdb doesn't clear watchpoints.

I've been following how gdb works unless there is good reason not to. There are a couple of reasons for doing this: first we draw on the experience that has been drawn by gdb. If the command use is analogous to something that happens in gdb then gdb's behavior is likely to be the one that is natural. Otherwise one presumes it would have been changed. And even if not, that's what people have come to expect....

Which leads into the second benefit: knowledge of gdb will help here and vice versa. I have a limited brain so remembering how this is is different from that is more effort than remembing one thing accross all debuggers I use.

I believe right now watchpoints are cleared and I suspect they shouldn't be.

Your thoughts?

I completely agree to your points. I guess you assumed that I understood the code better than I actually did :D
So this code clears a one-shot, internal breakpoint? Then this line surely should not be here, I'll delete it.

rocky · 2020-06-11T09:57:45Z

libdebugger/command/watch.c

+
+  added = add_command_breakpoint(psz_regex);
+  if (added) {
+    printf("Added command watchpoint: '%s'\n", psz_regex);


gdb lists the number of the breakpoint or watchpoint that gets added. This is helpful so that it can be used if you want to delete it. (See other comment about whether a watchpoint shoild be deleted).

It might be helpful to change added into an int, the breakpoint number for such purpose.

I think I'll try to match the logic of add_breakpoint() and print the messages inside the function. I also need to add the _() and the translation string... It takes time to do things properly :D
Which will make me ignore the return value of add_command_watchpoint() in dbg_cmd_watch(). It's a bad habit, but in this case all the error handling has already happened.

rocky · 2020-06-11T10:16:29Z

libdebugger/break.c

+    if (p->type == COMMAND_WATCH) {
+      int match_status = regexec(&p->regex, psz_expanded_command, (size_t) 0, NULL, 0);
+      if (match_status == 0) {
+        printf ("Command matched a watchpoint for '%s': %s\n", p->psz_text, psz_expanded_command);


Following gdb, it might be nice to add which breakpoint number matched.

Also it would be nice to give some idea of where the breakpoint matched. To show how this is desireable here is what I tried and what I got.

$ ./make -X remake<0> watch test remake<1> c Updating goal targets... File 'all' does not exist. File 'all-recursive' does not exist. Must remake target 'all-recursive'. Makefile:1403: target 'all-recursive' does not exist ... ##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Command matched a watchpoint for 'test': @fail=; \ if (target_option=k; case ${target_option-} in ?) ;; *) echo "am__make_running_with_option: internal error: invalid" "target option '${target_option-}' specified" >&2; exit 1;; esac; has_opt=no; sane_makeflags=$MAKEFLAGS; if { if test -z '0'; then false; elif test -n 'x86_64-pc-linux-gnu'; then true; elif test -n '4.3' && test -n '/tmp/remake'; then true; else false; fi; }; then sane_makeflags=$MFLAGS; else case $MAKEFLAGS in *\\[\ \ ]*) bs=\\; sane_makeflags=`printf '%s\n' "$MAKEFLAGS" | sed "s/$bs$bs[$bs $bs ]*//g"`;; esac; fi; skip_next=no; strip_trailopt () { flg=`printf '%s\n' "$flg" | sed "s/$1.*$//"`; }; for flg in $sane_makeflags; do test $skip_next = yes && { skip_next=no; continue; }; case $flg in *=*|--*) continue;; -*I) strip_trailopt 'I'; skip_next=yes;; -*I?*) strip_trailopt 'I';; -*O) strip_trailopt 'O'; skip_next=yes;; -*O?*) strip_trailopt 'O';; -*l) strip_trailopt 'l'; skip_next=yes;; -*l?*) strip_trailopt 'l';; -[dEDm]) skip_next=yes;; -[JT]) skip_next=yes;; esac; case $flg in *$target_option*) has_opt=yes; break;; esac; done; test $has_opt = yes); then \ failcom='fail=yes'; \ else \ failcom='exit 1'; \ fi; \ dot_seen=no; \ target=`echo all-recursive | sed s/-recursive//`; \ case "all-recursive" in \ distclean-* | maintainer-clean-*) list='lib po doc glob unittest libdebugger' ;; \ *) list='lib po doc glob unittest libdebugger' ;; \ esac; \ for subdir in $list; do \ echo "Making $target in $subdir"; \ if test "$subdir" = "."; then \ dot_seen=yes; \ local_target="$target-am"; \ else \ local_target="$target"; \ fi; \ (CDPATH="${ZSH_VERSION+.}:" && cd $subdir && /tmp/remake/./make $local_target) \ || eval $failcom; \ done; \ if test "$dot_seen" = "no"; then \ /tmp/remake/./make "$target-am" || exit 1; \ fi; test -z "$fail" wp (/tmp/remake/Makefile:1402) all-recursive remake<2>

Minimal would be to at least show the offset where the match occurs which available in regexc.

Better (and what I do in analogous situtations in other debuggers I've written) is to show the line containing the match and another line with an ^ to indicate the starting position and match length). For example. Here it would be:

if (target_option=k; case ${target_option-} in ?) ;; *) echo "am__make_running_with_option: internal error: invalid" "target option '${target_option-}' specified" >&2; exit 1;; esac; has_opt=no; sane_makeflags=$MAKEFLAGS; if { if test -z '0'; then false; elif test -n 'x86_64- ^^^^^^

(Except it was a bit of a hassle for me to manually to add the ^^^^ in this format.)

Also, after a breakpoint or watchpoint is hit, the number of times it has hit is updated in info break

Time to repay karmic debts. When, if not on vacation? Brushing up the PR now.
Let me do it...

Warning - I recently broke the 4.3 branch in trying to extend the debugger to allow "info tasks ".

That was part of larger scheme to allow remake --tasks <target-name> so it would just show the comment for <target-name>.

If it becomes a problem, I can revert that change and put it in a branch.

That's tricky! There are multi-line commands, and there are multi-line matches.
What if I only underline the first line of a match?

I added (start position:end position) and an underline that works for a multiline command.
Took me some time, and still there's a room for improvement:

it doesn't fully underline a multi-line match;

it should better print the line number of a command matched, if the command is multiline, like in the example.
But I believe it's already usable. Do you think it's enough to be committed?

rocky · 2020-06-11T10:40:20Z

libdebugger/command/help/watch.h

+ *
+ */
+#define watch_HELP_TEXT							\
+  "Add a "command watch" breakpoint that triggers when a command executed matches the given regex.\n"			\


Also: "command executed" -> "is about to be executed" since this happens before the command gets run.

rocky · 2020-06-12T09:20:02Z

I've been thinking about this some more.

The times when I have wanted something liike this has been for matching against specfic targets because there are patterns in the make file like:

.c.o:

but I want to stop before a particular target gets built. Ditto for a dependency. One way this could be added would be to change the watch command to add a parameter like @, <, and c for target, depedency and command.

However unless you are up for it, so as not to muddy the waters here, I think it best to wait until this is done.

What do you think?

singalen · 2020-06-14T05:49:51Z

Thanks for a kind words! Going through the review now.

singalen · 2020-06-14T06:32:45Z

Are you sure the documentation should go into info/watch.rst?
I didn't add info watch command, it is a standalone watch command, related to break.
Maybe I should add breakpoints/watch.rst instead?

singalen · 2020-06-14T06:53:10Z

Speaking of matching the targets and others, that sounds like a useful development of the idea.
I'll see if I'm up to it; anyway, I would suggest doing it incrementally, in another PR.

rocky · 2020-06-14T08:58:27Z

Are you sure the documentation should go into info/watch.rst?
I didn't add info watch command, it is a standalone watch command, related to break.
Maybe I should add breakpoints/watch.rst instead?

Yes, you are correct.

(gdb) help breakpoints
...
thbreak -- Set a temporary hardware assisted breakpoint
trace -- Set a tracepoint at specified location
watch -- Set a watchpoint for an expression

…mmand executed matches a particular regex.

rocky

Overall looks good. I see little problems listed below.

Note: I'll be busy the reset of the week, so any further comments will have to wait until the next weekend.

Problem 1 - "Wrong __data_start/_end pair"

The most serious was this the tests failing after setting a watchpoint and continuing.

In remake/tests/work/functions/guile.log.1 I see

Wrong __data_start/_end pair

To reproduce this:

$ ./make -X check
Reading makefiles...
Updating makefiles...
-> (/tmp/remake/Makefile:1310)
src/.deps/vpath.Po: 
remake<0> watch check
Watchpoint 1 for regex `check' added
remake<1> c
Updating goal targets...
 File 'check' does not exist.
...
##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
Command matched a watchpoint 1 at (1188:1193) 'check':
...
target=`echo check-recursive | sed s/-recursive//`; \
             ^^^^^
...
wp (/tmp/remake/Makefile:1402)
check-recursive

So far so good.

remake<2> c
...
functions/guile ......................................... Error running /tmp/remake/tests/../make (expected 0; got 134): '/tmp/remake/tests/../make' '-f' 'work/functions/guile.mk'

Caught signal 6!
Error running /tmp/remake/tests/../make (expected 0; got 134): '/tmp/remake/tests/../make' '-f' 'work/functions/guile.mk.1'

And the log has what is shown above.

Problem 2 - watch not linked in toc tree

$ cd doc/readthedocs/
$ make
Running Sphinx v3.2.0
making output directory... done
building [mo]: targets for 0 po files that are out of date
building [html]: targets for 70 source files that are out of date
updating environment: [new config] 70 added, 0 changed, 0 removed
reading sources... [100%] manpage                                               
looking for now-outdated files... none found
pickling environment... done
checking consistency... /tmp/remake/doc/readthedocs/debugger/commands/breakpoints/watch.rst: WARNING: document isn't included in any toctree
done

Problem 3 help text not fully correct

$ ./make -X
Reading makefiles...
Updating makefiles...
-> (/tmp/remake/Makefile:1310)
src/.deps/vpath.Po: 
remake<0> help
  Command                  Short Name  Aliases
...

  up [*amount*]                   (u)
  watch                           (W)
  write [*target* [*filename*]]   (w)

watch should mention the regex parameter. Which leads to:

Problem 4 - passing a null string doesn't warn of error or that nothing is done

$ ./make -X
Reading makefiles...
Updating makefiles...
-> (/tmp/remake/Makefile:1310)
src/.deps/vpath.Po: 
remake<0> watch
remake<1>

If I continue things proceed as normal. Shouldn't there be a warning or error if nothing was done (which appears to be the case)?

rocky requested changes Jun 11, 2020

View reviewed changes

rocky marked this pull request as draft June 11, 2020 10:38

rocky requested changes Jun 11, 2020

View reviewed changes

rocky marked this pull request as ready for review June 11, 2020 10:40

singalen force-pushed the remake-4-3-watchpoints branch from e83a126 to a874697 Compare August 10, 2020 04:20

rocky#107: Debugger "watchpoints": breakpoints that trigger when a co…

49ab165

…mmand executed matches a particular regex.

singalen force-pushed the remake-4-3-watchpoints branch from a874697 to 49ab165 Compare August 10, 2020 04:25

singalen requested a review from rocky August 10, 2020 04:42

rocky reviewed Aug 10, 2020

View reviewed changes

rocky force-pushed the remake-4-3 branch 5 times, most recently from 269a076 to 3048790 Compare January 22, 2022 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: #107: Debugger "watchpoints": breakpoints that trigger when a command… #108

WIP: #107: Debugger "watchpoints": breakpoints that trigger when a command… #108

singalen commented May 28, 2020

rocky commented May 28, 2020

singalen commented Jun 11, 2020

rocky left a comment

rocky Jun 11, 2020

rocky Jun 11, 2020

rocky Jun 11, 2020 •

edited

singalen Jun 14, 2020 •

edited

rocky Jun 11, 2020

singalen Jun 14, 2020 •

edited

rocky Jun 11, 2020 •

edited

rocky Jun 11, 2020

singalen Aug 8, 2020 •

edited

rocky Aug 8, 2020 •

edited

singalen Aug 8, 2020

singalen Aug 10, 2020 •

edited

rocky Jun 11, 2020

rocky commented Jun 12, 2020 •

edited

singalen commented Jun 14, 2020

singalen commented Jun 14, 2020

singalen commented Jun 14, 2020

rocky commented Jun 14, 2020

rocky left a comment

WIP: #107: Debugger "watchpoints": breakpoints that trigger when a command… #108

Are you sure you want to change the base?

WIP: #107: Debugger "watchpoints": breakpoints that trigger when a command… #108

Conversation

singalen commented May 28, 2020

rocky commented May 28, 2020

singalen commented Jun 11, 2020

rocky left a comment

Choose a reason for hiding this comment

rocky Jun 11, 2020

Choose a reason for hiding this comment

rocky Jun 11, 2020

Choose a reason for hiding this comment

rocky Jun 11, 2020 • edited

Choose a reason for hiding this comment

singalen Jun 14, 2020 • edited

Choose a reason for hiding this comment

rocky Jun 11, 2020

Choose a reason for hiding this comment

singalen Jun 14, 2020 • edited

Choose a reason for hiding this comment

rocky Jun 11, 2020 • edited

Choose a reason for hiding this comment

rocky Jun 11, 2020

Choose a reason for hiding this comment

singalen Aug 8, 2020 • edited

Choose a reason for hiding this comment

rocky Aug 8, 2020 • edited

Choose a reason for hiding this comment

singalen Aug 8, 2020

Choose a reason for hiding this comment

singalen Aug 10, 2020 • edited

Choose a reason for hiding this comment

rocky Jun 11, 2020

Choose a reason for hiding this comment

rocky commented Jun 12, 2020 • edited

singalen commented Jun 14, 2020

singalen commented Jun 14, 2020

singalen commented Jun 14, 2020

rocky commented Jun 14, 2020

rocky left a comment

Choose a reason for hiding this comment

Problem 1 - "Wrong __data_start/_end pair"

Problem 2 - watch not linked in toc tree

Problem 3 help text not fully correct

Problem 4 - passing a null string doesn't warn of error or that nothing is done

rocky Jun 11, 2020 •

edited

singalen Jun 14, 2020 •

edited

singalen Jun 14, 2020 •

edited

rocky Jun 11, 2020 •

edited

singalen Aug 8, 2020 •

edited

rocky Aug 8, 2020 •

edited

singalen Aug 10, 2020 •

edited

rocky commented Jun 12, 2020 •

edited