[m-rev.] for review: fix mantis bug #314
Zoltan Somogyi
zoltan.somogyi at runbox.com
Sun Mar 2 21:05:37 AEDT 2014
> > 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.
Done.
> > 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.
There is no formal classification, no enum type with ordinary and temp members.
Most code that deals with frames on the nondet stack consists of invocations of
the MR_redo and MR_fail macros, which treat all nondet stack frames the same.
> I have got used to "buffer frames" now, but perhaps "sentinel frame"
> might be a better name.
Agreed.
> You can leave either change to another commit, if you choose to make
> either one.
I have made them in this change.
> > -------------------
> >
> > 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.
Done in this one.
> > --- 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
It appears that mercury_conf_param.h was full of insane mixtures of spaces
and tabs. I have converted the whole of it to spaces.
zs [132] cat !$
cat post
> > 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.
Done.
> > 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.
There is no formal classification, no enum type with ordinary and temp members.
Most code that deals with frames on the nondet stack consists of invocations of
the MR_redo and MR_fail macros, which treat all nondet stack frames the same.
> I have got used to "buffer frames" now, but perhaps "sentinel frame"
> might be a better name.
Agreed.
> You can leave either change to another commit, if you choose to make
> either one.
I have made them in this change.
> > -------------------
> >
> > 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.
Done in this one.
> > --- 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
It appears that mercury_conf_param.h was full of insane mixtures of spaces
and tabs. I have converted the whole of it to spaces.
> > 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.
The code that accesses MR_zone_extend_threshold is guarded by the same
condition, "defined(MR_STACK_SEGMENTS) && !defined(MR_HIGHLEVEL_CODE)",
as the declaration of that field is guarded by in mercury_memory_zones.h.
I agree that the presence of a reference to MR_HIGHLEVEL_CODE seems strange,
but the strangeness was not introduced by this diff, and any move to "fix"
this situation should be a separate change.
> > +
> > +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.
Done.
> > 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.
An explicit cast of WHAT to MR_Word *? zone->MR_zone_extend_threshold
and zone->MR_zone_end are both ALREADY MR_Word * (after this change,
if not before).
> > + #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
Done.
> > +/*
> > +** 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.
Done, slightly reworded.
> > +
> > +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_*
OK.
> > 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.
Done, as "second and later".
> > ** 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
Fixed in both places.
> > /*
> > -** 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.
The middle two paragraphs of that suggestion are wrong.
MR_at_or_above_bottom_nondet_frame returns true for ALL nondet stack frames;
it is used solely to prevent stack dumps from descending into nonexistent
frames.
I reworded the comment, to clarify it, and to remove the irrelevant aside
about redoip hijacking.
> > --- 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 ...
Done, with s/reuse/abuse/.
> > +** 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.
Done, with slight changes.
> > - 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
Fixed.
> > + ** 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.
I think that would be overkill.
I did add to the part of mercury_stacks.h where MR_NONDET_FIXED_SIZE
and related macros are defined a pointer to the documentation of sentinel
frames in mercury_stacks.c.
> > +
> > +#ifdef MR_DEBUG_STACK_SEGMENTS
> > + printf("creating buffer frame:\n", incr);
>
> extra argument
Fixed.
>
> > @@ -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.
Done, here and in the only other place that needs to know about sentinel
frames, the code that generates stack traces.
> > + ** 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;;
>
> ;
Fixed.
> > 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.
Done.
> > + #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.
It is obvious.
> > /*
> > -** 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 *
Fixed.
> > #endif
> > + } else if (MR_streq(MR_optarg, "A")) {
> > + printf("MR_nondetstackdebug = %d\n", MR_nondetstackdebug);
> > + MR_lld_print_always_enabled = MR_TRUE;
>
> Delete printf?
Done.
> > @@ -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
Done.
> > @@ -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?
That would remove this code from the context that gives it its reason
for being. I added a comment about the block's purpose instead.
> > --- 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
Fixed.
> > 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
Fixed.
> Peter
Thanks for the review, Peter. The updated diff is attached.
I will commit this after a final round of bootchecks.
Zoltan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DIFF.mar2
Type: application/octet-stream
Size: 136643 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20140302/0888792c/attachment.obj>
More information about the reviews
mailing list