[m-dev.] for review: line numbers in the debugger (part 1)

[Tyson Dowd]

On 10-Nov-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Estimated hours taken: 
> 
> Support breakpoints on line numbers that correspond to calls. Internally,
> these are implemented as breakpoints on the proc layouts of the called
> procedure.
> 
> This required extending the debugging RTTI, so that associated with each
> module there is now a new data structure listing the source file names that
> contribute labels with layout structures to the code of the module. For each
> such source file, this table gives a list of all such labels arising from
> that file. The table entry for a label gives the line number within the file,
> and the pointer to the label layout structure.
> 
> compiler/llds.m:
> 	Add a context field to the call instruction.
> 
> compiler/continuation_info.m:
> 	Instead of the old division of continuation info about labels into
> 	trace ports and everything else, divide them into trace ports, resume
> 	points and return sites. Record contexts with trace ports, and record
> 	contexts and called procedure information with return sites.
> 
> compiler/code_info.m:
> 	Conform to the changes in continuation_info.m.
> 
> compiler/options.m:
> 	Add a new option that allows us to disable the generation of line
> 	number information for size benchmarking (it has no other use).
> 
> compiler/stack_layout.m:
> 	Generate the new components of the RTTI, unless the option says not to.
> 
> compiler/code_gen.m:
> compiler/pragma_c_gen.m:
> compiler/trace.m:
> 	Include contexts in the events we generate.
> 
> compiler/call_gen.m:
> 	Include contexts in the call LLDS instructions.
> 
> compiler/handle_options.m:
> 	Delete the code that tests or sets the deleted options.
> 
> compiler/mercury_compile.m:
> 	Delete the code that tests the deleted options.
> 
> compiler/basic_block.m:
> compiler/dupelim.m:
> compiler/frameopt.m:
> compiler/livemap.m:
> compiler/llds_common.m:
> compiler/llds_out.m:
> compiler/middle_rec.m:
> compiler/opt_debug.m:
> compiler/opt_util.m:
> compiler/value_number.m:
> compiler/vn_*.m:
> 	Trivial changes to conform to the changes to llds.m.
> 
> compiler/jumpopt.m:
> 	Do not optimize away jumps to labels with layout structures.
> 	The jumps we are particularly concerned about now are the jumps
> 	that return from procedure calls. Previously, it was okay to redirect
> 	returns from several calls so that all go to the same label, since
> 	the live variable information associated with the labels could be
> 	merged. However, we now also associate line numbers with calls, and
> 	these cannot be usefully merged.
> 
> compiler/optimize.m:
> 	Pass the information required by jumpopt to it.
> 
> doc/user_guide.texi:
> 	Document that you can now break at line numbers.
> 
> 	Document the new "context" command, and the -d or --detailed option
> 	of the stack command and the commands that set ancestor levels.
> 
> runtime/mercury_stack_layout.h:
> 	Extend the module layout structure definition with the new tables.
> 
> 	Remove the conditional facility for including label numbers in label
> 	layout structures. It hasn't been used in a long time, and neither
> 	Tyson or me expect to use it to debug either gc or the debugger itself,
> 	so it has no uses left; the line numbers have superseded it.
> 
> runtime/mercury_stack_trace.[ch]:
> 	Extend the code to print stack traces to also optionally print
> 	contexts.
> 
> 	Add some utility predicates currently used by the debugger that could
> 	also be use for debugging gc or for more detailed stack traces.
> 
> trace/mercury_trace_internal.c:
> 	Implement the "break <context>" command, the "context" command, and
> 	the -d or --detailed option of the stack command and the commands
> 	that set ancestor levels.
> 
> 	Conditionally define a conditionally used variable.
> 
> trace/mercury_trace_external.c:
> 	Minor changes to keep up with the changes to stack traces.
> 
> 	Delete an unused variable.
> 
> trace/mercury_trace_spy.[ch]:
> 	Check for breakpoints on contexts.
> 
> trace/mercury_trace_tables.[ch]:
> 	Add functions to search the RTTI data structures for labels
> 	corresponding to a given context.
> 
> trace/mercury_trace_vars.[ch]:
> 	Remember the context of the current environment.
> 
> tests/debugger/queen.{inp,exp}:
> 	Test the new capabilities of the debugger.
> 
> tests/debugger/*.{inp,exp}:
> 	Update the expected output of the debugger to account for contexts.
> 	In some cases, modify the input script to put contexts where they don't
> 	overflow lines.
> 
> Zoltan.
> 
> cvs diff: Diffing .
> cvs diff: Diffing bindist
> cvs diff: Diffing boehm_gc
> cvs diff: Diffing boehm_gc/Mac_files
> cvs diff: Diffing boehm_gc/cord
> cvs diff: Diffing boehm_gc/cord/private
> cvs diff: Diffing boehm_gc/include
> cvs diff: Diffing boehm_gc/include/private
> cvs diff: Diffing browser
> cvs diff: Diffing bytecode
> cvs diff: Diffing compiler



> Index: runtime/mercury_stack_trace.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_trace.c,v
> retrieving revision 1.32
> diff -u -b -r1.32 mercury_stack_trace.c
> --- mercury_stack_trace.c	1999/06/08 03:47:41	1.32
> +++ mercury_stack_trace.c	1999/11/10 03:41:43


> @@ -364,26 +398,59 @@
>  		** so we don't reserve space for the repeat counts.
>  		*/
>  	}
> +
> +	MR_print_call_trace_info(fp, entry_layout, base_sp, base_curfr);
> +	MR_print_proc_id(fp, entry_layout);
> +	if (strdiff(filename, "") && linenumber > 0) {
> +		fprintf(fp, " (%s:%d%s)", filename, linenumber,
> +			context_mismatch? " and others" : "");
> +	}

