Re: [PATCH 2/8] Fix follow-fork latent bug

[Pedro Alves <palves@redhat.com>]

On 04/13/2017 10:58 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> -	  old_chain = save_inferior_ptid ();
>> -	  save_current_program_space ();
>> +	  old_chain = save_current_space_and_thread ();
>>  
>>  	  inferior_ptid = child_ptid;
>>  	  add_thread (inferior_ptid);
>> +	  set_current_inferior (child_inf);
>>  	  child_inf->symfile_flags = SYMFILE_NO_READ;
> 
> Can we set up child thread_info inferior and pspace first, and then,
> call switch_to_thread_no_regs to switch them in one go?

Although it seemingly works, and I tried this patch on top
of the series:

diff --git c/gdb/infrun.c w/gdb/infrun.c
index fcafdc1..3c5f583 100644
--- c/gdb/infrun.c
+++ w/gdb/infrun.c
@@ -500,9 +500,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 
          old_chain = save_current_space_and_thread ();
 
-         inferior_ptid = child_ptid;
-         add_thread (inferior_ptid);
-         set_current_inferior (child_inf);
+         add_thread (child_ptid);
          child_inf->symfile_flags = SYMFILE_NO_READ;
 
          /* If this is a vfork child, then the address-space is
@@ -629,9 +627,7 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
         this new thread, before cloning the program space, and
         informing the solib layer about this new process.  */
 
-      inferior_ptid = child_ptid;
-      add_thread (inferior_ptid);
-      set_current_inferior (child_inf);
+      add_thread (child_ptid);
 
       /* If this is a vfork child, then the address-space is shared
         with the parent.  If we detached from the parent, then we can
@@ -657,6 +653,8 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
             the core, this wouldn't be required.  */
          solib_create_inferior_hook (0);
        }
+
+      switch_to_thread (child_ptid);
     }
 
   return target_follow_fork (follow_child, detach_fork);


I don't think I'd like to do that (at least not now), because it doesn't
look like clone_program_space and solib_create_inferior_hook really
work correctly if called without switching the current inferior to
point to the child.

For example, I set a breakpoint on current_inferior, active
while GDB is running clone_program_space, and with
"set detach-on-fork off" for example, we get many hits
which currently (it looks to me incorrectly) refer to the
parent inferior instead of the child inferior that is
loading symbols.  Here's a sample:

(top-gdb) bt
#0  0x00000000006c2baa in current_inferior() () at src/gdb/inferior.c:59
#1  0x00000000006a5026 in set_target_gdbarch(gdbarch*) (new_gdbarch=0x19aba30) at src/gdb/gdbarch.c:5438
#2  0x00000000005b21ac in set_gdbarch_from_file(bfd*) (abfd=0x1193540) at src/gdb/arch-utils.c:620
#3  0x000000000067f2a8 in exec_file_attach(char const*, int) (filename=0x1894a30, from_tty=0) at src/gdb/exec.c:383
#4  0x0000000000729985 in clone_program_space(program_space*, program_space*) (dest=0x1c8d3a0, src=0x11e8de0)
    at src/gdb/progspace.c:193
#5  0x00000000006c571a in follow_fork_inferior(int, int) (follow_child=0, detach_fork=0)
    at infrun.c:527

(top-gdb) bt
#0  0x00000000006c2baa in current_inferior() () at src/gdb/inferior.c:59
#1  0x00000000005b66f5 in invalidate_auxv_cache() () at src/gdb/auxv.c:345
#2  0x000000000070f705 in observer_executable_changed_notification_stub(void const*, void const*) (data=0x5b66ec <invalidate_auxv_cache()>, args_data=0x7fffffffd028) at ./observer.inc:364
#3  0x000000000070ef72 in generic_observer_notify(observer_list*, void const*) (subject=0x11085e0, args=0x7fffffffd028)
    at src/gdb/observer.c:167
#4  0x000000000070f796 in observer_notify_executable_changed() () at ./observer.inc:387
#5  0x000000000067f316 in exec_file_attach(char const*, int) (filename=0x1894a30 "gdb/testsuite/outputs/gdb.base/foll-fork/foll-fork", from_tty=0) at src/gdb/exec.c:399
#6  0x0000000000729985 in clone_program_space(program_space*, program_space*) (dest=0x1c8d3a0, src=0x11e8de0)
    at src/gdb/progspace.c:193

I'd rather not have to dig into this area too deeply at this point.

Thanks,
Pedro Alves