[m-dev.] for review: add MC++ implementation of library and runtime

[Tyson Dowd]

On 05-Dec-2000, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 04-Dec-2000, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> > +EMBEDALLDLLS=$(foreach file,$(ALLDLLS),/embed:$(file),$(patsubst %.dll,%,$(file)),Y)
> 
> Hmm, I couldn't parse that.  The `$(foreach ...)' command expects
> something of the form `$(foreach VAR,LIST,TEXT)', but your foreach
> command has four commas in it (not counting the ones nested inside `$(patsubst ...)'),
> not two.
> 
> I guess you and/or GNU Make are assuming that TEXT can contain commas.
> But this isn't documented in the GNU Make manual.

Yes.

If you want to embed a resource in a .dll, then you give the option
        /embed:filename

If you want to give it a specific name, you have to do
        /embed:filename,resourcename

And if you want to make it publically accessible, you have to add a ,Y
        /embed:filename,resourcename,Y

GNU Make doesn't allow $(patsubst ... ) rules to do double subtitutions
(the pattern I want is %.dll,%,Y).

Microsoft's option handling doesn't allow you to omit the resourcename
if you want to give a ,Y option.  So you *have* to do a double
substitution (hence the use of foreach instead of a simple pattern style
substitution) and you have to put commas in the "text" portion of the
foreach.

GNU Make says nothing about embedded commas (and possible escaping of
them if it were a problem) but it also never mentions that pattern
subsitutions are restricted to a single subsitution.

> > +
> > +mercury.dll: $(ALLDLLS)
> > +	al -out:mercury.dll $(ALLDLLS)
> 
> If you bother to autoconfigure ILASM, shouldn't you also autoconfigure "al"?

I only bothered with ILASM to find out the SDK path, so I wasn't going
to bother with al just yet.

> It's quite disturbing that this allocates an array of a different type
> than array__init.  I suspect that one of them must be wrong.
> 
> It's inefficient to allocate a new object each time just to get the
> type of System.Object.  There must be a better way to do that.
> For example, it would be better to have
> 
> 	static System.Object *obj = new System.Object;
> 	Array = (MR_Word) 
> 		Array::CreateInstance(obj->GetType(), 0);
> 
> if that is allowed.

I've made everything in array call SORRY.  I've marked this code as
possibly inefficienty.  It's pretty pointless to try to get efficiency
when I haven't tested it past successful compilation yet, and we can run
it easily to benchmark it.