I know we probably don't have anything about this in our style
guildelines, but I prefer a space before my operators.

> +void
> +MR_print_proc_id_trace_and_context(FILE *fp, MR_Context_Position pos,
> +	const MR_Stack_Layout_Entry *entry, Word *base_sp, Word *base_curfr,
> +	const char *path, const char *filename, int lineno, bool print_parent, 
> +	const char *parent_filename, int parent_lineno, int indent)
> +{
> +
> +	switch (pos) {
> +		case MR_CONTEXT_NOWHERE:
> +			fprintf(fp, " ");
> +			MR_print_call_trace_info(fp, entry,
> +				base_sp, base_curfr);
> +			MR_print_proc_id(fp, entry);
> +			if (strlen(path) > 0) {
> +				fprintf(fp, " %s", path);
> +			}
>  		fprintf(fp, "\n");
> +			break;
> +
> +		case MR_CONTEXT_BEFORE:
> +			MR_maybe_print_context(fp, filename, lineno);
> +			MR_maybe_print_parent_context(fp, print_parent,
> +				FALSE, parent_filename, parent_lineno);
> +			fprintf(fp, " ");
> +			MR_print_call_trace_info(fp, entry,
> +				base_sp, base_curfr);
> +			MR_print_proc_id(fp, entry);
> +			if (strlen(path) > 0) {
> +				fprintf(fp, " %s", path);
>  	}
> +			fprintf(fp, "\n");
> +			break;
> +
> +		case MR_CONTEXT_AFTER:
> +			fprintf(fp, " ");
> +			MR_print_call_trace_info(fp, entry,
> +				base_sp, base_curfr);
> +			MR_print_proc_id(fp, entry);
> +			if (strlen(path) > 0) {
> +				fprintf(fp, " %s", path);
> +			}
> +			MR_maybe_print_context(fp, filename, lineno);
> +			MR_maybe_print_parent_context(fp, print_parent,
> +				FALSE, parent_filename, parent_lineno);
> +			fprintf(fp, "\n");
> +			break;
> +
> +		case MR_CONTEXT_PREVLINE:
> +			MR_maybe_print_context(fp, filename, lineno);
> +			MR_maybe_print_parent_context(fp, print_parent,
> +				TRUE, parent_filename, parent_lineno);
> +			fprintf(fp, "\n%*s ", indent, "");
> +			MR_print_call_trace_info(fp, entry,
> +				base_sp, base_curfr);
> +			MR_print_proc_id(fp, entry);
> +			if (strlen(path) > 0) {
> +				fprintf(fp, " %s", path);
> +			}
> +			fprintf(fp, "\n");
> +			break;
> +
> +		case MR_CONTEXT_NEXTLINE:
> +			fprintf(fp, " ");
> +			MR_print_call_trace_info(fp, entry,
> +				base_sp, base_curfr);
> +			MR_print_proc_id(fp, entry);
> +			if (strlen(path) > 0) {
> +				fprintf(fp, " %s", path);
> +			}
> +			fprintf(fp, "\n%*s", indent, "");
> +			MR_maybe_print_context(fp, filename, lineno);
> +			MR_maybe_print_parent_context(fp, print_parent,
> +				TRUE, parent_filename, parent_lineno);
> +			fprintf(fp, "\n");
> +			break;
> +
> +		default:
> +			fatal_error("invalid MR_Context_Position");
> +	}
> +}

