[m-dev.] for review: fix for a bug reported by Warwick

[Zoltan Somogyi]

> Unfortunately, it doesn't quite work.  It appears (at least in some cases) 
> to restore the stack pointer and before testing the saved value of 
> `MR_trace_from_full', which means it's looking in the wrong part of the 
> stack, and hence sometimes making the wrong choice.

Yes, you are right. However, it seems to me that the problems you point out
in the rest of the review fully account for this behavior, and so the obvious
fix "should" work. I am attaching the full modified diff for you to test.

> An example which exhibits the bug is std_util:type_of/2.  If the sample 
> program I gave you still gives negative or decrementing numbers after the 
> above fixes, ...

The thing is that since the sample program peers under the hood, it is
*expected* to produce different answers when compiled with different trace
levels.

However, with the new diff, the numbers it gives you no longer decrement.

Zoltan.

cvs diff: Diffing .
Index: code_gen.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/code_gen.m,v
retrieving revision 1.71
diff -u -b -r1.71 code_gen.m
--- code_gen.m	1999/12/14 04:52:31	1.71
+++ code_gen.m	2000/01/25 09:39:56
@@ -362,7 +362,7 @@
 		code_gen__generate_entry(model_det, Goal, ResumePoint,
 			FrameInfo, EntryCode),
 		code_gen__generate_exit(model_det, FrameInfo, TraceSlotInfo,
-			BodyContext, _, ExitCode),
+			BodyContext, _, _, ExitCode),
 		{ Code =
 			tree(EntryCode,
 			tree(TraceCallCode,
@@ -391,7 +391,8 @@
 		code_gen__generate_entry(model_semi, Goal, ResumePoint,
 			FrameInfo, EntryCode),
 		code_gen__generate_exit(model_semi, FrameInfo, TraceSlotInfo,
-			BodyContext, RestoreDeallocCode, ExitCode),
+			BodyContext, RestoreDeallocCode, UndoFillCode,
+			ExitCode),
 
 		code_info__generate_resume_point(ResumePoint, ResumeCode),
 		{ code_info__resume_point_vars(ResumePoint, ResumeVarList) },
@@ -408,8 +409,9 @@
 			tree(ExitCode,
 			tree(ResumeCode,
 			tree(TraceFailCode,
+			tree(UndoFillCode,
 			tree(RestoreDeallocCode,
-			     FailCode)))))))
+			     FailCode))))))))
 		}
 	;
 		{ MaybeTraceCallLabel = no },
@@ -417,7 +419,7 @@
 		code_gen__generate_entry(model_semi, Goal, ResumePoint,
 			FrameInfo, EntryCode),
 		code_gen__generate_exit(model_semi, FrameInfo, TraceSlotInfo,
-			BodyContext, RestoreDeallocCode, ExitCode),
+			BodyContext, RestoreDeallocCode, _, ExitCode),
 		code_info__generate_resume_point(ResumePoint, ResumeCode),
 		{ Code =
 			tree(EntryCode,
@@ -443,7 +445,7 @@
 		code_gen__generate_entry(model_non, Goal, ResumePoint,
 			FrameInfo, EntryCode),
 		code_gen__generate_exit(model_non, FrameInfo, TraceSlotInfo,
-			BodyContext, _, ExitCode),
+			BodyContext, _, UndoFillCode, ExitCode),
 
 		code_info__generate_resume_point(ResumePoint, ResumeCode),
 		{ code_info__resume_point_vars(ResumePoint, ResumeVarList) },
@@ -453,13 +455,6 @@
 			% definition would be better than BodyContext.
 		trace__generate_external_event_code(fail, TraceInfo,
 			BodyContext, _, _, TraceFailCode),
-		{ TraceSlotInfo = trace_slot_info(_, _, yes(_)) ->
-			DiscardTraceTicketCode = node([
-				discard_ticket - "discard retry ticket"
-			])
-		;
-			DiscardTraceTicketCode = empty
-		},
 		{ FailCode = node([
 			goto(do_fail) - "fail after fail trace port"
 		]) },
@@ -470,7 +465,7 @@
 			tree(ExitCode,
 			tree(ResumeCode,
 			tree(TraceFailCode,
-			tree(DiscardTraceTicketCode,
+			tree(UndoFillCode,
 			     FailCode)))))))
 		}
 	;
@@ -479,7 +474,7 @@
 		code_gen__generate_entry(model_non, Goal, ResumePoint,
 			FrameInfo, EntryCode),
 		code_gen__generate_exit(model_non, FrameInfo, TraceSlotInfo,
-			BodyContext, _, ExitCode),
+			BodyContext, _, _, ExitCode),
 		{ Code =
 			tree(EntryCode,
 			tree(BodyCode,
@@ -633,6 +628,8 @@
 	%	a comment to mark epilogue start
 	%	code to place the output arguments where their caller expects
 	%	code to restore registers from some special slots
+	%	code to undo any actions during trace slot filling
+	%		that need to be undone on success
 	%	code to deallocate the stack frame
 	%	code to set r1 to TRUE (for semidet procedures only)
 	%	a jump back to the caller, including livevals information
@@ -642,6 +639,10 @@
 	% frame are also part of the failure epilog, which is handled by
 	% our caller; this is why we return RestoreDeallocCode.
 	%
+	% For a similar reason, we also return FailUndoCode, which contains
+	% the code to undo the actions during trace slot filling that may
+	% need to be done only in the failure epilog.
+	%
 	% At the moment the only special slots are the succip slot, and
 	% the tracing slots (holding the call sequence number, call event
 	% number, call depth, from-full indication, and trail state).
@@ -657,10 +658,10 @@
 
 :- pred code_gen__generate_exit(code_model::in, frame_info::in,
 	trace_slot_info::in, prog_context::in, code_tree::out, code_tree::out,
-	code_info::in, code_info::out) is det.
+	code_tree::out, code_info::in, code_info::out) is det.
 
 code_gen__generate_exit(CodeModel, FrameInfo, TraceSlotInfo, BodyContext,
-		RestoreDeallocCode, ExitCode) -->
+		RestoreDeallocCode, FailureUndoSlotFillCode, ExitCode) -->
 	{ StartComment = node([
 		comment("Start of procedure epilogue") - ""
 	]) },
@@ -676,7 +677,9 @@
 				will_not_call_mercury, no, no, no)
 				- ""
 		]) },
-		{ RestoreDeallocCode = empty },	% always empty for nondet code
+		{ RestoreDeallocCode = empty },
+		trace__undo_slot_fill_code(TraceSlotInfo, CodeModel,
+			_SuccessUndoSlotFillCode, FailureUndoSlotFillCode),
 		{ ExitCode =
 			tree(StartComment,
 			tree(UndefCode,
@@ -714,22 +717,10 @@
 				decr_sp(TotalSlots) - "Deallocate stack frame"
 			])
 		},
-		{
-			TraceSlotInfo = trace_slot_info(_, _, yes(_)),
-			CodeModel \= model_non
-		->
-			DiscardTraceTicketCode = node([
-				discard_ticket - "discard retry ticket"
-			])
-		;
-			DiscardTraceTicketCode = empty
-		},
 
-		{ RestoreDeallocCode =
-			tree(RestoreSuccipCode,
-			tree(DeallocCode,
-			     DiscardTraceTicketCode))
-		},
+		{ RestoreDeallocCode = tree(RestoreSuccipCode, DeallocCode) },
+		trace__undo_slot_fill_code(TraceSlotInfo, CodeModel,
+			SuccessUndoSlotFillCode, FailureUndoSlotFillCode),
 
 		code_info__get_maybe_trace_info(MaybeTraceInfo),
 		( { MaybeTraceInfo = yes(TraceInfo) } ->
@@ -770,8 +761,9 @@
 			]) },
 			{ AllSuccessCode =
 				tree(TraceExitCode,
+				tree(SuccessUndoSlotFillCode,
 				tree(RestoreDeallocCode,
-				     SuccessCode))
+				     SuccessCode)))
 			}
 		;
 			{ CodeModel = model_semi },
@@ -783,8 +775,9 @@
 			]) },
 			{ AllSuccessCode =
 				tree(TraceExitCode,
+				tree(SuccessUndoSlotFillCode,
 				tree(RestoreDeallocCode,
-				     SuccessCode))
+				     SuccessCode)))
 			}
 		;
 			{ CodeModel = model_non },
Index: trace.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/trace.m,v
retrieving revision 1.29
diff -u -b -r1.29 trace.m
--- trace.m	1999/12/16 19:06:52	1.29
+++ trace.m	2000/01/25 02:54:50
@@ -122,6 +122,13 @@
 :- pred trace__generate_slot_fill_code(trace_info::in, code_tree::out,
 	code_info::in, code_info::out) is det.
 
+	% Generate two code sequences to undo whatever effects of the trace
+	% slot fill code need to be undone when we leave from the procedure:
+	% the first is for the success epilog, the second for the failure
+	% epilog.
+:- pred trace__undo_slot_fill_code(trace_slot_info::in, code_model::in,
+	code_tree::out, code_tree::out, code_info::in, code_info::out) is det.
+
 	% If we are doing execution tracing, generate code to prepare for
 	% a call.
 :- pred trace__prepare_for_call(code_tree::out, code_info::in, code_info::out)
@@ -425,6 +432,41 @@
 		pragma_c([], [pragma_c_raw_code(TraceStmt)],
 			will_not_call_mercury, no, no, yes) - ""
 	])
+	}.
+
+trace__undo_slot_fill_code(TraceSlotInfo, CodeModel,
+		SuccessUndoFillCode, FailureUndoFillCode) -->
+	code_info__get_maybe_trace_info(MaybeTraceInfo),
+	{
+		MaybeTraceInfo = yes(TraceInfo),
+		TraceSlotInfo = trace_slot_info(_, _, yes(_))
+	->
+		trace_info_get_trace_type(TraceInfo, TraceType),
+		(
+			TraceType = shallow_trace(CallFromFullSlot),
+			trace__stackref_to_string(CallFromFullSlot,
+				CallFromFullSlotStr),
+			string__append_list(["\tif (", CallFromFullSlotStr,
+				") {\n\t\tMR_discard_ticket();\n\t}"],
+				UndoStmt)
+		;
+			TraceType = deep_trace,
+			UndoStmt = "MR_discard_ticket();"
+		),
+		UndoFillCode = node([
+			pragma_c([], [pragma_c_raw_code(UndoStmt)],
+				will_not_call_mercury, no, no, yes) - ""
+		]),
+		( CodeModel = model_non ->
+			SuccessUndoFillCode = empty,
+			FailureUndoFillCode = UndoFillCode
+		;
+			SuccessUndoFillCode = UndoFillCode,
+			FailureUndoFillCode = UndoFillCode
+		)
+	;
+		SuccessUndoFillCode = empty,
+		FailureUndoFillCode = empty
 	}.
 
 trace__prepare_for_call(TraceCode) -->
cvs diff: Diffing notes
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------