[m-dev.] for review: generating C layout structures

[Fergus Henderson]

I agree with all of Tyson's review comments.

On 08-Jan-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 	the C types of the structures generated by the Mercury compiler
> 	do not use a union, since a union cannot be initialized through
> 	its second member.

Incidentally, you can in C99:

	union u { int i; char *s; } x = { .s = "foo" };

However, that won't help us much for about 5-10 years ;-)

> +++ browser/dl.m	2001/01/07 11:32:57
> +:- pragma foreign_code("C",
> +"
> +static	int	MR_dl_closure_counter = 0;
> +").
> +
> +:- func make_closure_layout = int.
> +
> +:- pragma foreign_code("C", make_closure_layout = (ClosureLayout::out),
> +	[will_not_call_mercury, thread_safe],
> +"
> +	extern	int			MR_dl_closure_counter;

That won't work if the foreign_code gets inlined into a different module.

I sugest you make the variable `extern' rather than `static'.
Another possible alternative would be to declare it as a local static,
within the foreign_code, and use `pragma no_inline' for that procedure.

The variable name should start with `ML_' rather than `MR'.
(And perhaps `ML_DL' rather than `ML_dl'.)

> +	MR_Closure_Id			*closure_id;
> +	MR_Closure_Dyn_Link_Layout	*closure_layout;
> +
> +	closure_id = MR_GC_NEW(MR_Closure_Id);
> +	closure_id->proc_id.MR_proc_user.MR_user_pred_or_func = MR_PREDICATE;
> +	closure_id->proc_id.MR_proc_user.MR_user_decl_module = ""unknown"";
> +	closure_id->proc_id.MR_proc_user.MR_user_def_module = ""unknown"";
> +	closure_id->proc_id.MR_proc_user.MR_user_name = ""unknown"";
> +	closure_id->proc_id.MR_proc_user.MR_user_arity = -1;
> +	closure_id->proc_id.MR_proc_user.MR_user_mode = -1;

The relevant information to fill in all of those fields is passed to
dl__mercury_sym.  So it ought to fill them in properly.

This is actually an existing problem, not one introduced
by this change, so you don't need to address it in this change.
But since it was another of your changes that introduced this problem,
it would be nice if you could fix it.

> +	closure_id->module_name = ""dl"";
> +	closure_id->file_name = ""dl.m"";
> +	closure_id->line_number = ++MR_dl_closure_counter;
> +	closure_id->goal_path = """";

Why do you set the line number in this fashion?
Wouldn't it be more accurate to set it to the current line,
like this?

	closure_id->line_number = __LINE__;

While you're at it, a more robust way of setting the
file name would be

	closure_id->file_name = __FILE__;

> +	closure_layout = MR_GC_NEW(MR_Closure_Dyn_Link_Layout);
> +	closure_layout->closure_id = closure_id;
> +	closure_layout->type_params = NULL;
> +	closure_layout->num_all_args = 0;
> +
> +	ClosureLayout = (Word) closure_layout;
> +").

s/Word/MR_Word/

> +++ layout.m	Tue Jan  2 18:41:10 2001
> @@ -0,0 +1,131 @@
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 2001 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% Definitions of data structures for representing procedure layout structures
> +% within the compiler. When output by layout_out.m, values of most these types
> +% will correspond to the types defined in runtime/mercury_stack_layout.h;
> +% the documentation of those types can be found there.
> +% The code to generate the structures is in stack_layout.m.
...
> +:- type proc_layout_stack_traversal
> +	--->	proc_layout_stack_traversal(
> +			entry_label		:: maybe(label),
> +						% The proc entry label; will be
> +						% `no' if we don't have static
> +						% code addresses.
> +			succip_slot		:: maybe(int),
> +			stack_slot_count	:: int,
> +			detism			:: determinism
> +		).
> +
> +:- type proc_layout_exec_trace
> +	--->	proc_layout_exec_trace(
> +			call_label_layout	:: layout_name,
> +			proc_body		:: maybe(rval),
> +			var_names		:: list(int), % offsets
> +			max_var_num		:: int,
> +			max_r_num		:: int,
> +			maybe_from_full_slot	:: maybe(int),
> +			maybe_io_seq_slot	:: maybe(int),
> +			maybe_trail_slot	:: maybe(int),
> +			maybe_maxfr_slot	:: maybe(int),
> +			eval_method		:: eval_method,
> +			maybe_call_table_slot	:: maybe(int),
> +			maybe_decl_debug_slot	:: maybe(int)
> +		).
> +
> +:- type maybe_proc_id_and_exec_trace
> +	--->	no_proc_id
> +	;	proc_id_only
> +	;	proc_id_and_exec_trace(proc_layout_exec_trace).
> +
> +:- type file_layout_data
> +	--->	file_layout_data(
> +			file_name		:: string,
> +			line_no_label_list	:: assoc_list(int, layout_name)
> +		).
> +
> +:- type label_var_info
> +	--->	label_var_info(
> +			encoded_var_count	:: int,
> +			locns_types		:: rval,
> +			var_nums		:: rval,
> +			type_params		:: rval
> +		).
> +
> +:- type layout_data
> +	--->	label_layout_data(
> +			label			:: label,
> +			proc_layout_name	:: layout_name,
> +			maybe_port		:: maybe(trace_port),
> +			maybe_goal_path		:: maybe(int), % offset
> +			maybe_var_info		:: maybe(label_var_info)
> +		)
> +	;	proc_layout_data(
> +			proc_label,
> +			proc_layout_stack_traversal,
> +			maybe_proc_id_and_exec_trace
> +		)
> +	;	closure_proc_id_data(
> +			caller_proc_label	:: proc_label,
> +			caller_closure_seq_no	:: int,
> +			closure_proc_label	:: proc_label,
> +			closure_module_name	:: module_name,
> +			closure_file_name	:: string,
> +			closure_line_number	:: int,
> +			closure_goal_path	:: string
> +		)
> +	;	module_layout_data(
> +			module_name		:: module_name,
> +			string_table_size	:: int,
> +			string_table		:: string,
> +			proc_layout_names	:: list(layout_name),
> +			file_layouts		:: list(file_layout_data),
> +			trace_level		:: trace_level
> +		).
> +
> +:- type label_vars
> +	--->	label_has_var_info
> +	;	label_has_no_var_info.
> +
> +:- type proc_layout_user_or_compiler
> +	--->	user
> +	;	compiler.
> +
> +:- type proc_layout_kind
> +	--->	proc_layout_traversal
> +	;	proc_layout_proc_id(proc_layout_user_or_compiler)
> +	;	proc_layout_exec_trace(proc_layout_user_or_compiler).
> +
> +:- type layout_name
> +	--->	label_layout(label, label_vars)
> +	;	proc_layout(proc_label, proc_layout_kind)
> +		% A proc layout structure for stack tracing, accurate gc
> +		% and/or execution tracing.
> +	;	proc_layout_var_names(proc_label)
> +		% A vector of variable names (represented as offsets into
> +		% the string table) for a procedure layout structure.
> +	;	closure_proc_id(proc_label, int, proc_label)
> +	;	file_layout(module_name, int)
> +	;	file_layout_line_number_vector(module_name, int)
> +	;	file_layout_label_layout_vector(module_name, int)
> +	;	module_layout_file_vector(module_name)
> +	;	module_layout_proc_vector(module_name)
> +	;	module_layout(module_name).

It would be MUCH easier to read if the types were defined in
top-down order, rather than bottom-up.

Also, I think it would help a lot to document with each type what the
corresponding C type is, if any, as is done in rtti.m.

Some general documentation at the top of the file explaining
what layout structures are and what they are used for would be
helpful too.  Likewise for runtime/mercury_stack_layout.h,
which currently just has

** mercury_stack_layout.h -
**      Definitions for the stack layout data structures.

which is not very explanatory.
A brief explanantion along the lines of "these structures are used by
the debugger and the accurate garbage collector to trace the stacks..."
or something like that would help a lot.

> Index: compiler/layout_out.m
>
> +:- pred extract_layout_name(layout_data::in, layout_name::out) is det.
> +
> +extract_layout_name(label_layout_data(Label, _, _, _, yes(_)), LayoutName) :-
> +	LayoutName = label_layout(Label, label_has_var_info).
> +extract_layout_name(label_layout_data(Label, _, _, _, no), LayoutName) :-
> +	LayoutName = label_layout(Label, label_has_no_var_info).
> +extract_layout_name(proc_layout_data(ProcLabel, _, MaybeRest), LayoutName) :-
> +	maybe_proc_layout_and_exec_trace_kind(MaybeRest, ProcLabel, Kind),
> +	LayoutName = proc_layout(ProcLabel, Kind).
> +extract_layout_name(closure_proc_id_data(CallerProcLabel, SeqNo,
> +		ClosureProcLabel, _, _, _, _),
> +		closure_proc_id(CallerProcLabel, SeqNo, ClosureProcLabel)).
> +extract_layout_name(module_layout_data(ModuleName, _,_,_,_,_), LayoutName) :-
> +	LayoutName = module_layout(ModuleName).
> +
> +%-----------------------------------------------------------------------------%
> +
> +output_layout_data_decl(LayoutData, DeclSet0, DeclSet) -->
> +	output_layout_addr_storage_type_name(LayoutName, no),
> +	io__write_string(";\n"),
> +	{ extract_layout_name(LayoutData, LayoutName) },
> +	{ decl_set_insert(DeclSet0, data_addr(layout_addr(LayoutName)),
> +		DeclSet) }.

It would be clearer to define extract_layout_name after it is used.
(This is another top-down vs bottom-up issue.)

[to be continued...]

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------