[m-rev.] for review: integrity checks

[Fergus Henderson]

On 11-Jun-2003, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Add a mechanism that can detect data structure corruption. The mechanism takes
> the form of an option to mdb's forward movement commands, but is enabled only
> if the trace directory is compiled with an implementor-only compile-time flag.
> The option is therefore not documented.

The option should be documented in runtime/mercury_conf_param.h,
if not elsewhere.

> +++ trace/mercury_trace_vars.c	11 Jun 2003 09:13:58 -0000
> @@ -1255,3 +1255,70 @@
>  
>      return NULL;
>  }
> +
> +#ifdef  MR_TRACE_CHECK_INTEGRITY
> +
> +static void
> +MR_trace_check_integrity_on_cur_level(void)
> +{
> +    int         i;
> +
> +    for (i = 0; i < MR_point.MR_point_var_count; i++) {
> +        (void) MR_trace_browse_var(stdout, &MR_point.MR_point_vars[i],
> +                (MR_String) (MR_Integer) "", MR_trace_print,
> +                MR_BROWSE_CALLER_PRINT, MR_BROWSE_DEFAULT_FORMAT);
> +    }
> +}

A comment there would be helpful, e.g. explaining how calling
MR_trace_browse_var() serves to check integrity.


> +#define MR_INTEGRITY_ERROR_BUF_SIZE    512
> +
> +static  int         MR_check_max_mr_num;
> +static  MR_Word     MR_check_saved_regs[MR_MAX_FAKE_REG];

Why are those variables static globals, rather than being 
local to the MR_trace_check_integrity() function and having
automatic storage duration?

> +static  int         MR_check_integrity_seq_num = 0;

Likewise, why is that one global rather than being a static variable
local to the function?

> +void
> +MR_trace_check_integrity(const MR_Label_Layout *layout, MR_Trace_Port port)
> +{
> +    int         level;
> +    const char  *problem;
> +    char        buf[MR_INTEGRITY_ERROR_BUF_SIZE];
> +    MR_bool     saved_trace_enabled;
>
> +    saved_trace_enabled = MR_trace_enabled;
> +    MR_trace_enabled = MR_FALSE;
> +
> +    MR_compute_max_mr_num(MR_check_max_mr_num, layout);
> +    MR_restore_transient_registers();
> +    /* This also saves the regs in MR_fake_regs. */
> +    MR_copy_regs_to_saved_regs(MR_check_max_mr_num, MR_check_saved_regs);
> +	MR_trace_init_point_vars(layout, MR_check_saved_regs, port, MR_TRUE);
> +
> +    if (MR_point.MR_point_problem != NULL) {
> +        MR_fatal_error(problem);
> +    }
> +
> +    level = 0;
> +    do {
> +        MR_check_integrity_seq_num++;
> +#if 0
> +        /* enable this code if necessary for debugging */
> +        sprintf(buf, "integrity check at event %d, level %d, seq %d\n",
> +            MR_trace_event_number, level, MR_check_integrity_seq_num);
> +#endif
> +        MR_trace_report_msg = buf;
> +        fprintf(stdout, "%s", buf);
> +        fflush(stdout);

"buf" is uninitialized at this point, due to the `#if 0'.
Perhaps the "#endif" is in the wrong place??

Also, this diff refers to MR_trace_report_msg, which is not defined.

Otherwise that looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------