[m-rev.] for review: conditional breakpoints

Ian MacLarty maclarty at cs.mu.OZ.AU
Mon Jan 31 17:58:48 AEDT 2005


On Mon, Jan 31, 2005 at 12:34:00PM +1100, Zoltan Somogyi wrote:
> This is for both branches.

I think you should justify why this *new* feature should go into the release
branch in the log message.  Also I think if it's to go into the release then
the test case should be more thorough (I have made some suggestions for more
things to test in one of my comments below).

> browser/cterm.m:
> 	This new module tests whether a value in the program being debugged
> 	matches a term represented by the data structure defiend in

s/defiend/defined/

> trace/mercury_trace_vars.c:
> 	Change the code that checks for breakpoints to also evaluate the
> 	condition, if any.
> 
> 	Provide the facilities required to implement conditions. Besides
> 	exporting some previously private functions, this involved breaking up
> 	two existing functions into two pieces each, because condition checking
> 	wanted to reuse only parts of them.
> 
> 	Modify the implementation of the functions manipulating breakpoints
> 	to handle the new parts of spy point structures.
> 
> 	Modify the way we delete spy point structures to make doubly sure
> 	that we don't free memory twice; it is now MR_delete_spy_point that
> 	sets te spy_exists field to FALSE, after checking it.
> 

s/te/the/

> Index: browser/cterm.m
> ===================================================================
> @@ -0,0 +1,138 @@
> +%---------------------------------------------------------------------------%
> +% Copyright (C) 2005 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU Library General
> +% Public License - see the file COPYING.LIB in the Mercury distribution.
> +%---------------------------------------------------------------------------%

I think you should put the main author's name and a short description of the
purpose of the module here.

