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

[oan at missioncriticalit.com]

On 8/10/2007, "Zoltan Somogyi" <zs at csse.unimelb.edu.au> wrote:

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

I totally agree with you, but as you have to enter the breakpoints by
hand, the set will never contain a million elements because when you are
debugging an application, you are generally interested by the behavior
of one or two (or some at the maximum) predicates/functions. In this
point of view, I don't thing that even a milli-second could be gained
here.

I will think on the case where there is a million breakpoints.

>
>>  :- 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?

Good remark, I will change it immediately

>>
>> +:- 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? 

No

>And why do you variable names such as ProcId?

ProcId was choose in common here. As an instantiation of the ssdb_proc_id
type, it looks like logic to call it like that.

But I can easily change the name if you wish.

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

All other remarks were taken into account

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

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