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

Pedro Alves
2017-03-17T10:27:02Z

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