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

[Ralph Becket]

Julien Fischer, Thursday, 22 April 2010:
> 
> 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.

I did start putting code in an initialise directive in library.m, but
that seemed more of a hack.  It required:
- giving pretty_printer a special place in the library;
- exporting stuff from library.m;
- hence making library.m an "active" part of the library, rather than a
  bookkeeping mechanism.

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

Ah, that's a leftover I forgot to delete.

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

Ah, this was nagging me.  Thanks (I didn't know about the 'local' option
to foreign_decl - ta for pointing that one out!).

What's your preferred course of action here?

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