[m-rev.] for review: fix mantis bug #314

[Peter Wang]

On Sun, 02 Mar 2014 21:05:37 +1100 (EST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > > 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.
> 

Ok.  I meant only to save saying "ordinary ordinary" frames.
It won't come up often anyway.

> > > 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).
> 

Never mind.

> > 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.

Ugh, I don't know how I ended up with that.

> > 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.

Ok.

> > > 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.
> 

Extra word in: "the current segment of on the nondet stack"

Peter