[m-dev.] for review: new debugger command set, part 2

[Zoltan Somogyi]

> > +#include <unistd.h>
> 
> Please document why.
> Non-ANSI depencies ought to be minimized.

That was for getopt; now that you have added it to the runtime,
I can replace it.

> The use of `(', '|' and ')' is confusing, IMHO.
> I think it would be better to list the different possibilities
> on different lines, e.g.
> 
> 	Usage:
> 		enable <number>
> 		enable *

As we have discussed, it would be better to move straight to a dynamic,
two-level (category/command) help system, so for this change I will leave
this as it is.

> Let me bring your attention to 2(c) in the GPL:
> 
>  |     c) If the modified program normally reads commands interactively
>  |     when run, you must cause it, when started running for such
>  |     interactive use in the most ordinary way, to print or display an
>  |     announcement including an appropriate copyright notice and a
>  |     notice that ...

I agree with the desirability of printing a banner; that's what that
code was therefore. I even agree that we should print something like what
gdb does. But what does "modified program" mean here? mdb is not a modified
gdb, it is not a modified getopt, etc. Mdb is GPL'd, but we own it, and
I think we can make the banner anything we want.

> This is a very long function; it might be better to
> split out the code for each command into seperate
> subroutines.  The code for handling "break", at least,
> is quite long and complex and should be a seperate
> subroutine.  Also, there's a lot of duplicate code in the
> handling of that case, which should be avoided.

The problem in many of these cases (although not this one) is that factoring
out common code requires generalizations that can lead to longer and *less*
readable code.

> Instead of having a seperate function for each possibility,
> i.e. whether the module, arity, or proc number were specified,
> you should have a single function which gets passed a
> MR_Proc_Specifier struct, whose fields can have special
> values that indicate "any", e.g. a NULL module name means
> any module matches.

That is a good idea.

Actually, some other specification components may be NULL, but at the moment
the module name cannot be; I'll change it.

> >  static void
> > +MR_trace_restart(const MR_Stack_Layout_Label *this_label, int seqno, int 
       depth)
> > +{
> > +	args = checked_malloc(arg_max * sizeof(Word));
> 
> Storage allocated by checked_malloc() is NOT scanned by the conservative
> garbage collector.  Either you should add a comment indicating your
> reasoning as to why this should use checked_malloc(),

In this case, the array only contains copies of things that the Boehm
collector *will* scan, so it is all right, and the accurate garbage collector
will never be invoked between the malloc and the free.

> You should check all your other calls to checked_malloc() too.

I will.

> Also it would be useful to define a macro like make_array() which
> calls checked_malloc() instead of newmem(), and to use it rather
> than calling checked_malloc() directly.

> > +
> > +			case MR_SPY_ALL:
> > +				enabled = TRUE;
> > +				action = max(action, point->spy_action);
> 
> You need a `break;' here, I think.

Thanks; that was a bug.

> > +		/* make sure the MR_spied_procs array is big enough */
> > +		if (MR_spied_procs_next >= MR_spied_procs_max) {
> > +			if (MR_spied_procs_max == 0) {
> > +				MR_spied_procs_max = INIT_SPY_TABLE_SIZE;
> > +				MR_spied_procs = checked_malloc(
> > +							MR_spied_procs_max *
> > +							sizeof(MR_Spied_Proc));
> > +			} else {
> > +				MR_spied_procs_max *= 2;
> > +				MR_spied_procs = checked_realloc(
> > +							MR_spied_procs,
> > +							MR_spied_procs_max *
> > +							sizeof(MR_Spied_Proc));
> > +			}
> > +		}
> 
> That code fragment too is something I have seen many occurrences of
> (although the variable names are always different).
> You should use a macro.

How about

#define	MR_ensure_big_enough(base, type, init)		  		    \
	do {								    \
		if (base##_next >= base##_max) {			    \
			if (base##s == NULL) {				    \
				base##_max = init;			    \
				base##s = checked_malloc(		    \
						base##_max * sizeof(type)); \
			} else {					    \
				base##_max *= 2;			    \
				base##s = checked_realloc(base##s, 	    \
						base##_max * sizeof(type)); \
			}						    \
		}							    \
	} while(0)

> > +** gcc gets an internal error ("fixed or forbidden register was spilled")
> > +** if it isn't a function.
> 
> Please name the version number and architecture for which this occurs.

2.7.2.3, hydra.

> (Did you send a bug report to the gcc developers?)

No. I think I remember we had this sort of bug before; I don't recall
what happened about them. They may be fixed in 2.8.

> > +static MR_Module_Info *
> > +MR_search_module_info(const char *name)
> > +{
> > +	int	lo;
> > +	int	hi;
> > +	int	mid;
> > +	int	diff;
> > +
> > +	lo = 0;
> > +	hi = MR_module_info_next-1;
> > +	while (lo <= hi) {
> > +		mid = (lo + hi) / 2;
> > +		diff = strcmp(MR_module_infos[mid].MR_module_name, name);
> > +		if (diff == 0) {
> > +			return &MR_module_infos[mid];
> > +		} else if (diff < 0) {
> > +			lo = mid + 1;
> > +		} else {
> > +			hi = mid - 1;
> > +		}
> > +	}
> 
> More duplicate code.

Although there are other binary searches, there are no other binary searches
on this array.

> > Index: runtime/mercury_trace_util.h
> > +extern	int	MR_trace_get_register_number(MR_Live_Lval locn);
> 
> Please document that function; make sure you explain the role of the
> magic number -1.
> 
> (The other functions declared in that header ought to be documented too...
> though not necessarily as part of this change.)

I will try.

> > +  #if 0
> >  		printf("max detstack used:  %6ld words\n",
> >  			(long)(detstack_zone->max - detstack_zone->min));
> >  		printf("max nondstack used: %6ld words\n",
> >  			(long) (nondetstack_zone->max - nondetstack_zone->min);
> > +  #endif
> >  	}
> >  #endif
> 
> This change was not mentioned in the log message.
> I don't understand it.

That was part of the change making the runtime compile with
-DMR_LOWLEVEL_DEBUG, by disabling incorrect references to stack zones.
With Tom's help, I have since fixed them instead.

Zoltan.