[m-rev.] for review: improperly initialised rtti structures in java

[Julien Fischer]

On Thu, 25 Jun 2009, Peter Wang wrote:

> Branches: main
>
> The Java backend was sometimes generating code like this to initialise RTTI
> data structures:
>
>    foo_type_info.init(...,
>        /* cast */ new TypeInfo_Struct(bar_type_ctor_info), ...);
>    bar_type_ctor_info.init(...);
>
> where `bar_type_ctor_info' is actually a type_info.
>
> The problem is that the fields of the non-initialised `bar_type_ctor_info'
> would be copied into the new TypeInfo_Struct object.  The "cast" is of course
> also unnecessary as the bar is already a TypeInfo_Struct.
>
> This patch attempts to fix the problem in two ways:
>
> 1. Don't allocate a new TypeInfo_Struct object to emulate the "cast" unless the
> value is actually a TypeCtorInfo_Struct, avoiding the problem of copying
> uninitialised fields.  Currently this is implemented by a runtime check because
> the MLDS `cast' operation doesn't record the original type of the value being
> cast.

(You presumably saw the the XXX regrading that MLDS cast op?)

> 2. Instead of relying on mlds_to_rtti.m to return a list of RTTI data
> structures where sub-structures appear before the structures that reference
> them, explicitly perform a topological sort.  This should be more robust.
>
>
> Unrelated change: use pre-allocated PseudoTypeInfo instances for common
> variable numbers (1 through 5).
>
>
> compiler/rtti_to_mlds.m:
>        Add a function to order a list of RTTI definitions as above.
>
>        Use cons instead of list.append in some places now that we can.
>
> compiler/mlds_to_java.m:
>        Call the function to order RTTI definitions before outputing
>        the initialisations.
>
>        Call TypeInfo_Struct.maybe_new instead of allocating new
>        TypeInfo_Structs.
>
>        Generate code that uses pre-allocated PseudoTypeInfo instances.
>
> java/runtime/TypeInfo_Struct.java:
>        Add a `maybe_new' method for "casting" TypeCtorInfo_Structs to
>        TypeInfos or returning the argument unchanged.
>
>        Delete a copy constructor; now unused.
>
>        Add some assertions.
>
> java/runtime/PseudoTypeInfo.java:
>        Add static instances of PseudoTypeInfo.
>
> tests/hard_coded/Mmakefile:
> tests/hard_coded/java_rtti_bug.exp:
> tests/hard_coded/java_rtti_bug.m:
>        Add test case.
>
> diff --git a/compiler/mlds_to_java.m b/compiler/mlds_to_java.m
> index 0c7a995..bd22bc6 100644
> --- a/compiler/mlds_to_java.m
> +++ b/compiler/mlds_to_java.m
> @@ -98,6 +98,7 @@
> :- import_module ml_backend.ml_code_util.  % for ml_gen_local_var_decl_flags.
> :- import_module ml_backend.ml_type_gen.   % for ml_gen_type_name
> :- import_module ml_backend.ml_util.
> +:- import_module ml_backend.rtti_to_mlds.
> :- import_module parse_tree.builtin_lib_types.
> :- import_module parse_tree.file_names.    % for mercury_std_library_name.
> :- import_module parse_tree.java_names.
> @@ -1752,15 +1753,27 @@ output_rtti_assignments(Indent, ModuleInfo,
> ModuleName, Defns, !IO) :-
>         Defns = []
>     ;
>         Defns = [_ | _],
> +        OrderedDefns = order_mlds_rtti_defns(Defns),
>         indent_line(Indent, !IO),
>         io.write_string("static {\n", !IO),
>         list.foldl(
> -            output_rtti_defn_assignments(Indent + 1, ModuleInfo, ModuleName),
> -            Defns, !IO),
> +            output_rtti_defns_assignments(Indent + 1, ModuleInfo, ModuleName),
> +            OrderedDefns, !IO),
>         indent_line(Indent, !IO),
>         io.write_string("}\n", !IO)
>     ).
>
> +:- pred output_rtti_defns_assignments(indent::in, module_info::in,
> +    mlds_module_name::in, list(mlds_defn)::in, io::di, io::uo) is det.
> +
> +output_rtti_defns_assignments(Indent, ModuleInfo, ModuleName, Defns, !IO) :-
> +    % Separate cliques.
> +    indent_line(Indent, !IO),
> +    io.write_string("//\n", !IO),
> +    list.foldl(
> +        output_rtti_defn_assignments(Indent, ModuleInfo, ModuleName),
> +        Defns, !IO).
> +
> :- pred output_rtti_defn_assignments(indent::in, module_info::in,
>     mlds_module_name::in, mlds_defn::in, io::di, io::uo) is det.
>
> @@ -3414,19 +3427,27 @@ output_unop(ModuleInfo, Unop, Expr, ModuleName, !IO) :-
>         % Similarly for conversions from TypeCtorInfo to TypeInfo.
>         (
>             Type = mlds_pseudo_type_info_type,
> -            Expr = ml_const(mlconst_int(_))
> +            Expr = ml_const(mlconst_int(N))
>         ->
>             maybe_output_comment("cast", !IO),
> -            io.write_string("new jmercury.runtime.PseudoTypeInfo(", !IO),
> -            output_rval(ModuleInfo, Expr, ModuleName, !IO),
> -            io.write_string(")", !IO)
> +            ( have_preallocated_pseudo_type_var(N) ->
> +                io.write_string("jmercury.runtime.PseudoTypeInfo.K", !IO),
> +                io.write_int(N, !IO)
> +            ;
> +                io.write_string("new jmercury.runtime.PseudoTypeInfo(", !IO),
> +                output_rval(ModuleInfo, Expr, ModuleName, !IO),
> +                io.write_string(")", !IO)
> +            )
>         ;
>             ( Type = mercury_type(_, ctor_cat_system(cat_system_type_info), _)
>             ; Type = mlds_type_info_type
>             )
>         ->
> +            % XXX we really should be able to tell if we are casting a
> +            % TypeCtorInfo or a TypeInfo

That's probably going to be rather difficult as the compiler doesn't
keep track of where type_ctor_infos are acting as type_infos properly,
i.e. the common case optimization described at the beginning of
compiler/polymorphism.m.  (This is already involved in several open
bugs).  This is one the the next major rewrite of polymorphism.m is
going to have do properly.

>             maybe_output_comment("cast", !IO),
> -            io.write_string("new jmercury.runtime.TypeInfo_Struct(", !IO),
> +            io.write_string("jmercury.runtime.TypeInfo_Struct.maybe_new(",
> +                !IO),
>             output_rval(ModuleInfo, Expr, ModuleName, !IO),
>             io.write_string(")", !IO)

That looks okay.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------