[m-rev.] refactor nondet stack traversal code
Fergus Henderson
fjh at cs.mu.OZ.AU
Tue Nov 11 21:05:16 AEDT 2003
On 11-Nov-2003, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> On 11-Nov-2003, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > The reason for that is that when we're stepping over the "terminal top
> > frame of a nondet side branch", I/O happens, but func doesn't get called.
> >
> > (That's not a bug, is it? That is, is it right to not traverse the
> > variables in such a frame?)
>
> I would think that *any* I/O during garbage collection would be a very
> obvious bug.
I should have said "I/O happens (if the FILE pointer dump_fp is non-null)".
For GC, we pass dump_fp == NULL, so no I/O occurs.
> when the current code prints a message about "terminal top frame",
> then the frame contains no roots,
OK, so there is no bug there either. Good.
> It would
> better to call func with an extra parameter, or to call a different function,
> to signify that the debugger should take action but agc shouldn't.
>
> The current code has two jobs: traversal, and printing out information about
> each frame. Leaving in a FILE * argument implies, at least to me, that you
> haven't separated the two completely enough.
That is a fair criticism. But separating the two would actually make
the interface more complicated, so I'm not sure that it is worth it.
There's really three jobs: traversal, categorization of frames,
and printing the results of that categorization. It would be possible
to separate the _printing_ out, at the cost of a more complicated
interface. But the categorization can't be separated out, since
which category a frame is in depends on how the traversal got there.
And this categorization is needed only for debugging, not for accurate GC.
I suppose I could use an enum for the results of categorization,
and pass this enum to func, as shown in the diff below. But I'm not
sure that this is really an improvement. This version is 24 lines longer,
and instead of an undesirable FILE * parameter and I/O code in the static
function MR_dump_nondet_stack_frame(), we now have an two undesirable
parameters (the category enum, plus an top_fr pointer) in externally
visible interface, namely the function pointer type which is used for the
parameter to the extern function MR_traverse_nondet_stack_from_layout(),
With this patch, the interface is a little bit more complicated;
as well as the two extra parameters, the garbage collector's function
would have to start with
if (category == MR_TERMINAL_TOP_FRAME_ON_SIDE_BRANCH) {
return;
}
Of course, these complications don't really _need_ to be propagated to
the interface. I could add another wrapper function which contains
the above three lines. But this would add even more complication.
So I don't really think this is any cleaner. But I don't mind really.
I'm happy to commit this relative diff below, or even to add another
level of wrapper functions to avoid modifying the external interface.
Whichever you think is nicer.
----------
Estimated hours taken: 1
Branches: main
Some more refactoring.
runtime/mercury_stack_trace.h:
runtime/mercury_stack_trace.c:
Move the code for dumping the information about the frame category
from MR_step_over_nondet_frame() to MR_dump_nondet_stack_frame().
Workspace: /home/jupiter/fjh/ws-jupiter/mercury
Index: runtime/mercury_stack_trace.c
===================================================================
RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_trace.c,v
retrieving revision 1.57
diff -u -d -r1.57 mercury_stack_trace.c
--- runtime/mercury_stack_trace.c 11 Nov 2003 09:12:26 -0000 1.57
+++ runtime/mercury_stack_trace.c 11 Nov 2003 10:03:52 -0000
@@ -36,7 +36,7 @@
static MR_Traverse_Nondet_Frame_Func MR_dump_nondet_stack_frame;
static const char *MR_step_over_nondet_frame(
MR_Traverse_Nondet_Frame_Func *func, void *func_data,
- FILE *fp, int level_number, MR_Word *fr);
+ int level_number, MR_Word *fr);
static void MR_init_nondet_branch_infos(MR_Word *base_maxfr,
const MR_Label_Layout *top_layout, MR_Word *base_sp,
MR_Word *base_curfr);
@@ -449,7 +449,7 @@
if (print_vars && base_maxfr > MR_nondet_stack_trace_bottom) {
problem = MR_step_over_nondet_frame(
MR_dump_nondet_stack_frame, fp,
- fp, level_number, base_maxfr);
+ level_number, base_maxfr);
if (problem != NULL) {
fprintf(fp, "%s\n", problem);
return;
@@ -463,13 +463,42 @@
}
static void
-MR_dump_nondet_stack_frame(void *fp,
- const MR_Label_Layout *top_layout, MR_Word *base_sp, MR_Word *base_curfr,
- int level_number)
+MR_dump_nondet_stack_frame(void *fp, MR_Nondet_Frame_Category category,
+ MR_Word *top_fr, const MR_Label_Layout *top_layout, MR_Word *base_sp,
+ MR_Word *base_curfr, int level_number)
{
- /* XXX we ignore the return value */
- (*MR_address_of_trace_browse_all_on_level) (fp, top_layout, base_sp,
- base_curfr, level_number, MR_TRUE);
+ FILE *dump_fp = fp;
+
+ switch (category) {
+ case MR_INTERNAL_FRAME_ON_SIDE_BRANCH:
+ fprintf(dump_fp, " internal frame on nondet side branch ");
+ MR_printnondstackptr(top_fr);
+ fprintf(dump_fp, "\n");
+ break;
+ case MR_FRAME_ON_MAIN_BRANCH:
+ fprintf(dump_fp, " on main nondet branch ");
+ MR_printnondstackptr(top_fr);
+ fprintf(dump_fp, "\n");
+ break;
+ case MR_TERMINAL_TOP_FRAME_ON_SIDE_BRANCH:
+ fprintf(dump_fp, " terminal top frame of a nondet side branch ");
+ MR_printnondstackptr(base_curfr);
+ fprintf(dump_fp, "\n");
+ break;
+ case MR_TOP_FRAME_ON_SIDE_BRANCH:
+ fprintf(dump_fp, " top frame of a nondet side branch ");
+ MR_printnondstackptr(base_curfr);
+ fprintf(dump_fp, "\n");
+ break;
+ default:
+ MR_fatal_error("invalid MR_Nondet_Frame_Category");
+ }
+
+ if (category != MR_TERMINAL_TOP_FRAME_ON_SIDE_BRANCH) {
+ /* XXX we ignore the return value */
+ (*MR_address_of_trace_browse_all_on_level) (dump_fp, top_layout,
+ base_sp, base_curfr, level_number, MR_TRUE);
+ }
}
void
@@ -508,7 +537,7 @@
level_number++;
if (base_maxfr > MR_nondet_stack_trace_bottom) {
problem = MR_step_over_nondet_frame(func, func_data,
- NULL, level_number, base_maxfr);
+ level_number, base_maxfr);
if (problem != NULL) {
MR_fatal_error(problem);
}
@@ -536,7 +565,7 @@
static const char *
MR_step_over_nondet_frame(MR_Traverse_Nondet_Frame_Func *func,
- void *func_data, FILE *dump_fp, int level_number, MR_Word *fr)
+ void *func_data, int level_number, MR_Word *fr)
{
MR_Stack_Walk_Step_Result result;
MR_Determinism determinism;
@@ -551,24 +580,19 @@
MR_Code *redoip;
MR_Code *success;
const char *problem;
+ MR_Nondet_Frame_Category category;
if (MR_find_matching_branch(fr, &branch)) {
base_sp = MR_nondet_branch_infos[branch].branch_sp;
base_curfr = MR_nondet_branch_infos[branch].branch_curfr;
label_layout = MR_nondet_branch_infos[branch].branch_layout;
topfr = MR_nondet_branch_infos[branch].branch_topfr;
- if (dump_fp) {
- if (base_sp == NULL) {
- fprintf(dump_fp, " internal frame on nondet side branch ");
- MR_printnondstackptr(topfr);
- fprintf(dump_fp, "\n");
- } else {
- fprintf(dump_fp, " on main nondet branch ");
- MR_printnondstackptr(topfr);
- fprintf(dump_fp, "\n");
- }
+ if (base_sp == NULL) {
+ category = MR_INTERNAL_FRAME_ON_SIDE_BRANCH;
+ } else {
+ category = MR_FRAME_ON_MAIN_BRANCH;
}
- (*func)(func_data, label_layout, base_sp, base_curfr,
+ (*func)(func_data, category, topfr, label_layout, base_sp, base_curfr,
level_number);
MR_erase_temp_redoip(fr);
proc_layout = label_layout->MR_sll_entry;
@@ -621,12 +645,8 @@
}
if (redoip == NULL) {
- if (dump_fp) {
- fprintf(dump_fp,
- " terminal top frame of a nondet side branch ");
- MR_printnondstackptr(fr);
- fprintf(dump_fp, "\n");
- }
+ (*func)(func_data, MR_TERMINAL_TOP_FRAME_ON_SIDE_BRANCH,
+ NULL, NULL, NULL, fr, level_number);
MR_erase_temp_redoip(fr);
success = MR_succip_slot(fr);
@@ -642,12 +662,8 @@
}
label_layout = internal->i_layout;
- if (dump_fp) {
- fprintf(dump_fp, " top frame of a nondet side branch ");
- MR_printnondstackptr(fr);
- fprintf(dump_fp, "\n");
- }
- (*func)(func_data, label_layout, NULL, fr, level_number);
+ (*func)(func_data, MR_TOP_FRAME_ON_SIDE_BRANCH, NULL, label_layout,
+ NULL, fr, level_number);
MR_erase_temp_redoip(fr);
/*
Index: runtime/mercury_stack_trace.h
===================================================================
RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_trace.h,v
retrieving revision 1.30
diff -u -d -r1.30 mercury_stack_trace.h
--- runtime/mercury_stack_trace.h 11 Nov 2003 09:12:26 -0000 1.30
+++ runtime/mercury_stack_trace.h 11 Nov 2003 09:59:58 -0000
@@ -100,7 +100,15 @@
** function for each frame.
*/
+typedef enum {
+ MR_FRAME_ON_MAIN_BRANCH,
+ MR_INTERNAL_FRAME_ON_SIDE_BRANCH,
+ MR_TOP_FRAME_ON_SIDE_BRANCH,
+ MR_TERMINAL_TOP_FRAME_ON_SIDE_BRANCH
+} MR_Nondet_Frame_Category;
+
typedef void MR_Traverse_Nondet_Frame_Func(void *user_data,
+ MR_Nondet_Frame_Category category, MR_Word *top_fr,
const MR_Label_Layout *layout, MR_Word *base_sp,
MR_Word *base_curfr, int level_number);
--
Fergus Henderson <fjh at cs.mu.oz.au> | "I have always known that the pursuit
The University of Melbourne | of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh> | -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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