Debugging through exec() (Linux MAY_FOLLOW_EXEC)

Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 4682 bytes --]

Hi,

please review the proposed functionality to be able to debug though exec().
In fact I am not aware of any change until you type "catch exec".
Daniel Jacobowitz has objections to the correctness of the patch.

Some demo of the attached patch functionality:

(gdb) tcatch exec
Catchpoint 1 (exec)
(gdb) run
Starting program: /tmp/execve
Executing new program: /tmp/hello
[Switching to process 1588]
### Notice here the symbol is no longer resolved as gdb still did not detect
### right after exec() that it has shared library loaded.
0x44031840 in ?? ()
(gdb) tbreak main
Breakpoint 2 at 0x8048395: file hello.c, line 6.
(gdb) continue
Continuing.
[Switching to process 1588]
main () at hello.c:6
6               puts("hello world");
(gdb) continue
Continuing.
hello world
Program exited normally.
(gdb) run
### New messages notifying the loaded executable changed.
Restoring the program name before exec().
Restoring the symbol table before exec().
Starting program: /tmp/execve
Executing new program: /tmp/hello
hello world
Program exited normally.
(gdb)

Original behavior was:

(gdb) tcatch exec
Catchpoint 1 (exec)
(gdb) run
Starting program: /tmp/execve
### Symbol is resolved here but the shared library is in fact no longer valid.
0x44031840 in _start () from /lib/ld-linux.so.2
(gdb) tbreak main
### Break address/sourcefile is stale here and thus disfunctional.
Breakpoint 2 at 0x80483b4: file execve.c, line 7.
(gdb) continue
Continuing.
hello world
Program exited normally.
(gdb)


Please read the mails from Daniel Jacobowitz below:


On Tue, 08 Aug 2006 18:01:13 +0200, Daniel Jacobowitz wrote:
> On Sat, Aug 05, 2006 at 06:41:44PM +0200, Jan Kratochvil wrote:
> > Hi Mark,
> > 
> > On Mon, 31 Jul 2006 22:38:43 +0200, Mark Kettenis wrote:
> > ...
> > > That WNOHANG is wrong;
> > 
> > In fact yes, the patch is more correct without that WNOHANG hack there.
> > 
> > 
> > 2006-07-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* inf-ptrace.c (inf_ptrace_mourn_inferior): waitpid(2) only if there
> > 	is valid inferior_ptid to wait for.
> > 	* linux-fork.c (linux_fork_mourn_inferior): Ditto.
> > 	* infrun.c (follow_exec): Unconditionally enabled by MAY_FOLLOW_EXEC.
> > 	Provide restoration of exec_bfd and symfile_objfile for any new "run".
> > 	* linux-thread-db.c (thread_db_wait): Handle TARGET_WAITKIND_EXECD.
> > 	* linux-thread-db.c (thread_db_mourn_inferior): Turn off threading.
> > 	* foll-exec.exp: Uncoditionally enabled for all platforms.
> > 	Relaxed regex to apply besides HP-UX also for GNU/Linux backtrace.
> 
> Sorry, but I can't approve this patch.  I think someone needs to
> discuss the concept and interface on gdb@ first.
> 
> You're using make_run_cleanup to restore the inferior file.  This means
> it happens before "run".  Any time we choose to change the file
> silently will be surprising (especially to front ends like Eclipse) but
> that time will be pretty surprising to the user too:
> 
> ... info files shows first prog
> (gdb) catch exec
> (gdb) run
> ... info files shows second prog
> (gdb) continue
> ... exits.
> ... info files shows second prog
> (gdb) run
> ... starts first prog!
> 
> I also think that changing inferior_ptid before mourning is pretty
> strange.  The whole way follow_exec works is a hack and not
> particularly well defined.
> 
> This is a hard problem to solve; I think a half-finished solution would
> be worse than leaving it unsolved.
> 
> -- 
> Daniel Jacobowitz
> CodeSourcery


Daniel's earlier mail:

