[m-rev.] Breakpoint added in source-to-source debugger

[Zoltan Somogyi]

On 08-Oct-2007, Olivier Annet <oan at missioncriticalit.com> wrote:
> --- compiler/ssdebug.m	4 Oct 2007 01:04:47 -0000	1.3
> +++ compiler/ssdebug.m	8 Oct 2007 07:28:01 -0000
> @@ -198,7 +198,7 @@
>          goal_util.generate_simple_call(SSDBModule, "handle_event",		
>              pf_predicate, only_mode, detism_det, purity_impure,
>              [ProcIdVar, CallVar],
> -            Features, InstMapSrc, !.ModuleInfo, Context, HandleCallEventGoal),		
> +            Features, InstMapSrc, !.ModuleInfo, Context, HandleCallEventGoal),
>              %
>              % Build the following two goals
>              %   ExitVar = ssdb_exit,
> @@ -210,7 +210,7 @@
>          goal_util.generate_simple_call(SSDBModule, "handle_event",		
>              pf_predicate, only_mode, detism_det, purity_impure,
>              [ProcIdVar, ExitVar],
> -            Features, InstMapSrc, !.ModuleInfo, Context, HandleExitEventGoal),		
> +            Features, InstMapSrc, !.ModuleInfo, Context, HandleExitEventGoal),

What happened to those two lines? In the rare case where you have to change
white space on a line, you shouldn't bother reviers with it; get your diff
with diff -ub.

> @@ -247,21 +247,26 @@
>  
>  make_proc_id_construction(PredInfo,
>          _ProcInfo, Goals, ProcIdVar, !Varset, !Vartypes) :-
> -    Name = pred_info_name(PredInfo),
> +    SymModName = pred_info_module(PredInfo),
> +    ModName = sym_name_to_string(SymModName),
> +    PredName = pred_info_name(PredInfo),

Please use ModuleName instead of ModName.

> +                % The set of breakpoint added.
> +                ssdb_breakpoints    :: set(breakpoint)

This is unnecessarily slow. Using a map from module_name to the set of
predicate names with breakpoints in that module would be faster, both because
maps are balanced trees while sets are lists, and because if you have
breakpoints on more than one predicate in a module other than the current one,
a single comparison can ignore them all.

>  :- type what_next
>      --->    what_next_step
>      ;       what_next_next
> +    ;       what_next_continue
>      ;       what_next_finish(int).
>  
>  
>      %
> -    % Type filled by the handle_event function to determine the next stop of
> -    % the prompt function
> +    % Type used by the handle_event predicate to determine the next stop of
> +    % the prompt predicate.
>      %
>  :- type next_stop
>      --->    step
>      ;       next(int)
> +    ;       continue
>      ;       final_port(int).

Why the prefixes on one type but not the other? What is the reason for the
difference between the two?
>  
> +:- type breakpoint
> +    --->    breakpoint(
> +                bp_module_name  :: string,
> +                bp_pred_name    :: string
> +    ).

Both here and in the compiler: did you make a conscious choice to work
with predicates instead of procedures? If yes, why? And why do you variable
names such as ProcId?

>  handle_event(ProcId, Event) :-
>      impure get_event_num_inc(EventNum),
>      impure update_depth(Event, PrintDepth),
>  
> -    (
> -        Event = ssdb_call,
> +    ( Event = ssdb_call,

Undo this change.

> -        E = elem(ProcId, InitialState),
> -        stack.push(InitialState ^ ssdb_stack, E, FinalStack),
> +        Element = elem(ProcId, InitialState),
> +        stack.push(InitialState ^ ssdb_stack, Element, FinalStack),

I don't think "Element" is the description you want here; consider
"StackFrame".

>          NextStop0 = final_port(StopCSN),
>          (
>              Event = ssdb_exit,
> -            is_same_event(StopCSN, PrintCSN, Stop)
> +            is_same_event(StopCSN, CSN, Stop)
>          ;
>              Event = ssdb_call,
>              Stop = no
> @@ -183,13 +206,15 @@
>              io.write_string("       ", !IO),
>              io.write_int(EventNum, !IO),
>              io.write_string("\t", !IO),
> +            io.write_string(ProcId ^ module_name, !IO),
> +            io.write_string(".", !IO),
>              io.write_string(ProcId ^ proc_name, !IO),

You will need a predicate for printing predicate/procedure ids.

> +    
> +    %
> +    % Return the current event number.
> +    %
> +:- semipure pred get_event_num(int::out) is det.

Please stick to our conventions for comments on predicate/function
declarations.

>  prompt(ShadowStack, Depth, WhatNext, !IO) :-
>      io.write_string("ssdb> ", !IO),
> -        %read a string in input and return a string
> +    % Read a string in input and return a string.
>      io.read_line_as_string(Result, !IO), 
>      (
>          Result = ok(String0),
> -            %string minus any single trailing newline character
> +        % String minus any single trailing newline character.
>          String = string.chomp(String0), 
>  
> -        ( 
> -            String = "h" ->
> +        ( String = "h" ->
> +            io.nl(!IO),
> +            io.write_string("s      :: step", !IO),
>              io.nl(!IO),
> -            io.write_string("s   :: step", !IO),
> +            io.write_string("n      :: next", !IO),
>              io.nl(!IO),
> -            io.write_string("n   :: next", !IO),
> +            io.write_string("b X Y  :: insert breakpoint where :", !IO),
> +            io.write_string(" X = module name", !IO),
> +            io.write_string(" and Y = predicate name", !IO),
>              io.nl(!IO),
> -            io.write_string("f   :: finish", !IO),
> +            io.write_string("c      :: next", !IO),
> +            io.nl(!IO),
> +            io.write_string("f      :: finish", !IO),
>              io.nl(!IO),
>              io.nl(!IO),
>              impure prompt(ShadowStack, Depth, WhatNext, !IO)
>          
> -        ; 
> -            String = "n" ->
> +        ; String = "n" ->
>              WhatNext = what_next_next
> +
>          ;
> -            (
> -                String = "s"
> -            ;
> -                String = ""
> +            ( String = "s"
> +            ; String = ""
>              )
> -            ->
> +        ->
>                  WhatNext = what_next_step
> -        ;
> -            String = "f" ->
> +
> +        ; String = "c" ->
> +            WhatNext = what_next_continue
> +
> +        ; 
> +            Words = string.words(String),
> +            Words = ["b", ModuleName, ProcedureName] 

You should break up the line into words *before* you start looking for
individual commands' names.

If I were you, I would design a Mercury type for commands, and totally separate
the code for parsing (converting a line into this structure) from the code for
actually validating and executing the commands.

Otherwise, the diff is fine.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------