[m-rev.] for review: fix bug with solver types and mutable initialisation

Julien Fischer juliensf at cs.mu.OZ.AU
Mon Mar 20 18:19:37 AEDT 2006


For review by anyone.  (This was reported to mercury-bugs the other day
by Ralph.)

Estimated hours taken: 6
Branches: main

Fix a bug with solver types and mutables that was causing the compiler to
abort in deep profiling grades (The HLDS was incorrect in all grades, it just
didn't show in most of them).  The bug occurred when a mutable was initialised
with a non-ground value, as in the following example derived from Ralph's sat
solver:

	:- mutable(global, sat_literal, _, any, [untrailed]).

(sat_literal is some solver_type).

The problem was that when adding the clauses for the mutable initialisation
predicates to the HLDS we did not consider that the initial value might be a
variable, as in this case, and attached an empty varset to the clause instead
of one containing the variable.

For the intialisation predicate for the above mutable, we have the following
at stage 197:

	msat.initialise_mutable_global :-
		msat.'__Initialise__'(V_1),
		impure msat.set_global(V_1).

The prog_varset is (incorrectly) empty at this point - it should contain
V_1.

Deep profiling (stage 205), now comes along and starts allocating fresh
variables.  The results pretty much speak for themselves ;-)

	msat.initialise_mutable_global :-
		ProcStaticLayout = deep_profiling_proc_layout(...),
		impure det_call_port_code_sr(ProcStaticLayout, TopCSD,
			MiddleCSD, ActivationPtr),
		SiteNum = 0,
		impure prepare_for_normal_call(SiteNum),
		msat.'__Initialise__'(TopCSD),		% *** should be V_1
		SiteNum = 1,
		impure prepare_for_normal_call(SiteNum),
		impure msat.set_global(TopCSD),		% *** should be V_1
		impure det_exit_port_code_sr(TopCSD, MiddleCSD, ActivationPtr).

The fix is to attach the varset we use at parse time to the mutable items and
when adding the clauses for the mutable initialisation predicate to the HLDS,
to use that varset instead of an empty one.

compiler/prog_item.m:
compiler/prog_io.m:
	Attach the varset to the mutable item, in case the initial value term
	has a non-ground value.

compiler/make_hlds_passes.m:
	Use the above varset when constructing the clauses for the mutable
	initialisation predicates in order to avoid the bug outlined above.

compiler/equiv_type.m:
compiler/mercury_to_mercury.m:
compiler/module_qual.m:
compiler/modules.m:
compiler/recompilation.check.m:
compiler/recompilation.version.m:
	Conform to the above changes to the mutable item.

tests/valid/Mmakefile:
tests/valid/solver_type_mutable_bug.m:
	Test case for the above derived from msat.m by Ralph.

Julien.

Index: compiler/equiv_type.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/equiv_type.m,v
retrieving revision 1.59
diff -u -r1.59 equiv_type.m
--- compiler/equiv_type.m	17 Mar 2006 01:40:16 -0000	1.59
+++ compiler/equiv_type.m	20 Mar 2006 05:28:41 -0000
@@ -418,9 +418,9 @@
     ).

 replace_in_item(ModuleName,
-        mutable(MutName, Type0, InitValue, Inst0, Attrs),
+        mutable(MutName, Type0, InitValue, Inst0, Attrs, Varset),
         _Context, EqvMap, EqvInstMap,
-        mutable(MutName, Type, InitValue, Inst, Attrs),
+        mutable(MutName, Type, InitValue, Inst, Attrs, Varset),
         [], !Info) :-
     QualName = qualified(ModuleName, MutName),
     maybe_record_expanded_items(ModuleName, QualName, !.Info, ExpandedItems0),
Index: compiler/make_hlds_passes.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/make_hlds_passes.m,v
retrieving revision 1.33
diff -u -r1.33 make_hlds_passes.m
--- compiler/make_hlds_passes.m	17 Mar 2006 03:52:28 -0000	1.33
+++ compiler/make_hlds_passes.m	20 Mar 2006 07:03:10 -0000
@@ -122,6 +122,7 @@
 :- import_module set.
 :- import_module std_util.
 :- import_module string.
