[m-rev.] For review: ensure proper initialisation of pretty_printer

[Julien Fischer]

On Thu, 22 Apr 2010, Ralph Becket wrote:

> If this could be reviewed quickly, that would be great.  I'm using
> this patch to track down a problem in the G12 IC solver.
>
> Estimated hours taken: 2
> Branches: main
>
> There is a race condition between the initialisation of modules
> and mutables, mainly because there is no guaranteed order of
> initialisation.  This is a problem for pretty_printer.m since
> we would like other modules to be able to register formatters
> for types they define via an ':- initialise' directive.

IMO, this is hacky way of fixing the problem.  A better solution would be to
ensure that initialisation of any global variables required by the
pretty-printer is done as part the standard library's initialisation, in
ML_init_io_state.  That is guaranteed to occur before any :- initialise
directives are processed.

> This patch fixes the problem by using C instead of a mutable
> for the default formatter map variable.

That doesn't seem to be the case -- io_formatter_map is still a mutable
below.

> Index: library/pretty_printer.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/pretty_printer.m,v
> retrieving revision 1.9
> diff -u -r1.9 pretty_printer.m
> --- library/pretty_printer.m	18 May 2009 05:55:13 -0000	1.9
> +++ library/pretty_printer.m	22 Apr 2010 05:39:14 -0000
> @@ -884,7 +884,7 @@
> %-----------------------------------------------------------------------------%
> % Convenience predicates.
>
> -:- mutable(io_formatter_map, formatter_map, initial_formatter_map, ground,
> +:- mutable(io_formatter_map, formatter_map, new_formatter_map, ground,
>     [attach_to_io_state, untrailed, thread_local]).

Is the change to making this thread_local intentional?  (I remember we
discussed this in-person yesterday, but thought that we had concluded
the opposite.)

> :- mutable(io_pp_params, pp_params, pp_params(78, 100, triangular(100)),
> @@ -892,16 +892,76 @@
>
> %-----------------------------------------------------------------------------%
>
> +    % Because there is no guaranteed order of module initialisation, we need
> +    % to ensure that we do the right thing if other modules try to update the
> +    % default formatter_map before this module has been initialised.
> +    %
> +    % All of this machinery is needed to avoid a race condition between
> +    % initialise directives and initialisation of mutables.
> +    %
> +:- pragma foreign_decl("C",
> +"
> +    static MR_Bool pretty_printer_is_initialised = MR_FALSE;
> +    static MR_Word pretty_printer_default_formatter_map = NULL;
> +").

This is wrong, if we end up with references to either variable outside
of pretty_printer.o then linking will fail.

Either do:

 	:- pragma foreign_dec("C", "
 	    extern MR_Bool pretty_printer_is_initialised;
 	    extern MR_Word pretty_printer_default_formatter_map;
 	").

 	:- pragma foreign_code("C", "
 	    MR_Bool pretty_printer_is_initialised = MR_FALSE;
 	    MR_Word pretty_printer_default_formatter_map = NULL;
 	").

or:
 	:- pragma foreign_decl("C", local,
     		static MR_Bool pretty_printer_is_initialised = MR_FALSE;
     		static MR_Word pretty_printer_default_formatter_map = NULL;
 	").

and then _ensure_ that any foreign procs. in pretty_printer.m cannot be
inlined in other modules.

I recommend the first; it is less error prone.

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