[m-dev.] For review: Stacks dump in the external debugger (round 2)

[Fergus Henderson]

On 16-Feb-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> 
> This change implement stack dump commands for the external debugger.
> 
> The MR_dump_stack_record_print() defined in runtime/mercury_stack_trace.c is
> the function that prints the contents of the stack. We move the definition of
> this function to mercury_trace_internal.c and we pass down that function as a
> parameter of MR_dump_stack_from_layout(). The rational for this change is that
> we can then define a new MR_dump_stack_record_print() in
> mercury_trace_external.c that prints the data to the socket as a Prolog term
> and pass down the address of that new function to MR_dump_stack_from_layout().

s/new MR_dump_stack_record_print
 /new function MR_dump_stack_record_print_to_socket/ ?

> trace/mercury_trace_external.c:
> 	Define new MR_dump_stack_record_print() and MR_print_proc_id() that
> 	sends data to the socket as Prolog terms.	

s/new MR_dump_stack_record_print
 /new function MR_dump_stack_record_print_to_socket/ ?
s/MR_print_proc_id/MR_print_proc_id_to_socket/ ?

> 	Implement the ancestors, non det and registers stack dump.

I don't understand that sentence.

> runtime/mercury_stack_trace.[ch]
> trace/mercury_trace.[ch]:
> trace/mercury_trace_internal.[ch]:
> 	Add MR_dump_stack_record_print() as an argument of all the 
> 	functions that needs it in mercury_stack_trace.c.

s/needs/need/

Also the argument name shouldn't start with `MR_'.
Our naming convention is that function arguments and local
variables don't need `MR_' prefixes -- `MR_' prefixes are
only needed for things that are likely to cause name clashes
with things defined by the user, i.e. macros and types defined
in header files, and functions and global variables (or constants)
with external linkage.  (We often also use `MR_' prefixes for
functions or global variables with static linkage; that is
not strictly necessary, but makes it easier to change something
from static to extern or vice versa.)

> 	Moves the definition of detism_names() from mercury_stack_trace.c
> 	to mercury_trace.c. This function is needed by both version of
> 	MR_dump_stack_record_print().

s/moves/move/
s/version/versions/

s/detism_names()/detism_names[]/, since it is an array not a function.

Since you gave it external linkage, it now needs an `MR_' prefix.

> --- mercury_stack_trace.c	1998/12/17 13:36:59	1.24
> +++ mercury_stack_trace.c	1999/02/16 12:40:34
>  static	void	MR_dump_stack_record_init(void);
>  static	void	MR_dump_stack_record_frame(FILE *fp,
>  			const MR_Stack_Layout_Entry *,
> +			Word *base_sp, Word *base_curfr, 
> +			void *MR_dump_stack_record_print(
> +				FILE *, const MR_Stack_Layout_Entry *, 
> +				int, int, Word *, Word *));
> +static	void	MR_dump_stack_record_flush(FILE *fp, 
> +			void *MR_dump_stack_record_print(
> +				FILE *, const MR_Stack_Layout_Entry *, 
> +				int, int, Word *, Word *));

The argument should be named e.g. `print_stack_record'
rather than `MR_dump_stack_record_print'.

>  const char *
>  MR_dump_stack_from_layout(FILE *fp, const MR_Stack_Layout_Entry *entry_layout,
> -	Word *det_stack_pointer, Word *current_frame, bool include_trace_data)
> +	Word *det_stack_pointer, Word *current_frame, bool include_trace_data,
> +	void *MR_dump_stack_record_print(FILE *, const MR_Stack_Layout_Entry *, 
> +	int, int, Word *, Word *))

Likewise.

> Index: runtime/mercury_stack_trace.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_trace.h,v
> retrieving revision 1.14
> diff -u -r1.14 mercury_stack_trace.h
> --- mercury_stack_trace.h	1998/12/17 13:37:00	1.14
> +++ mercury_stack_trace.h	1999/02/16 12:40:35
> @@ -56,7 +56,10 @@
>  extern	const char	*MR_dump_stack_from_layout(FILE *fp,
>  				const MR_Stack_Layout_Entry *entry_layout,
>  				Word *det_stack_pointer, Word *current_frame,
> -				bool include_trace_data);
> +				bool include_trace_data,
> +				void *MR_dump_stack_record_print(FILE *, 
> +					const MR_Stack_Layout_Entry *, 
> +					int, int, Word *, Word *));

Likewise.

> Index: trace/mercury_trace.c
> +++ mercury_trace.h	1999/02/16 12:40:36
> @@ -98,6 +98,8 @@
>  	bool			MR_trace_must_check;
>  } MR_Trace_Cmd_Info;
>  
> +extern const char * detism_names[];

s/detism_names/MR_detism_names/

Also, you need a comment explaining the semantics of this array.

(When you make some something exported, the minimum documentation
requirements go up.)

> Index: trace/mercury_trace_external.c
> +	MR_REQUEST_STACK         = 10,/* print the ancestors list             */
> +	MR_REQUEST_NONDET_STACK  = 11,/* print the non det stack	      */

s/non det/nondet/

> +static void
> +MR_dump_stack_record_print_to_socket(FILE *fp, 
> +	const MR_Stack_Layout_Entry *entry_layout, int count, int start_level, 
> +	Word *base_sp, Word *base_curfr)
> +{
> +	MR_send_message_to_socket_format( "%d.\n ", (char *) start_level);
> +
> +	if (count > 1) {
> +		MR_send_message_to_socket_format( " %d*.\n ", (char *) count);

Here, and in lots of other places, you are using a "%d" format, but
passing a `char *'.  That won't work.
You should implement MR_send_message_to_socket_format properly,
using <stdarg.h> and vprintf():

	/* in C file */

	#include <stdarg.h>

	void    
	MR_send_message_to_socket_format(const char *format, ...)
	{
		va_list args;

		va_start(args, format);
		vfprintf(MR_debugger_socket_out.file, format, args);
		va_end(args);
		fflush(MR_debugger_socket_out.file);
		MR_debugger_socket_out.line_number++;
	}

Then you won't need all those horrible casts.

One drawback of this approach is that because you're using `...',
you won't get static type checking of the arguments.  Fortunately,
gcc has an extension that fixes that.  So use the following:

	/* in header file */

	/*
	** Use a GNU C extension to enforce static type checking
	** for printf-style functions. 
	** (See the "Function attributes" section of "C extensions"
	** chapter of the GNU C manual for detailed documentation.)
	*/
	#ifdef __GNUC__
	  #define MR_LIKE_PRINTF(format_argnum, vars_argnum) \
	    __attribute__ ((format (printf, (format_argnum), (vars_argnum))))
	#else
	  #define MR_LIKE_PRINTF(n, m) /* nothing */
	#endif

	void MR_send_message_to_socket_format(const char *format, ...)
		MR_LIKE_PRINTF(1, 2);

One more point: in a lot of places you are sending multiple terms over
the socket when I think it would make more sense to send a single term.
Sending a single term makes more sense conceptually (you send a single
request, you get a single response), would be more efficient (fewer
system calls, fewer packets, and fewer context switches), and also
avoids the danger of the debugger and debuggee getting out of sync if
they disagree about the number of terms that they expect,

I would like to see another diff for this one.

Cheers,
	Fergus.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.