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

[Pedro Alves <palves@redhat.com>]

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