Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: remake-4-3
Choose a base branch
from

Conversation

singalen
Copy link

… 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
Copy link
Collaborator

rocky commented May 28, 2020

Thanks. I'll be trying it out soon.

@singalen
Copy link
Author

So what do you think?

Copy link
Collaborator

@rocky rocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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"?
Copy link
Collaborator

@rocky rocky Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@singalen singalen Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@singalen singalen Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Collaborator

@rocky rocky Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@singalen singalen Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@rocky rocky Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@singalen singalen Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rocky marked this pull request as draft June 11, 2020 10:38
*
*/
#define watch_HELP_TEXT \
"Add a "command watch" breakpoint that triggers when a command executed matches the given regex.\n" \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

rocky commented Jun 12, 2020

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
Copy link
Author

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

@singalen
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

rocky commented Jun 14, 2020

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

…r when a command executed matches a particular regex.
Copy link
Collaborator

@rocky rocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rocky force-pushed the remake-4-3 branch 5 times, most recently from 269a076 to 3048790 Compare January 22, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants