Re: [PATCH v4 4/5] Share fork_inferior et al with gdbserver

[Pedro Alves <palves@redhat.com>]

On 03/17/2017 01:02 AM, Sergio Durigan Junior wrote:
> On Monday, March 13 2017, Pedro Alves wrote:
>>
>> [I think at least some of my comments I sent earlier
>> today on v3 still apply, so I won't repeat them.]
> 
> You mean I haven't addressed everything you commented?  If that is the
> case, I truly apologize.  I'll go over the comments again.

No, I meant what I said.  :-)  I had sent comments to patches in v3 on 
the same day immediately before reading v4 and replying to this thread,
and some of the comments I had sent to v3 applied to v4 too, so I
didn't repeat them against v4:

 https://sourceware.org/ml/gdb-patches/2017-03/msg00189.html
 https://sourceware.org/ml/gdb-patches/2017-03/msg00190.html
 https://sourceware.org/ml/gdb-patches/2017-03/msg00191.html

> Offhand I don't see how to get rid of these functions (on fork_inferior)
> without creating even more stubs on gdbserver, but I'll see.

The first line of action would be I think to look at why do we
even need to reference inferior_ptid there, see if we can
do something else that avoids it.

>>> -  if (new_argv[0] == NULL)
>>> +  if (new_argv.empty () || new_argv[0] == NULL)
>>>      {
>>>        /* GDB didn't specify a program to run.  Use the program from the
>>>  	 last run with the new argument list.  */
>>>  
>>> -      if (program_argv == NULL)
>>> +      if (program_argv.empty ())
>>>  	{
>>>  	  write_enn (own_buf);
>>> -	  freeargv (new_argv);
>>> +	  free_vector_argv (new_argv);
>>>  	  return 0;
>>>  	}
>>>  
>>> -      new_argv[0] = strdup (program_argv[0]);
>>> -      if (new_argv[0] == NULL)
>>> +      new_argv.push_back (strdup (program_argv[0]));
>>> +      if (new_argv.empty () || new_argv[0] == NULL)
>>
>> On the "empty()" check: you've just pushed an element to the vector,
>> so the vector can't be empty.
> 
> Indeed.
> 
>> On the "== NULL" check: IIUC, the old NULL check was there to
>> handle strdup returning NULL due to out-of-memory.
>> See NULL checks and comments further above in this function.
>> Now that you're using a std::vector, that doesn't work or make
>> sense any longer, since if push_back fails to allocate space for
>> its internal buffer (with operator new), our operator new replacement
>> (common/new-op.c) calls malloc_failure, which aborts gdbserver.
>>
>> Not sure it makes sense to handle out-of-memory specially in
>> the gdb/rsp-facing functions nowadays (maybe git blame/log/patch
>> submission for that code shows some guidelines).  Maybe (or, probably)
>> it's OK to stop caring about it, but then we should consistently remove
>> left over code, by using xstrdup instead and remove the NULL checks.
> 
> OK, so from what I understood, this part of the code can be removed,
> right?  I guess removing all the leftover code is something to be done
> in another patch series.

Well, it only becomes "left over code" when we switch to use vector.

Is it possible to split these C++fication changes (std::vector/std::string)
to a separate preparatory patch?

>>> +void
>>> +free_vector_argv (std::vector<char *> &v)
>>> +{
>>> +  for (char *&i : v)
>>
>> Iterating over pointers, no need for reference indirection,
>> copy is fine:
>>
>>   for (char *el: v)
>>     xfree (el);
> 
> Wait, I could swear I had removed this!  Anyway, doing it now, thanks.

You had removed it elsewhere in the patch, but missed here.

> 
>>> +    xfree (i);
>>> +
>>> +  v.clear ();
>>> +}
>>> +
>>> +/* See common/common-utils.h.  */
>>> +
>>> +std::string
>>> +stringify_argv (const std::vector<char *> &argv)
>>> +{
>>> +  std::string ret;
>>> +
>>> +  for (auto s : argv)
>>> +    ret += s + std::string (" ");
>>> +
>>> +  /* Erase the last whitespace.  */
>>> +  ret.erase (ret.end () - 1);
>>
>> Are we always sure ARGV wasn't empty here?
> 
> No.  And I see what you mean: we should check it before erasing the last
> whitespace.  Will fix.

Exactly.

>>> @@ -627,6 +625,8 @@ win32_create_inferior (char *program, char **program_args)
>>>    int argc;
>>>    PROCESS_INFORMATION pi;
>>>    DWORD err;
>>> +  std::string program_args = stringify_argv (program_argv);
>>> +  char *args = (char *) program_args.c_str ();
>>
>> Why do we need the cast?
> 
> Because "args" is a char *?  I haven't checked if "args" could be made a
> const, but I can.

Right, why is it that "char *" needs to be non-const.  The cast
was just what made the alarm bell ring.  :-)

Thanks,
Pedro Alves