On Mon, 24 Jul 2006 21:03:32 +0200, Daniel Jacobowitz wrote:
> Jan Kratochvil wrote:
> > On Fri, 21 Jul 2006 20:44:21 +0200, Daniel Jacobowitz wrote:
> > ...
> > > Turning on MAY_FOLLOW_EXEC is not a good idea.  No one really knows how
> > > that behavior works, a lot of it doesn't, and the way it implicitly
> > > changes the symbol file is very disorienting.  Please don't mix it up
> > > with the fix for your current bug.
> > 
> > Still I am for MAY_FOLLOW_EXEC as it improves the user experience and makes
> > debugging of exec()ing processes much more convenient - without having to find
> > out how each child gets executed and replay such conditions by hand.
> > 
> > As gdb-6.5 has been released and the MAY_FOLLOW_EXEC feature IMO generally
> > works for GNU/Linux - isn't appropriate to enable it and settle it down?
> > I would even like to fix any issues possibly roaring its head.
> 
> Does anyone else have an opinion on this?  I'm starting to think you're
> right - we should turn it on, invite people to use it, and see what
> happens.
> 
> The reason I find it so disorienting is this:
[ silent changing of the loaded exec + symbol file ]


Thanks,
Jan Kratochvil

[-- Attachment #2: gdb-6.5-follow-exec-v3.patch --]
[-- Type: text/plain, Size: 7662 bytes --]

Index: gdb/inf-ptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/inf-ptrace.c,v
retrieving revision 1.32
diff -u -p -r1.32 inf-ptrace.c
--- gdb/inf-ptrace.c	24 Jan 2006 22:34:34 -0000	1.32
+++ gdb/inf-ptrace.c	14 Aug 2006 15:04:53 -0000
@@ -167,7 +167,8 @@ inf_ptrace_mourn_inferior (void)
      Do not check whether this succeeds though, since we may be
      dealing with a process that we attached to.  Such a process will
      only report its exit status to its original parent.  */
-  waitpid (ptid_get_pid (inferior_ptid), &status, 0);
+  if (! ptid_equal (inferior_ptid, null_ptid) && target_has_execution)
+    waitpid (ptid_get_pid (inferior_ptid), &status, 0);
 
   unpush_target (ptrace_ops_hack);
   generic_mourn_inferior ();
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.213
diff -u -p -r1.213 infrun.c
--- gdb/infrun.c	22 Jul 2006 14:48:03 -0000	1.213
+++ gdb/infrun.c	14 Aug 2006 15:04:56 -0000
@@ -47,6 +47,7 @@
 #include "language.h"
 #include "solib.h"
 #include "main.h"
+#include "objfiles.h"
 
 #include "gdb_assert.h"
 #include "mi/mi-common.h"
@@ -109,10 +110,10 @@ int sync_execution = 0;
 static ptid_t previous_inferior_ptid;
 
 /* This is true for configurations that may follow through execl() and
-   similar functions.  At present this is only true for HP-UX native.  */
+   similar functions.  */
 
 #ifndef MAY_FOLLOW_EXEC
-#define MAY_FOLLOW_EXEC (0)
+#define MAY_FOLLOW_EXEC (1)
 #endif
 
 static int may_follow_exec = MAY_FOLLOW_EXEC;
@@ -375,6 +376,45 @@ follow_inferior_reset_breakpoints (void)
   insert_breakpoints ();
 }
 
+static void
+follow_exec_restore_execfile (void *filename_new_untyped)
+{
+  char *filename_new = filename_new_untyped;
+
+  /* filename_new == NULL is not expected.  */
+  if (filename_new == NULL && exec_bfd != NULL)
+    exec_file_clear (0);
+  /* exec_bfd == NULL is not expected.  */
+  if (filename_new != NULL && 
+      (exec_bfd == NULL || strcmp (get_exec_file (0), filename_new)))
+    {
+      /* The filename will be printed already below: Starting program: %s  */
+      printf_unfiltered (_("Restoring the program name before exec().\n"));
+      exec_file_attach (filename_new, 0);
+    }
+
+  free (filename_new);
+}
+
+static void
+follow_exec_restore_symfile (void *filename_new_untyped)
+{
+  char *filename_new = filename_new_untyped;
+
+  /* symfile_objfile == NULL is not expected.  */
+  if (filename_new == NULL && symfile_objfile != NULL)
+    symbol_file_clear (0);
+  if (filename_new != NULL && (symfile_objfile == NULL
+      || strcmp (symfile_objfile->name, filename_new)))
+    {
+      /* The filename will be printed already below: Starting program: %s  */
+      printf_unfiltered (_("Restoring the symbol table before exec().\n"));
+      symbol_file_add_main (filename_new, 0);
+    }
+
+  free (filename_new);
+}
+
 /* EXECD_PATHNAME is assumed to be non-NULL. */
 
 static void
