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

[Fergus Henderson]

On 08-Jul-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> > > +#include <unistd.h>
> > 
> > Please document why.
> > Non-ANSI depencies ought to be minimized.
> 
> That was for getopt;

In that case, it should have been

	#include "mercury_getopt.h"

since different systems declare getopt() in different places
(some in <unistd.h>, some in <stdlib.h>).

> now that you have added it to the runtime,
> I can replace it.

Yep.  But I suggest you just use "mercury_getopt.h" for now;
the change to use the GNU getopt can be a separate change.

> > 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.

We can, but then anyone who wants to modify our work will need to 
either also modify the banner to make it conform to 2(c),
or get our permission.  This would be a pain.

Actually now that I think about it the issue may not arise, since the
Mercury runtime (and GNU getopt) are under the LGPL, not the GPL, and
the LGPL doesn't contain the same requirement.  However, the LGPL does
include the GPL by reference, since it allows any LGPL'ed work to be
relicensed under the GPL.  So I guess the issue could arise after all.
Thus it seems simplest to just include a banner that conforms to GPL 2(c).

> > 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.

No comment on that one? ;-)

> > > +		/* 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)

That would be fine.

> > > +** 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.

Sorry, I wasn't clear -- I meant name it in the comment.
(s/hydra/i686-pc-linux-gnu/)

> > (Did you send a bug report to the gcc developers?)
> 
> No. I think I remember we had this sort of bug before;

Previously they were for code that did memcpy()-like things,
such as structure assignments.  If this is the same bug, then we
should use the same work-around (MR_memcpy()).  If it is a different
bug, then we should report it.

Ah, I see that the code involved was in fact doing something
similar to memmove():

+               /* move all the spied procs above the one to be inserted */
+               for (i = MR_spied_procs_next - 1;
+                               i >= 0 && MR_need_move(i, entry); i--) {
+                       MR_spied_procs[i+1] = MR_spied_procs[i];
+               }

It might be better to write the code for MR_need_move() inline and
instead change the loop body to 

                        MR_memcpy(&MR_spied_procs[i+1], MR_spied_procs[i],
				sizeof(MR_spied_procs[i]));

Note that MR_memcpy() is already defined in mercury_misc.c.
The reason for this is just that it's better to use the same work-around
in all cases rather than using a different work-around for each case.

> > > +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.

Ah.  Why does MR_insert_module_info use a linear search rather than
a binary search?

I suppose it has to do a linear memmove() anyway, so it doesn't matter
to much.  But a binary search should be faster.

Anyway, you could use a single macro for all binary searches:

/*
** // In C++ terminology:
** template <class T>
** MR_bsearch(T array[], int num_elements, int compare(T, T), T key,
**		int& element, bool& found):
** ... (insert documentation here) ...
*/
#define MR_bsearch(array, num_elements, compare, key, element, found)	\
	do {								\
		int	lo;						\
		int	hi;						\
		int	mid;						\
		int	diff;						\
									\
		lo = 0;							\
		hi = (num_elements) - 1;				\
		(found) = FALSE;					\
		while (lo <= hi) {					\
			mid = (lo + hi) / 2;				\
			diff = compare((array)[mid], (key));		\
			if (diff == 0) {				\
				(found) = TRUE;				\
				break;					\
			} else if (diff < 0) {				\
				lo = mid + 1;				\
			} else {					\
				hi = mid - 1;				\
			}						\
		}							\
		(element) = mid;					\
	} while(0);

If you want to get a little more tricky, then you can reduce the
number of parameters:

/*
** MR_bsearch(int num_elements, COMPARE, int& element, bool& found):
** ... (insert more documentation here) ...
** The `COMPARE' parameter should be an expression
** which compares the value at the index specified by the
** current value of the `element' parameter with the desired value
** and returns <0, 0, or >0 according to whether it is less than,
** equal to, or greater than the desired value.
*/
#define MR_bsearch(num_elements, COMPARE, element, found)		\
	do {								\
		int	lo;						\
		int	hi;						\
		int	mid;						\
		int	diff;						\
									\
		lo = 0;							\
		hi = (num_elements) - 1;				\
		(found) = FALSE;					\
		while (lo <= hi) {					\
			(element) = (lo + hi) / 2;			\
			diff = (COMPARE);				\
			if (diff == 0) {				\
				(found) = TRUE;				\
				break;					\
			} else if (diff < 0) {				\
				lo = mid + 1;				\
			} else {					\
				hi = mid - 1;				\
			}						\
		}							\
	} while(0);

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