[m-rev.] ssdb code reorganization

[Peter Wang]

On 2007-10-29, Olivier Annet <oan at missioncriticalit.com> wrote:
> Hi,
> 
> Could someone review my code please.
> 
> Olivier.
> 
> ===================================================================
> 
> 
> Estimated hours taken: 2
> Branches: main
> 
> The code has been reorganized for optimization reasons.
> 
> compiler/ssdebug.m:
> 	Four different predicates (one for each different event type) have 
> 	been made because, in the futur, they could recieve differents 
> 	arguments.

future receive different

> 
> ssdb/ssdb.m:
> 	Four different predicates of handle_event (one for each different 
> 	event type) have been made because there are some differences in their 
> 	body.

bodies

> 	Different predicates (set_stop, print_event_info and set_next_stop) 
> 	have been made because a lot of code are share in each handle 
> 	event.

because they share a lot of code.

> 
> Index: compiler/ssdebug.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/ssdebug.m,v
> retrieving revision 1.7
> diff -u -r1.7 ssdebug.m
> --- compiler/ssdebug.m	29 Oct 2007 03:00:43 -0000	1.7
> +++ compiler/ssdebug.m	29 Oct 2007 05:55:22 -0000
> @@ -250,8 +250,8 @@
>          %
>          % Generate the call to handle_event(call).
>          %
> -        make_call_handle_event(ssdb_call, ProcIdVar, CallArgListVar,
> -            HandleEventCallGoals, !ModuleInfo, !Varset, !Vartypes),
> +        make_handle_event_call(ProcIdVar, CallArgListVar, HandleEventCallGoal,
> +            !ModuleInfo, !Varset, !Vartypes),
>         
>          %
>          % Get the updated InstMap.
> @@ -280,16 +280,16 @@
>          %
>          % Generate the call to handle_event(exit).
>          %
> -        make_call_handle_event(ssdb_exit, ProcIdVar, ExitArgListVar,
> -            HandleEventExitGoals, !ModuleInfo, !Varset, !Vartypes),
> +        make_handle_event_exit(ProcIdVar, ExitArgListVar, HandleEventExitGoal, 
> +            !ModuleInfo, !Varset, !Vartypes),
>  
>  
>          %
>          % Organize the order of the generated code.
>          %
> -        ConjGoals = ProcIdGoals ++ CallArgListGoals ++ HandleEventCallGoals ++ 
> -            [BodyGoal1 | ExitArgListGoals] ++ HandleEventExitGoals ++ 
> -	    RenamingGoals,
> +        ConjGoals = ProcIdGoals ++ CallArgListGoals ++ [HandleEventCallGoal, 
> +            BodyGoal1 | ExitArgListGoals] ++ [HandleEventExitGoal | 
> +            RenamingGoals],

Might as well keep the lists on the same line.

> @@ -467,30 +467,84 @@
>  
>  %-----------------------------------------------------------------------------%
>  
> +    %
> +    % Build the following goal : handle_event_call(ProcId, Arguments).
> +    %
> +:- pred make_handle_event_call(prog_var::in, prog_var::in, hlds_goal::out, 
> +    module_info::in, module_info::out, prog_varset::in, prog_varset::out, 
> +    vartypes::in, vartypes::out) is det.
> +
> +make_handle_event_call(ProcIdVar, ArgListVar, HandleEventGoal, !ModuleInfo, 
> +    !Varset, !Vartypes) :-

Indent, here and elsewhere.

[...]
> +    %
> +    % Build the following goal : handle_event_redo(ProcId, Arguments).
> +    %
> +:- pred make_handle_event_redo(prog_var::in, prog_var::in, hlds_goal::out, 
> +    module_info::in, module_info::out, prog_varset::in, prog_varset::out, 
> +    vartypes::in, vartypes::out) is det.
> +
> +make_handle_event_redo(ProcIdVar, ArgListVar, HandleEventGoal, !ModuleInfo, 
> +    !Varset, !Vartypes) :-
> +
> +    SSDBModule = mercury_ssdb_builtin_module,
> +    Features = [],
> +    InstMapSrc = [],
> +    Context = term.context_init,
> +    goal_util.generate_simple_call(SSDBModule, "handle_event_redo", 
> +        pf_predicate, only_mode, detism_det, purity_impure, 
> +        [ProcIdVar, ArgListVar], Features, InstMapSrc, !.ModuleInfo, Context, 
> +        HandleEventGoal).

Some of these make_handle_event_* predicates can share code I think.

