Re: [PATCH v3 5/6] Share fork_inferior et al with gdbserver

Pedro Alves
2017-03-13T15:34:07Z

On 03/07/2017 07:51 PM, Sergio Durigan Junior wrote:
> On Wednesday, February 15 2017, Pedro Alves wrote:
>

>>> +int
>>> +fork_inferior (char *exec_file_arg, char *allargs, char **env,
>>> + void (*traceme_fun) (void), void (*init_trace_fun) (int),
>>> + void (*pre_trace_fun) (void), char *shell_file_arg,
>>> + void (*exec_fun)(const char *file, char * const *argv,
>>> + char * const *env))
>>> +{
>>
>>> + /* Retain a copy of our environment variables, since the child will
>>> + replace the value of environ and if we're vforked, we have to
>>> + restore it. */
>>> + save_our_env = environ;
>>> +
>>> + /* Likewise the current UI. */
>>> + save_ui = current_ui;
>>
>> Hmm, making this compile on gdbserver is of course a hack,
>> since there's not such thing as a "UI" concept on gdbserver...
>>
>>> +
>>> + /* Tell the terminal handling subsystem what tty we plan to run on;
>>> + it will just record the information for later. */
>>> + new_tty_prefork (inferior_io_terminal);
>>
>> I wonder about calling here instead a more generalized hook, and
>> moving the save_ui saving and the new_tty_prefork calls to
>> that hook's gdb implementation. Might be easier to do that after
>> the series is in...
>
> Right, it is a hack indeed, and not a beautiful one.
>
> I implemented a bunch of new functions that solve this problem. Two of
> them, tty_{pre,post}fork_hook, are responsible for saving/restoring the
> current 'struct ui' and also for calling the new_tty_{pre,post}fork
> functions (which are now static). One more function was needed:
> switch_ui_postfork, which is responsible for switching the current_ui to
> main_ui, as is done currently after we successfully fork. These 3
> functions are implemented only on GDB; they're stubs on gdbserver.

Thanks. Sounds better, though the "ui" in switch_ui_postfork makes me
wonder whether it's generalized enough, but I'll leave further
comments (if any) for v4, which I haven't read yet.

>>> --- a/gdb/gdbserver/inferiors.c
>>> +++ b/gdb/gdbserver/inferiors.c
>>> @@ -29,6 +29,8 @@ struct thread_info *current_thread;
>>>
>>> #define get_thread(inf) ((struct thread_info *)(inf))
>>>
>>> +ptid_t inferior_ptid;
>>
>> What do we need this for? gdbserver already has
>> a "current thread" global. Another one looks like asking
>> for out-of-sync trouble.
>
> This is needed because fork_inferior et al reference this variable
> directly, and so I moved the 'extern' declaration of it to commom/.
>
> A possible solution to this would be to create a get/set pair of
> functions for it, but I'm not sure this would be a good idea due to (a)
> the number of direct references to it, and (b) the fact that these
> functions would probably end up being stubs on gdbserver as well.

Or we can re-evaluate why do fork_inferior et al need to reference
the variable directly.

>>> -/* Add a process to the common process list, and set its private
>>> - data. */
>>> +/* Update process represented by PID with necessary info. */
>>>
>>> static struct process_info *
>>> -linux_add_process (int pid, int attached)
>>> +linux_update_process (int pid)
>>
>> I'm not sure I understand the need for this yet. I need
>> to look deeper. "update what? why?" Or maybe the
>> comments should be improved. :-)
>
> The reason these 'update' functions were created is because
> fork_inferior already creates the process/thread structures, but we (the
> caller) still need to fill in some of the fields of these structures
> with more information. They are the same functions that existed before,
> but now we work with an existing process/thread, while before we
> *created* these structures.

I think that that's only somewhat clear because you're looking at
a diff. ISTM that someone looking at the resulting code with
no other context has no clue what's being updated, and/or why. All one
has to go by is "update". But, what kind of information is being
updated? I guess what I'm saying is that "Update process" is so vague
that it's borderline meaningless.

