[m-dev.] Bug #309

[Mark Brown]

Hi Paul,

On Wed, Jan 22, 2014 at 11:02 AM, Paul Bone <paul at bone.id.au> wrote:
>
> I've been looking at bug #309.  The problem is caused when a special value
> like store.store(S) or io.state is existentially typed.  I have some
> questions about the correct fix, but first I'll explain the bug.  Consider:
>
>     :- some[S] pred root_env(environment(S)::out, S::uo) is det => store(S).
>
>     :- implementation.
>
>     root_env(Environment, Store) :-
>         store.init(Store0),
>         store.new_mutvar(map.init, Mappings, Store0, Store),
>         Environment = root_environment(Mappings).
>
> Recall that:
>
>     :- some[S] pred store.init(store(S)::uo) is det.
>
> and that the typeclass store(S) is also defined in store.m.
>
>
> When the user's code is compiled, root_env returns an Environment, and Store
> and the typeclass info variable for store(S).  In the HLDS casts are
> introduced to assign each variable to it's existentially typed equivalent so
> that the HLDS is type correct.
>
> Before code generation the compiler allocates stack slots for variables that
> need them, for special variables like io and state(S) dummy stack slots with
> negative numbers are allocated.
>
> Finally during HLDS->LLDS code generation (before LLDS optimizations) the
> epilogue for the procedure is generated, it must copy each output variable
> into the register where the callers expect to find it.  This means copying
> the existentially typed version of Store (named ExistQStore) into MR_r2.
>
>     /*Start of procedure epilogue*/
>         MR_r1 = MR_sv(1);
>             /* Placing ExistQTypeClassInfo_for_store */
>         MR_r2 = MR_tempr1;
>             /* Placing ExistQEnvironment */
>         MR_r3 = !!! Error !!!;
>             /* Placing ExistQStore */
>         MR_decr_sp_and_return(2);
>             /* Return from procedure call */
>     The compiler crashed before it could write the rval for the 3rd
>     assignment, as it would have written an assignment from a negative stack
>     slot.  I've written the remaining instruction from a compiler that I've
>     'changed'.
>
> ExistQStore was stored in MR_r2 after the exist_cast (Yes that's wrong, I'll
> come back to it) however the second instruction clobbered MR_r2.  This eas
> deemed okay because ExistQStore was also stored in the stack, in stack slot
> -2.  This assignment crashes the compiler when it tries to write it out.
>
> As mentioned above store(S) (like io) is special and supposed to never
> actually appear in generated code,

It's never *required* to appear, but it still does. If you pass IO to
list.foldl, for example, the implementation will copy around a piece
of garbage for you.

> this is true but it's not true when it's
> existentially typed - it cannot be as the type is abstract.  So if there was
> no register conflict when generating the procedure's epilogue then an
> arbitrary garbage value would have been saved into MR_r2, and the calling
> program would pass it around and as long as it was never coerced into the
> wrong type it would have been safe, it's never actually used or
> dereferenced.  This appears to be a harmless bug,

It's harmless, but not a bug. There's no information here, and garbage
faithfully represents that.

> it only becomes harmful
> when a register conflict occurs and the compiler crashes.

Trying to generate code that reads something out of a dummy slot is a bug.

>
> The compiler currently tries to avoid conflicts by copying about-to-be
> clobbered information out of the way, it uses the existing stack slot if
> there is one.  I modified this behaviour to ignore stack slots with negative
> numbers, in these cases a new register is used and tried it MR_r5, the
> compiler happily generates the code below, which will probably run
> correctly, well equally as correctly as the above bug without a register
> conflict.
>
>     /*Start of procedure epilogue*/
>         MR_r1 = MR_sv(1);
>             /* Placing ExistQTypeClassInfo_for_store */
>         MR_r2 = MR_tempr1;
>             /* Placing ExistQEnvironment */
>         MR_r3 = MR_r5;
>             /* Placing ExistQStore */
>         MR_decr_sp_and_return(2);
>             /* Return from procedure call */
>
> So my question is, how should this _really_ be fixed. Is a hack like the one
> I've used 'good enough'?

Possibly. But why not just leave off the assignment to MR_r3 altogether?

>  Or on the other extreme, should creating an
> existential version of store(S) or io be an error?

Definitely not.

Cheers,
Mark.