> 
> > Index: library/benchmarking.m
> ...
> > +:- pragma foreign_code("MC++", report_stats, will_not_call_mercury,
> > +"
> > +	// Do nothing
> > +").
> > +
> > +:- pragma foreign_code("MC++", report_full_memory_stats, will_not_call_mercury,
> > +"
> > +	// Do nothing
> > +").
> 
> There should be some XXX comments there.
> I'm sure the dotnet library must provide some access to performance statistics.

I spent some time looking into this, and it does, but not in managed
code in the current version.  You have to go via COM to get access to
the system performance counters.

I've made the code SORRY instead.

> > +:- pragma foreign_code("MC++",
> > +	get_user_cpu_miliseconds(Time::out), [will_not_call_mercury],
> > +"
> > +	// This won't return the elapsed time since program start,
> > +	// as it begins timing after the first call.
> > +	// For computing time differences it should be fine.
> > +	// XXX this is documented but not here.  Perhaps it is fixed
> > +	// in .NET Frameworks Beta1?
> > +	// Time = (int) (1000 * Diagnostics::Counter::GetElapsed());
> > +	MR_Runtime::SORRY(""foreign code for this function"");
> > +").
> 
> The comment here is confusing.
> What is documented but not here?
> What do you mean by "here"?

I got 'method not found' errors.

It still happens.  I don't know why -- it doesn't seem to find
'Diagnostics'.  I will have to spend some time looking into it at some
point in the future.

> >  :- impure pred do_nothing(T::in) is det.
> > -:- pragma c_code(do_nothing(X::in), [will_not_call_mercury, thread_safe], "
> > +:- pragma foreign_code("C", 
> > +	do_nothing(X::in), [will_not_call_mercury, thread_safe], "
> > +	ML_benchmarking_dummy_word = (MR_Word) X;
> > +").
> > +:- pragma foreign_code("MC++", 
> > +	do_nothing(X::in), [will_not_call_mercury, thread_safe],
> > +"
> > +	volatile MR_Word ML_benchmarking_dummy_word;
> >  	ML_benchmarking_dummy_word = (MR_Word) X;
> >  ").
> 
> The comment above says that you assign the output to a volatile global
> variable, but here you're assigning it to a volatile local variable.
> 
> I think it would be safer to assign it to a volatile static variable.

Can't do that, the MC++ compiler barfs.  

Another SORRY here until I can look into it futher.

> The numbering of the generic_res_call functions is odd;
> why is it called `call3', rather than `call0'?

Because it implements a generic call of an arity 3 procedure with a
result.  It's a little like call/3.

> > +:- pragma foreign_code("MC++", copy(_X::ui, _Y::uo),
> > +	[may_call_mercury], "
> > +	MR_Runtime::SORRY(""foreign code for this function"");
> > +").
> > +
> > +:- pragma foreign_code("MC++", copy(_X::in, _Y::uo),
> > +	[may_call_mercury], "
> > +	MR_Runtime::SORRY(""foreign code for this function"");
> > +").
> 
> I tried to refrain from commenting on calls to SORRY(), but this one
> looks fairly simple to implement: isn't there a Clone() function or
> something like that in System.Object?

Yes, but it does shallow copies.  There are some Clone() methods that do

I already spent over an hour looking into this, and it looks like we
will have to implement deep_copy using RTTI.

> 
> > +:- pragma foreign_code("MC++", unify(X::in, Y::in),
> > +	[may_call_mercury], "
> > +{
> > +        MR_TypeInfo             type_info;
> > +        MR_TypeCtorInfo         type_ctor_info;
> > +        MR_Box                  tmp;
> > +        int                     arity;
> > +        MR_TypeInfoParams       params;
> > +	
> > +        type_info = (MR_TypeInfo) TypeInfo_for_T;
> > +
> > +        type_ctor_info = dynamic_cast<MR_Word> (type_info->GetValue(0));
> > +        if (type_ctor_info == 0) {
> > +            type_ctor_info = type_info;
> > +        }
> > +
> > +        // XXX insert code to handle higher order....
> > +        if (0) {
> > +
> > +        } else {
> > +            arity = mr_convert::ToInt32(type_ctor_info->GetValue(0));
> > +            // params = ???
> > +        }
> > +
> > +        // args = params;
> > +
> > +	switch(arity) {
> > +	case 0: 
> > +                SUCCESS_INDICATOR = generic::generic_call2(
> > +                	type_ctor_info->GetValue(1),
> > +			X, Y);
> 
> It would be a good idea to assign type_ctor_info->GetValue(1) to a
> named local variable.  I guess that "GetValue(1)" must be the field
> which holds the unify procedure?  If so, it would be a very good idea
> to define macros for the magic numbers like 1 and 3 above.

There used to be such marcos, but Zoltan deleted them when changing to a
typed representation for RTTI.

I've added some new ones, but haven't gone the whole hog as I'm not sure
how long we will continue to use this representation.

> 
> > +    static void
> > +    mercury__builtin____Compare____int_0_0(
> > +        MR_Word_Ref result, MR_Integer x, MR_Integer y)
> > +    {
> > +                    MR_COMPARE_LESS);
> > +            MR_newobj(*result, r, 0);
> > +    }
> 
> There seems to be a line or two missing there.
> 
> Better try compiling it ;-)

This was a problem cvs diff.

> > +    static int
> > +    mercury__builtin__do_unify__int_0_0(MR_Box x, MR_Box y)
> > +    {
> > +            return mercury__builtin____Unify____int_0_0(
> > +                    mr_convert::ToInt32(x), 
> > +                    mr_convert::ToInt32(y)); 
> > +    }
> 
> `mr_convert' should at least be `MR_convert'.
> 
> But even `MR_convert' is not a good name to use for a top-level
> module.  Nor is `MR_Runtime'.  For a system that supports proper
> namespaces, we should use them, rather than `MR_' prefixes.
> 
> Likewise we shouldn't be using `mercury__builtin__' prefixes,
> instead it should be `mercury::builtin::' or something like that.
> 
> You may want to argue that it's not important to get the names right now. 
> But my experience with getting the names right in the C runtime is
> that it is much much better to get the names right as soon as
> possible; anything else will only increase the pain.

I've fixed mr_convert and MR_Runtime.  They are now
mercury::runtime::convert and mercury::runtime::errors.

I've also replaced the mercury__builtin__ with mercury::builtin::

> > +:- pragma foreign_code("MC++",
> > +	get_determinism_2(_Pred::pred(out, di, uo) is cc_multi,
> > +			Det::out(bound(cc_multi))),
> > +	will_not_call_mercury,
> > +	"MR_newobj(Det, ML_CC_MULTI, 0);"
> > +).
> 
> That code may soon all be wrong, when we change the way enums are
> represented in the IL back-end.  Rather than using `MR_newobj', you
> should use `MR_newenum' or something like that.

Ok, fixed that.

> 
> > +XXX :- external stops us from using this
> > +
> > +:- pragma foreign_code("MC++", builtin_throw(_T::in), [will_not_call_mercury], "
> > +        mercury_exception *ex;
> > +
> > +        // XXX should look for string objects and set them as the message
> > +
> > +        if (false) {
> > +            ex = new mercury_exception;
> > +        } else {
> > +            ex = new mercury_exception(""hello"");
> > +        }
> > +
> > +        throw ex; 
> 
> The argument of the mercury_exception type should be `System.Object'
> (or perhaps `Mercury.Object'), not `System.String'.

This is just a stub, it's not real exception support.  I've been trying
to get it to use the string constructor of the builtin exception type,
as this will print a message for uncaught exceptions. 

> 
> > Index: library/float.m
> 
> I notice that there are no test cases in our test suite for
> several of the routines that you implemented MC++ version for,
> including at least float__ceiling_to_int, float__floor_to_int,
> float__round_to_int, and float__truncate_to_int.  It would be a good
> idea to add some.

This can be said of *lots* of the library modules.

> >  	% Maximum floating-point number
> > -:- pragma c_code(float__max = (Max::out),
> > +:- pragma foreign_code("C", float__max = (Max::out),
> >  		[will_not_call_mercury, thread_safe],
> >  	"Max = ML_FLOAT_MAX;").
> > +:- pragma foreign_code("MC++", float__max = (Max::out),
> > +		[will_not_call_mercury, thread_safe],
> > +	"Max = Double::MaxValue;").
> 
> Is that System.Double?
> Is `Double' the right type to use here?
> I think you should probably be using a typedef of some kind,
> e.g. `MR_Float'.

Sounds like a great idea.  The MC++ compiler won't accept it thought.
I can get it to give an internal compiler error, or it complains that
mercury::MR_BoxedFloat isn't a class/struct/union.

So I'm going to stop wasting time for the moment and leave this as is
for now.

> > Index: library/io.m
> > @@ -1130,6 +1130,13 @@
> >  	#endif
> >  ").
> >  
> > +:- pragma foreign_code("MC++", "
> > +	static MR_Word ML_io_stream_names;
> > +	static MR_Word ML_io_user_globals;
> > +	static int next_id;
> > +").
> 
> Hmm...  you're relying here on foreign "MC++" code not being inlined
> into different translation units (like we do with foreign "C" code).
> 
> I think at least it is worth a comment explaining why the MC++
> version is different from the C version.

Either I don't understand this comment, or you are confusing
foreign_code with foreign_decls.

foreign_code cannot be inlined into multiple different translation units.
It can only appear once.  It has to be put into a single
translation unit.  No other foreign_code can rely on it being put into
it's own translation unit (and it could be said that I'm breaking this
rule because I don't write a separate decl).

foreign_*decls* can be inlined into multiple different translation units.

> 
> > +:- pragma foreign_code("MC++", 
> > +		io__get_globals(Globals::uo, IOState0::di, IOState::uo), 
> > +		will_not_call_mercury, "
> > +	Globals = ML_io_user_globals;
> > +	update_io(IOState0, IOState);
> > +").
> > +
> > +:- pragma foreign_code("MC++", 
> > +		io__set_globals(Globals::di, IOState0::di, IOState::uo), 
> > +		will_not_call_mercury, "
> > +	ML_io_user_globals = Globals;
> > +	update_io(IOState0, IOState);
> > +").
> 
> The `update_io' there should be `ML_update_io'.
> Likewise for everywhere else in io.m and time.m.
> And likewise for `initial_io_state' and `final_io_state'.
> 
> I guess those belong in a separate change.

Yes, that's a good idea.  

> 
> > +#define MR_DownCast(Cast, Expr) dynamic_cast<Cast>(Expr)
> > +#define MR_UpCast(Cast, Expr) ((Cast) (Expr))
> 
> Hmm, these macros are not very safe; they can be used to perform casts
> of kinds other than the kind implied by the name.  The name expresses
> the intent, which is good, but this intent is not enforced.
> 
> It may be worth mentioning this in a comment.

Ok.

> 
> > +:- pragma foreign_code("MC++", "
> >  
> > +static MR_MercuryFile new_mercury_file(IO::Stream *stream, int line_number) {
> > +	MR_MercuryFile mf = new MR_MercuryFileStruct();
> > +	mf->stream = stream;
> > +	mf->line_number = line_number;
> > +	mf->id = next_id++;
> > +	return mf;
> > +}
> > +
> > +static MR_MercuryFile mercury_stdin =
> > +	new_mercury_file(Console::OpenStandardInput(), 1);
> > +static MR_MercuryFile mercury_stdout =
> > +	new_mercury_file(Console::OpenStandardOutput(), 1);
> > +static MR_MercuryFile mercury_stderr =
> > +	new_mercury_file(Console::OpenStandardError(), 1);
> 
> Hmm, this will cause problems for GUI programs that don't want a
> console window.  I think for Windows these streams should ideally be
> created lazily, so that you don't get a blank console window if you're
> not using it.

To even consider this Microsoft will have to fix the outstanding bug
that ilasm has no syntax or command line options for creating GUI
programs.

> It would be OK to just add an XXX comment for now.

Perhaps XXX should actually be -- "use Peter Ross's stream library".
The io library is starting to become extremely kludgy, and makes many
assumptions about having a POSIX environment.

> 
> > +static MR_MercuryFile mercury_stdin_binary =
> > +	new_mercury_file(0, 1);
> > +static MR_MercuryFile mercury_stdout_binary =
> > +	new_mercury_file(0, 1);
> > +
> > +static MR_MercuryFile mercury_current_text_input =
> > +	new_mercury_file(Console::OpenStandardInput(), 1);
> > +static MR_MercuryFile mercury_current_text_output =
> > +	new_mercury_file(Console::OpenStandardOutput(), 1);
> > +static MR_MercuryFile mercury_current_binary_input =
> > +        new_mercury_file(0, 1);
> > +static MR_MercuryFile mercury_current_binary_output =
> > +        new_mercury_file(0, 1);
> 
> The current_* streams should point to the same objects as
> mercury_stdin, mercury_stdout, etc., rather than being copies.

I'll XXX that too.  It's not trivial to arrange this.

> 
> > +:- pragma foreign_code("MC++", "
> > +
> > +MR_MercuryFile
> > +static mercury_open(MR_String filename, MR_String type)
> > +{
> > +        MR_MercuryFile mf = new MR_MercuryFileStruct();
> > +        IO::FileMode fa;
> > +        IO::Stream *stream;
> > +
> > +                // XXX get this right...
> > +        if (type == ""r"") {
> > +                fa = IO::FileMode::Open;
> > +        } else if (type == ""w"") {
> > +                fa = IO::FileMode::Append;
> > +        } else {
> > +                fa = IO::FileMode::OpenOrCreate;
> > +        }
> 
> That looks very scary.  I would rather have a call to SORRY() than a
> file open command that might get things wrong.

Then a SORRY you shall get.

I did mention that all this stuff is experimental right, and will as
likely delete all your files as work? 

> > +:- pragma foreign_code("MC++", "
> > +
> > +static void
> > +mercury_print_string(MR_MercuryFile mf, MR_String s)
> > +{
> > +        IO::StreamWriter *w = new IO::StreamWriter(mf->stream);
> > +        w->Write(s);
> > +        w->Flush();
> 
> That is likely to be quite inefficient, I'd guess.
> An XXX comment is warranted.

Ok.

> 
> > +static void
> > +mercury_print_binary_string(MR_MercuryFile mf, MR_String s)
> > +{
> > +        IO::StreamWriter *w = new IO::StreamWriter(mf->stream);
> > +        w->Write(s);
> > +        w->Flush();
> > +}
> > +
> > +").
> 
> Likewise.
> 
> > +static void
> > +mercury_close(MR_MercuryFile mf)
> > +{
> > +        if (mf != mercury_stdin &&
> > +            mf != mercury_stdout &&
> > +            mf != mercury_stderr)
> > +        {
> > +                mf->stream->Close();
> > +                mf->stream = 0;
> > +        }
> > +}
> 
> s/0/NULL/

This would of course involve #defining NULL.

It's not currently defined because I'm deliberatley not using the
"standard" system libraries (just in case they drag in more non .NET
stuff).


> > +:- pragma foreign_code("MC++",
> > +	io__flush_binary_output(_Stream::in, IO0::di, IO::uo),
> > +		[may_call_mercury, thread_safe], "{
> > +	MR_Runtime::SORRY(""foreign code for this function"");
> > +	update_io(IO0, IO);
> > +}").
> 
> Any reason why the code for this needs to be different
> to the code for io__flush_output?

No, it just hadn't been implemented yet.  Since the implementation is
just and paste I'll do it now.


Phew, finally got through this part...

-- 
       Tyson Dowd           # 
                            #  Surreal humour isn't everyone's cup of fur.
     trd at cs.mu.oz.au        # 
http://www.cs.mu.oz.au/~trd #
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------