> +:- pragma export(match_with_cterm(in, in, out), "ML_BROWSE_match_with_cterm").
> +
> +% Uncomment these and the unsafe_perform_ios below to debug match_with_cterm
> +% and its callers in the trace directory.
> +% :- import_module io, unsafe.
> +% :- pragma promise_pure(match_with_cterm/3).
> +
> +match_with_cterm(Term, CTerm, Match) :-
> +	deconstruct(Term, include_details_cc, TermFunctor, _, TermArgs),
> +	cterm_deconstruct(CTerm, CTermFunctor, CTermArgs),
> +	% impure unsafe_perform_io(io__write_string("comparing <")),
> +	% impure unsafe_perform_io(io__write_string(TermFunctor)),
> +	% impure unsafe_perform_io(io__write_string("> to <")),
> +	% impure unsafe_perform_io(io__write_string(CTermFunctor)),
> +	% impure unsafe_perform_io(io__write_string(">\n")),
> +	( TermFunctor = CTermFunctor ->
> +		match_with_cterms(TermArgs, CTermArgs, Match)
> +	; CTermFunctor = "_" ->

Would it not be quicker to do this test first?

> +		Match = yes
> +	;
> +		Match = no
> +	).
> +
...
> +:- pragma foreign_proc(c, 
> +	cterm_deconstruct(Term::in, Functor::out, Args::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	if (Term == NULL) {
> +		MR_fatal_error(""cterm_deconstruct: NULL term"");
> +	}
> +
> +	Functor = Term->term_functor;
> +	Args = Term->term_args;
> +").
> +
> +:- pragma foreign_proc(c, 
> +	cterm_head_tail(Args::in, Head::out, Tail::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	if (Args == NULL) {
> +		SUCCESS_INDICATOR = MR_FALSE;
> +	} else {
> +		Head = Args->args_head;
> +		Tail = Args->args_tail;
> +		SUCCESS_INDICATOR = MR_TRUE;
> +	}
> +").
> +
> +:- pragma foreign_proc(c, 
> +	cterm_deconstruct(Term::in, Functor::out, Args::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	if (Term == NULL) {
> +		MR_fatal_error(""cterm_deconstruct: NULL term"");
> +	}
> +
> +	Functor = Term->term_functor;
> +	Args = Term->term_args;
> +").
> +
> +:- pragma foreign_proc(c, 
> +	cterm_head_tail(Args::in, Head::out, Tail::out),
> +	[will_not_call_mercury, promise_pure],
> +"
> +	if (Args == NULL) {
> +		SUCCESS_INDICATOR = MR_FALSE;
> +	} else {
> +		Head = Args->args_head;
> +		Tail = Args->args_tail;
> +		SUCCESS_INDICATOR = MR_TRUE;
> +	}
> +").

The definitions of cterm_deconstruct/3 and cterm_head_tail/3 seem to be
duplicated.

> Index: doc/user_guide.texi
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/doc/user_guide.texi,v
> retrieving revision 1.414
> diff -u -b -r1.414 user_guide.texi
> --- doc/user_guide.texi	28 Jan 2005 02:59:10 -0000	1.414
> +++ doc/user_guide.texi	31 Jan 2005 01:15:10 -0000
> @@ -3022,8 +3022,43 @@
>  @item break info
>  Lists the details, status and print lists of all break points.
>  @sp 1
> - at item ignore [-E at var{ignore-count}] [-I at var{ignore-count}] @var{num}
> + at item condition [-n at var{break-num}] [-p] [-v] @var{varname}[@var{pathspec}] @var{op} @var{term}
> + at kindex condition (mdb command)
> +Attaches a condition to the most recent breakpoint,
> +or, if the @samp{n} or @samp{break-num} is given;

Should this be "or, breakpoint @var{break-num} if the @samp{-n} option is
given;"?  The sentense doesn't make sense as it stands.

> +execution won't stop at the breakpoint if the condition is false.
> + at sp 1
> +The condition is a match between a variable live at the breakpoint,
> +or a part thereof, and @var{term}.
> +It is ok for @var{term} to contain spaces.
> +The term from the program to be matched
> +is specified by @var{varname};
> +if it is followed by @var{pathspec} (without a space),
> +it specifies that the match is to be
> +against the specified part of @var{varname}.

Does this mean if I wanted to check if the first argument of an atom, say
p(1,2), was equal to 1, I'd need to give the command `condition HeadVar__1 = 1'?
It'd be nice (and consistent with other parts of mdb) if the argument number
instead of the var name could be used, though this can be done later.

> + at sp 1
> +There are two values allowed for @var{op}.
> +If @var{op} is @samp{=}, the condition is true
> +if the term specified by @var{printspec} matches @var{term}.

"printspec" doesn't appear anywhere in the command usage string.

> +If @var{op} is @samp{!=}, the condition is true
> +if the term specified by @var{printspec} doesn't match @var{term}.

See above comment.

Also wouldn't it be better to use "\=" instead of "!=" since "\=" is closer to
Mercury syntax?


> Index: runtime/mercury_trace_term.c
> ===================================================================
> RCS file: runtime/mercury_trace_term.c
> diff -N runtime/mercury_trace_term.c
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ runtime/mercury_trace_term.c	30 Jan 2005 15:47:29 -0000
> @@ -0,0 +1,175 @@
> +/*
> +** vim: ts=4 sw=4 expandtab
> +*/
> +/*
> +** Copyright (C) 2005 The University of Melbourne.
> +** This file may only be copied under the terms of the GNU Library General
> +** Public License - see the file COPYING.LIB in the Mercury distribution.
> +*/
> +
> +/*
> +** This file contains code to manage terms, for conditional breakpoints.
> +**
> +** Author: Zoltan Somogyi.
> +*/
> +
> +#include "mercury_imp.h"
> +#include "mercury_trace_term.h"
> +#include <stdlib.h>
> +
> +static  MR_CArgs    MR_create_cargs(char *str, char **rest);
> +static  void        MR_delete_cargs(MR_CArgs args);
> +
> +/**************************************************************************/
> +
> +MR_CTerm
> +MR_create_cterm(char *str, char **rest)
> +{
> +    MR_CTerm    term;
> +    MR_CArgs    args;
> +    int         i;
> +
> +    /*
> +    ** We don't have to worry about skipping white space, because there
> +    ** won't be any, except inside strings.
> +    */

This seems to conflict with the documentation that says a term may contain
spaces.  Are the spaces removed before the string is passed to this function?

> +
> +    if (str[0] == '"') {
> +        i = 1;
> +        while (str[i] != '\0' && str[i] != '"') {
> +            i++;
> +        }
> +
> +        if (str[i] == '"') {
> +            term = MR_malloc(sizeof(struct MR_CTerm_Struct));
> +            term->term_functor = &str[0];
> +            term->term_args = NULL;
> +            *rest = &str[i + 1];
> +            return term;
> +        }
> +
> +        return NULL;
> +    }

This doesn't seem to handle strings like """" or "\"", or escaped characters
like `\n'.  Is this handled elsewhere?  If it's not then this limitation
should be documented in the user guide.

> +
> +    if (! MR_isalnumunder(str[0]) || MR_isupper(str[0])) {
> +        return NULL;
> +    }
> +
> +    i = 0;
> +    while (MR_isalnumunder(str[i])) {
> +        i++;
> +    }
> +
> +    term = MR_malloc(sizeof(struct MR_CTerm_Struct));
> +
> +    if ((str[i] == '\0') || str[i] == ',' || str[i] == ')') {
> +        term->term_functor = str;
> +        term->term_args = NULL;
> +        *rest = &str[i];
> +        return term;
> +    } else if (str[i] == '(') {
> +        str[i] = '\0';
> +        term->term_functor = str;
> +        args = MR_create_cargs(&str[i + 1], rest);
> +        if (args == NULL) {
> +            MR_free(term);
> +            return NULL;
> +        }
> +
> +        term->term_args = args;
> +        return term;
> +    }
> +
> +    MR_free(term);
> +    return NULL;
> +}
> +
> +static MR_CArgs
> +MR_create_cargs(char *str, char **rest)
> +{
> +    char            *more;
> +    MR_CTerm        term;
> +    MR_CArgs        args;
> +    MR_CArgs        tail;
> +    int             i;
> +
> +    term = MR_create_cterm(str, &more);
> +    if (term == NULL) {
> +        return NULL;
> +    }
> +
> +    args = MR_malloc(sizeof(struct MR_CArgs_Struct));
> +    args->args_head = term;
> +
> +    if (more[0] == ')') {
> +        more[0] = '\0'; /* to terminate the just previous functor, if any */
> +        args->args_tail = NULL;
> +        *rest = &more[1];
> +        return args;
> +    } else if (more[0] == ',') {
> +        more[0] = '\0'; /* to terminate the just previous functor, if any */
> +        i = 1;
> +        while (more[i] != '\0' && MR_isspace(more[i])) {
> +            i++;
> +        }

Are cases like "f(1 , 2)" handled (where there is a space before and after the comma)?

> +
> +        if (more[i] == '\0') {
> +            MR_free(args);
> +            return NULL;
> +        }
> +
> +        tail = MR_create_cargs(more + i, rest);
> +        if (tail == NULL) {
> +            MR_free(args);
> +            return NULL;
> +        }
> +
> +        args->args_tail = tail;
> +        return args;
> +    }
> +
> +    MR_free(args);
> +    return NULL;
> +}
> +

> Index: tests/debugger/cond.inp
> ===================================================================
> RCS file: tests/debugger/cond.inp
> diff -N tests/debugger/cond.inp
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ tests/debugger/cond.inp	30 Jan 2005 17:32:05 -0000
> @@ -0,0 +1,35 @@
> +echo on
> +context none
> +break p
> +condition X = yes(_)
> +continue
> +print
> +delete *
> +finish 1
> +break p
> +condition X = yes(3)
> +continue
> +print
> +delete *
> +finish 1
> +break p
> +condition -v Y = yes( 3)
> +continue
> +print
> +delete *
> +finish 1
> +break p
> +condition -v Y^1 != 3
> +continue
> +print
> +delete *
> +finish 1
> +break q
> +condition -v Y != "abc "
> +continue
> +print
> +continue
> +print
> +delete *
> +finish 1
> +continue

You should test where the term has more than one argument and where one of the
arguments is itself a term with arguments.

Also you should test where the term path or variable doesn't exist and the "-v"
or "-p" options aren't supplied.

> Index: trace/mercury_trace_internal.c
> ===================================================================
> @@ -7036,15 +7229,19 @@
>                  }
>              }
>  
> -            if (lag) {
> +            if (lag != 0) {
>                  line[char_pos - lag] = line[char_pos];
>              }
>              char_pos++;
>          }
>      }
>  
> -    if (quoted) {
> -        return "unmatched quote";
> +    if (single_quoted) {
> +        return "unmatched single quote";
> +    }
> +
> +    if (double_quoted) {
> +        return "unmatched double quote";
>      }

There should be a test case for unmatched double quotes.

Otherwise that looks fine.

Ian.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------



More information about the reviews mailing list