[m-rev.] for review: Call mdb 'list' external command using posix_spawnp.

[Zoltan Somogyi]

2020-10-05 16:49 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> +:- pragma foreign_decl("C", local, "
> +// See library/io.m regarding declartion of the environ global variable.

Fix spelling.

> +    rc = posix_spawn_file_actions_init(&file_actions);
> +    if (rc == -1) {
> +        *ret_errno = errno;
> +        goto error_cleanup0;
> +    }

I would give the labels error_cleanup[01] names that reflect
exactly *what* they clean up, since that is less error prone.

I don't have any experience with building the action
descriptors or spawn attribute lists, so I can't effectively check these.
It should therefore be checked by machine, so you should add
a test of the mdb "list" command to a test case in tests/debugger.
(Such a test should have been added earlier, but it seems it wasn't.)

> %---------------------------------------------------------------------------%
> %---------------------------------------------------------------------------%
> diff --git a/trace/mercury_trace_cmd_browsing.c b/trace/mercury_trace_cmd_browsing.c
> index 0341c214d..343314f94 100644
> --- a/trace/mercury_trace_cmd_browsing.c
> +++ b/trace/mercury_trace_cmd_browsing.c
> @@ -901,6 +901,8 @@ MR_trace_cmd_list(char **words, int word_count,
>     const MR_ProcLayout     *entry_ptr;
>     const char              *filename;
>     int                     lineno;
> +    int                     first_lineno;
> +    int                     last_lineno;
>     MR_Word                 *base_sp_ptr;
>     MR_Word                 *base_curfr_ptr;
>     MR_Unsigned             num = MR_num_context_lines;
> @@ -923,16 +925,22 @@ MR_trace_cmd_list(char **words, int word_count,
>         MR_make_aligned_string(aligned_filename, (MR_String) filename);
>     );
> +    first_lineno = lineno - num;
> +    last_lineno = lineno + num;
> +    if (first_lineno < 1) {
> +        first_lineno = 1;
> +    }
> +
>     if (MR_listing_cmd != NULL && strlen(MR_listing_cmd) > 0) {
>         MR_TRACE_CALL_MERCURY(
>             ML_LISTING_list_file_with_command(MR_mdb_out, MR_mdb_err,
>                 MR_listing_cmd, (char *) aligned_filename,
> -                lineno - num, lineno + num, lineno, MR_listing_path);
> +                first_lineno, last_lineno, lineno, MR_listing_path);

Having a contract between this code and the external list program
guarantee that first_lineno is never too small but NOT guarantee
that last_lineno is never too high seems ... odd. I would make
the contract symmetrical. Is it documented anywhere?

Apart from that, the diff is fine.

Have you considered adding posix_spawn/spawnp to extras/posix?

Zoltan.