@@ -382,6 +422,7 @@ follow_exec (int pid, char *execd_pathna
 {
   int saved_pid = pid;
   struct target_ops *tgt;
+  struct objfile *objfile, *objfile_temp;
 
   if (!may_follow_exec)
     return;
@@ -427,6 +468,32 @@ follow_exec (int pid, char *execd_pathna
     error (_("Could find run target to save before following exec"));
 
   gdb_flush (gdb_stdout);
+
+  /* During the common "run" bare command we should run again the original
+   * program spawning us.  Stacking order is correct this way.  */
+  make_run_cleanup (follow_exec_restore_symfile, (!symfile_objfile ? NULL :
+                    xstrdup (symfile_objfile->name)));
+  make_run_cleanup (follow_exec_restore_execfile,
+                    (!exec_bfd ? NULL : xstrdup (exec_bfd->filename)));
+
+  /* As symbol_file_add_main()->thread_db_new_objfile()->check_for_thread_db()
+   * would fine already loaded libpthread.so while the threading structures
+   * would not be yet initialized for this early inferior.
+   * Call before target_mourn_inferior() as it will breakpoint_re_set().  */
+#ifdef CLEAR_SOLIB
+  CLEAR_SOLIB ();
+#else
+  clear_solib ();
+#endif
+  /* Do not: symbol_file_clear()->clear_symtab_users()->breakpoint_re_set().  */
+  ALL_OBJFILES_SAFE (objfile, objfile_temp)
+  {
+    free_objfile (objfile);
+  }
+  symfile_objfile = NULL;
+
+  /* Avoid stucked waitpid(2) as PID inferior_ptid is still running.  */
+  inferior_ptid = null_ptid;
   target_mourn_inferior ();
   inferior_ptid = pid_to_ptid (saved_pid);
   /* Because mourn_inferior resets inferior_ptid. */
Index: gdb/linux-fork.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-fork.c,v
retrieving revision 1.7
diff -u -p -r1.7 linux-fork.c
--- gdb/linux-fork.c	27 Apr 2006 23:03:41 -0000	1.7
+++ gdb/linux-fork.c	14 Aug 2006 15:04:56 -0000
@@ -366,7 +366,8 @@ linux_fork_mourn_inferior (void)
      only report its exit status to its original parent.  */
   int status;
 
-  waitpid (ptid_get_pid (inferior_ptid), &status, 0);
+  if (! ptid_equal (inferior_ptid, null_ptid) && target_has_execution)
+    waitpid (ptid_get_pid (inferior_ptid), &status, 0);
 
   /* OK, presumably inferior_ptid is the one who has exited.
      We need to delete that one from the fork_list, and switch
Index: gdb/linux-thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-thread-db.c,v
retrieving revision 1.19
diff -u -p -r1.19 linux-thread-db.c
--- gdb/linux-thread-db.c	2 Aug 2006 10:24:00 -0000	1.19
+++ gdb/linux-thread-db.c	14 Aug 2006 15:04:57 -0000
@@ -884,6 +884,10 @@ thread_db_wait (ptid_t ptid, struct targ
       return pid_to_ptid (GET_PID (ptid));
     }
 
+  /* Threading structures got reset.  Return as nonthreaded.  */
+  if (ourstatus->kind == TARGET_WAITKIND_EXECD)
+    return pid_to_ptid (GET_PID (ptid));
+
   if (ourstatus->kind == TARGET_WAITKIND_STOPPED
       && ourstatus->value.sig == TARGET_SIGNAL_TRAP)
     /* Check for a thread event.  */
@@ -975,6 +979,9 @@ thread_db_mourn_inferior (void)
      the inferior, so that we don't try to uninsert them.  */
   remove_thread_event_breakpoints ();
 
+  /* Destroy thread info; it's no longer valid.  */
+  init_thread_list ();
+
   /* Detach thread_db target ops.  */
   unpush_target (&thread_db_ops);
   using_thread_db = 0;
Index: gdb/testsuite/gdb.base/foll-exec.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/foll-exec.exp,v
retrieving revision 1.3
diff -u -p -r1.3 foll-exec.exp
--- gdb/testsuite/gdb.base/foll-exec.exp	10 Aug 2006 05:27:20 -0000	1.3
+++ gdb/testsuite/gdb.base/foll-exec.exp	14 Aug 2006 15:04:58 -0000
@@ -47,12 +47,6 @@ if  { [gdb_compile "${srcdir}/${subdir}/
 }
 
 
-# Until "catch exec" is implemented on other targets...
-#
-if ![istarget "hppa*-hp-hpux*"] then {
-    continue
-}
-
 proc zap_session {} {
    global gdb_prompt
    global binfile
@@ -214,7 +208,9 @@ proc do_exec_tests {} {
    setup_xfail hppa2.0w-hp-hpux* CLLbs16760
    send_gdb "continue\n"
    gdb_expect {
-     -re ".*Executing new program:.*${testfile2}.*Catchpoint .*(exec\'d .*${testfile2}).*in .START..*$gdb_prompt $"\
+     # It is OS dependent and no symbols may be found, GNU/Linux has "_start"
+     # while HP-UX has " in .START..*$gdb_prompt" etc.
+     -re ".*Executing new program:.*${testfile2}.*Catchpoint .*(exec\'d .*${testfile2}).*in .*$gdb_prompt $"\
                      {pass "hit catch exec"}
      -re "$gdb_prompt $" {fail "hit catch exec"}
      timeout         {fail "(timeout) hit catch exec"}

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Eli Zaretskii]

> Date: Mon, 14 Aug 2006 17:06:28 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Daniel Jacobowitz <drow@false.org>
> 
> please review the proposed functionality to be able to debug though exec().

Thanks.  Please wait for others to comment; what's below are
relatively minor comments.

> +  if (filename_new != NULL && 
> +      (exec_bfd == NULL || strcmp (get_exec_file (0), filename_new)))

Please don't use literal strcmp for file names; use FILENAME_CMP
instead.  That's because of case-insensitive file systems and other
similar complications with file-name comparisons.  The assumption that
file names are just strings is non-portable.

> +      printf_unfiltered (_("Restoring the program name before exec().\n"));

Please don't use "exec()" to indicate that it's a function: it looks
like a call to `exec' with no arguments, which is not what you want.
Just `exec' (with single quotes) is all you need.

> +  if (filename_new != NULL && (symfile_objfile == NULL
> +      || strcmp (symfile_objfile->name, filename_new)))

File-name comparison again.

> +      printf_unfiltered (_("Restoring the symbol table before exec().\n"));

"exec()" again.

Finally, if this change has user-visible effects, it should be
accompanied by a suitable change in NEWS and the user manual.

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Daniel Jacobowitz]

On Tue, Aug 15, 2006 at 12:20:12AM +0300, Eli Zaretskii wrote:
> > Date: Mon, 14 Aug 2006 17:06:28 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > Cc: Daniel Jacobowitz <drow@false.org>
> > 
> > please review the proposed functionality to be able to debug though exec().
> 
> Thanks.  Please wait for others to comment; what's below are
> relatively minor comments.

Eli, have you got an opinion on the broader question, by chance?

This is really an issue of use cases and interface; the technical
changes I'm ok to handle by myself, but there are broader questions
about how this would work (did work, on HP/UX, at some point in the
past) and should work.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Eli Zaretskii]

> Date: Mon, 14 Aug 2006 17:38:49 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>, gdb@sourceware.org
> 
> Eli, have you got an opinion on the broader question, by chance?
> 
> This is really an issue of use cases and interface; the technical
> changes I'm ok to handle by myself, but there are broader questions
> about how this would work (did work, on HP/UX, at some point in the
> past) and should work.

I was afraid you'll ask ;-)

The thing is, I couldn't figure out the broader context of this
suggested change, from what was posted.  There's not enough info; for
example, no source code of the test program which was used to
demonstrate the change in behavior.  The simple transcript of a debug
session was not nearly as self-explanatory as Jan seemed to think.

I will be happy to think about this and speak my views, if Jan (or
someone else) describes in more detail what is wrong with the current
behavior, what is the idea behind his proposal, and how it will change
GDB's behavior across `exec'.

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Jan Kratochvil]

On Tue, 15 Aug 2006 05:22:21 +0200, Eli Zaretskii wrote:
> Daniel Jacobowitz wrote:
> > Eli, have you got an opinion on the broader question, by chance?
...
> The thing is, I couldn't figure out the broader context of this
> suggested change, from what was posted.  There's not enough info;

Currently gdb does not notice `exec' at all - it does not apply the inferior
changes to its debugging information about the inferior.  By default in the
point of `exec' it will lose any breakpoints and the execution will continue
without any gdb influence and the program will just finish uncaught.

If you turn on "catch exec" you will get back the control after `exec' changes
the virtual memory context but gdb will still expect the original executable is
still loaded. All the sources will be displayed for the original program and
breakpoints put to the addresses according to the debuginfo of the original
program, still everything will be messed up as a complately different
executable is already running.

In fact the original intention of this patch was different - gdb locked up on
any `exec' from a threaded program.
	https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=182116
	Cannot find user-level thread for LWP 4256: generic error

it was fixed by a more simple way by Daniel, though:

2006-07-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
            Daniel Jacobowitz  <dan@codesourcery.com>

	* linux-thread-db.c (thread_db_wait): Remove libthread_db
	after exec events.

So I no longer consider this patch as mission critical as before. Still it is
useful for debugging programs like javac(1) or gcc(1) which are in fact only
wrappers. Some years ago I had to always watch (usually using strace(1)) which
next program gets executed and restart the debugging from the spawned process
with the found arguments and environment variables. This patch will just
simplify it, do not care whether this program is the final one or just the
spawning stub, just run it through gdb and catch the problematic point.


And from the principal point - currently (without this patch) "catch exec"
command is just broken - after it catches `exec' the state of the debugger and
the debugging program is inconsistent.


Thanks for the interest,
Jan

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Eli Zaretskii]

> Date: Tue, 15 Aug 2006 16:28:19 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb@sourceware.org
> 
> Currently gdb does not notice `exec' at all - it does not apply the inferior
> changes to its debugging information about the inferior.  By default in the
> point of `exec' it will lose any breakpoints and the execution will continue
> without any gdb influence and the program will just finish uncaught.
> 
> If you turn on "catch exec" you will get back the control after `exec' changes
> the virtual memory context but gdb will still expect the original executable is
> still loaded. All the sources will be displayed for the original program and
> breakpoints put to the addresses according to the debuginfo of the original
> program, still everything will be messed up as a complately different
> executable is already running.

Thanks for the explanations, but I seem to be dense today, because I
don't get to the cheese yet.

Perhaps if you could describe a scenario where the current behavior is
inconvenient, and what does your patch change in this respect, I will
understand the issues.

Please also tell how, if at all, the suggested changes interact with
follow-fork mode.

Re: Debugging through exec() (Linux MAY_FOLLOW_EXEC)

[Jan Kratochvil]

On Fri, 18 Aug 2006 14:27:44 +0200, Eli Zaretskii wrote:
...
> Thanks for the explanations, but I seem to be dense today, because I
> don't get to the cheese yet.
> 
> Perhaps if you could describe a scenario where the current behavior is
> inconvenient, and what does your patch change in this respect, I will
> understand the issues.

I have a Perl script spawning scp(1) in PTY.  This scp(1) will not print the
progress bar if spawned by this Perl script, it shows it otherwise (it uses
isatty() to detect it).  I would like to debug scp(1) there as the bug is
(apparently) not reproducible when scp(1) is executed standalone.
Sure there are many ways how to debug it but I feel this is the right sample
problem for "catch exec".

Unfortunately that my patch will not provide correct enough debugging state
after the "exec" event for the new inferior.  Patch needs to be improven.


Regards,
Jan