> Index: ssdb/ssdb.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/ssdb/ssdb.m,v
> retrieving revision 1.7
> diff -u -r1.7 ssdb.m
> --- ssdb/ssdb.m	29 Oct 2007 03:00:44 -0000	1.7
> +++ ssdb/ssdb.m	29 Oct 2007 05:55:22 -0000
> @@ -61,10 +61,24 @@
>  :- type pos == int.
>  
>      %
> -    % This routine is called at each event that occurs.
> +    % This routine is called at each call event that occurs.
>      %
> -:- impure pred handle_event(ssdb_proc_id::in, ssdb_event_type::in, 
> -    list_var_value::in) is det.
> +:- impure pred handle_event_call(ssdb_proc_id::in, list_var_value::in) is det.
> +
> +    %
> +    % This routine is called at each exit event that occurs.
> +    %
> +:- impure pred handle_event_exit(ssdb_proc_id::in, list_var_value::in) is det.
> +
> +    %
> +    % This routine is called at each fail event that occurs.
> +    %
> +:- impure pred handle_event_fail(ssdb_proc_id::in, list_var_value::in) is det.
> +
> +    %
> +    % This routine is called at each redo event that occurs.
> +    %
> +:- impure pred handle_event_redo(ssdb_proc_id::in, list_var_value::in) is det.
>  
>  %----------------------------------------------------------------------------%
>  %----------------------------------------------------------------------------%
> @@ -167,118 +181,133 @@
>  
>  %----------------------------------------------------------------------------%
>  
> -
>      %
>      % Write the event out and call the prompt.
> -    % XXX Not yet implemented : redo, fail.
>      %
> -handle_event(ProcId, Event, ListVarValue) :-
> -    
> +handle_event_call(ProcId, ListVarValue) :-
> +    Event = ssdb_call,
>      impure get_event_num_inc(EventNum),
>      impure update_depth(Event, PrintDepth),
>  
> -    ( 
> -        Event = ssdb_call,
> -        % set the new CSN.
> -        impure get_csn_inc(_),
> -
> -        % set the list_var_value of the debugger state  with the list received
> -        impure set_list_var_value(ListVarValue),
> -
> -        % Push the new stack frame on top of the shadow stack.
> -        semipure get_debugger_state(InitialState),
> -        StackFrame = elem(ProcId, InitialState),
> -        stack.push(InitialState ^ ssdb_stack, StackFrame, FinalStack),
> -        StateEv = InitialState ^ ssdb_stack := FinalStack,
> -        impure set_debugger_state(StateEv)
> -    ;
> -        % Just get the top stack frame. It will be popped at the end of
> -        % handle_event. We need to leave the frame in place, e.g. for
> -        % printing variables.
> -        Event = ssdb_exit,
> -        impure set_list_var_value_in_stack(ListVarValue),
> -        semipure get_debugger_state(InitialState),
> -        stack.top_det(InitialState ^ ssdb_stack, StackFrame)
> -
> -    ;
> -        Event = ssdb_redo,
> -        error("ssdb_redo: not yet implemented")
> -    ;
> -        Event = ssdb_fail,
> -        semipure get_debugger_state(InitialState),
> -        stack.top_det(InitialState ^ ssdb_stack, StackFrame)
> -    ),
> +    % set the new CSN.

Capitals.


> +    %
> +    % Write the event out and call the prompt.
> +    %
> +handle_event_exit(ProcId, ListVarValue) :-
> +    Event = ssdb_exit,
> +    impure get_event_num_inc(EventNum),
> +    impure update_depth(Event, PrintDepth),
> +
> +    % Just get the top stack frame. It will be popped at the end of
> +    % handle_event. We need to leave the frame in place, e.g. for
> +    % printing variables.

at the end of the procedure.

> +    impure set_list_var_value_in_stack(ListVarValue),
> +    semipure get_debugger_state(State0),
> +    stack.top_det(State0 ^ ssdb_stack, StackFrame),
> +
> +    CSN = StackFrame ^ se_initial_state ^ ssdb_csn,
> +
> +    set_stop(Event, CSN, State0, ProcId, Stop),
> +    (
> +        Stop = yes,
> +        some [!IO] 
>          (
> -            ( Event = ssdb_exit
> -	    ; Event = ssdb_fail
> -	    ),
> -            is_same_event(StopCSN, CSN, Stop)
> -        ;
> -            Event = ssdb_call,
> -            Stop = no
> +            impure invent_io(!:IO),
> +            
> +            print_event_info(Event, EventNum, ProcId, PrintDepth, CSN, !IO),  
> +         
> +            semipure get_shadow_stack(ShadowStack),
> +            impure prompt(Event, ShadowStack, 0, WhatNext, !IO),
> +
> +            impure consume_io(!.IO),
> +
> +            set_next_stop(CSN, WhatNext, NextStop),
> +
> +            % Set the last update :  breakpoints.

I don't understand that comment.

> @@ -419,6 +444,82 @@
>      impure set_debugger_state(State).
>  
>  
> +    %
> +    % Set Stop, if Stop equals yes, we call the prompt.
> +    %
> +:- pred set_stop(ssdb_event_type::in, int::in, debugger_state::in, 
> +    ssdb_proc_id::in, bool::out) is det.

The name doesn't match what it does.  Maybe `should_stop_at_event'?

> +
> +set_stop(Event, CSN, State, ProcId, Stop) :-
> +
> +    NextStop = State ^ ssdb_next_stop,
> +    (
> +        NextStop = ns_step,
> +        Stop = yes
> +    ;
> +        NextStop = ns_next(StopCSN),
> +        is_same_event(StopCSN, CSN, Stop)

is_same_event should be is_same_csn.

> +    ;
> +        NextStop = ns_continue,
> +        ( set.contains(State ^ ssdb_breakpoints, 
> +            breakpoint(ProcId ^ module_name, ProcId ^ proc_name)) 
> +        ->

Fix the formatting.

Peter

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