>>> @@ -2870,62 +2893,91 @@ handle_v_run (char *own_buf)
>>> new_argc++;
>>> }
>>>
>>> - new_argv = (char **) calloc (new_argc + 2, sizeof (char *));
>>> - if (new_argv == NULL)
>>> - {
>>> - write_enn (own_buf);
>>> - return 0;
>>> - }
>>> -
>>> - i = 0;
>>> - for (p = own_buf + strlen ("vRun;"); *p; p = next_p)
>>> + for (i = 0, p = own_buf + strlen ("vRun;"); *p; p = next_p, ++i)
>>> {
>>> next_p = strchr (p, ';');
>>> if (next_p == NULL)
>>> next_p = p + strlen (p);
>>>
>>> - if (i == 0 && p == next_p)
>>> - new_argv[i] = NULL;
>>> + if (p == next_p)
>>> + new_argv.push_back ("''");
>>> else
>>> {
>>> /* FIXME: Fail request if out of memory instead of dying. */
>>> - new_argv[i] = (char *) xmalloc (1 + (next_p - p) / 2);
>>> - hex2bin (p, (gdb_byte *) new_argv[i], (next_p - p) / 2);
>>> - new_argv[i][(next_p - p) / 2] = '\0';
>>> + size_t len = 1 + (next_p - p) / 2;
>>> + char *s = (char *) xmalloc (len);
>>> + char *ss = (char *) xmalloc (len * 2);
>>> + char *tmp_s, *tmp_ss;
>>> + int need_quote;
>>> +
>>> + hex2bin (p, (gdb_byte *) s, (next_p - p) / 2);
>>> + s[(next_p - p) / 2] = '\0';
>>> +
>>> + tmp_s = s;
>>> + tmp_ss = ss;
>>> + need_quote = 0;
>>> + while (*tmp_s != '\0')
>>> + {
>>> + switch (*tmp_s)
>>> + {
>>> + case '\n':
>>> + *tmp_ss = '\'';
>>> + ++tmp_ss;
>>> + need_quote = 1;
>>> + break;
>>> +
>>> + case '\'':
>>> + *tmp_ss = '\\';
>>> + ++tmp_ss;
>>> + break;
>>> +
>>> + default:
>>> + break;
>>> + }
>>> +
>>> + *tmp_ss = *tmp_s;
>>> + ++tmp_ss;
>>> + ++tmp_s;
>>> + }
>>> +
>>> + if (need_quote)
>>> + *tmp_ss++ = '\'';
>>
>> Hmm, is this quoting stuff being moved from somewhere,
>> or it is new?
>
> This is new, even though GDB has a lot of places that do the same
> thing...

OK, but then what is the code doing? Can we at least add some
comment?

>
>>> +
>>> + *tmp_ss = '\0';
>>> + new_argv.push_back (ss);
>>> + xfree (s);
>>> }
>>>
>>> if (*next_p)
>>> next_p++;
>>> - i++;
>>> }
>>> - new_argv[i] = NULL;
>>
>>>
>>> + /* Gather information about the environment. */
>>> + our_environ = make_environ ();
>>> + init_environ (our_environ);
>>> +
>>> initialize_async_io ();
>>> initialize_low ();

>>> @@ -39,8 +39,14 @@ set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
>>> set msg "gdbserver exits cleanly"
>>> set saw_exiting 0
>>> expect {
>>> - # This is what we get on ptrace-based targets.
>>> - -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" {
>>> + # This is what we get on ptrace-based targets with
>>> + # startup-with-shell disabled.
>>> + -re "stdin/stdout redirected.*gdbserver: Cannot exec
>>> non-existing-program\r\ngdbserver: Error: No such file or
>>> directory\r\n\r\nDuring startup program exited with code
>>> 127\.\r\nExiting\r\n$" {
>>> + set saw_exiting 1
>>> + exp_continue
>>
>> Shouldn't this be a part of the next patch?
>
> Not really. I put this here because without it a regreession is
> introduced, and I wanted each patch to be regression-free.

I'm confused then. The comment above says:

"This is what we get on ptrace-based targets with startup-with-shell disabled"

But the patch's intro said:

"I decided to go ahead and implement a partial support for starting the
inferior with a shell on gdbserver, although the full feature comes in
the next patch. The user won't have the option to disable the
startup-with-shell, and also won't be able to change which shell
gdbserver will use (other than setting the $SHELL environment
variable, that is)."

So which one is right?

Thanks,
Pedro Alves