[m-rev.] for review: C foreign types

[Fergus Henderson]

On 05-Mar-2002, Peter Ross <peter.ross at miscrit.be> wrote:
> 
> +++ doc/reference_manual.texi	5 Mar 2002 12:05:48 -0000
> +The C @samp{pragma foreign_type} declaration is of the form:
> +
> + at example
> +:- pragma foreign_type(c, @var{MercuryTypeName}, @var{CForeignType}).
> + at end example
> +
> +The @var{CForeignType} can be any C type name that obeys the following
> +restrictions.
> +The type must fit into a machine word.

The notion "machine word" is not defined.

Function types and incomplete types should not be allowed.

Array types should probably not be allowed (due to their
non-orthogonal semantics for parameter-passing etc. in C).

Enum types, struct types, and floating point types should ideally be allowed,
but your implementation only works for integer types and pointer types.

There should be test cases for all these
(tests for the cases which are not allowed should go in tests/invalid).

> +The type must be contain no parts that would have to be output after a
> +variable name in the generated C code.

s/The type/The type name/
"must be contain no parts" -- fix grammar.

It would be nicer to rephrase this in a way that didn't force
the reader to think like a compiler implementor.

> Index: compiler/foreign.m
...
>  to_exported_type(ModuleInfo, Type) = ExportType :-
...
> +		( Body = foreign_type(MaybeIL, MaybeC) ->
> +			( Target = c,
> +				( MaybeC = yes(c(NameStr)),
> +					Name = unqualified(NameStr)
> +				; MaybeC = no,
> +					error("to_exported_type: no C type")
> +				)
> +			; Target = il, 
> +				( MaybeIL = yes(il(_, _, Name))
> +				; MaybeIL = no,
> +					error("to_exported_type: no IL type")
> +				)
> +			; Target = java,
> +				error("to_exported_type: java NYI")
> +			; Target = asm,
> +				( MaybeC = yes(c(NameStr)),
> +					Name = unqualified(NameStr)
> +				; MaybeC = no,
> +					error("to_exported_type: no C type")
> +				)
> +			),

These should all call `sorry', rather than `error'.
And the error messages should be better; users might well encounter them.

Or alternative, if this is supposed to be checked properly
at some earlier stage in the front-end, use `unexpected'
rather than `sorry' or `error', and put a comment explaining why.

> +			ExportType = foreign(Name)
>  		;
>  			ExportType = mercury(Type)
>  		)
> @@ -593,8 +615,12 @@
>  to_type_string(Lang, ModuleInfo, Type) =
>  	to_type_string(Lang, to_exported_type(ModuleInfo, Type)).
>  
> -to_type_string(c, foreign(_ForeignType)) = _ :-
> -	sorry(this_file, "foreign types on a C backend").
> +to_type_string(c, foreign(ForeignType)) = Result :-
> +	( ForeignType = unqualified(Result0) ->
> +		Result = Result0
> +	;
> +		error("to_type_string: qualifed C type")

s/qualifed/qualified/

> Index: compiler/llds_out.m
> @@ -1970,6 +1970,11 @@
>  	->
>  		output_rval_as_type(Rval, float)
>  	;
> +		( { MaybeForeignType = yes(ForeignTypeStr) } ->
> +			io__write_string("(" ++ ForeignTypeStr ++ ") ")
> +		;
> +			[]
> +		),
>  		output_rval_as_type(Rval, word)

As mentioned above, that won't handle floating point types
or struct types correctly.

> @@ -2009,6 +2014,11 @@
>  		io__write_string(VarName),
>  		io__write_string(")")
>  	;
> +		( { MaybeForeignType = yes(_) } ->
> +			output_llds_type_cast(word)
> +		;
> +			[]
> +		),
>  		io__write_string(VarName)

Likewise here.

> Index: compiler/make_hlds.m
> @@ -794,6 +791,61 @@
>  		add_pragma_type_spec(Pragma, Context, Module0, Module,
>  			Info0, Info)
>  	;
> +		{ Pragma = foreign_type(_, _, Name) }
> +	->
> +		{ TypeId = Name - 0 },
> +		{ module_info_types(Module0, Types) },
> +		{ TypeStr = error_util__describe_sym_name_and_arity(
> +				Name / 0) },
> +		( 
> +		    { map__search(Types, TypeId, Defn) },
> +		    { hlds_data__get_type_defn_body(Defn, Body) },
> +		    { Body = foreign_type(MaybeIL, MaybeC) }
> +		->
> +		    { module_info_globals(Module0, Globals) },
> +		    { globals__lookup_bool_option(Globals, target_code_only,
> +			    TargetCode) },
> +		    ( { TargetCode = yes } ->
> +			{ globals__get_target(Globals, Target) },
> +			( { Target = c },
> +			    ( { MaybeC = yes(_) },
> +			    	{ Module = Module0 }
> +			    ; { MaybeC = no },
> +				{ ErrorPieces = [
> +				    words("Error: No C pragma"),
> +				    words("foreign_type for"),
> +				    fixed(TypeStr)
> +				] },
> +				error_util__write_error_pieces(Context,
> +					0, ErrorPieces),
> +				{ module_info_incr_errors(Module0, Module) }
> +			    )
> +			; { Target = il },
> +			    ( { MaybeIL = yes(_) },
> +			    	{ Module = Module0 }
> +			    ; { MaybeIL = no },
> +				{ ErrorPieces = [
> +				    words("Error: No IL pragma"),
> +				    words("foreign_type for"),
> +				    fixed(TypeStr)
> +				] },
> +				error_util__write_error_pieces(Context,
> +					0, ErrorPieces),
> +				{ module_info_incr_errors(Module0, Module) }
> +			    )
> +			; { Target = java },
> +			    	{ Module = Module0 }
> +			; { Target = asm },
> +			    	{ Module = Module0 }
> +			)
> +		    ;
> +			{ Module = Module0 }
> +		    )
> +		;
> +		    { error("add_item_clause: unable to find foreign type") }
> +		),
> +		{ Info = Info0 }	

This code should all be in a sub-routine.

Why are you checking the value of the `target_code_only' option here?
That should be completely irrelevant.  Checking that here is wrong.
Maybe there is something you need to check, but `target_code_only' is not it.

s/Error: No/Error: no/
s/foreign type/foreign type declaration/

There should be more verbose explanations if the --verbose-error-messages
option is enabled.

> Index: compiler/ml_code_gen.m
..
> @@ -858,7 +860,16 @@
>  	module_info_types(ModuleInfo, Types),
>  	list__filter_map((pred(TypeDefn::in, Import::out) is semidet :-
>  			hlds_data__get_type_defn_body(TypeDefn, Body),
> -			Body = foreign_type(_, _, Location),
> +			Body = foreign_type(MaybeIL, _MaybeC),
> +			( Target = c,
> +				fail
> +			; Target = il,
> +				MaybeIL = yes(il(_, Location, _))
> +			; Target = java,
> +				fail
> +			; Target = asm,
> +				fail
> +			),

Extract that code out as a det function that returns a maybe type
or a list, so that we'll know to modify it when new targets are added.

> +++ compiler/ml_code_util.m	5 Mar 2002 12:05:33 -0000
> @@ -2113,14 +2113,11 @@
>  ml_type_might_contain_pointers(mlds__native_float_type) = no.
>  ml_type_might_contain_pointers(mlds__native_bool_type) = no.
>  ml_type_might_contain_pointers(mlds__native_char_type) = no.
> -ml_type_might_contain_pointers(mlds__foreign_type(_, _, _)) = _ :-
> -	% It might contain pointers, so it's not safe to return `no',
> -	% but it also might not be word-sized, so it's not safe to
> -	% return `yes'.  Currently this case should not occur, since
> -	% currently `foreign_type' is only used for the IL back-end,
> -	% where GC is handled by the target language.
> -	unexpected(this_file, "--gc accurate and foreign_type").
> -	
> +	% Due to constraints from the LLDS back-end this type must be
> +	% word sized and on all other backends where this type is
> +	% supported garbage collection is handled by the target
> +	% language, so it is safe to return yes.
> +ml_type_might_contain_pointers(mlds__foreign_type(_)) = yes.

This is wrong.  The LLDS back-end constraints only imply that
the type can be at most word sized.  They do not prevent the
type from being smaller.  If the type is smaller, we'll be in
trouble.  A variable of this type will have its address taken,
cast to `MR_Word *', dereferenced, and the resulting lvalue
assigned to, and we'll end up clobbering adjacent values in memory.

Revent this fragment of the diff, then delete the second sentence in
the original comment, and change `unexpected' to `sorry'.

> Index: compiler/mlds.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mlds.m,v
> retrieving revision 1.85
> diff -u -r1.85 mlds.m
> --- compiler/mlds.m	3 Mar 2002 17:27:08 -0000	1.85
> +++ compiler/mlds.m	5 Mar 2002 12:05:34 -0000
> @@ -628,12 +628,9 @@
>  	;	mlds__native_float_type
>  	;	mlds__native_char_type
>  
> -		% This is a type of the MLDS target language.  Currently
> -		% this is only used by the il backend.
> +		% This is a type of the MLDS target language.

s/MLDS //

> @@ -1634,10 +1631,28 @@
>  		module_info_types(ModuleInfo, Types),
>  		map__search(Types, TypeId, TypeDefn),
>  		hlds_data__get_type_defn_body(TypeDefn, Body),
> -		Body = foreign_type(IsBoxed, ForeignType, ForeignLocation)
> +		Body = foreign_type(MaybeIL, MaybeC)
>  	->
> -		MLDSType = mlds__foreign_type(IsBoxed,
> -				ForeignType, ForeignLocation)
> +		module_info_globals(ModuleInfo, Globals),
> +		globals__get_target(Globals, Target),
> +		( Target = c,
> +			( MaybeC = yes(CForeignType),
> +				ForeignType = c(CForeignType)
> +			; MaybeC = no,
> +				error("mercury_type_to_mlds_type: No C foreign type")
> +			)
> +		; Target = il,
> +			( MaybeIL = yes(ILForeignType),
> +				ForeignType = il(ILForeignType)
> +			; MaybeIL = no,
> +				error("mercury_type_to_mlds_type: No IL foreign type")
> +			)

Use "unexpected" or "sorry" rather than "error".
If it is "unexpected", add a comment explaining why.

> Index: compiler/mlds_to_c.m
> +mlds_output_pragma_export_type(prefix, mlds__foreign_type(ForeignType)) -->
> +	( { ForeignType = c(c(Name)) },
> +		io__write_string(Name)
> +	; { ForeignType = il(_) },
> +		{ error("mlds_output_pragma_export_type: il foreign_type") }

Use `unexpected'.

> +mlds_output_type_prefix(mlds__foreign_type(ForeignType)) -->
> +	( { ForeignType = c(c(Name)) },
> +		io__write_string(Name)
> +	; { ForeignType = il(_) },
> +		{ error("mlds_output_type_prefix: il foreign_type") }

Likewise.

> Index: compiler/mlds_to_gcc.m
> --- compiler/mlds_to_gcc.m	18 Feb 2002 07:00:57 -0000	1.63
> +++ compiler/mlds_to_gcc.m	5 Mar 2002 12:05:37 -0000
> @@ -1685,7 +1685,7 @@
>  	).
>  build_type(mercury_type(Type, TypeCategory, _), _, _, GCC_Type) -->
>  	build_mercury_type(Type, TypeCategory, GCC_Type).
> -build_type(mlds__foreign_type(_, _, _), _, _, _) --> 
> +build_type(mlds__foreign_type(_), _, _, _) --> 
>  	{ sorry(this_file, "foreign_type not implemented") }.

There should at least be an XXX comment here.

But I think it should be OK to handle mlds__foreign_types the same as
mlds__generic_type here:

	build_type(mlds__foreign_type(_), _, _, 'MR_Box') --> [].

If not, why not?

> Index: compiler/pragma_c_gen.m
...
> +:- pred get_maybe_foreign_type_name((type)::in, maybe(string)::out,
> +		code_info::in, code_info::out) is det.
> +
> +get_maybe_foreign_type_name(Type, MaybeForeignType) -->
> +	code_info__get_module_info(Module),
> +	{ module_info_types(Module, Types) },
> +	{ 
> +		type_to_type_id(Type, TypeId, _SubTypes),
> +		map__search(Types, TypeId, Defn),
> +		hlds_data__get_type_defn_body(Defn, Body),
> +		Body = foreign_type(_MaybeIL, MaybeC)
> +	->
> +		( MaybeC = yes(c(Name)),
> +			MaybeForeignType = yes(Name)
> +		; MaybeC = no,
> +			error("get_maybe_foreign_type_name: no c foreigm type")

s/foreigm/foreign/

Use "unexpected" rather than "error".
Add a comment explaining why this case shouldn't occur.

> Index: compiler/prog_data.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/prog_data.m,v
> retrieving revision 1.79
> diff -u -r1.79 prog_data.m
> --- compiler/prog_data.m	26 Feb 2002 02:45:49 -0000	1.79
> +++ compiler/prog_data.m	5 Mar 2002 12:05:41 -0000
> +:- type il_foreign_type
>  	--->	il(

probably s/il/il_foreign_type/ would be nicer.

> +:- type c_foreign_type
> +	--->	c(
> +			string		% The C type name
> +		).

Likewise s/c/c_foreign_type/ here.

> Index: compiler/mlds_to_c.m
...
> @@ -1635,8 +1639,12 @@
>  mlds_output_type_prefix(mlds__native_bool_type)  -->
>  	io__write_string("MR_bool").
>  mlds_output_type_prefix(mlds__native_char_type)  --> io__write_string("char").
> -mlds_output_type_prefix(mlds__foreign_type(_, _, _)) -->
> -	{ error("mlds_output_type_prefix: foreign_type") }.
> +mlds_output_type_prefix(mlds__foreign_type(ForeignType)) -->
> +	( { ForeignType = c(c(Name)) },
> +		io__write_string(Name)
> +	; { ForeignType = il(_) },
> +		{ error("mlds_output_type_prefix: il foreign_type") }
> +	).

This should just output `MR_Box'.  Otherwise there is no way to be binary
compatible with the `--target asm' back-end.

This may require some changes elsewhere, to insert appropriate conversions
at the C interface (pragma foreign_proc and pragma export), if this is
not done already.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------