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

[Zoltan Somogyi]

> In that case, it should have been
> 
> 	#include "mercury_getopt.h"

But the file you checked in is just getopt.h, not mercury_getopt.h.

BTW, getopt.h is causing lots of warnings from the C compiler about
missing prototypes, presumably because we don't pass the right set of
#defines.

> > > 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? ;-)

In these changes, it just so happens that I never need anything that just does
what make_array does; the more complex macro I proposed is what I need.

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

I did; the above was for your information.

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

I don't know what the memcpy bug was, so I can't say whether it is the same or
not.

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

In this case, the problem was with the condition in the loop, not the body.
The bug stayed when I replaced the structure assignment in the loop with
MR_memcpy, as well as when I moved the call to MR_memcpy outside the loop.
(In fact, I added a variant MR_memcpy_fromhigh just so that the latter
would be OK, but to no avail.) The key to the problem seems to be the use
of the specific function strcmp in the condition of the loop; if I replace
that with a call to another function, the bug goes away.

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

It would, but a binary search + linear move is slower than a one-pass
linear search & move, because it never has to load an array element twice.

In fact, in most cases there is a binary search beforehand anyway, because
we don't know whether we want to insert the entry into the array until we
have checked whether it is already there. However, exploiting the information
left around by the binary search to shortcut the linear search is not trivial.

And, as you observed, the data structures involved are all small, so
the speed of the chosen algorithm does not really matter.

Zoltan.