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

[Julien Fischer]

On Thu, 22 Apr 2010, Ralph Becket wrote:

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

It's wrong in any case - the system wouldn't treat initialise directives
in library.m any differently from ones in pretty_printer.m, i.e. the
race condition is still there.

What I was referring to is io.init_state/2 (export to C as
ML_io_init_state), which is called before any initialise directives.
No changes to library.m are necessary.  It would mean importing (and
indeed foreign_importing) the pretty_printer module into io.m, but that
should be okay.

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

As I said, the first one.

The other thing that is required here is to have a lock associated with
the default formatter map in .par grades.

I suggest having a look at the handling of the I/O globals in the io
module (grep for ML_io_user_globals), which has very similar requirements.

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