[m-rev.] for review: fix mantis bug #314
Peter Wang
novalazy at gmail.com
Thu Feb 27 11:13:13 AEDT 2014
On Fri, 21 Feb 2014 17:30:24 +1100 (EST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> The diff is with -b; the actual diff has some more white space changes.
>
> For review by Peter Wang and/or Julien.
>
> I don't think this diff should go on the release branch until we have more
> experience with it. On the other hand, the existing code for nondet stack
> segments was already broken, so it may not matter too much.
I agree it should not appear on the 14.01 branch.
> Fix Mantis bug 314 for tmp frames created by nondet procedures.
> Also fix some bugs in related code, and improve the related debugging
> infrastructure.
>
> -------------------
>
> runtime/mercury_stacks.[ch]:
> Fix bug 314 for temp frames created by nondet procedures. The fix will
> probably also work for *det* procedures that create tmp frames on the
Might as well use "temp" consistently.
> nondet stack, but I can't think of a way to test that, because det
> procedures create such frames only in very specific circumstances,
> and I cannot think of a way to nest a recursive call inside those
> circumstances.
>
> The problem was that when we were creating new temp frames on
> the nondet stack, we did not check whether the current nondet stack segment
> had room for them. We now do.
>
> The stack trace tracing code needs to know the size of each nondet stack
> frame, since it uses the size to classify frames as temp or ordinary.
> The size is given by the difference in address between the address of the
> frame and the address of the previous frame. This difference would yield
> an incorrect size and hence an incorrect frame classification if a temp
> frame were allowed to have a frame on a different segment as its
> immediate predecessor.
>
> We prevent this by putting an ordinary (i.e. non-temp) frame at the bottom
> of every new nondet stack segment as a buffer. We hand-build this frame,
> since it is not an "ordinary" ordinary frame. It is not created by a call,
> so it has no meaningful success continuation, and since it does not make
> any calls, no other frame's success continuation can point to it either.
>
> If backtracking reaches this buffer frame, we use this fact to free
> all the segments beyond the one the buffer frame is in, but keep the
> frame the buffer frame is in, since we are likely to need it again.
>
> Document the reason why MR_incr_sp_leaf() does not have to check
> whether a new stack segment is needed. (See the fix to llds_out_instr.m
> below.)
>
> runtime/mercury_stack_trace.[ch]:
> When traversing the nondet stack, treat the buffer frame specially.
> We have to, since it is an ordinary frame (i.e. it is not a temp frame),
> but it is not an "ordinary" ordinary frame: it does not make calls,
> and hence calls cannot return to it, and it does not return to any
> other frame either. It therefore does not have the layout structures
> (label and proc) that the nondet stack traversal expects to find.
Is there a reason not to classify them separately from ordinary frames?
The term "non-temp frames" can still cover ordinary and buffer frames.
I have got used to "buffer frames" now, but perhaps "sentinel frame"
might be a better name.
You can leave either change to another commit, if you choose to make
either one.
> -------------------
>
> runtime/mercury_context.c:
> runtime/mercury_engine.[ch]:
> runtime/mercury_misc.h:
> compiler/notes/failure.html:
> Fix white space.
>
failure.html should mention the differences between buffer frames and
ordinary ordinary frames. You can do it in another commit.
> diff --git a/runtime/mercury_conf_param.h b/runtime/mercury_conf_param.h
> index ba89750..8d4fdfb 100644
> --- a/runtime/mercury_conf_param.h
> +++ b/runtime/mercury_conf_param.h
> @@ -421,6 +421,10 @@
> ** Enables low-level debugging messages when updating the list of
> ** stack segments.
> **
> +** MR_DEBUG_STACK_SEGMENTS_SET_SIZE
> +** If set, we reduce the effective size of each segment
> +** to this number of words.
> +**
indentation
> @@ -875,6 +889,30 @@ MR_print_detstackptr(FILE *fp, const MR_Word *s)
> }
>
> void
> +MR_print_zone(FILE *fp, const MR_MemoryZone *zone)
> +{
> + fprintf(fp, "zone %p:\n", zone);
> + fprintf(fp, " bottom %p, top %p\n",
> + zone->MR_zone_bottom, zone->MR_zone_top);
> + fprintf(fp, " min %p, max %p, hardmax %p",
> + zone->MR_zone_min, zone->MR_zone_max, zone->MR_zone_hardmax);
> +#if defined(MR_STACK_SEGMENTS) && !defined(MR_HIGHLEVEL_CODE)
> + fprintf(fp, ", extend %p",
> + zone->MR_zone_extend_threshold);
> +#endif
> + fprintf(fp, "\n");
> +}
MR_HIGHLEVEL_CODE is redundant, or is it deliberate?
I see that it occurs a few times in the runtime.
> +
> +void
> +MR_print_zones(FILE *fp, const MR_MemoryZones *zones)
> +{
> + if (zones != NULL) {
> + MR_print_zone(fp, zones->MR_zones_head);
> + MR_print_zones(fp, zones->MR_zones_tail);
> + }
> +}
Replace with a loop.
> diff --git a/runtime/mercury_memory_zones.c b/runtime/mercury_memory_zones.c
> index 207aaa4..1cdeaa0 100644
> --- a/runtime/mercury_memory_zones.c
> +++ b/runtime/mercury_memory_zones.c
...
> @@ -849,8 +853,29 @@ MR_setup_redzones(MR_MemoryZone *zone)
> #endif
>
> #if defined(MR_STACK_SEGMENTS) && !defined(MR_HIGHLEVEL_CODE)
> - zone->MR_zone_extend_threshold = (char *) zone->MR_zone_end
> - - MR_stack_margin_size;
> + zone->MR_zone_extend_threshold =
> + zone->MR_zone_end - MR_stack_margin_size_words;
I think an explicit cast to MR_Word * is clearer.
> + #ifdef MR_DEBUG_STACK_SEGMENTS_SET_SIZE
> + /*
> + ** Stack segment code is much easier to debug if the segments are small.
> + ** Since the segments have to be at least a page in size, we can cheat by
> + ** limiting the size of the part of the segment that we choose to use.
> + **
> + ** Note that since we don't access to the zone name here, we apply
have access
> + ** MR_DEBUG_STACK_SEGMENTS_SET_SIZE to all new zones. Ideally, we
> + ** should apply it only to the zones we are interested in. As it is,
> + ** setting MR_DEBUG_STACK_SEGMENTS_SET_SIZE to too small a value
> + ** will also slow down the parts of the program that use zones that
> + ** you are *not* interested in right now.
> + */
> +
> + if (zone->MR_zone_extend_threshold >
> + (zone->MR_zone_min + MR_DEBUG_STACK_SEGMENTS_SET_SIZE))
> + {
> + zone->MR_zone_extend_threshold =
> + zone->MR_zone_min + MR_DEBUG_STACK_SEGMENTS_SET_SIZE;
> + }
> + #endif
>
> assert((MR_Word *) zone->MR_zone_extend_threshold > zone->MR_zone_min);
>
> diff --git a/runtime/mercury_overflow.h b/runtime/mercury_overflow.h
> index 1dfb497..eb9ff90 100644
> --- a/runtime/mercury_overflow.h
> +++ b/runtime/mercury_overflow.h
> @@ -12,13 +12,49 @@
> #ifndef MERCURY_OVERFLOW_H
> #define MERCURY_OVERFLOW_H
>
> +#include "mercury_types.h"
> +#include "mercury_memory_zones.h"
> +
> +/*
> +** If maxfr is not in any of the zones of the nondet stack, abort the program,
> +** with the abort message including the message in `error', and (if not NULL)
> +** the location in `where'.
> +**
> +** If maxfr *is* in one of the zones of the nondet stack, then update
> +** its MR_zone_max field if maxfr exceeds it.
> +*/
Please add a hint:
If maxfr *is* in one of the zones of the nondet stack, then update
its MR_zone_max field if maxfr exceeds it, as the highest address
used in the zone so far.
> +
> +extern void MR_nondetstack_inclusion_check(MR_Word *maxfr,
> + const char *error, const char *where);
> +
> +typedef enum {
> + MR_OZONE_DETSTACK, MR_OZONE_NONDETSTACK, MR_OZONE_HEAP, MR_OZONE_OTHER
> +} MR_OverflowZone;
Cute, but I suggest MR_OVERFLOW_ZONE_*
> diff --git a/runtime/mercury_stack_trace.c b/runtime/mercury_stack_trace.c
> index 22c7114..8363465 100644
> --- a/runtime/mercury_stack_trace.c
> +++ b/runtime/mercury_stack_trace.c
...
> @@ -1199,13 +1207,19 @@ MR_dump_nondet_stack_from_layout(FILE *fp, MR_Word *limit_addr,
> return;
> }
>
> - if (limit_addr != NULL && base_maxfr < limit_addr) {
> - fprintf(fp, "<reached limit of dumped region>\n");
> - return;
> - }
> -
> - frame_size = base_maxfr - MR_prevfr_slot(base_maxfr);
> - if (frame_size == MR_NONDET_TEMP_SIZE) {
> + /*
> + ** Note that the actual frame size is NOT the apparent frame size
> + ** for the buffer frames at the beginnings of nondet stack segments
> + ** after the first. However, in such cases, the apparent size
at the beginning of the second and subsequent nondet stack segments.
> + ** will be either negative (if the logically previous segment is
> + ** at a higher address than the segment that holds the buffer frame)
> + ** or a positive number that is least as big as the size of the buffer
> + ** frame (if the logically previous segment is at a lower address).
> + ** Therefore if the apparent size is MR_NONDET_TEMP_SIZE or
> + ** MR_DET_TEMP_SIZE, that must be the actual size as well.
> + */
> + apparent_frame_size = base_maxfr - MR_prevfr_slot(base_maxfr);
> + if (apparent_frame_size == MR_NONDET_TEMP_SIZE) {
> MR_print_nondetstackptr(fp, base_maxfr);
> fprintf(fp, ": temp\n");
> fprintf(fp, " redoip: ");
> diff --git a/runtime/mercury_stack_trace.h b/runtime/mercury_stack_trace.h
> index d133465..64d5582 100644
> --- a/runtime/mercury_stack_trace.h
> +++ b/runtime/mercury_stack_trace.h
> @@ -127,12 +127,12 @@ extern const char *MR_dump_stack_from_layout_clique(FILE *fp,
> ** MR_dump_nondet_stack
> **
> ** This function dumps the control slots of the nondet stack.
> -** If limit_addr is nonnull, dumps only frames above limit_addr.
> -** If limit is nonzero, dumps at most limit frames.
> +** If frame_limit is nonzero, dumps at most limit frames.
> +** If line__limit is nonzero, dumps at most limit lines.
> ** The output format is not meant to be intelligible to non-implementors.
> */
line_limit
> @@ -140,15 +140,14 @@ extern void MR_dump_nondet_stack(FILE *fp, MR_Word *limit_addr,
> ** MR_dump_nondet_stack_from_layout
> **
> ** This function dumps the nondet stack.
> -** If limit_addr is nonnull, dumps only frames above limit_addr.
> -** If limit is nonzero, dumps at most limit frames.
> +** If frame_limit is nonzero, dumps at most limit frames.
> +** If line__limit is nonzero, dumps at most limit lines.
line_limit
> @@ -268,21 +267,43 @@ extern MR_StackWalkStepResult
> const char **problem_ptr);
>
> /*
> -** MR_stack_trace_bottom should be set to the address of global_success,
> +** MR_stack_trace_bottom_ip should be set to the address of global_success,
> ** the label main/2 goes to on success. Stack dumps terminate when they
> ** reach a stack frame whose saved succip slot contains this address.
> */
>
> -extern MR_Code *MR_stack_trace_bottom;
> +extern MR_Code *MR_stack_trace_bottom_ip;
>
> /*
> -** MR_nondet_stack_trace_bottom should be set to the address of the buffer
> +** MR_nondet_stack_trace_bottom_fr should be set to the address of the buffer
> ** nondet stack frame created before calling main. Nondet stack dumps terminate
> ** when they reach a stack frame whose redoip contains this address. Note that
> ** the redoip and redofr slots of this frame may be hijacked.
> +**
> +** Since address comparisons with MR_nondet_stack_trace_bottom_fr make sense
> +** only if the other pointer is also in the same stack segment (if the grade
> +** allows stack segments), record the identity of the nondet stack segment
> +** that contains MR_nondet_stack_trace_bottom_fr.
> */
I suggest:
The bottom nondet stack frame is created before calling main;
MR_nondet_stack_trace_bottom_fr holds its address. Nondet stack
dumps terminate when they reach a stack frame whose redoip contains
this address. Note that the redoip and redofr slots of this frame
may be hijacked.
MR_above_bottom_nondet_frame evaluates to true if fr is on the same
segment as the bottom frame (if stack segments are used), and fr has
a higher address than the bottom frame.
MR_at_or_above_bottom_nondet_frame additionally evaluates to true if
fr has the SAME address as the bottom frame.
MR_nondet_stack_trace_bottom_zone holds the identity of the stack
segment containing the bottom frame.
>
> -extern MR_Word *MR_nondet_stack_trace_bottom;
> +extern MR_Word *MR_nondet_stack_trace_bottom_fr;
> +#ifdef MR_STACK_SEGMENTS
> +extern MR_MemoryZone *MR_nondet_stack_trace_bottom_zone;
> +#endif
> +#ifndef MR_STACK_SEGMENTS
> + #define MR_above_bottom_nondet_frame(fr) \
> + ((fr) > MR_nondet_stack_trace_bottom_fr)
> + #define MR_at_or_above_bottom_nondet_frame(fr) \
> + ((fr) >= MR_nondet_stack_trace_bottom_fr)
> +#else
> + #define MR_above_bottom_nondet_frame(fr) \
> + (!MR_in_zone((fr), MR_nondet_stack_trace_bottom_zone) \
> + || ((fr) > MR_nondet_stack_trace_bottom_fr))
> + #define MR_at_or_above_bottom_nondet_frame(fr) \
> + (!MR_in_zone((fr), MR_nondet_stack_trace_bottom_zone) \
> + || ((fr) >= MR_nondet_stack_trace_bottom_fr))
> +#endif
>
> /*
> ** The different Mercury determinisms are internally represented by integers.
> diff --git a/runtime/mercury_stacks.c b/runtime/mercury_stacks.c
> index fc19903..0daa965 100644
> --- a/runtime/mercury_stacks.c
> +++ b/runtime/mercury_stacks.c
...
> @@ -238,6 +245,16 @@ MR_Word *MR_new_detstack_segment(MR_Word *sp, int n)
> MR_stackvar(1) = (MR_Word) old_sp;
> MR_stackvar(2) = (MR_Word) MR_succip;
>
> + /*
> + ** This may not be for a leaf procedure; we use it to avoid a check for
> + ** whether we have run out of the new detstack segment.
we reuse the macro to avoid a check for ...
> + ** XXX It is *theoretically* possible for a single stack frame to need
> + ** more memory than is available in the whole of the new segment.
> + ** However, if this is true, then the program is screwed anyway.
> + ** We cannot save it, though we *could* give a meaningful error message
> + ** instead of just leaving the program to crash.
> + */
> MR_incr_sp_leaf(n);
>
> #ifdef MR_DEBUG_STACK_SEGMENTS
> @@ -250,89 +267,228 @@ MR_Word *MR_new_detstack_segment(MR_Word *sp, int n)
> return MR_sp;
> }
>
> +/*
> +** There are two ways that normal execution can remove a nondet stack frame.
> +**
> +** - backward execution can remove the currently top nondet stack frame
> +** by invoking MR_fail(), and
> +** - forward execution can remove a sequence of nondet stack frames
> +** at the top of the nondet stack when it commits to a solution.
> +**
> +** We implement commits by taking a snapshot of maxfr and later restoring it.
> +** Since the nondet stack frames cut away by such a restoration of maxfr
> +** do not get any control at commits, freeing nondet stack segments only
> +** when control reaches the placeholder frame at the bottom of such frames
> +** is clearly not sufficient on its own to eventually recover all nondet stack
> +** segments.
> +**
> +** We could make commits free all nondet stack segments beyond the one
> +** containing the restored maxfr. However, that solution has three problems.
> +**
> +** - It requires stack-segment-specific code at commits.
> +** - It requires more code at commits, slowing them down.
> +** - It is likely that the freed segment will be needed quite soon. Freeing a
> +** zone and then allocating it again for the same purpose is probably
> +** slower than simply keeping and reusing it.
> +**
> +** We therefore adopt the following technique:
> +**
> +** 1 When we fail back to the last frame of a nondet stack segment, we
> +** KEEP that segment, but free any other segments beyond this.
> +**
> +** 2 When we run out of the current nondet stack segment, we check whether
> +** we already have the next segment. If not, we allocate a new one; if yes,
> +** we keep it, again freeing any segments beyond the one we have just started
> +** using.
... we check whether we have already allocated the next segment.
If not, we allocate a new one. If so, we keep that segment, but free
any segments beyond it.
> +**
> +** 3 We do not recover nondet stack stack segments at commits.
> +**
> +** Parts 1 and 2 above each limit the amount of allocated but not currently
> +** used memory to about one segment. It is possible for the amount of
> +** allocated but not currently used nondet stack space to exceed twice the
> +** size of a segment, possibly by a lot, but in only one circumstance:
> +** *after* a commit cuts away several segments of nondet stack, and *before*
> +** the next time the program reaches either end (min or max) of the current
> +** segment of the nondet stack. If the program uses the nondet stack at all
> +** intensively, and if nondet stack segments are small, then this period of
> +** time *should* be acceptably small. The gain we get from accepting this
> +** downside is that we don't have to deal with segments except when we are
> +** at a segment boundary.
> +*/
> +
> static MR_MemoryZone *MR_rewind_nondetstack_segments(MR_Word *maxfr);
> -void
> -MR_nondetstack_segment_extend_slow_path(MR_Word *old_maxfr, int incr)
> +MR_Word *
> +MR_new_nondetstack_segment(MR_Word *maxfr, int incr)
> {
> - MR_Word *new_maxfr;
> - MR_MemoryZones *list;
> - MR_MemoryZone *new_zone;
> + MR_Word *buffer_maxfr;
> + MR_Word *old_maxfr;
> + MR_Word *old_curfr;
> + MR_MemoryZone *new_cur_zone;
> + MR_MemoryZones *new_prev_zones;
>
> - /*
> - ** Pop off the nondet stack segments until maxfr is within the bounds of
> - ** the top segment.
> - */
> - new_zone = MR_rewind_nondetstack_segments(old_maxfr);
> + old_maxfr = maxfr;
> + old_curfr = MR_curfr;
>
> - /* Try to make a frame on the top segment. */
> - new_maxfr = old_maxfr + incr;
> - if (new_maxfr < (MR_Word *) MR_CONTEXT(MR_ctxt_nondetstack_zone)->
> - MR_zone_extend_threshold)
> - {
> - MR_maxfr_word = (MR_Word) new_maxfr;
> - if (new_zone != NULL) {
> - MR_release_zone(new_zone);
> - }
> - return;
> - }
> +#ifdef MR_DEBUG_STACK_SEGMENTS
> + printf("\nadding new nondet stack segment");;
> + printf("\ncontext: %p", &MR_ENGINE(MR_eng_context));
> + printf("\nold maxfr: ");
> + MR_printnondetstack(stdout, old_maxfr);
> + printf("\nold curfr: ");
> + MR_printnondetstack(stdout, old_curfr);
> + printf("\n");
> +#endif
>
> - if (new_zone == NULL) {
> - /* We perform explicit overflow checks so redzones just waste space. */
> - new_zone = MR_create_or_reuse_zone("nondetstack_segment",
> + new_cur_zone = MR_rewind_nondetstack_segments(maxfr);
> + if (new_cur_zone == NULL) {
> + /*
> + ** There is no old segment to reuse in the nondet stack itself,
> + ** so allocate a new one (possibly one that was freed earlier).
> + **
> + ** Note that we perform explicit overflow checks, so redzones
> + ** would just waste space.
> + */
> + new_cur_zone = MR_create_or_reuse_zone("nondetstack_segment",
> MR_nondetstack_size, 0, 0, MR_default_handler);
> }
>
> - list = MR_GC_malloc_uncollectable_attrib(sizeof(MR_MemoryZones),
> +#ifdef MR_DEBUG_STACK_SEGMENTS
> + printf("\nbefore creating new nondet segment:\n");
> + MR_print_zone(stdout, MR_CONTEXT(MR_ctxt_nondetstack_zone));
> + printf("\n");
> +#endif
> +
> + new_prev_zones = MR_GC_malloc_uncollectable_attrib(sizeof(MR_MemoryZones),
> MR_ALLOC_SITE_RUNTIME);
> + new_prev_zones->MR_zones_head = MR_CONTEXT(MR_ctxt_nondetstack_zone);
> + new_prev_zones->MR_zones_tail = MR_CONTEXT(MR_ctxt_prev_nondetstack_zones);
> + MR_CONTEXT(MR_ctxt_prev_nondetstack_zones) = new_prev_zones;
> + MR_CONTEXT(MR_ctxt_nondetstack_zone) = new_cur_zone;
> + MR_CONTEXT(MR_ctxt_maxfr) = new_cur_zone->MR_zone_min;
> +
> + MR_maxfr_word = (MR_Word) MR_CONTEXT(MR_ctxt_maxfr);
> #ifdef MR_DEBUG_STACK_SEGMENTS
> - printf("create new nondet segment: old zone: %p, old maxfr %p\n",
> - MR_CONTEXT(MR_ctxt_nondetstack_zone), old_maxfr);
> - printf("old maxfr: ");
> - MR_printnondetstack(stdout, old_maxfr);
> + printf("\nafter creating new nondet segment\n", incr);
extra argument
> + printf("new maxfr: ");
> + MR_printnondetstack(stdout, MR_maxfr);
> + printf("\nnew cur zone:\n");
> + MR_print_zone(stdout, MR_CONTEXT(MR_ctxt_nondetstack_zone));
> + printf("new prev zones:\n");
> + MR_print_zones(stdout, MR_CONTEXT(MR_ctxt_prev_nondetstack_zones));
> + printf("\n");
> + fflush(stdout);
> #endif
>
> - list->MR_zones_head = MR_CONTEXT(MR_ctxt_nondetstack_zone);
> - list->MR_zones_tail = MR_CONTEXT(MR_ctxt_prev_nondetstack_zones);
> - MR_CONTEXT(MR_ctxt_prev_nondetstack_zones) = list;
> - MR_CONTEXT(MR_ctxt_nondetstack_zone) = new_zone;
> - MR_CONTEXT(MR_ctxt_maxfr) =
> - MR_CONTEXT(MR_ctxt_nondetstack_zone)->MR_zone_min;
> + /*
> + ** The stack trace tracing code needs to know the size of each nondet
> + ** stack frame, since it uses the size to classify frames as temp or
> + ** ordinary. The size is given by the difference in address between
> + ** the address of the frame and the address of the previous frame.
> + ** This difference would yield an incorrect size and hence an incorrect
> + ** frame classification if a temp frame were allowed to have a frame
> + ** on a different segment as its immediate predecessor.
> + **
> + ** We prevent this by putting an ordinary (i.e. non-temp) frame at the
> + ** bottom of every new nondet stack segment as a buffer. We hand-build
> + ** this frame, since it is not an "ordinary" ordinary frame. It is not
> + ** created by a call, so it has no meaningful success continuation,
> + ** and since it does not make any calls, no other frame's success
> + ** continuation can point to it either.
> + **
> + ** We store three pieces of information in the buffer frame.
> + **
> + ** - The maxfr at the time the buffer frame was created, which we store
> + ** in the prevfr slot. This is actually the address of the logically
> + ** previous frame, so we are using the slot for its intended purpose,
> + ** but the difference between the addresses of the two frames is NOT
> + ** the size of the buffer frame.
> + **
> + ** - The curfr at the time the buffer frame was created, which we store
> + ** in the succfr slot. This is NOT actually the frame of the success
> + ** continuation; we can store it there because this frame HAS no
> + ** meaningful success continuation, so the slot is not needed for its
> + ** intended purpose.
> + **
> + ** - The address of the MR_MemoryZone structure of the zone containing
> + ** the buffer frame, which we store in framevar 1. This is used by
> + ** the code of MR_pop_nondetstack_segment.
> + */
> +
> + buffer_maxfr = MR_maxfr + (MR_NONDET_FIXED_SIZE + 1);
> + MR_prevfr_slot_word(buffer_maxfr) = (MR_Word) old_maxfr;
> + MR_succfr_slot_word(buffer_maxfr) = (MR_Word) old_curfr;
> + MR_succip_slot_word(buffer_maxfr) =
> + (MR_Word) MR_ENTRY(MR_do_not_reached);
> + MR_redofr_slot_word(buffer_maxfr) = (MR_Word) buffer_maxfr;
> + MR_redoip_slot_word(buffer_maxfr) =
> + (MR_Word) MR_ENTRY(MR_pop_nondetstack_segment);
> + MR_based_framevar(buffer_maxfr, 1) = (MR_Word) new_cur_zone;
I suggest adding buffer frame-specific macros, MR_NONDET_BUFFRAME_SIZE,
MR_nondet_bufframe_maxfr_slot, MR_nondet_bufframe_curfr_slot, etc.
and moving the comment block alongside the macro definitions.
> +
> +#ifdef MR_DEBUG_STACK_SEGMENTS
> + printf("creating buffer frame:\n", incr);
extra argument
> + printf("buffer_maxfr: ");
> + MR_printnondetstack(stdout, buffer_maxfr);
> + printf("\nbuffer frame's prevfr slot: ");
> + MR_printnondetstack(stdout, MR_prevfr_slot(buffer_maxfr));
> + printf("\nbuffer frame's succfr slot: ");
> + MR_printnondetstack(stdout, MR_succfr_slot(buffer_maxfr));
> + printf("\nbuffer frame's redofr slot: ");
> + MR_printnondetstack(stdout, MR_redofr_slot(buffer_maxfr));
> + printf("\n");
> +#endif
>
> - MR_maxfr_word = (MR_Word) (MR_CONTEXT(MR_ctxt_maxfr) + incr);
> + /*
> + ** Reserve space for the new nondet stack frame on top of the buffer frame.
> + */
> + MR_maxfr_word = (MR_Word) (buffer_maxfr + incr);
>
> #ifdef MR_DEBUG_STACK_SEGMENTS
> - printf("create new nondet segment: new zone: %p, new maxfr %p\n",
> - MR_CONTEXT(MR_ctxt_nondetstack_zone), MR_maxfr);
> + printf("after creating buffer frame and reserving %d words:\n", incr);
> printf("new maxfr: ");
> MR_printnondetstack(stdout, MR_maxfr);
> + printf("\n");
> + fflush(stdout);
> #endif
> +
> + return MR_maxfr;
> }
> @@ -392,6 +568,96 @@ MR_define_entry(MR_pop_detstack_segment);
> MR_fatal_error("MR_pop_detstack_segment reached\n");
> #endif /* MR_STACK_SEGMENTS */
>
> +MR_define_entry(MR_pop_nondetstack_segment);
> +#ifdef MR_STACK_SEGMENTS
> +{
> + MR_Word *buffer_frame;
> + MR_Word *orig_maxfr;
> + MR_Word *orig_curfr;
> + MR_MemoryZone *orig_zone;
> + MR_MemoryZone *cur_zone;
> + MR_MemoryZones *prev_zones;
> + unsigned num_segments_removed;
> + MR_bool released_orig_zone;
> +
> + buffer_frame = MR_maxfr;
> + orig_maxfr = (MR_Word *) MR_prevfr_slot(buffer_frame);
> + orig_curfr = (MR_Word *) MR_succfr_slot(buffer_frame);
> + orig_zone = (MR_MemoryZone *) MR_based_framevar(buffer_frame, 1);
> +
Cross-reference MR_new_nondetstack_segment unless adding buffer
frame-specific macros.
> + cur_zone = MR_CONTEXT(MR_ctxt_nondetstack_zone);
> + prev_zones = MR_CONTEXT(MR_ctxt_prev_nondetstack_zones);
> +
> +#ifdef MR_DEBUG_STACK_SEGMENTS
> + printf("\nbefore removing old nondet segment:\n");
> +
> + printf("orig maxfr: ");
> + MR_print_nondetstackptr(stdout, orig_maxfr);
> + printf("\norig curfr: ");
> + MR_print_nondetstackptr(stdout, orig_curfr);
> + printf("\norig zone:\n");
> + MR_print_zone(stdout, orig_zone);
> +
> + printf("\ncur zone:\n");
> + MR_print_zone(stdout, cur_zone);
> + printf("prev zones:\n");
> + MR_print_zones(stdout, prev_zones);
> +
> + fflush(stdout);
> +#endif
> +
> + /*
> + ** As explained in the big comment above, we do not free the zone
> + ** of the segment we are leaving. It is very likely that we will need
> + ** it again, very soon, and reusing it from the list of nondet stack
> + ** segments in the context is significantly cheaper than reusing it
> + ** from the general pool.
> + */
> +
> + num_segments_removed = 0;
> + while (cur_zone != orig_zone) {
> + MR_MemoryZones *list_node_to_free;;
;
> + num_segments_removed++;
> + MR_release_zone(cur_zone);
> +
> + list_node_to_free = prev_zones;
> + cur_zone = prev_zones->MR_zones_head;
> + prev_zones = prev_zones->MR_zones_tail;
> + MR_GC_free_attrib(list_node_to_free);
> + }
> +
> + MR_CONTEXT(MR_ctxt_nondetstack_zone) = cur_zone;
> + MR_CONTEXT(MR_ctxt_prev_nondetstack_zones) = prev_zones;
> +
> + MR_CONTEXT(MR_ctxt_curfr) = orig_curfr;
> + MR_CONTEXT(MR_ctxt_maxfr) = orig_maxfr;
> + MR_curfr_word = (MR_Word) orig_curfr;
> + MR_maxfr_word = (MR_Word) orig_maxfr;
> +
> +#ifdef MR_DEBUG_STACK_SEGMENTS
> + printf("\nafter removing %d old nondet segment(s):\n",
> + num_segments_removed);
> + printf("cur zone:\n");
> + MR_print_zone(stdout, MR_CONTEXT(MR_ctxt_nondetstack_zone));
> + printf("prev zones:\n");
> + MR_print_zones(stdout, MR_CONTEXT(MR_ctxt_prev_nondetstack_zones));
> +
> + printf("maxfr: ");
> + MR_print_nondetstackptr(stdout, MR_maxfr);
> + printf("\ncurfr: ");
> + MR_print_nondetstackptr(stdout, MR_curfr);
> + printf("\n");
> +
> + fflush(stdout);
> +#endif
> +
> + MR_redo();
> +}
> +#else /* ! MR_STACK_SEGMENTS */
> + MR_fatal_error("MR_pop_nondetstack_segment reached\n");
> +#endif /* MR_STACK_SEGMENTS */
> +
> MR_END_MODULE
>
> #endif /* !MR_HIGHLEVEL_CODE */
> diff --git a/runtime/mercury_stacks.h b/runtime/mercury_stacks.h
> index 220b643..a845476 100644
> --- a/runtime/mercury_stacks.h
> +++ b/runtime/mercury_stacks.h
...
>
> - #define MR_nondetstack_extend_and_check(n) \
> + /*
> + ** The reason why we check for MR_maxfr < zone_min is that MR_maxfr may be
> + ** in a different stack segment than MR_CONTEXT(MR_ctxt_nondetstack_zone).
> + ** See the comment for MR_new_nondetstack_segment in mercury_stacks.c.
> + */
Please describe the prevfr argument.
> +
> + #define MR_nondetstack_extend_and_check(prevfr, incr) \
> do { \
> MR_Word *new_maxfr; \
> MR_Word *threshold; \
> - int incr; \
> \
> threshold = (MR_Word *) MR_CONTEXT(MR_ctxt_nondetstack_zone)-> \
> MR_zone_extend_threshold; \
> - incr = MR_NONDET_FIXED_SIZE + (n); \
> new_maxfr = MR_maxfr + incr; \
> if (MR_maxfr < MR_CONTEXT(MR_ctxt_nondetstack_zone->MR_zone_min) \
> || new_maxfr > threshold) \
> { \
> MR_save_registers(); \
> - MR_nondetstack_segment_extend_slow_path(MR_maxfr, incr); \
> + new_maxfr = MR_new_nondetstack_segment(MR_maxfr, (incr)); \
> MR_restore_registers(); \
> - } else { \
> - MR_maxfr_word = (MR_Word) new_maxfr; \
> + prevfr = new_maxfr - (incr); \
> } \
> + MR_maxfr_word = (MR_Word) new_maxfr; \
> } while (0)
>
> extern MR_Word *MR_new_detstack_segment(MR_Word *sp, int n);
> - extern void MR_nondetstack_segment_extend_slow_path(
> - MR_Word *old_maxfr, int n);
> +
> + /*
> + ** This function reserves a stack frame of n words on the nondet stack,
> + ** after conceptually (a) freeing any nondet stack segments that are wholly
> + ** above old_maxfr, and (b) creating a new nondet stack segment.
> + ** In practice, it will reuse an existing memory zone, if one is available.
> + ** It returns the address that should be the new value of maxfr.
> + */
> + extern MR_Word *MR_new_nondetstack_segment(MR_Word *old_maxfr, int n);
>
> #else /* !MR_STACK_SEGMENTS */
>
> - #define MR_detstack_extend_and_check(n) \
> + #define MR_detstack_extend_and_check(incr) \
> do { \
> - MR_sp_word = (MR_Word) (MR_sp + (n)); \
> + MR_sp_word = (MR_Word) (MR_sp + (incr)); \
> } while (0)
>
> - #define MR_detstack_extend_and_no_check(n) \
> + #define MR_detstack_extend_and_no_check(incr) \
> do { \
> - MR_sp_word = (MR_Word) (MR_sp + (n)); \
> + MR_sp_word = (MR_Word) (MR_sp + (incr)); \
> } while (0)
>
> - #define MR_nondetstack_extend_and_check(n) \
> + #define MR_nondetstack_extend_and_check(prevfr, incr) \
> do { \
> - MR_maxfr_word = (MR_Word) \
> - (MR_maxfr + (MR_NONDET_FIXED_SIZE + (n))); \
> + MR_maxfr_word = (MR_Word) (MR_maxfr + (incr)); \
Say why prevfr is unused in this case unless it is obvious
from the preceding description.
> diff --git a/runtime/mercury_wrapper.c b/runtime/mercury_wrapper.c
> index d33ebcd..34f2dda 100644
> --- a/runtime/mercury_wrapper.c
> +++ b/runtime/mercury_wrapper.c
...
> @@ -186,19 +193,20 @@ size_t MR_gen_nondetstack_zone_size = 4 * sizeof(MR_Word);
> size_t MR_heap_margin_size = 7 * 1024 * sizeof(MR_Word);
> #endif
>
> -double MR_heap_expansion_factor = 2.0;
> -
> /*
> -** When we use stack segments, we reserve the last MR_stack_margin_size bytes
> -** of each stack segment for leaf procedures. This way, leaf procedures that
> -** do not need this much stack space can allocate their stack space *without*
> -** incurring the cost of a test.
> +** When we use stack segments, we reserve the last MR_stack_margin_size_words
> +** words of each stack segment for leaf procedures. This way, leaf procedures
> +* that* do not need this much stack space can allocate their stack space
mislaid *
> @@ -2160,12 +2170,18 @@ MR_process_options(int argc, char **argv)
> #ifdef MR_NATIVE_GC
> MR_agc_debug = MR_TRUE;
> #endif
> + } else if (MR_streq(MR_optarg, "A")) {
> + printf("MR_nondetstackdebug = %d\n", MR_nondetstackdebug);
> + MR_lld_print_always_enabled = MR_TRUE;
Delete printf?
> @@ -2349,6 +2368,10 @@ MR_process_options(int argc, char **argv)
> MR_lld_print_enabled = 0;
> }
>
> + if (MR_lld_print_always_enabled) {
> + MR_lld_print_enabled = 1;
> + }
> +
MR_TRUE
> @@ -2848,8 +2871,30 @@ MR_define_entry(MR_do_interpreter);
> MR_succip_word = (MR_Word) MR_LABEL(wrapper_not_reached);
> MR_mkframe("interpreter", 1, MR_LABEL(global_fail));
>
> - MR_nondet_stack_trace_bottom = MR_maxfr;
> - MR_stack_trace_bottom = MR_LABEL(global_success);
> + MR_stack_trace_bottom_ip = MR_LABEL(global_success);
> + MR_nondet_stack_trace_bottom_fr = MR_maxfr;
> +#ifdef MR_STACK_SEGMENTS
> + {
> + MR_MemoryZone *cur_zone;
> + MR_MemoryZones *prev_zones;
> +
> + cur_zone = MR_CONTEXT(MR_ctxt_nondetstack_zone);
> + prev_zones = MR_CONTEXT(MR_ctxt_prev_nondetstack_zones);
> + while (MR_TRUE) {
> + if (MR_in_zone(MR_maxfr, cur_zone)) {
> + MR_nondet_stack_trace_bottom_zone = cur_zone;
> + break;
> + }
> +
> + if (prev_zones == NULL) {
> + MR_fatal_error("MR_maxfr is not in a nondetstack zone");
> + }
> +
> + cur_zone = prev_zones->MR_zones_head;
> + prev_zones = prev_zones->MR_zones_tail;
> + }
> + }
> +#endif
Factor this block out?
> diff --git a/runtime/mercury_wrapper.h b/runtime/mercury_wrapper.h
> index 1780cee..aa29b4f 100644
> --- a/runtime/mercury_wrapper.h
> +++ b/runtime/mercury_wrapper.h
> @@ -248,19 +251,19 @@ extern size_t MR_heap_margin_size;
...
> /*
> -** number of contexts to create per loop controlled loop.
> +** The nnumber of contexts to create per loop controlled loop.
> */
> extern MR_Unsigned MR_num_contexts_per_loop_control;
nnumber
> diff --git a/tests/hard_coded/Mmakefile b/tests/hard_coded/Mmakefile
> index a020b68..f89897b 100644
> --- a/tests/hard_coded/Mmakefile
> +++ b/tests/hard_coded/Mmakefile
...
> @@ -429,6 +431,15 @@ else
> mutable_decl
> endif
>
> +# Some tests are intented to test the handling of stack segments,
> +# and would generate stack overflows in grades that don't use stack segments.
intended
Peter
More information about the reviews
mailing list