Date: Thu, 28 Jun 2001 20:51:11 +0300
From: "Eli Zaretskii" <eliz AT is DOT elta DOT co DOT il>
Sender: halo1 AT zahav DOT net DOT il
To: "Mark E." <snowball3 AT bigfoot DOT com>
Message-Id: <1659-Thu28Jun2001205110+0300-eliz@is.elta.co.il>
X-Mailer: Emacs 20.6 (via feedmail 8.3.emacs20_6 I) and Blat ver 1.8.9
CC: djgpp-workers AT delorie DOT com
In-reply-to: <3B39E3C5.6290.765DBB@localhost> (snowball3@bigfoot.com)
Subject: Re: v2loadimage and proxy
References:  <3B39E3C5 DOT 6290 DOT 765DBB AT localhost>
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

> From: "Mark E." <snowball3 AT bigfoot DOT com>
> Date: Wed, 27 Jun 2001 13:46:45 -0400
> 
> This patch adds proxy support to v2loadimage. Comments?

Thanks.  I think this is a very good design, but there are a few
boring details which we need to get right (it's always like this with
these aspects of subprogram invocation).

> + static
> + const char * get_arg(const char *ptr, const char **beg, const char **end)
> + {
> +   while (*ptr && isspace(*ptr))
> +     ++ptr;
> +   if (!is_quote(*ptr))
> +   {
> +     *beg = ptr;
> +     while (*ptr && !isspace(*ptr))
> +       ++ptr;
> +     *end = ptr;
> +   }
> +   else
> +   {

This assumes that the arguments can only have quotes at their
beginning and end.  This is not so: a user could say something like
this in GDB:

	run c:/"Program Files"/foo/bar/"My Input File".dat

and expect it to work, because it works from the shell prompt, and
also works in stock v2loadimage from v2.03.

(Yes' I've seen your modified version of get_arg, but it also makes
the same assumption.)

> +   if (*tb_space > 126)
> +   {
> +     sprintf(proxy, "%s=%04x %04lx %04lx", proxy_string, (unsigned int)argc,
> +                                         tbuf / 16, tbuf & 0x0f);
> +     putenv(proxy);
> +   }

This adds the proxy variable to the debugger's own environment.  I
think we should remove this variable from the debugger's environment
after v2loadimage returns, since the only reason you put it there is
so that it will be copied into the debuggee's PSP.  We don't really
want it in the debugger itself.

> +   /* Now insert the command line into argv[].  */
> +   i = 1;
> +   while (i < argc)
> +   {
> +     ptr = get_arg(ptr, &beg, &end);
> +     arg_len = end - beg;
> +     dosmemput(beg, arg_len, argv_ptr);

If the argument is quoted, this seems to cut off the closing quote:
`get_arg' sets `beg' to point to the opening quote, while `end' points
to the closing quote.  So "end - beg" is one character too few, right?
Or did I miss something?

> +   /* Create the proxy variable now so the child's environment
> +      has the correct size.  */
> +   while (*cmdline && isspace(*cmdline))
> +     ++cmdline;
> +   proxy_argc = make_proxy_var(program, cmdline, __tb, __tb_size, &proxy_space);

This seems to assume that `cmdline' is a C-style string.  But that's
not so: v2loadimage requires `cmdline' to be in the format of the DOS
command tail (see "info libc alpha v2loadimage"): it begins with a
byte that gives the command-line length, then up to 126 characters of
the command line itself, then the CR character, and no terminating
null(!).

So you need at least to get past the first byte, before you start
handling `cmdline' as a normal string.  And since it's not
null-terminated, the code should keep that in mind; for example,
get_arg should watch for the trailing CR instead of a null.  In
particular, that trailing CR should not be copied to the transfer
buffer with the last argument.

Also, if we now allow long command lines, we will need to fix the
leading byte (set it to 126) and truncate it with a CR at
cmdline[127], before it gets copied to the debuggee's PSP.