+:- import_module svvarset.
 :- import_module varset.

 %-----------------------------------------------------------------------------%
@@ -449,7 +450,7 @@
 add_item_decl_pass_1(Item, Context, !Status, !ModuleInfo, no, !IO) :-
     % We add the initialise decl and the foreign_decl on the second pass and
     % the foreign_proc clauses on the third pass.
-    Item = mutable(Name, Type, _InitValue, Inst, MutAttrs),
+    Item = mutable(Name, Type, _InitValue, Inst, MutAttrs, _MutVarset),
     !.Status = item_status(ImportStatus, _),
     ( status_defined_in_this_module(ImportStatus, yes) ->
         module_info_get_name(!.ModuleInfo, ModuleName),
@@ -631,7 +632,7 @@
         true
     ).
 add_item_decl_pass_2(Item, Context, !Status, !ModuleInfo, !IO) :-
-    Item = mutable(Name, _Type, _InitTerm, _Inst, MutAttrs),
+    Item = mutable(Name, _Type, _InitTerm, _Inst, MutAttrs, _MutVarset),
     !.Status = item_status(ImportStatus, _),
     ( ImportStatus = exported ->
         error_is_exported(Context, "`mutable' declaration", !IO),
@@ -1140,7 +1141,7 @@
         module_info_incr_errors(!ModuleInfo)
     ).
 add_item_clause(Item, !Status, Context, !ModuleInfo, !QualInfo, !IO) :-