Hmmmm.... I'm not completely happy with this.  The creeping featuritis
of the debugger makes a formatting function 60 lines of rather similar
looking code.

Can I suggest something like:


	if (pos == MR_CONTEXT_PREVLINE || pos == MR_CONTEXT_BEFORE) {
		MR_maybe_print_context(fp, filename, lineno);
		MR_maybe_print_parent_context(fp, print_parent,
			(pos == MR_CONTEXT_PREVLINE), parent_filename,
			parent_lineno);
	}

	if (pos == MR_CONTEXT_PREVLINE) {
		fprintf(fp, "\n%*s", indent, "");
	} else {
		fprintf(fp, " ");
	}

	MR_print_call_trace_info(fp, entry,
		base_sp, base_curfr);
	MR_print_proc_id(fp, entry);
	if (strlen(path) > 0) {
		fprintf(fp, " %s", path);
	}

	if (pos == MR_CONTEXT_NEXTLINE) {
		fprintf(fp, "\n%*s", indent, "");
	}
	if (pos == MR_CONTEXT_NEXTLINE || pos == MR_CONTEXT_AFTER) {
		MR_maybe_print_context(fp, filename, lineno);
		MR_maybe_print_parent_context(fp, print_parent,
			(pos == MR_CONTEXT_NEXTLINE), parent_filename,
			parent_lineno);
	}
	fprintf(fp, "\n");

It's half the size, admittedly less efficient (but who cares, it's doing
I/O) and I find it easy to see what parameter controls what formatting
option.  I also think it's easier to maintain. 

I guess a drawback is if you are planning on adding even more options to
the enum.

But why can't I define a printf style formatting string for where
I want to print everything?  (actually, that isn't as tongue in cheek as
it might seem). 

I wouldn't worry about this comment for this commit, but if
the debugger keeps getting features like this it's going to become
a nightmare.

> Index: runtime/mercury_stack_trace.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_trace.h,v
> retrieving revision 1.18
> diff -u -b -r1.18 mercury_stack_trace.h
> --- mercury_stack_trace.h	1999/06/08 03:47:41	1.18
> +++ mercury_stack_trace.h	1999/11/10 03:16:58

