[m-dev.] Bug #309

[Paul Bone]

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, 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 only becomes harmful
when a register conflict occurs and the compiler crashes.

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'?  Or on the other extreme, should creating an
existential version of store(S) or io be an error?  It doesn't make sense to
me why doing this is useful so I'm leaning to the latter option, but it may
be considered too heavy handed.

Thanks.



-- 
Paul Bone
http://www.bone.id.au