-    Item = mutable(Name, Type, InitTerm, Inst, MutAttrs),
+    Item = mutable(Name, Type, InitTerm, Inst, MutAttrs, MutVarset),
     ( status_defined_in_this_module(!.Status, yes) ->
         module_info_get_name(!.ModuleInfo, ModuleName),
         varset.new_named_var(varset.init, "X", X, ProgVarSet0),
@@ -1203,8 +1204,12 @@
         add_item_clause(initialise(compiler(mutable_decl),
                 mutable_init_pred_sym_name(ModuleName, Name), 0 /* Arity */),
             !Status, Context, !ModuleInfo, !QualInfo, !IO),
-        InitClause = clause(compiler(mutable_decl), varset.init, predicate,
-            mutable_init_pred_sym_name(ModuleName, Name), [],
+        %
+        % See the comments for prog_io.parse_mutable_decl for the reason
+        % why we _must_ use MutVarset here.
+        %
+        InitClause = clause(compiler(mutable_decl), MutVarset,
+            predicate, mutable_init_pred_sym_name(ModuleName, Name), [],
             call(mutable_set_pred_sym_name(ModuleName, Name),
                 [InitTerm], purity_impure) - Context),
         add_item_clause(InitClause, !Status, Context, !ModuleInfo, !QualInfo,
Index: compiler/mercury_to_mercury.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/mercury_to_mercury.m,v
retrieving revision 1.285
diff -u -r1.285 mercury_to_mercury.m
--- compiler/mercury_to_mercury.m	17 Mar 2006 01:40:27 -0000	1.285
+++ compiler/mercury_to_mercury.m	20 Mar 2006 07:05:06 -0000
@@ -761,13 +761,18 @@
     io.write_string("/", !IO),
     io.write_int(Arity, !IO),
     io.write_string(".\n", !IO).
-mercury_output_item(_, mutable(Name, Type, InitTerm, Inst, Attrs), _, !IO) :-
+mercury_output_item(_, Mutable, _, !IO) :-
+    Mutable = mutable(Name, Type, InitTerm, Inst, Attrs, MutVarset),
     io.write_string(":- mutable(", !IO),
     io.write_string(Name, !IO),
     io.write_string(", ", !IO),
     mercury_output_type(varset.init, no, Type, !IO),
     io.write_string(", ", !IO),
-    mercury_output_term(InitTerm, varset.init, no, !IO),
+    %
+    % See the comments for prog_io.read_mutable_decl for the reason we
+    % _must_ use MutVarset here.
+    %
+    mercury_output_term(InitTerm, MutVarset, no, !IO),
     io.write_string(", ", !IO),
     mercury_output_inst(Inst, varset.init, !IO),
     io.write_string(", ", !IO),
Index: compiler/module_qual.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/module_qual.m,v
retrieving revision 1.127
diff -u -r1.127 module_qual.m
--- compiler/module_qual.m	8 Mar 2006 02:25:33 -0000	1.127
+++ compiler/module_qual.m	20 Mar 2006 05:31:16 -0000
@@ -339,7 +339,7 @@
 collect_mq_info_2(instance(_, _, _, _, _, _), !Info).
 collect_mq_info_2(initialise(_, _, _), !Info).
 collect_mq_info_2(finalise(_, _, _), !Info).
-collect_mq_info_2(mutable(_, _, _, _, _), !Info).
+collect_mq_info_2(mutable(_, _, _, _, _, _), !Info).

 :- pred collect_mq_info_qualified_symname(sym_name::in,
     mq_info::in, mq_info::out) is det.
@@ -733,8 +733,8 @@
         !Info, yes, !IO).

 module_qualify_item(
-        mutable(Name, Type0, InitTerm, Inst0, Attrs) - Context,
-        mutable(Name, Type, InitTerm, Inst, Attrs) - Context,
+        mutable(Name, Type0, InitTerm, Inst0, Attrs, Varset) - Context,
+        mutable(Name, Type, InitTerm, Inst, Attrs, Varset) - Context,
         !Info, yes, !IO) :-
     mq_info_set_error_context(mqec_mutable(Name) - Context, !Info),
     qualify_type(Type0, Type, !Info, !IO),
Index: compiler/modules.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/modules.m,v
retrieving revision 1.380
diff -u -r1.380 modules.m
--- compiler/modules.m	17 Mar 2006 01:40:32 -0000	1.380
+++ compiler/modules.m	20 Mar 2006 05:33:00 -0000
@@ -1328,7 +1328,7 @@
     item_and_context::in, item_list::in, item_list::out) is det.

 handle_mutable_in_private_interface(ModuleName, Item - Context, !Items) :-
-    ( Item = mutable(MutableName, Type, _Value, Inst, Attrs) ->
+    ( Item = mutable(MutableName, Type, _Value, Inst, Attrs, _Varset) ->
         NonPureGetPredDecl =
             prog_mutable.nonpure_get_pred_decl(ModuleName, MutableName,
                 Type, Inst),
@@ -5728,7 +5728,7 @@
 get_item_foreign_code(Globals, Item - Context, !Info) :-
     ( Item = pragma(_, Pragma) ->
         do_get_item_foreign_code(Globals, Pragma, Context, !Info)
-    ; Item = mutable(_, _, _, _, _) ->
+    ; Item = mutable(_, _, _, _, _, _) ->
         % Mutables introduce foreign_procs, but mutable declarations
         % won't have been expanded by the time we get here so we need
         % to handle them separately.
@@ -7318,7 +7318,7 @@
 item_needs_imports(promise(_, _, _, _)) = yes.
 item_needs_imports(initialise(_, _, _)) = yes.
 item_needs_imports(finalise(_, _, _)) = yes.
-item_needs_imports(mutable(_, _, _, _, _)) = yes.
+item_needs_imports(mutable(_, _, _, _, _, _)) = yes.
 item_needs_imports(nothing(_)) = no.

 :- pred item_needs_foreign_imports(item::in, foreign_language::out) is nondet.
@@ -7335,7 +7335,7 @@
 item_needs_foreign_imports(pragma(_, foreign_code(Lang, _)), Lang).
 item_needs_foreign_imports(pragma(_, foreign_proc(Attrs, _, _, _, _, _, _)),
         foreign_language(Attrs)).
-item_needs_foreign_imports(mutable(_, _, _, _, _), Lang) :-
+item_needs_foreign_imports(mutable(_, _, _, _, _, _), Lang) :-
     foreign_language(Lang).

 :- pred include_in_int_file_implementation(item::in) is semidet.
@@ -7659,7 +7659,7 @@
 reorderable_item(pred_or_func_mode(_, _, _, _, _, _, _)) = no.
 reorderable_item(initialise(_, _, _)) = no.
 reorderable_item(finalise(_, _, _)) = no.
-reorderable_item(mutable(_, _, _, _, _)) = no.
+reorderable_item(mutable(_, _, _, _, _, _)) = no.

 :- pred is_chunkable(item_and_context::in) is semidet.

@@ -7739,7 +7739,7 @@
 chunkable_item(clause(_, _, _, _, _, _)) = yes.
 chunkable_item(initialise(_, _, _)) = yes.
 chunkable_item(finalise(_, _, _)) = yes.
-chunkable_item(mutable(_, _, _, _, _)) = no.
+chunkable_item(mutable(_, _, _, _, _, _)) = no.
 chunkable_item(nothing(_)) = yes.

     % Given a list of items for which symname_ordered succeeds, we need to keep
Index: compiler/prog_io.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/prog_io.m,v
retrieving revision 1.261
diff -u -r1.261 prog_io.m
--- compiler/prog_io.m	17 Mar 2006 01:40:36 -0000	1.261
+++ compiler/prog_io.m	20 Mar 2006 06:59:12 -0000
@@ -1801,16 +1801,24 @@
 % then foreign_procs are created that have the thread_safe attribute
 % set.  If the `untrailed' attribute is specified in <attribute_list>
 % then the code for trailing the mutable variable in the set predicate
-% is omitted.
+% is omitted
+
+% NOTE: we must attach the varset to the mutable item because if the
+%       initial value is non-ground, then the initial value will be a
+%       variable and the mutable initialisation predicate will contain
+%       references to it.  Ignoring the varset may lead to later
+%       compiler passes attempting to reuse this variable when fresh
+%       variables are allocated.

 :- pred parse_mutable_decl(module_name::in, varset::in, list(term)::in,
     maybe1(item)::out) is semidet.

-parse_mutable_decl(_ModuleName, _VarSet, Terms, Result) :-
+parse_mutable_decl(_ModuleName, Varset, Terms, Result) :-
     Terms = [NameTerm, TypeTerm, ValueTerm, InstTerm | OptMutAttrsTerm],
     parse_mutable_name(NameTerm, NameResult),
     parse_mutable_type(TypeTerm, TypeResult),
     term.coerce(ValueTerm, Value),
+    varset.coerce(Varset, ProgVarset),
     parse_mutable_inst(InstTerm, InstResult),
     (
         OptMutAttrsTerm = [],
@@ -1825,7 +1833,7 @@
         InstResult = ok(Inst),
         MutAttrsResult = ok(MutAttrs)
     ->
-        Result = ok(mutable(Name, Type, Value, Inst, MutAttrs))
+        Result = ok(mutable(Name, Type, Value, Inst, MutAttrs, ProgVarset))
     ;
         NameResult = error(Msg, Term)
     ->
Index: compiler/prog_item.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/prog_item.m,v
retrieving revision 1.7
diff -u -r1.7 prog_item.m
--- compiler/prog_item.m	17 Mar 2006 01:40:37 -0000	1.7
+++ compiler/prog_item.m	20 Mar 2006 05:22:47 -0000
@@ -219,7 +219,8 @@
                 mut_type                        :: mer_type,
                 mut_init_value                  :: prog_term,
                 mut_inst                        :: mer_inst,
-                mut_attrs                       :: mutable_var_attributes
+                mut_attrs                       :: mutable_var_attributes,
+                mut_varset                      :: prog_varset
             )

             % Used for items that should be ignored (for the
Index: compiler/recompilation.check.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/recompilation.check.m,v
retrieving revision 1.26
diff -u -r1.26 recompilation.check.m
--- compiler/recompilation.check.m	17 Mar 2006 01:40:38 -0000	1.26
+++ compiler/recompilation.check.m	20 Mar 2006 05:41:42 -0000
@@ -878,7 +878,7 @@
 check_for_ambiguities(_, _, _, instance(_, _, _, _, _, _) - _, !Info).
 check_for_ambiguities(_, _, _, initialise(_, _, _) - _, !Info).
 check_for_ambiguities(_, _, _, finalise(_, _, _) - _, !Info).
-check_for_ambiguities(_, _, _, mutable(_, _, _, _, _) - _, !Info).
+check_for_ambiguities(_, _, _, mutable(_, _, _, _, _, _) - _, !Info).
 check_for_ambiguities(_, _, _, nothing(_) - _, !Info).

 :- pred check_class_method_for_ambiguities(need_qualifier::in, timestamp::in,
Index: compiler/recompilation.version.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/recompilation.version.m,v
retrieving revision 1.40
diff -u -r1.40 recompilation.version.m
--- compiler/recompilation.version.m	17 Mar 2006 01:40:39 -0000	1.40
+++ compiler/recompilation.version.m	20 Mar 2006 05:42:41 -0000
@@ -547,7 +547,7 @@
 item_to_item_id_2(instance(_, _, _, _, _, _), no).
 item_to_item_id_2(initialise(_, _, _), no).
 item_to_item_id_2(finalise(_, _, _), no).
-item_to_item_id_2(mutable(_, _, _, _, _), no).
+item_to_item_id_2(mutable(_, _, _, _, _, _), no).
 item_to_item_id_2(nothing(_), no).

 :- type maybe_pred_or_func_id == pair(maybe(pred_or_func), sym_name_and_arity).
@@ -709,8 +709,8 @@
     ( Item2 = initialise(O, A, B) -> yes ; no ).
 item_is_unchanged(finalise(O, A, B), Item2) =
     ( Item2 = finalise(O, A, B) -> yes ; no ).
-item_is_unchanged(mutable(A, B, C, D, E), Item2) =
-    ( Item2 = mutable(A, B, C, D, E) -> yes ; no ).
+item_is_unchanged(mutable(A, B, C, D, E, F), Item2) =
+    ( Item2 = mutable(A, B, C, D, E, F) -> yes ; no ).

 item_is_unchanged(Item1, Item2) = Result :-
     Item1 = pred_or_func(TVarSet1, _, ExistQVars1, PredOrFunc,
Index: tests/valid/Mmakefile
===================================================================
RCS file: /home/mercury1/repository/tests/valid/Mmakefile,v
retrieving revision 1.167
diff -u -r1.167 Mmakefile
--- tests/valid/Mmakefile	6 Mar 2006 04:30:37 -0000	1.167
+++ tests/valid/Mmakefile	20 Mar 2006 06:20:13 -0000
@@ -176,6 +176,7 @@
 	soln_context \
 	solv \
 	solver_type_bug \
+	solver_type_mutable_bug \
 	some_switch \
 	spurious_purity_warning \
 	stack_alloc \
Index: tests/valid/solver_type_mutable_bug.m
===================================================================
RCS file: tests/valid/solver_type_mutable_bug.m
diff -N tests/valid/solver_type_mutable_bug.m
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ tests/valid/solver_type_mutable_bug.m	20 Mar 2006 06:52:39 -0000
@@ -0,0 +1,28 @@
+% The following program, derived from msat.m by Ralph Becket, caused
+% rotd-2006-03-19 to abort in deep profiling grades.  The problem was that the
+% varset attached to the mutable initialisation procedure was incorrect and
+% the deep profiling transformation was clobbering existing variables when it
+% tried to introduce new ones.  The bug eventually showed up as an abort in
+% the code generator.
+
+:- module solver_type_mutable_bug.
+:- interface.
+
+:- solver type sat_literal.
+:- pred new_raw_sat_literal(sat_literal::oa) is det.
+
+:- implementation.
+
+:- solver type sat_literal
+    where   representation  is int,
+            initialisation  is new_raw_sat_literal.
+
+:- pragma foreign_proc("C",
+    new_raw_sat_literal(A::oa),
+    [promise_pure, will_not_call_mercury],
+"
+    A = 3;
+").
+
+:- mutable(global, sat_literal, _, any,    [untrailed]).
+
--------------------------------------------------------------------------
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