> @@ -143,29 +150,69 @@
>  ** the names that are usually used to denote determinisms.
>  */
>  
> -extern const char * MR_detism_names[];
> +extern	const char *MR_detism_names[];
>  
>  /*
> +** MR_find_context attempts to look up the file name and line number
> +** corresponding to a label identified by its layout structure. If successful,
> +** it fills in *fileptr and *lineptr accordingly, and returns TRUE; otherwise,
> +** it returns FALSE.
> +*/
> +
> +extern	bool	MR_find_context(const MR_Stack_Layout_Label *label,
> +			const char **fileptr, int *lineptr);
> +
> +/*
> +** If the procedure has trace layout information and the relevant one of
> +** base_sp and base_curfr is not NULL, MR_print_call_trace_info prints
> +** the call event number, call sequence number and call depth of the call
> +** stored in the stack frame of the procedure.
> +*/
> +
> +extern	void	MR_print_call_trace_info(FILE *fp,
> +			const MR_Stack_Layout_Entry *entry,
> +			Word *base_sp, Word *base_curfr);
> +
> +/*
>  ** MR_print_proc_id prints an identification of the given procedure,
>  ** consisting of "pred" or "func", module name, pred or func name, arity,
> -** mode number and determinism, followed by an optional extra string,
> -** and a newline.
> -**
> -** If the procedure has trace layout information and the relevant one of
> -** base_sp and base_curfr is not NULL, it also prints the call event number,
> -** call sequence number and call depth of the call.
> +** mode number and determinism. It does not output a newline, so that
> +** the caller can pu something else after the procedure id on the same line.

s/pu/put/

> +	} else if (streq(words[0], "context")) {
> +		if (word_count == 2) {
> +			if (streq(words[1], "none")) {
> +				MR_context_position = MR_CONTEXT_NOWHERE;
> +				if (MR_trace_internal_interacting) {
> +					fprintf(MR_mdb_out, "Contexts "
> +						"will not be printed.\n");
> +				}
> +			} else if (streq(words[1], "before")) {
> +				MR_context_position = MR_CONTEXT_BEFORE;
> +				if (MR_trace_internal_interacting) {
> +					fprintf(MR_mdb_out, "Contexts "
> +						"will be printed before, "
> +						"on the same line.\n");
> +				}
> +			} else if (streq(words[1], "after")) {
> +				MR_context_position = MR_CONTEXT_AFTER;
> +				if (MR_trace_internal_interacting) {
> +					fprintf(MR_mdb_out, "Contexts "
> +						"will be printed after, "
> +						"on the same line.\n");
> +				}
> +			} else if (streq(words[1], "prevline")) {
> +				MR_context_position = MR_CONTEXT_PREVLINE;
> +				if (MR_trace_internal_interacting) {
> +					fprintf(MR_mdb_out, "Contexts "
> +						"will be printed "
> +						"on the previous line.\n");
> +				}
> +			} else if (streq(words[1], "nextline")) {
> +				MR_context_position = MR_CONTEXT_NEXTLINE;
> +				if (MR_trace_internal_interacting) {
> +					fprintf(MR_mdb_out, "Contexts "
> +						"will be printed "
> +						"on the next line.\n");
> +				}
> +			} else {
> +				MR_trace_usage("parameter", "context");
> +			}
> +		} else if (word_count == 1) {
> +			fprintf(MR_mdb_out, "Contexts are ");
> +			switch (MR_context_position) {
> +			case MR_CONTEXT_NOWHERE:
> +				fprintf(MR_mdb_out, "not printed.");
> +				break;
> +
> +			case MR_CONTEXT_BEFORE:
> +				fprintf(MR_mdb_out,
> +					"printed before, on the same line.");
> +				break;
> +
> +			case MR_CONTEXT_AFTER:
> +				fprintf(MR_mdb_out,
> +					"printed after, on the same line.");
> +				break;
> +
> +			case MR_CONTEXT_PREVLINE:
> +				fprintf(MR_mdb_out,
> +					"printed on the previous line.");
> +				break;
> +
> +			case MR_CONTEXT_NEXTLINE:
> +				fprintf(MR_mdb_out,
> +					"printed on the next line.");
> +				break;
> +
> +			default:
> +				fatal_error("invalid MR_context_position");
> +			}
> +		} else {
> +			MR_trace_usage("parameter", "context");
> +		}

A short table of constant strings and MR_context_position values 
could do all this in about 5 lines.  Doesn't really matter though.


Other than those comments the diff looks fine.


-- 
       Tyson Dowd           # 
                            #  Surreal humour isn't eveyone'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
--------------------------------------------------------------------------