[patch] Forbid run with a core file loaded

[patch] Forbid run with a core file loaded

[Jan Kratochvil]

Hi,

there is already a protection against loading a core file when a program is
running.

But one still can now run a program when a core file is loaded.  Moreover GDB
then crashes on `quit'.

(gdb) file sleep
[...]
(gdb) start
[...]
(gdb) core-file core.5841 
A program is being debugged already.  Kill it? (y or n) y
[...]
Program terminated with signal 11, Segmentation fault.
[...]
(gdb) start
Starting program: /bin/sleep 
^^^^^^^^^^^^^^^^ !!!
[...]
(gdb) quit
A debugging session is active.
	Inferior 1 [process 13887] will be killed.
Quit anyway? (y or n) y
inferior.c:362: internal-error: find_inferior_pid: Assertion `pid != 0' failed.


Forbid even the latter case.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Forbid run with a core file loaded.
	* infcmd.c (run_command_1) <core_bfd>: New.

gdb/testsuite/
2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Forbid run with a core file loaded.
	* gdb.base/corefile.exp (load core again, start with core)
	(started with core): New tests.

--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -483,6 +483,15 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main)
 
   dont_repeat ();
 
+  if (core_bfd)
+    {
+      if (!from_tty
+	  || query (_("Core file is already loaded.  Unload it? ")))
+	core_file_command (NULL, from_tty);
+      if (core_bfd)
+	error (_("Core file not unloaded."));
+    }
+
   kill_if_already_running (from_tty);
 
   init_wait_for_inferior ();
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -180,3 +180,15 @@ gdb_load ${binfile}
 gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (reinit)"
 
 gdb_test "core" "No core file now."
+
+# Test a run (start) command will clear any loaded core file.
+
+gdb_test "core-file $corefile" "Core was generated by .*" "load core again"
+
+set test "start with core"
+gdb_test_multiple "start" $test {
+    -re {Core file is already loaded.  Unload it[?] [(]y or n[)] } {
+	pass $test
+    }
+}
+gdb_test "y" {No core file now\..*reakpoint [0-9]+, main.*} "started with core"

Re: [patch] Forbid run with a core file loaded

[Mark Kettenis]

> Date: Fri, 21 May 2010 15:47:19 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> Hi,
> 
> there is already a protection against loading a core file when a program is
> running.

That makes sense.  But what you are suggesting doesn't.

I often start gdb and load a core file to investigate a problem.  Then
I set a breakpoint at some point before the crash and run the program
again.  This used to work just fine.

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Friday 21 May 2010 15:47:07, Mark Kettenis wrote:
> I often start gdb and load a core file to investigate a problem.  Then
> I set a breakpoint at some point before the crash and run the program
> again.  This used to work just fine.

If you're refering to getting back to debugging the core when
the running program exits, it never worked correctly.  You'd get a better
experience if your core had no threads at all.  If you didn't trip on an
assertion, and weird problems for having gdb consult things in the process
target, then the core target (which wouldn't make sense for the running
program), then the exec target, when you'd get back to debugging
the core, you'd find your threads had disapeared.  That's just an example.
Here are others <http://sourceware.org/ml/gdb-patches/2008-08/msg00290.html>.

For this to work correctly, we'd need to rethink the single target-stack,
into maybe multiple target stacks, or something else radically different.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Mark Kettenis]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Fri, 21 May 2010 16:05:01 +0100
> 
> On Friday 21 May 2010 15:47:07, Mark Kettenis wrote:
> > I often start gdb and load a core file to investigate a problem.  Then
> > I set a breakpoint at some point before the crash and run the program
> > again.  This used to work just fine.
> 
> If you're refering to getting back to debugging the core when
> the running program exits, it never worked correctly.  You'd get a better
> experience if your core had no threads at all.  If you didn't trip on an
> assertion, and weird problems for having gdb consult things in the process
> target, then the core target (which wouldn't make sense for the running
> program), then the exec target, when you'd get back to debugging
> the core, you'd find your threads had disapeared.  That's just an example.
> Here are others <http://sourceware.org/ml/gdb-patches/2008-08/msg00290.html>.

I don't think I go back again to the core file very often, but it
seems to work fine with gdb 6.3 for a single-threaded process.

> For this to work correctly, we'd need to rethink the single target-stack,
> into maybe multiple target stacks, or something else radically different.

Dunno.  Isn't it enough to pop the core layer when you run?

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Friday 21 May 2010 16:37:44, Mark Kettenis wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Fri, 21 May 2010 16:05:01 +0100
> > 
> > On Friday 21 May 2010 15:47:07, Mark Kettenis wrote:
> > > I often start gdb and load a core file to investigate a problem.  Then
> > > I set a breakpoint at some point before the crash and run the program
> > > again.  This used to work just fine.
> > 
> > If you're refering to getting back to debugging the core when
> > the running program exits, it never worked correctly.  You'd get a better
> > experience if your core had no threads at all.  If you didn't trip on an
> > assertion, and weird problems for having gdb consult things in the process
> > target, then the core target (which wouldn't make sense for the running
> > program), then the exec target, when you'd get back to debugging
> > the core, you'd find your threads had disapeared.  That's just an example.
> > Here are others <http://sourceware.org/ml/gdb-patches/2008-08/msg00290.html>.
> 
> I don't think I go back again to the core file very often, but it
> seems to work fine with gdb 6.3 for a single-threaded process.

Yes, that's the case I said would most likely work for the basic things
in the other mail.

> 
> > For this to work correctly, we'd need to rethink the single target-stack,
> > into maybe multiple target stacks, or something else radically different.
> 
> Dunno.  Isn't it enough to pop the core layer when you run?

Errr, if you pop it, then what are you getting back to?  I was talking
about a design that allows correct debugging of the process, while still
debugging the core.  poping the core layer, clearing all current
inferior state, and later reload the core works around it, of
course... but that's not the same as leaving the core target on
that stack, pushing the process statum, allowing debugging it,
and once that is poped, assume the core target and its inferior
are in a sane gdb internal state, which is what gdb assumes today.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Joel Brobecker]

> > there is already a protection against loading a core file when a program is
> > running.
> 
> That makes sense.  But what you are suggesting doesn't.
> 
> I often start gdb and load a core file to investigate a problem.  Then
> I set a breakpoint at some point before the crash and run the program
> again.  This used to work just fine.

There might have been a poor choice of words in the subject - Based on
the patch, Jan is not proposing to actually forbid that operation.
Rather, he's suggesting we add a confirmation query before actually
restarting the inferior.  Would you agree to such a change?

-- 
Joel

Re: [patch] Forbid run with a core file loaded

[Mark Kettenis]

> Date: Fri, 21 May 2010 08:04:28 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > there is already a protection against loading a core file when a
> > > program is running.
> > 
> > That makes sense.  But what you are suggesting doesn't.
> > 
> > I often start gdb and load a core file to investigate a problem.  Then
> > I set a breakpoint at some point before the crash and run the program
> > again.  This used to work just fine.
> 
> There might have been a poor choice of words in the subject - Based on
> the patch, Jan is not proposing to actually forbid that operation.
> Rather, he's suggesting we add a confirmation query before actually
> restarting the inferior.  Would you agree to such a change?

I'd think that would be pretty pointless.  You don't really lose state
if you use the "run" command.  You can easily get back to looking at
the core file by reloading it using the "core" command.

In general these "Are you sure?" type questions are un-Unixy and
rather annoying in my opinion.  They make sense if the user is about
to take an action that is going to lose state (such as loading a core
file while a program is running), but I think we shouldn't add them in
this case.

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Fri, 21 May 2010 17:31:06 +0200, Mark Kettenis wrote:
> In general these "Are you sure?" type questions are un-Unixy and
> rather annoying in my opinion.  They make sense if the user is about
> to take an action that is going to lose state (such as loading a core
> file while a program is running), but I think we shouldn't add them in
> this case.

That makes sense, removed the question.


On Fri, 21 May 2010 17:02:31 +0200, Joel Brobecker wrote:
> > +set test "start with core"
> > +gdb_test_multiple "start" $test {
> > +    -re {Core file is already loaded.  Unload it[?] [(]y or n[)] } {
> > +	pass $test
> > +    }
> > +}
> 
> We should not use the "start" command in testcases, as it does not
> work with remote targets. I'm afraid we're going to have to rely on
> the run command instead.

OK, used runto_main now.  Attach test does not run with gdbserver the same way
gdb.base/attach.exp does not.  Tested this testfile with runtest-gdbserver
(only on localhost).


On Fri, 21 May 2010 17:06:07 +0200, Pedro Alves wrote:
> I don't think it makes that much sense nowadays to have a
> distinct core_stratum vs process_stratum.

OK, removed core_stratum.


> It core_ops was a process_stratum, pushing linux-nat.c would auto discard
> the core, as you can't have two targets of the same stratum on the stack.

Followed it although some ordering adjustments were required.


> The only main use for that stratum nowadays is for find_core_target,
> I think.

Replaced.  Unaware if some 3rd patches were using secondary core_stratum, it
probably could not work anyway.


> > 	Forbid run with a core file loaded.
> > 	* infcmd.c (run_command_1) <core_bfd>: New.
> 
> What about "attach"?

Forgot, implemented/fixed.


No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


gdb/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* corefile.c (core_target): New variable.
	(core_file_command): Remove variable t, use core_target.
	* corelow.c (core_ops): Make it static.
	(init_core_ops): Change to process_stratum.  Initialize CORE_TARGET.
	* gdbarch.sh (core_pid_to_str): Remove core_stratum from its comment.
	* gdbarch.h: Regenerate.
	* gdbcore.h (core_target): New declaration.
	* inf-ptrace.c (inf_ptrace_create_inferior): Move push_target before
	fork_inferior, comment it.
	(inf_ptrace_attach): Call pop_all_targets_above.
	* record.c (record_open): Replace core_stratum by a core_bfd check.
	* target.c (find_core_target): Remove.
	* target.h (enum strata) <core_stratum>: Remove.
	(find_core_target): Remove declaration.
	* tracepoint.c (init_tfile_ops) <to_stratum>: Remove comment.

gdb/doc/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.texinfo (Active Targets): Remove core_stratum.  Include
	record_stratum example.

gdb/testsuite/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.base/corefile.exp (run: load core again)
	(run: sanity check we see the core file, run: with core)
	(run: core file is cleared, attach: load core again)
	(attach: sanity check we see the core file, attach: with core)
	(attach: core file is cleared): New tests.
	* gdb.base/coremaker.c (main): New parameters.  Implement "sleep" argv.

--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -59,6 +59,10 @@ static int exec_file_hook_count = 0;	/* size of array */
 /* Binary file diddling handle for the core file.  */
 
 bfd *core_bfd = NULL;
+
+/* corelow.c target (if included for this gdb target).  */
+
+struct target_ops *core_target;
 \f
 
 /* Backward compatability with old way of specifying core files.  */
@@ -66,18 +70,15 @@ bfd *core_bfd = NULL;
 void
 core_file_command (char *filename, int from_tty)
 {
-  struct target_ops *t;
-
   dont_repeat ();		/* Either way, seems bogus. */
 
-  t = find_core_target ();
-  if (t == NULL)
+  if (core_target == NULL)
     error (_("GDB can't read core files on this machine."));
 
   if (!filename)
-    (t->to_detach) (t, filename, from_tty);
+    (core_target->to_detach) (core_target, filename, from_tty);
   else
-    (t->to_open) (filename, from_tty);
+    (core_target->to_open) (filename, from_tty);
 }
 \f
 
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -100,7 +100,7 @@ static void init_core_ops (void);
 
 void _initialize_corelow (void);
 
-struct target_ops core_ops;
+static struct target_ops core_ops;
 
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
@@ -911,11 +911,17 @@ init_core_ops (void)
   core_ops.to_thread_alive = core_thread_alive;
   core_ops.to_read_description = core_read_description;
   core_ops.to_pid_to_str = core_pid_to_str;
-  core_ops.to_stratum = core_stratum;
+  core_ops.to_stratum = process_stratum;
   core_ops.to_has_memory = core_has_memory;
   core_ops.to_has_stack = core_has_stack;
   core_ops.to_has_registers = core_has_registers;
   core_ops.to_magic = OPS_MAGIC;
+
+  if (core_target)
+    internal_error (__FILE__, __LINE__, 
+		    _("init_core_ops: core target already exists (\"%s\")."),
+		    core_target->to_longname);
+  core_target = &core_ops;
 }
 
 void
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -609,8 +609,7 @@ v:struct core_regset_section *:core_regset_sections:const char *name, int len:::
 # core file into buffer READBUF with length LEN.
 M:LONGEST:core_xfer_shared_libraries:gdb_byte *readbuf, ULONGEST offset, LONGEST len:readbuf, offset, len
 
-# How the core_stratum layer converts a PTID from a core file to a
-# string.
+# How the core target converts a PTID from a core file to a string.
 M:char *:core_pid_to_str:ptid_t ptid:ptid
 
 # BFD target to use when generating a core file.
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -666,8 +666,7 @@ typedef LONGEST (gdbarch_core_xfer_shared_libraries_ftype) (struct gdbarch *gdba
 extern LONGEST gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, LONGEST len);
 extern void set_gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdbarch_core_xfer_shared_libraries_ftype *core_xfer_shared_libraries);
 
-/* How the core_stratum layer converts a PTID from a core file to a
-   string. */
+/* How the core target converts a PTID from a core file to a string. */
 
 extern int gdbarch_core_pid_to_str_p (struct gdbarch *gdbarch);
 
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -108,6 +108,8 @@ extern void specify_exec_file_hook (void (*hook) (char *filename));
 
 extern bfd *core_bfd;
 
+extern struct target_ops *core_target;
+
 /* Whether to open exec and core files read-only or read-write.  */
 
 extern int write_files;
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -121,11 +121,12 @@ inf_ptrace_create_inferior (struct target_ops *ops,
 {
   int pid;
 
+  /* Clear possible core file with its process_stratum.  */
+  push_target (ops);
+
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
 		       NULL, NULL);
 
-  push_target (ops);
-
   /* On some targets, there must be some explicit synchronization
      between the parent and child processes after the debugger
      forks, and before the child execs the debuggee program.  This
@@ -194,6 +195,10 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
   if (pid == getpid ())		/* Trying to masturbate?  */
     error (_("I refuse to debug myself!"));
 
+  /* target_pid_to_str already uses the target.  Clear possible core file with
+     its process_stratum.  */
+  pop_all_targets_above (ops->to_stratum - 1, 0);
+
   if (from_tty)
     {
       exec_file = get_exec_file (0);
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -958,7 +958,7 @@ record_open (char *name, int from_tty)
   record_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint;
   record_beneath_to_stopped_data_address = tmp_to_stopped_data_address;
 
-  if (current_target.to_stratum == core_stratum)
+  if (core_bfd)
     record_core_open_1 (name, from_tty);
   else
     record_open_1 (name, from_tty);
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2699,31 +2699,6 @@ find_run_target (void)
   return (count == 1 ? runable : NULL);
 }
 
-/* Find a single core_stratum target in the list of targets and return it.
-   If for some reason there is more than one, return NULL.  */
-
-struct target_ops *
-find_core_target (void)
-{
-  struct target_ops **t;
-  struct target_ops *runable = NULL;
-  int count;
-
-  count = 0;
-
-  for (t = target_structs; t < target_structs + target_struct_size;
-       ++t)
-    {
-      if ((*t)->to_stratum == core_stratum)
-	{
-	  runable = *t;
-	  ++count;
-	}
-    }
-
-  return (count == 1 ? runable : NULL);
-}
-
 /*
  * Find the next target down the stack from the specified target.
  */
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -65,8 +65,7 @@ enum strata
   {
     dummy_stratum,		/* The lowest of the low */
     file_stratum,		/* Executable files, etc */
-    core_stratum,		/* Core dump files */
-    process_stratum,		/* Executing processes */
+    process_stratum,		/* Executing processes or core dump files */
     thread_stratum,		/* Executing threads */
     record_stratum,		/* Support record debugging */
     arch_stratum		/* Architecture overrides */
@@ -1496,8 +1495,6 @@ extern void find_default_create_inferior (struct target_ops *,
 
 extern struct target_ops *find_run_target (void);
 
-extern struct target_ops *find_core_target (void);
-
 extern struct target_ops *find_target_beneath (struct target_ops *);
 
 /* Read OS data object of type TYPE from the target, and return it in
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -4050,8 +4050,6 @@ init_tfile_ops (void)
   tfile_ops.to_get_trace_status = tfile_get_trace_status;
   tfile_ops.to_trace_find = tfile_trace_find;
   tfile_ops.to_get_trace_state_variable_value = tfile_get_trace_state_variable_value;
-  /* core_stratum might seem more logical, but GDB doesn't like having
-     more than one core_stratum vector.  */
   tfile_ops.to_stratum = process_stratum;
   tfile_ops.to_has_all_memory = tfile_has_all_memory;
   tfile_ops.to_has_memory = tfile_has_memory;
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -15024,33 +15024,21 @@ and @code{show architecture}.
 @cindex active targets
 @cindex multiple targets
 
-There are three classes of targets: processes, core files, and
-executable files.  @value{GDBN} can work concurrently on up to three
-active targets, one in each class.  This allows you to (for example)
-start a process and inspect its activity without abandoning your work on
-a core file.
-
-For example, if you execute @samp{gdb a.out}, then the executable file
-@code{a.out} is the only active target.  If you designate a core file as
-well---presumably from a prior run that crashed and coredumped---then
-@value{GDBN} has two active targets and uses them in tandem, looking
-first in the corefile target, then in the executable file, to satisfy
-requests for memory addresses.  (Typically, these two classes of target
-are complementary, since core files contain only a program's
-read-write memory---variables and so on---plus machine status, while
-executable files contain only the program text and initialized data.)
-
-When you type @code{run}, your executable file becomes an active process
-target as well.  When a process target is active, all @value{GDBN}
-commands requesting memory addresses refer to that target; addresses in
-an active core file or executable file target are obscured while the
-process target is active.
-
-Use the @code{core-file} and @code{exec-file} commands to select a new
-core file or executable target (@pxref{Files, ,Commands to Specify
-Files}).  To specify as a target a process that is already running, use
-the @code{attach} command (@pxref{Attach, ,Debugging an Already-running
-Process}).
+There are multiple classes of targets such as: processes, executable files or
+recording sessions.  Core files belong to the process class, there can be
+active only one of a core or a running process.  Otherwise @value{GDBN} can
+work concurrently on multiple active targets, one in each class.  This allows
+you to (for example) start a process and inspect its activity while still
+having access to the executable file after the process finishes.  Or if you
+start process recording (@pxref{Reverse Execution}) and @code{reverse-step}
+there you are presented a virtual layer of the recording target while the
+process target remains stopped at the chronologically last point of the process
+execution.
+
+Use the @code{core-file} and @code{exec-file} commands to select a new core
+file or executable target (@pxref{Files, ,Commands to Specify Files}).  To
+specify as a target a process that is already running, use the @code{attach}
+command (@pxref{Attach, ,Debugging an Already-running Process}).
 
 @node Target Commands
 @section Commands for Managing Targets
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -180,3 +180,62 @@ gdb_load ${binfile}
 gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (reinit)"
 
 gdb_test "core" "No core file now."
+
+
+# Test a run (start) command will clear any loaded core file.
+
+gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
+gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
+
+set test "run: with core"
+if [runto_main] {
+    pass $test
+} else {
+    fail $test
+}
+
+set test "run: core file is cleared"
+gdb_test_multiple "info files" $test {
+    "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    "\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_exit
+
+
+# Test an attach command will clear any loaded core file.
+
+if ![is_remote target] {
+    set test "attach: spawn sleep"
+    set res [remote_spawn host "$binfile sleep"];
+    if { $res < 0 || $res == "" } {
+	perror "$test failed."
+	fail $test
+	return
+    }
+    set pid [exp_pid -i $res]
+    # We do not care of the startup phase where it will be caught.
+
+    gdb_start
+
+    gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
+    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+
+    gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+
+    set test "attach: core file is cleared"
+    gdb_test_multiple "info files" $test {
+	"\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	    fail $test
+	}
+	"\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    gdb_exit
+}
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -133,8 +133,14 @@ func1 ()
   func2 ();
 }
 
-int main ()
+int
+main (int argc, char **argv)
 {
+  if (argc == 2 && strcmp (argv[1], "sleep") == 0)
+    {
+      sleep (60);
+      return 0;
+    }
   mmapdata ();
   func1 ();
   return 0;

Re: [patch] Forbid run with a core file loaded

[Eli Zaretskii]

> Date: Sun, 23 May 2010 20:54:40 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: gdb-patches@sourceware.org
> 
> +There are multiple classes of targets such as: processes, executable files or
> +recording sessions.  Core files belong to the process class, there can be
> +active only one of a core or a running process.

Something is wrong with the last sentence.

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Sun, 23 May 2010 22:14:41 +0200, Eli Zaretskii wrote:
> > +There are multiple classes of targets such as: processes, executable files or
> > +recording sessions.  Core files belong to the process class, there can be
> > +active only one of a core or a running process.
> 
> Something is wrong with the last sentence.

Used now:
	Core files belong to the process class making core file and process
	mutually exclusive.


On Sun, 23 May 2010 23:08:44 +0200, Pedro Alves wrote:
> On Sunday 23 May 2010 19:54:40, Jan Kratochvil wrote:
> > @@ -194,6 +195,10 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
> > +  pop_all_targets_above (ops->to_stratum - 1, 0);
> 
> Unfortunately, this would break multiprocess.  For example, you may
> be attaching to your second process (e.g., "start", "add-inferior",
> "inferior 2", "attach $pid"), and the thread stratum needs to remain
> pushed for the first inferior/process.

OK, fixed it by unpush_target only.

I hope you agree the target stack should be made specific for each inferior.
Like I proposed to fix target_gdbarch making it specific for each inferior.
	[01/03] Update target_gdbarch for current inferior
	http://sourceware.org/ml/gdb-patches/2010-01/msg00228.html


> > +  /* Clear possible core file with its process_stratum.  */
> > +  push_target (ops);
> > +
> >    pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
> >                        NULL, NULL);
> >  
> > -  push_target (ops);
> 
> This makes sense, as indeed fork_inferior touches the current
> target, even today, but, can you check what happens if fork_inferior
> throws an error?

OK, I have placed there a cleanup.


No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.

OK to check-in?


Thanks,
Jan


gdb/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* corefile.c (core_target): New variable.
	(core_file_command): Remove variable t, use core_target.
	* corelow.c (core_ops): Make it static.
	(init_core_ops): Change to process_stratum.  Initialize CORE_TARGET.
	* defs.h (make_cleanup_unpush_target): New prototype.
	* gdbarch.sh (core_pid_to_str): Remove core_stratum from its comment.
	* gdbarch.h: Regenerate.
	* gdbcore.h (core_target): New declaration.
	* inf-ptrace.c (inf_ptrace_create_inferior): New variable back_to.
	Move push_target before fork_inferior, comment it.
	(inf_ptrace_attach): Call unpush_target for core_target.
	* record.c (record_open): Replace core_stratum by a core_bfd check.
	* target.c (find_core_target): Remove.
	* target.h (enum strata) <core_stratum>: Remove.
	(find_core_target): Remove declaration.
	* tracepoint.c (init_tfile_ops) <to_stratum>: Remove comment.
	* utils.c (do_unpush_target, make_cleanup_unpush_target): New functions.

gdb/doc/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.texinfo (Active Targets): Remove core_stratum.  Include
	record_stratum example.

gdb/testsuite/
2010-05-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.base/corefile.exp (run: load core again)
	(run: sanity check we see the core file, run: with core)
	(run: core file is cleared, attach: load core again)
	(attach: sanity check we see the core file, attach: with core)
	(attach: core file is cleared): New tests.
	* gdb.base/coremaker.c (main): New parameters.  Implement "sleep" argv.

--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -59,6 +59,10 @@ static int exec_file_hook_count = 0;	/* size of array */
 /* Binary file diddling handle for the core file.  */
 
 bfd *core_bfd = NULL;
+
+/* corelow.c target (if included for this gdb target).  */
+
+struct target_ops *core_target;
 \f
 
 /* Backward compatability with old way of specifying core files.  */
@@ -66,18 +70,15 @@ bfd *core_bfd = NULL;
 void
 core_file_command (char *filename, int from_tty)
 {
-  struct target_ops *t;
-
   dont_repeat ();		/* Either way, seems bogus. */
 
-  t = find_core_target ();
-  if (t == NULL)
+  if (core_target == NULL)
     error (_("GDB can't read core files on this machine."));
 
   if (!filename)
-    (t->to_detach) (t, filename, from_tty);
+    (core_target->to_detach) (core_target, filename, from_tty);
   else
-    (t->to_open) (filename, from_tty);
+    (core_target->to_open) (filename, from_tty);
 }
 \f
 
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -100,7 +100,7 @@ static void init_core_ops (void);
 
 void _initialize_corelow (void);
 
-struct target_ops core_ops;
+static struct target_ops core_ops;
 
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
@@ -911,11 +911,17 @@ init_core_ops (void)
   core_ops.to_thread_alive = core_thread_alive;
   core_ops.to_read_description = core_read_description;
   core_ops.to_pid_to_str = core_pid_to_str;
-  core_ops.to_stratum = core_stratum;
+  core_ops.to_stratum = process_stratum;
   core_ops.to_has_memory = core_has_memory;
   core_ops.to_has_stack = core_has_stack;
   core_ops.to_has_registers = core_has_registers;
   core_ops.to_magic = OPS_MAGIC;
+
+  if (core_target)
+    internal_error (__FILE__, __LINE__, 
+		    _("init_core_ops: core target already exists (\"%s\")."),
+		    core_target->to_longname);
+  core_target = &core_ops;
 }
 
 void
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -348,6 +348,9 @@ extern struct cleanup *make_cleanup_obstack_free (struct obstack *obstack);
 
 extern struct cleanup *make_cleanup_restore_integer (int *variable);
 
+struct target_ops;
+extern struct cleanup *make_cleanup_unpush_target (struct target_ops *ops);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -609,8 +609,7 @@ v:struct core_regset_section *:core_regset_sections:const char *name, int len:::
 # core file into buffer READBUF with length LEN.
 M:LONGEST:core_xfer_shared_libraries:gdb_byte *readbuf, ULONGEST offset, LONGEST len:readbuf, offset, len
 
-# How the core_stratum layer converts a PTID from a core file to a
-# string.
+# How the core target converts a PTID from a core file to a string.
 M:char *:core_pid_to_str:ptid_t ptid:ptid
 
 # BFD target to use when generating a core file.
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -666,8 +666,7 @@ typedef LONGEST (gdbarch_core_xfer_shared_libraries_ftype) (struct gdbarch *gdba
 extern LONGEST gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, LONGEST len);
 extern void set_gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdbarch_core_xfer_shared_libraries_ftype *core_xfer_shared_libraries);
 
-/* How the core_stratum layer converts a PTID from a core file to a
-   string. */
+/* How the core target converts a PTID from a core file to a string. */
 
 extern int gdbarch_core_pid_to_str_p (struct gdbarch *gdbarch);
 
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -108,6 +108,8 @@ extern void specify_exec_file_hook (void (*hook) (char *filename));
 
 extern bfd *core_bfd;
 
+extern struct target_ops *core_target;
+
 /* Whether to open exec and core files read-only or read-write.  */
 
 extern int write_files;
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -119,12 +119,17 @@ inf_ptrace_create_inferior (struct target_ops *ops,
 			    char *exec_file, char *allargs, char **env,
 			    int from_tty)
 {
+  struct cleanup *back_to;
   int pid;
 
+  /* Clear possible core file with its process_stratum.  */
+  push_target (ops);
+  back_to = make_cleanup_unpush_target (ops);
+
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
 		       NULL, NULL);
 
-  push_target (ops);
+  discard_cleanups (back_to);
 
   /* On some targets, there must be some explicit synchronization
      between the parent and child processes after the debugger
@@ -194,6 +199,12 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
   if (pid == getpid ())		/* Trying to masturbate?  */
     error (_("I refuse to debug myself!"));
 
+  /* target_pid_to_str already uses the target.  Clear possible core file with
+     its process_stratum.  Targets above must remain in place as the target
+     stack is shared across multiple inferiors.  */
+  if (core_target)
+    unpush_target (core_target);
+
   if (from_tty)
     {
       exec_file = get_exec_file (0);
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -958,7 +958,7 @@ record_open (char *name, int from_tty)
   record_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint;
   record_beneath_to_stopped_data_address = tmp_to_stopped_data_address;
 
-  if (current_target.to_stratum == core_stratum)
+  if (core_bfd)
     record_core_open_1 (name, from_tty);
   else
     record_open_1 (name, from_tty);
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2699,31 +2699,6 @@ find_run_target (void)
   return (count == 1 ? runable : NULL);
 }
 
-/* Find a single core_stratum target in the list of targets and return it.
-   If for some reason there is more than one, return NULL.  */
-
-struct target_ops *
-find_core_target (void)
-{
-  struct target_ops **t;
-  struct target_ops *runable = NULL;
-  int count;
-
-  count = 0;
-
-  for (t = target_structs; t < target_structs + target_struct_size;
-       ++t)
-    {
-      if ((*t)->to_stratum == core_stratum)
-	{
-	  runable = *t;
-	  ++count;
-	}
-    }
-
-  return (count == 1 ? runable : NULL);
-}
-
 /*
  * Find the next target down the stack from the specified target.
  */
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -65,8 +65,7 @@ enum strata
   {
     dummy_stratum,		/* The lowest of the low */
     file_stratum,		/* Executable files, etc */
-    core_stratum,		/* Core dump files */
-    process_stratum,		/* Executing processes */
+    process_stratum,		/* Executing processes or core dump files */
     thread_stratum,		/* Executing threads */
     record_stratum,		/* Support record debugging */
     arch_stratum		/* Architecture overrides */
@@ -1496,8 +1495,6 @@ extern void find_default_create_inferior (struct target_ops *,
 
 extern struct target_ops *find_run_target (void);
 
-extern struct target_ops *find_core_target (void);
-
 extern struct target_ops *find_target_beneath (struct target_ops *);
 
 /* Read OS data object of type TYPE from the target, and return it in
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -4049,8 +4049,6 @@ init_tfile_ops (void)
   tfile_ops.to_get_trace_status = tfile_get_trace_status;
   tfile_ops.to_trace_find = tfile_trace_find;
   tfile_ops.to_get_trace_state_variable_value = tfile_get_trace_state_variable_value;
-  /* core_stratum might seem more logical, but GDB doesn't like having
-     more than one core_stratum vector.  */
   tfile_ops.to_stratum = process_stratum;
   tfile_ops.to_has_all_memory = tfile_has_all_memory;
   tfile_ops.to_has_memory = tfile_has_memory;
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -352,6 +352,24 @@ make_cleanup_restore_integer (int *variable)
 			   xfree);
 }
 
+/* Helper for make_cleanup_unpush_target.  */
+
+static void
+do_unpush_target (void *arg)
+{
+  struct target_ops *ops = arg;
+
+  unpush_target (ops);
+}
+
+/* Return a new cleanup that unpushes OPS.  */
+
+struct cleanup *
+make_cleanup_unpush_target (struct target_ops *ops)
+{
+  return make_my_cleanup (&cleanup_chain, do_unpush_target, ops);
+}
+
 struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -15027,33 +15027,20 @@ and @code{show architecture}.
 @cindex active targets
 @cindex multiple targets
 
-There are three classes of targets: processes, core files, and
-executable files.  @value{GDBN} can work concurrently on up to three
-active targets, one in each class.  This allows you to (for example)
-start a process and inspect its activity without abandoning your work on
-a core file.
-
-For example, if you execute @samp{gdb a.out}, then the executable file
-@code{a.out} is the only active target.  If you designate a core file as
-well---presumably from a prior run that crashed and coredumped---then
-@value{GDBN} has two active targets and uses them in tandem, looking
-first in the corefile target, then in the executable file, to satisfy
-requests for memory addresses.  (Typically, these two classes of target
-are complementary, since core files contain only a program's
-read-write memory---variables and so on---plus machine status, while
-executable files contain only the program text and initialized data.)
-
-When you type @code{run}, your executable file becomes an active process
-target as well.  When a process target is active, all @value{GDBN}
-commands requesting memory addresses refer to that target; addresses in
-an active core file or executable file target are obscured while the
-process target is active.
-
-Use the @code{core-file} and @code{exec-file} commands to select a new
-core file or executable target (@pxref{Files, ,Commands to Specify
-Files}).  To specify as a target a process that is already running, use
-the @code{attach} command (@pxref{Attach, ,Debugging an Already-running
-Process}).
+There are multiple classes of targets such as: processes, executable files or
+recording sessions.  Core files belong to the process class making core file
+and process mutually exclusive.  Otherwise @value{GDBN} can work concurrently
+on multiple active targets, one in each class.  This allows you to (for
+example) start a process and inspect its activity while still having access to
+the executable file after the process finishes.  Or if you start process
+recording (@pxref{Reverse Execution}) and @code{reverse-step} there you are
+presented a virtual layer of the recording target while the process target
+remains stopped at the chronologically last point of the process execution.
+
+Use the @code{core-file} and @code{exec-file} commands to select a new core
+file or executable target (@pxref{Files, ,Commands to Specify Files}).  To
+specify as a target a process that is already running, use the @code{attach}
+command (@pxref{Attach, ,Debugging an Already-running Process}).
 
 @node Target Commands
 @section Commands for Managing Targets
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -177,3 +177,62 @@ gdb_load ${binfile}
 gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (reinit)"
 
 gdb_test "core" "No core file now."
+
+
+# Test a run (start) command will clear any loaded core file.
+
+gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
+gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
+
+set test "run: with core"
+if [runto_main] {
+    pass $test
+} else {
+    fail $test
+}
+
+set test "run: core file is cleared"
+gdb_test_multiple "info files" $test {
+    "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    "\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_exit
+
+
+# Test an attach command will clear any loaded core file.
+
+if ![is_remote target] {
+    set test "attach: spawn sleep"
+    set res [remote_spawn host "$binfile sleep"];
+    if { $res < 0 || $res == "" } {
+	perror "$test failed."
+	fail $test
+	return
+    }
+    set pid [exp_pid -i $res]
+    # We do not care of the startup phase where it will be caught.
+
+    gdb_start
+
+    gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
+    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+
+    gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+
+    set test "attach: core file is cleared"
+    gdb_test_multiple "info files" $test {
+	"\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	    fail $test
+	}
+	"\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    gdb_exit
+}
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -133,8 +133,14 @@ func1 ()
   func2 ();
 }
 
-int main ()
+int
+main (int argc, char **argv)
 {
+  if (argc == 2 && strcmp (argv[1], "sleep") == 0)
+    {
+      sleep (60);
+      return 0;
+    }
   mmapdata ();
   func1 ();
   return 0;

Re: [patch] Forbid run with a core file loaded

[Eli Zaretskii]

> Date: Sun, 6 Jun 2010 21:50:34 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: mark.kettenis@xs4all.nl, brobecker@adacore.com, gdb-patches@sourceware.org
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.
> 
> OK to check-in?

The part for the manual is okay with me.  Thanks.

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Sunday 06 June 2010 20:50:34, Jan Kratochvil wrote:

> > Unfortunately, this would break multiprocess.  For example, you may
> > be attaching to your second process (e.g., "start", "add-inferior",
> > "inferior 2", "attach $pid"), and the thread stratum needs to remain
> > pushed for the first inferior/process.
> 
> OK, fixed it by unpush_target only.
> 
> I hope you agree the target stack should be made specific for each inferior.

I've gone back and forth on that for a while with
the multi-exec work.  It's trickier than that.  Consider the extended-remote
target --- if you do "add-inferior; <inf 2 created>; inferior 2; run",
if we simply had a target stack per inferior, then when you get to
"run", you'd only have "exec" on the inferior 2's stack, but you wanted
extended-remote to handle creating the inferior.

> Like I proposed to fix target_gdbarch making it specific for each inferior.
> 	[01/03] Update target_gdbarch for current inferior
> 	http://sourceware.org/ml/gdb-patches/2010-01/msg00228.html

Maybe, though we probably still need a target_gdbarch to represent the
target interface's properties, instead of any particular inferior it
it controling.  The canonical example is the remote target, running
with multi-process enabled.


> OK, I have placed there a cleanup.

Thanks.  You'd need to make sure the cleanup doesn't pop the
target if there are still live inferiors to debug, so to not break
other inferiors in multi-process (see the have_inferiors checks
in inf-ptrace.c).

> 
>  
> +  /* Clear possible core file with its process_stratum.  */
> +  push_target (ops);
> +  back_to = make_cleanup_unpush_target (ops);
> +
>    pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
>  		       NULL, NULL);
>  
> -  push_target (ops);
> +  discard_cleanups (back_to);
>  
>    /* On some targets, there must be some explicit synchronization
>       between the parent and child processes after the debugger
> @@ -194,6 +199,12 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
>    if (pid == getpid ())		/* Trying to masturbate?  */
>      error (_("I refuse to debug myself!"));
>  
> +  /* target_pid_to_str already uses the target.  Clear possible core file with
> +     its process_stratum.  Targets above must remain in place as the target
> +     stack is shared across multiple inferiors.  */
> +  if (core_target)
> +    unpush_target (core_target);

I'd rather either get rid of the target_pid_to_str calls before
the push, or push earlier (with a cleanup), than adding knowledge
about the core target here.  You can even  consider those target_pid_to_str
calls buggy, for accessing the exec or process stratum target depending
on when they are called.

> +
> +# Test an attach command will clear any loaded core file.
> +
> +if ![is_remote target] {
> +    set test "attach: spawn sleep"
> +    set res [remote_spawn host "$binfile sleep"];
> +    if { $res < 0 || $res == "" } {
> +	perror "$test failed."
> +	fail $test

Do we need both perror and fail?

> +	return
> +    }
> +    set pid [exp_pid -i $res]
> +    # We do not care of the startup phase where it will be caught.

I don't understand this comment.  What's "it"?  Please rephrase.
Did you mean "We don't care whether the program is still in the startup
phase when we attach"?

Other than that, it looks good to me, and is good to check in, if
Mark or others don't have objections.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Tom Tromey]

Jan> I hope you agree the target stack should be made specific for each
Jan> inferior.

Pedro> I've gone back and forth on that for a while with the multi-exec
Pedro> work.  It's trickier than that.

How else would you propose letting people attach to multiple inferiors
using multiple targets?  This seems like a reasonable thing to want to
do, e.g., debugging a distributed application of some kind.

Pedro> Consider the extended-remote target --- if you do "add-inferior;
Pedro> <inf 2 created>; inferior 2; run", if we simply had a target
Pedro> stack per inferior, then when you get to "run", you'd only have
Pedro> "exec" on the inferior 2's stack, but you wanted extended-remote
Pedro> to handle creating the inferior.

I don't really know enough about the target API, but this seems fixable.
"Target stack per inferior" does not necessarily imply that the targets
cannot be shared, just that conceptually each inferior carries its own.

Tom

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Tuesday 08 June 2010 03:32:29, Tom Tromey wrote:
> Jan> I hope you agree the target stack should be made specific for each
> Jan> inferior.
> 
> Pedro> I've gone back and forth on that for a while with the multi-exec
> Pedro> work.  It's trickier than that.
> 
> How else would you propose letting people attach to multiple inferiors
> using multiple targets?  This seems like a reasonable thing to want to
> do, e.g., debugging a distributed application of some kind.

It is.  It's the obvious next step.  It's the missing step into being
able to debug parallel MPI programs running on different machines.  But
that also obviously needs to consider user interface issues -- more than
just an internal implementation detail.

> Pedro> Consider the extended-remote target --- if you do "add-inferior;
> Pedro> <inf 2 created>; inferior 2; run", if we simply had a target
> Pedro> stack per inferior, then when you get to "run", you'd only have
> Pedro> "exec" on the inferior 2's stack, but you wanted extended-remote
> Pedro> to handle creating the inferior.
> 
> I don't really know enough about the target API, but this seems fixable.
> "Target stack per inferior" does not necessarily imply that the targets
> cannot be shared, just that conceptually each inferior carries its own.

Of course everything is fixable.  tricker != impossible != undesirable.
If you go the simply route of simply sharing the target stack between
all inferiors that share the same process_stratum target, you have what
we have today.  None of that would make Jan's patch any different, because it
would still need to be careful to not pop random targets off the stack
when _one_ inferior is gone.  If you say that you'd only share the _targets_ and
not the _stack_ between inferiors, then you'd have to rethink things like
the target->beneath pointer, as that is embedded within the target_ops object.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Mon, 07 Jun 2010 13:20:57 +0200, Pedro Alves wrote:
> On Sunday 06 June 2010 20:50:34, Jan Kratochvil wrote:
> > I hope you agree the target stack should be made specific for each inferior.
> 
> I've gone back and forth on that for a while with
> the multi-exec work.  It's trickier than that.  Consider the extended-remote
> target --- if you do "add-inferior; <inf 2 created>; inferior 2; run",
> if we simply had a target stack per inferior, then when you get to
> "run", you'd only have "exec" on the inferior 2's stack, but you wanted
> extended-remote to handle creating the inferior.

I understand it just cannot be moved into `struct inferior' without any other
changes.  As the month passed I se I am not going to implement it soon.


> > OK, I have placed there a cleanup.
> 
> Thanks.  You'd need to make sure the cleanup doesn't pop the
> target if there are still live inferiors to debug, so to not break
> other inferiors in multi-process (see the have_inferiors checks
> in inf-ptrace.c).

Rather chose the safest way to never do any change which is not required.


> I'd rather either get rid of the target_pid_to_str calls before
> the push, or push earlier (with a cleanup),

OK, chose the latter (that is "push earlier").


> > +# Test an attach command will clear any loaded core file.
> > +
> > +if ![is_remote target] {
> > +    set test "attach: spawn sleep"
> > +    set res [remote_spawn host "$binfile sleep"];
> > +    if { $res < 0 || $res == "" } {
> > +	perror "$test failed."
> > +	fail $test
> 
> Do we need both perror and fail?

Removed perror.

------------------------------------------------------------------------------
|In general we need, as "perror" is a different category than pass/fail/xfail.
|perror is there to ignore later results if there are too many errors.
|But there is "return" in that block again so the real perror functionality is
|not used anyway.
|
|IMO it would be right to just call `xfail $test'.  Unfortunately xfail is
|a very quiet action commonly being ignored IMO contrary to this case which
|means some severe testsuite environment failure.
|
|Anyway left there only "fail" now which is not too quiet.  But it is also
|wrong as it is not a proven gdb-tool failure.  I miss some loudxfail.
------------------------------------------------------------------------------


> > +	return
> > +    }
> > +    set pid [exp_pid -i $res]
> > +    # We do not care of the startup phase where it will be caught.
> 
> I don't understand this comment.  What's "it"?  Please rephrase.
> Did you mean "We don't care whether the program is still in the startup
> phase when we attach"?

Used your sentence


> Other than that, it looks good to me, and is good to check in,

It should probably go into 7.2.  As there are some new changes I am asking for
new approval.


No regressions on {x86_64,x86_64-m32,i686}-fedora13-linux-gnu.


Thanks,
Jan


gdb/
2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* corefile.c (core_target): New variable.
	(core_file_command): Remove variable t, use core_target.
	* corelow.c (core_ops): Make it static.
	(init_core_ops): Change to process_stratum.  Initialize CORE_TARGET.
	* defs.h (make_cleanup_unpush_target): New prototype.
	* gdbarch.h: Regenerate.
	* gdbarch.sh (core_pid_to_str): Remove core_stratum from its comment.
	* gdbcore.h (core_target): New declaration.
	* inf-ptrace.c (inf_ptrace_create_inferior, inf_ptrace_attach): New
	variables ops_already_pushed and back_to.  Use push_target,
	make_cleanup_unpush_target and discard_cleanups calls.
	* record.c (record_open): Replace core_stratum by a core_bfd check.
	* target.c (target_is_pushed): New function.
	(find_core_target): Remove.
	* target.h (enum strata) <core_stratum>: Remove.
	(target_is_pushed): New declaration.
	(find_core_target): Remove declaration.
	* tracepoint.c (init_tfile_ops) <to_stratum>: Remove comment.
	* utils.c (do_unpush_target, make_cleanup_unpush_target): New functions.

gdb/doc/
2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.texinfo (Active Targets): Remove core_stratum.  Include
	record_stratum example.

gdb/testsuite/
2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Make core files the process_stratum.
	* gdb.base/corefile.exp (run: load core again)
	(run: sanity check we see the core file, run: with core)
	(run: core file is cleared, attach: load core again)
	(attach: sanity check we see the core file, attach: with core)
	(attach: core file is cleared): New tests.
	* gdb.base/coremaker.c (main): New parameters.  Implement "sleep" argv.

--- a/gdb/corefile.c
+++ b/gdb/corefile.c
@@ -59,6 +59,10 @@ static int exec_file_hook_count = 0;	/* size of array */
 /* Binary file diddling handle for the core file.  */
 
 bfd *core_bfd = NULL;
+
+/* corelow.c target (if included for this gdb target).  */
+
+struct target_ops *core_target;
 \f
 
 /* Backward compatability with old way of specifying core files.  */
@@ -66,18 +70,15 @@ bfd *core_bfd = NULL;
 void
 core_file_command (char *filename, int from_tty)
 {
-  struct target_ops *t;
-
   dont_repeat ();		/* Either way, seems bogus. */
 
-  t = find_core_target ();
-  if (t == NULL)
+  if (core_target == NULL)
     error (_("GDB can't read core files on this machine."));
 
   if (!filename)
-    (t->to_detach) (t, filename, from_tty);
+    (core_target->to_detach) (core_target, filename, from_tty);
   else
-    (t->to_open) (filename, from_tty);
+    (core_target->to_open) (filename, from_tty);
 }
 \f
 
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -100,7 +100,7 @@ static void init_core_ops (void);
 
 void _initialize_corelow (void);
 
-struct target_ops core_ops;
+static struct target_ops core_ops;
 
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
@@ -911,11 +911,17 @@ init_core_ops (void)
   core_ops.to_thread_alive = core_thread_alive;
   core_ops.to_read_description = core_read_description;
   core_ops.to_pid_to_str = core_pid_to_str;
-  core_ops.to_stratum = core_stratum;
+  core_ops.to_stratum = process_stratum;
   core_ops.to_has_memory = core_has_memory;
   core_ops.to_has_stack = core_has_stack;
   core_ops.to_has_registers = core_has_registers;
   core_ops.to_magic = OPS_MAGIC;
+
+  if (core_target)
+    internal_error (__FILE__, __LINE__, 
+		    _("init_core_ops: core target already exists (\"%s\")."),
+		    core_target->to_longname);
+  core_target = &core_ops;
 }
 
 void
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -352,6 +352,9 @@ extern struct cleanup *make_cleanup_obstack_free (struct obstack *obstack);
 
 extern struct cleanup *make_cleanup_restore_integer (int *variable);
 
+struct target_ops;
+extern struct cleanup *make_cleanup_unpush_target (struct target_ops *ops);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -672,8 +672,7 @@ typedef LONGEST (gdbarch_core_xfer_shared_libraries_ftype) (struct gdbarch *gdba
 extern LONGEST gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, LONGEST len);
 extern void set_gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdbarch_core_xfer_shared_libraries_ftype *core_xfer_shared_libraries);
 
-/* How the core_stratum layer converts a PTID from a core file to a
-   string. */
+/* How the core target converts a PTID from a core file to a string. */
 
 extern int gdbarch_core_pid_to_str_p (struct gdbarch *gdbarch);
 
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -611,8 +611,7 @@ v:struct core_regset_section *:core_regset_sections:const char *name, int len:::
 # core file into buffer READBUF with length LEN.
 M:LONGEST:core_xfer_shared_libraries:gdb_byte *readbuf, ULONGEST offset, LONGEST len:readbuf, offset, len
 
-# How the core_stratum layer converts a PTID from a core file to a
-# string.
+# How the core target converts a PTID from a core file to a string.
 M:char *:core_pid_to_str:ptid_t ptid:ptid
 
 # BFD target to use when generating a core file.
--- a/gdb/gdbcore.h
+++ b/gdb/gdbcore.h
@@ -108,6 +108,8 @@ extern void specify_exec_file_hook (void (*hook) (char *filename));
 
 extern bfd *core_bfd;
 
+extern struct target_ops *core_target;
+
 /* Whether to open exec and core files read-only or read-write.  */
 
 extern int write_files;
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -121,10 +121,23 @@ inf_ptrace_create_inferior (struct target_ops *ops,
 {
   int pid;
 
+  /* Do not change either targets above or the same target if already present.
+     The reason is the target stack is shared across multiple inferiors.  */
+  int ops_already_pushed = target_is_pushed (ops);
+  struct cleanup *back_to;
+
+  if (! ops_already_pushed)
+    {
+      /* Clear possible core file with its process_stratum.  */
+      push_target (ops);
+      back_to = make_cleanup_unpush_target (ops);
+    }
+
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
 		       NULL, NULL);
 
-  push_target (ops);
+  if (! ops_already_pushed)
+    discard_cleanups (back_to);
 
   /* On some targets, there must be some explicit synchronization
      between the parent and child processes after the debugger
@@ -189,11 +202,24 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
   pid_t pid;
   struct inferior *inf;
 
+  /* Do not change either targets above or the same target if already present.
+     The reason is the target stack is shared across multiple inferiors.  */
+  int ops_already_pushed = target_is_pushed (ops);
+  struct cleanup *back_to;
+
   pid = parse_pid_to_attach (args);
 
   if (pid == getpid ())		/* Trying to masturbate?  */
     error (_("I refuse to debug myself!"));
 
+  if (! ops_already_pushed)
+    {
+      /* target_pid_to_str already uses the target.  Also clear possible core
+	 file with its process_stratum.  */
+      push_target (ops);
+      back_to = make_cleanup_unpush_target (ops);
+    }
+
   if (from_tty)
     {
       exec_file = get_exec_file (0);
@@ -226,7 +252,8 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
      target, it should decorate the ptid later with more info.  */
   add_thread_silent (inferior_ptid);
 
-  push_target(ops);
+  if (! ops_already_pushed)
+    discard_cleanups (back_to);
 }
 
 #ifdef PT_GET_PROCESS_STATE
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -962,7 +962,7 @@ record_open (char *name, int from_tty)
   record_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint;
   record_beneath_to_stopped_data_address = tmp_to_stopped_data_address;
 
-  if (current_target.to_stratum == core_stratum)
+  if (core_bfd)
     record_core_open_1 (name, from_tty);
   else
     record_open_1 (name, from_tty);
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1037,6 +1037,30 @@ pop_all_targets (int quitting)
   pop_all_targets_above (dummy_stratum, quitting);
 }
 
+/* Return 1 if T is now pushed in the target stack.  Return 0 otherwise.  */
+
+int
+target_is_pushed (struct target_ops *t)
+{
+  struct target_ops **cur;
+
+  /* Check magic number.  If wrong, it probably means someone changed
+     the struct definition, but not all the places that initialize one.  */
+  if (t->to_magic != OPS_MAGIC)
+    {
+      fprintf_unfiltered (gdb_stderr,
+			  "Magic number of %s target struct wrong\n",
+			  t->to_shortname);
+      internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
+    }
+
+  for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
+    if (*cur == t)
+      return 1;
+
+  return 0;
+}
+
 /* Using the objfile specified in OBJFILE, find the address for the
    current thread's thread-local storage with offset OFFSET.  */
 CORE_ADDR
@@ -2770,31 +2794,6 @@ find_run_target (void)
   return (count == 1 ? runable : NULL);
 }
 
-/* Find a single core_stratum target in the list of targets and return it.
-   If for some reason there is more than one, return NULL.  */
-
-struct target_ops *
-find_core_target (void)
-{
-  struct target_ops **t;
-  struct target_ops *runable = NULL;
-  int count;
-
-  count = 0;
-
-  for (t = target_structs; t < target_structs + target_struct_size;
-       ++t)
-    {
-      if ((*t)->to_stratum == core_stratum)
-	{
-	  runable = *t;
-	  ++count;
-	}
-    }
-
-  return (count == 1 ? runable : NULL);
-}
-
 /*
  * Find the next target down the stack from the specified target.
  */
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -68,8 +68,7 @@ enum strata
   {
     dummy_stratum,		/* The lowest of the low */
     file_stratum,		/* Executable files, etc */
-    core_stratum,		/* Core dump files */
-    process_stratum,		/* Executing processes */
+    process_stratum,		/* Executing processes or core dump files */
     thread_stratum,		/* Executing threads */
     record_stratum,		/* Support record debugging */
     arch_stratum		/* Architecture overrides */
@@ -1485,6 +1484,8 @@ extern void pop_all_targets (int quitting);
    strictly above ABOVE_STRATUM.  */
 extern void pop_all_targets_above (enum strata above_stratum, int quitting);
 
+extern int target_is_pushed (struct target_ops *t);
+
 extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
 					       CORE_ADDR offset);
 
@@ -1546,8 +1547,6 @@ extern void find_default_create_inferior (struct target_ops *,
 
 extern struct target_ops *find_run_target (void);
 
-extern struct target_ops *find_core_target (void);
-
 extern struct target_ops *find_target_beneath (struct target_ops *);
 
 /* Read OS data object of type TYPE from the target, and return it in
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -4098,8 +4098,6 @@ init_tfile_ops (void)
   tfile_ops.to_get_trace_status = tfile_get_trace_status;
   tfile_ops.to_trace_find = tfile_trace_find;
   tfile_ops.to_get_trace_state_variable_value = tfile_get_trace_state_variable_value;
-  /* core_stratum might seem more logical, but GDB doesn't like having
-     more than one core_stratum vector.  */
   tfile_ops.to_stratum = process_stratum;
   tfile_ops.to_has_all_memory = tfile_has_all_memory;
   tfile_ops.to_has_memory = tfile_has_memory;
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -352,6 +352,24 @@ make_cleanup_restore_integer (int *variable)
 			   xfree);
 }
 
+/* Helper for make_cleanup_unpush_target.  */
+
+static void
+do_unpush_target (void *arg)
+{
+  struct target_ops *ops = arg;
+
+  unpush_target (ops);
+}
+
+/* Return a new cleanup that unpushes OPS.  */
+
+struct cleanup *
+make_cleanup_unpush_target (struct target_ops *ops)
+{
+  return make_my_cleanup (&cleanup_chain, do_unpush_target, ops);
+}
+
 struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -15343,33 +15343,20 @@ and @code{show architecture}.
 @cindex active targets
 @cindex multiple targets
 
-There are three classes of targets: processes, core files, and
-executable files.  @value{GDBN} can work concurrently on up to three
-active targets, one in each class.  This allows you to (for example)
-start a process and inspect its activity without abandoning your work on
-a core file.
-
-For example, if you execute @samp{gdb a.out}, then the executable file
-@code{a.out} is the only active target.  If you designate a core file as
-well---presumably from a prior run that crashed and coredumped---then
-@value{GDBN} has two active targets and uses them in tandem, looking
-first in the corefile target, then in the executable file, to satisfy
-requests for memory addresses.  (Typically, these two classes of target
-are complementary, since core files contain only a program's
-read-write memory---variables and so on---plus machine status, while
-executable files contain only the program text and initialized data.)
-
-When you type @code{run}, your executable file becomes an active process
-target as well.  When a process target is active, all @value{GDBN}
-commands requesting memory addresses refer to that target; addresses in
-an active core file or executable file target are obscured while the
-process target is active.
-
-Use the @code{core-file} and @code{exec-file} commands to select a new
-core file or executable target (@pxref{Files, ,Commands to Specify
-Files}).  To specify as a target a process that is already running, use
-the @code{attach} command (@pxref{Attach, ,Debugging an Already-running
-Process}).
+There are multiple classes of targets such as: processes, executable files or
+recording sessions.  Core files belong to the process class making core file
+and process mutually exclusive.  Otherwise @value{GDBN} can work concurrently
+on multiple active targets, one in each class.  This allows you to (for
+example) start a process and inspect its activity while still having access to
+the executable file after the process finishes.  Or if you start process
+recording (@pxref{Reverse Execution}) and @code{reverse-step} there you are
+presented a virtual layer of the recording target while the process target
+remains stopped at the chronologically last point of the process execution.
+
+Use the @code{core-file} and @code{exec-file} commands to select a new core
+file or executable target (@pxref{Files, ,Commands to Specify Files}).  To
+specify as a target a process that is already running, use the @code{attach}
+command (@pxref{Attach, ,Debugging an Already-running Process}).
 
 @node Target Commands
 @section Commands for Managing Targets
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -177,3 +177,62 @@ gdb_load ${binfile}
 gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (reinit)"
 
 gdb_test "core" "No core file now."
+
+
+# Test a run (start) command will clear any loaded core file.
+
+gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
+gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
+
+set test "run: with core"
+if [runto_main] {
+    pass $test
+} else {
+    fail $test
+}
+
+set test "run: core file is cleared"
+gdb_test_multiple "info files" $test {
+    -re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_exit
+
+
+# Test an attach command will clear any loaded core file.
+
+if ![is_remote target] {
+    set test "attach: spawn sleep"
+    set res [remote_spawn host "$binfile sleep"];
+    if { $res < 0 || $res == "" } {
+	fail $test
+	return
+    }
+    set pid [exp_pid -i $res]
+    # We don't care whether the program is still in the startup phase when we
+    # attach.
+
+    gdb_start
+
+    gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
+    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+
+    gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+
+    set test "attach: core file is cleared"
+    gdb_test_multiple "info files" $test {
+	-re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	    fail $test
+	}
+	-re "\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    gdb_exit
+}
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -133,8 +133,14 @@ func1 ()
   func2 ();
 }
 
-int main ()
+int
+main (int argc, char **argv)
 {
+  if (argc == 2 && strcmp (argv[1], "sleep") == 0)
+    {
+      sleep (60);
+      return 0;
+    }
   mmapdata ();
   func1 ();
   return 0;

Re: [patch] Forbid run with a core file loaded

[Eli Zaretskii]

> Date: Thu, 8 Jul 2010 19:16:48 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, mark.kettenis@xs4all.nl,
>         brobecker@adacore.com, gdb-patches@sourceware.org,
>         Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> 
> gdb/doc/
> 2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Make core files the process_stratum.
> 	* gdb.texinfo (Active Targets): Remove core_stratum.  Include
> 	record_stratum example.

This part is okay, but please fix the punctuation as shown below:

 There are multiple classes of targets such, as: processes, executable files or
 recording sessions.  Core files belong to the process class, making core file
 and process mutually exclusive.  Otherwise, @value{GDBN} can work concurrently
 on multiple active targets, one in each class.  This allows you to (for
 example) start a process and inspect its activity, while still having access to
 the executable file after the process finishes.  Or if you start process
 recording (@pxref{Reverse Execution}) and @code{reverse-step} there, you are
 presented a virtual layer of the recording target, while the process target
 remains stopped at the chronologically last point of the process execution.

Thanks.

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Thu, 08 Jul 2010 20:25:46 +0200, Eli Zaretskii wrote:
> This part is okay, but please fix the punctuation as shown below:
> 
>  There are multiple classes of targets such, as: processes, executable files or
                                      ^^^^^^^^^^^
Isn't it a typo?  But probably it is not, OK, thanks.

>  recording sessions.  Core files belong to the process class, making core file
[...]


On Mon, 19 Jul 2010 16:37:19 +0200, Pedro Alves wrote:
> This is okay, thanks.

Checked-in.


Joel, requesting approval for the 7.2 branch.
There is also less intrusive patch
	http://cvs.fedoraproject.org/viewvc/rpms/gdb/F-13/gdb-bz594560-core-vs-process.patch?content-type=text%2Fplain&view=co
which does not remove the process_stratum.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-07/msg00107.html

--- src/gdb/ChangeLog	2010/07/19 07:55:41	1.11998
+++ src/gdb/ChangeLog	2010/07/19 17:51:22	1.11999
@@ -1,3 +1,26 @@
+2010-07-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Make core files the process_stratum.
+	* corefile.c (core_target): New variable.
+	(core_file_command): Remove variable t, use core_target.
+	* corelow.c (core_ops): Make it static.
+	(init_core_ops): Change to process_stratum.  Initialize CORE_TARGET.
+	* defs.h (make_cleanup_unpush_target): New prototype.
+	* gdbarch.h: Regenerate.
+	* gdbarch.sh (core_pid_to_str): Remove core_stratum from its comment.
+	* gdbcore.h (core_target): New declaration.
+	* inf-ptrace.c (inf_ptrace_create_inferior, inf_ptrace_attach): New
+	variables ops_already_pushed and back_to.  Use push_target,
+	make_cleanup_unpush_target and discard_cleanups calls.
+	* record.c (record_open): Replace core_stratum by a core_bfd check.
+	* target.c (target_is_pushed): New function.
+	(find_core_target): Remove.
+	* target.h (enum strata) <core_stratum>: Remove.
+	(target_is_pushed): New declaration.
+	(find_core_target): Remove declaration.
+	* tracepoint.c (init_tfile_ops) <to_stratum>: Remove comment.
+	* utils.c (do_unpush_target, make_cleanup_unpush_target): New functions.
+
 2010-07-19  Hui Zhu  <teawater@gmail.com>
 
 	* breakpoint.c (single_step_breakpoints_inserted): New
--- src/gdb/corefile.c	2010/05/13 23:53:32	1.58
+++ src/gdb/corefile.c	2010/07/19 17:51:23	1.59
@@ -59,6 +59,10 @@
 /* Binary file diddling handle for the core file.  */
 
 bfd *core_bfd = NULL;
+
+/* corelow.c target (if included for this gdb target).  */
+
+struct target_ops *core_target;
 \f
 
 /* Backward compatability with old way of specifying core files.  */
@@ -66,18 +70,15 @@
 void
 core_file_command (char *filename, int from_tty)
 {
-  struct target_ops *t;
-
   dont_repeat ();		/* Either way, seems bogus. */
 
-  t = find_core_target ();
-  if (t == NULL)
+  if (core_target == NULL)
     error (_("GDB can't read core files on this machine."));
 
   if (!filename)
-    (t->to_detach) (t, filename, from_tty);
+    (core_target->to_detach) (core_target, filename, from_tty);
   else
-    (t->to_open) (filename, from_tty);
+    (core_target->to_open) (filename, from_tty);
 }
 \f
 
--- src/gdb/corelow.c	2010/05/13 23:53:32	1.100
+++ src/gdb/corelow.c	2010/07/19 17:51:23	1.101
@@ -100,7 +100,7 @@
 
 void _initialize_corelow (void);
 
-struct target_ops core_ops;
+static struct target_ops core_ops;
 
 /* An arbitrary identifier for the core inferior.  */
 #define CORELOW_PID 1
@@ -911,11 +911,17 @@
   core_ops.to_thread_alive = core_thread_alive;
   core_ops.to_read_description = core_read_description;
   core_ops.to_pid_to_str = core_pid_to_str;
-  core_ops.to_stratum = core_stratum;
+  core_ops.to_stratum = process_stratum;
   core_ops.to_has_memory = core_has_memory;
   core_ops.to_has_stack = core_has_stack;
   core_ops.to_has_registers = core_has_registers;
   core_ops.to_magic = OPS_MAGIC;
+
+  if (core_target)
+    internal_error (__FILE__, __LINE__, 
+		    _("init_core_ops: core target already exists (\"%s\")."),
+		    core_target->to_longname);
+  core_target = &core_ops;
 }
 
 void
--- src/gdb/defs.h	2010/06/26 06:44:47	1.275
+++ src/gdb/defs.h	2010/07/19 17:51:23	1.276
@@ -352,6 +352,9 @@
 
 extern struct cleanup *make_cleanup_restore_integer (int *variable);
 
+struct target_ops;
+extern struct cleanup *make_cleanup_unpush_target (struct target_ops *ops);
+
 extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *);
 
 extern struct cleanup *make_my_cleanup (struct cleanup **,
--- src/gdb/gdbarch.h	2010/07/06 12:56:22	1.417
+++ src/gdb/gdbarch.h	2010/07/19 17:51:23	1.418
@@ -672,8 +672,7 @@
 extern LONGEST gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdb_byte *readbuf, ULONGEST offset, LONGEST len);
 extern void set_gdbarch_core_xfer_shared_libraries (struct gdbarch *gdbarch, gdbarch_core_xfer_shared_libraries_ftype *core_xfer_shared_libraries);
 
-/* How the core_stratum layer converts a PTID from a core file to a
-   string. */
+/* How the core target converts a PTID from a core file to a string. */
 
 extern int gdbarch_core_pid_to_str_p (struct gdbarch *gdbarch);
 
--- src/gdb/gdbarch.sh	2010/07/06 12:56:23	1.513
+++ src/gdb/gdbarch.sh	2010/07/19 17:51:23	1.514
@@ -611,8 +611,7 @@
 # core file into buffer READBUF with length LEN.
 M:LONGEST:core_xfer_shared_libraries:gdb_byte *readbuf, ULONGEST offset, LONGEST len:readbuf, offset, len
 
-# How the core_stratum layer converts a PTID from a core file to a
-# string.
+# How the core target converts a PTID from a core file to a string.
 M:char *:core_pid_to_str:ptid_t ptid:ptid
 
 # BFD target to use when generating a core file.
--- src/gdb/gdbcore.h	2010/01/01 07:31:32	1.38
+++ src/gdb/gdbcore.h	2010/07/19 17:51:23	1.39
@@ -108,6 +108,8 @@
 
 extern bfd *core_bfd;
 
+extern struct target_ops *core_target;
+
 /* Whether to open exec and core files read-only or read-write.  */
 
 extern int write_files;
--- src/gdb/inf-ptrace.c	2010/02/15 17:35:49	1.70
+++ src/gdb/inf-ptrace.c	2010/07/19 17:51:23	1.71
@@ -121,10 +121,23 @@
 {
   int pid;
 
+  /* Do not change either targets above or the same target if already present.
+     The reason is the target stack is shared across multiple inferiors.  */
+  int ops_already_pushed = target_is_pushed (ops);
+  struct cleanup *back_to;
+
+  if (! ops_already_pushed)
+    {
+      /* Clear possible core file with its process_stratum.  */
+      push_target (ops);
+      back_to = make_cleanup_unpush_target (ops);
+    }
+
   pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
 		       NULL, NULL);
 
-  push_target (ops);
+  if (! ops_already_pushed)
+    discard_cleanups (back_to);
 
   /* On some targets, there must be some explicit synchronization
      between the parent and child processes after the debugger
@@ -189,11 +202,24 @@
   pid_t pid;
   struct inferior *inf;
 
+  /* Do not change either targets above or the same target if already present.
+     The reason is the target stack is shared across multiple inferiors.  */
+  int ops_already_pushed = target_is_pushed (ops);
+  struct cleanup *back_to;
+
   pid = parse_pid_to_attach (args);
 
   if (pid == getpid ())		/* Trying to masturbate?  */
     error (_("I refuse to debug myself!"));
 
+  if (! ops_already_pushed)
+    {
+      /* target_pid_to_str already uses the target.  Also clear possible core
+	 file with its process_stratum.  */
+      push_target (ops);
+      back_to = make_cleanup_unpush_target (ops);
+    }
+
   if (from_tty)
     {
       exec_file = get_exec_file (0);
@@ -226,7 +252,8 @@
      target, it should decorate the ptid later with more info.  */
   add_thread_silent (inferior_ptid);
 
-  push_target(ops);
+  if (! ops_already_pushed)
+    discard_cleanups (back_to);
 }
 
 #ifdef PT_GET_PROCESS_STATE
--- src/gdb/record.c	2010/07/19 07:55:43	1.51
+++ src/gdb/record.c	2010/07/19 17:51:23	1.52
@@ -962,7 +962,7 @@
   record_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint;
   record_beneath_to_stopped_data_address = tmp_to_stopped_data_address;
 
-  if (current_target.to_stratum == core_stratum)
+  if (core_bfd)
     record_core_open_1 (name, from_tty);
   else
     record_open_1 (name, from_tty);
--- src/gdb/target.c	2010/07/16 20:04:41	1.260
+++ src/gdb/target.c	2010/07/19 17:51:23	1.261
@@ -1037,6 +1037,30 @@
   pop_all_targets_above (dummy_stratum, quitting);
 }
 
+/* Return 1 if T is now pushed in the target stack.  Return 0 otherwise.  */
+
+int
+target_is_pushed (struct target_ops *t)
+{
+  struct target_ops **cur;
+
+  /* Check magic number.  If wrong, it probably means someone changed
+     the struct definition, but not all the places that initialize one.  */
+  if (t->to_magic != OPS_MAGIC)
+    {
+      fprintf_unfiltered (gdb_stderr,
+			  "Magic number of %s target struct wrong\n",
+			  t->to_shortname);
+      internal_error (__FILE__, __LINE__, _("failed internal consistency check"));
+    }
+
+  for (cur = &target_stack; (*cur) != NULL; cur = &(*cur)->beneath)
+    if (*cur == t)
+      return 1;
+
+  return 0;
+}
+
 /* Using the objfile specified in OBJFILE, find the address for the
    current thread's thread-local storage with offset OFFSET.  */
 CORE_ADDR
@@ -2770,31 +2794,6 @@
   return (count == 1 ? runable : NULL);
 }
 
-/* Find a single core_stratum target in the list of targets and return it.
-   If for some reason there is more than one, return NULL.  */
-
-struct target_ops *
-find_core_target (void)
-{
-  struct target_ops **t;
-  struct target_ops *runable = NULL;
-  int count;
-
-  count = 0;
-
-  for (t = target_structs; t < target_structs + target_struct_size;
-       ++t)
-    {
-      if ((*t)->to_stratum == core_stratum)
-	{
-	  runable = *t;
-	  ++count;
-	}
-    }
-
-  return (count == 1 ? runable : NULL);
-}
-
 /*
  * Find the next target down the stack from the specified target.
  */
--- src/gdb/target.h	2010/07/07 16:15:18	1.185
+++ src/gdb/target.h	2010/07/19 17:51:23	1.186
@@ -68,8 +68,7 @@
   {
     dummy_stratum,		/* The lowest of the low */
     file_stratum,		/* Executable files, etc */
-    core_stratum,		/* Core dump files */
-    process_stratum,		/* Executing processes */
+    process_stratum,		/* Executing processes or core dump files */
     thread_stratum,		/* Executing threads */
     record_stratum,		/* Support record debugging */
     arch_stratum		/* Architecture overrides */
@@ -1485,6 +1484,8 @@
    strictly above ABOVE_STRATUM.  */
 extern void pop_all_targets_above (enum strata above_stratum, int quitting);
 
+extern int target_is_pushed (struct target_ops *t);
+
 extern CORE_ADDR target_translate_tls_address (struct objfile *objfile,
 					       CORE_ADDR offset);
 
@@ -1546,8 +1547,6 @@
 
 extern struct target_ops *find_run_target (void);
 
-extern struct target_ops *find_core_target (void);
-
 extern struct target_ops *find_target_beneath (struct target_ops *);
 
 /* Read OS data object of type TYPE from the target, and return it in
--- src/gdb/tracepoint.c	2010/07/01 15:36:17	1.191
+++ src/gdb/tracepoint.c	2010/07/19 17:51:23	1.192
@@ -4098,8 +4098,6 @@
   tfile_ops.to_get_trace_status = tfile_get_trace_status;
   tfile_ops.to_trace_find = tfile_trace_find;
   tfile_ops.to_get_trace_state_variable_value = tfile_get_trace_state_variable_value;
-  /* core_stratum might seem more logical, but GDB doesn't like having
-     more than one core_stratum vector.  */
   tfile_ops.to_stratum = process_stratum;
   tfile_ops.to_has_all_memory = tfile_has_all_memory;
   tfile_ops.to_has_memory = tfile_has_memory;
--- src/gdb/utils.c	2010/06/26 06:44:47	1.236
+++ src/gdb/utils.c	2010/07/19 17:51:23	1.237
@@ -352,6 +352,24 @@
 			   xfree);
 }
 
+/* Helper for make_cleanup_unpush_target.  */
+
+static void
+do_unpush_target (void *arg)
+{
+  struct target_ops *ops = arg;
+
+  unpush_target (ops);
+}
+
+/* Return a new cleanup that unpushes OPS.  */
+
+struct cleanup *
+make_cleanup_unpush_target (struct target_ops *ops)
+{
+  return make_my_cleanup (&cleanup_chain, do_unpush_target, ops);
+}
+
 struct cleanup *
 make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function,
 		  void *arg,  void (*free_arg) (void *))
--- src/gdb/doc/ChangeLog	2010/07/13 20:51:34	1.1086
+++ src/gdb/doc/ChangeLog	2010/07/19 17:51:24	1.1087
@@ -1,3 +1,10 @@
+2010-07-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+	    Eli Zaretskii  <eliz@gnu.org>
+
+	Make core files the process_stratum.
+	* gdb.texinfo (Active Targets): Remove core_stratum.  Include
+	record_stratum example.
+
 2010-07-13  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.texinfo (Index Files): New node.
--- src/gdb/doc/gdb.texinfo	2010/07/13 20:51:34	1.740
+++ src/gdb/doc/gdb.texinfo	2010/07/19 17:51:24	1.741
@@ -15391,33 +15391,20 @@
 @cindex active targets
 @cindex multiple targets
 
-There are three classes of targets: processes, core files, and
-executable files.  @value{GDBN} can work concurrently on up to three
-active targets, one in each class.  This allows you to (for example)
-start a process and inspect its activity without abandoning your work on
-a core file.
-
-For example, if you execute @samp{gdb a.out}, then the executable file
-@code{a.out} is the only active target.  If you designate a core file as
-well---presumably from a prior run that crashed and coredumped---then
-@value{GDBN} has two active targets and uses them in tandem, looking
-first in the corefile target, then in the executable file, to satisfy
-requests for memory addresses.  (Typically, these two classes of target
-are complementary, since core files contain only a program's
-read-write memory---variables and so on---plus machine status, while
-executable files contain only the program text and initialized data.)
-
-When you type @code{run}, your executable file becomes an active process
-target as well.  When a process target is active, all @value{GDBN}
-commands requesting memory addresses refer to that target; addresses in
-an active core file or executable file target are obscured while the
-process target is active.
-
-Use the @code{core-file} and @code{exec-file} commands to select a new
-core file or executable target (@pxref{Files, ,Commands to Specify
-Files}).  To specify as a target a process that is already running, use
-the @code{attach} command (@pxref{Attach, ,Debugging an Already-running
-Process}).
+There are multiple classes of targets such, as: processes, executable files or
+recording sessions.  Core files belong to the process class, making core file
+and process mutually exclusive.  Otherwise, @value{GDBN} can work concurrently
+on multiple active targets, one in each class.  This allows you to (for
+example) start a process and inspect its activity, while still having access to
+the executable file after the process finishes.  Or if you start process
+recording (@pxref{Reverse Execution}) and @code{reverse-step} there, you are
+presented a virtual layer of the recording target, while the process target
+remains stopped at the chronologically last point of the process execution.
+
+Use the @code{core-file} and @code{exec-file} commands to select a new core
+file or executable target (@pxref{Files, ,Commands to Specify Files}).  To
+specify as a target a process that is already running, use the @code{attach}
+command (@pxref{Attach, ,Debugging an Already-running Process}).
 
 @node Target Commands
 @section Commands for Managing Targets
--- src/gdb/testsuite/ChangeLog	2010/07/14 14:54:58	1.2381
+++ src/gdb/testsuite/ChangeLog	2010/07/19 17:51:24	1.2382
@@ -1,3 +1,13 @@
+2010-07-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Make core files the process_stratum.
+	* gdb.base/corefile.exp (run: load core again)
+	(run: sanity check we see the core file, run: with core)
+	(run: core file is cleared, attach: load core again)
+	(attach: sanity check we see the core file, attach: with core)
+	(attach: core file is cleared): New tests.
+	* gdb.base/coremaker.c (main): New parameters.  Implement "sleep" argv.
+
 2010-07-14  Ken Werner  <ken.werner@de.ibm.com>
 
 	* gdb.arch/altivec-abi.exp: New tests.
--- src/gdb/testsuite/gdb.base/corefile.exp	2010/05/24 22:03:59	1.21
+++ src/gdb/testsuite/gdb.base/corefile.exp	2010/07/19 17:51:25	1.22
@@ -177,3 +177,62 @@
 gdb_test "up" "#\[0-9\]* *\[0-9xa-fH'\]* in .* \\(.*\\).*" "up in corefile.exp (reinit)"
 
 gdb_test "core" "No core file now."
+
+
+# Test a run (start) command will clear any loaded core file.
+
+gdb_test "core-file $corefile" "Core was generated by .*" "run: load core again"
+gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "run: sanity check we see the core file"
+
+set test "run: with core"
+if [runto_main] {
+    pass $test
+} else {
+    fail $test
+}
+
+set test "run: core file is cleared"
+gdb_test_multiple "info files" $test {
+    -re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	fail $test
+    }
+    -re "\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
+gdb_exit
+
+
+# Test an attach command will clear any loaded core file.
+
+if ![is_remote target] {
+    set test "attach: spawn sleep"
+    set res [remote_spawn host "$binfile sleep"];
+    if { $res < 0 || $res == "" } {
+	fail $test
+	return
+    }
+    set pid [exp_pid -i $res]
+    # We don't care whether the program is still in the startup phase when we
+    # attach.
+
+    gdb_start
+
+    gdb_test "core-file $corefile" "Core was generated by .*" "attach: load core again"
+    gdb_test "info files" "\r\nLocal core dump file:\r\n.*" "attach: sanity check we see the core file"
+
+    gdb_test "attach $pid" "Attaching to process $pid\r\n.*" "attach: with core"
+
+    set test "attach: core file is cleared"
+    gdb_test_multiple "info files" $test {
+	-re "\r\nLocal core dump file:\r\n.*\r\n$gdb_prompt $" {
+	    fail $test
+	}
+	-re "\r\n$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    gdb_exit
+}
--- src/gdb/testsuite/gdb.base/coremaker.c	2010/01/01 07:32:00	1.8
+++ src/gdb/testsuite/gdb.base/coremaker.c	2010/07/19 17:51:25	1.9
@@ -133,8 +133,14 @@
   func2 ();
 }
 
-int main ()
+int
+main (int argc, char **argv)
 {
+  if (argc == 2 && strcmp (argv[1], "sleep") == 0)
+    {
+      sleep (60);
+      return 0;
+    }
   mmapdata ();
   func1 ();
   return 0;

Re: [patch] Forbid run with a core file loaded

[Eli Zaretskii]

> Date: Mon, 19 Jul 2010 20:01:17 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: mark.kettenis@xs4all.nl, gdb-patches@sourceware.org,
>         emachado@linux.vnet.ibm.com
> 
> On Thu, 08 Jul 2010 20:25:46 +0200, Eli Zaretskii wrote:
> > This part is okay, but please fix the punctuation as shown below:
> > 
> >  There are multiple classes of targets such, as: processes, executable files or
>                                       ^^^^^^^^^^^
> Isn't it a typo?  But probably it is not, OK, thanks.

If you mean the comma after "such", then yes, it's better be removed.

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Mon, 19 Jul 2010 20:03:55 +0200, Eli Zaretskii wrote:
> > On Thu, 08 Jul 2010 20:25:46 +0200, Eli Zaretskii wrote:
> > > This part is okay, but please fix the punctuation as shown below:
> > > 
> > >  There are multiple classes of targets such, as: processes, executable files or
> >                                       ^^^^^^^^^^^
> > Isn't it a typo?  But probably it is not, OK, thanks.
> 
> If you mean the comma after "such", then yes, it's better be removed.

Checked-in this one.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2010-07/msg00108.html

--- src/gdb/doc/ChangeLog	2010/07/19 17:51:24	1.1087
+++ src/gdb/doc/ChangeLog	2010/07/19 18:11:31	1.1088
@@ -1,4 +1,8 @@
 2010-07-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.texinfo (Active Targets): Fix wrong comma placement.
+
+2010-07-19  Jan Kratochvil  <jan.kratochvil@redhat.com>
 	    Eli Zaretskii  <eliz@gnu.org>
 
 	Make core files the process_stratum.
--- src/gdb/doc/gdb.texinfo	2010/07/19 17:51:24	1.741
+++ src/gdb/doc/gdb.texinfo	2010/07/19 18:11:31	1.742
@@ -15391,7 +15391,7 @@
 @cindex active targets
 @cindex multiple targets
 
-There are multiple classes of targets such, as: processes, executable files or
+There are multiple classes of targets such as: processes, executable files or
 recording sessions.  Core files belong to the process class, making core file
 and process mutually exclusive.  Otherwise, @value{GDBN} can work concurrently
 on multiple active targets, one in each class.  This allows you to (for

Re: [patch] Forbid run with a core file loaded

[Joel Brobecker]

> Joel, requesting approval for the 7.2 branch.
> There is also less intrusive patch
> 	http://cvs.fedoraproject.org/viewvc/rpms/gdb/F-13/gdb-bz594560-core-vs-process.patch?content-type=text%2Fplain&view=co
> which does not remove the process_stratum.

I am inclined hold off this patch off the branch (patch is too intrusive
this late in the game, problem is too minor [IMO]).  But Pedro has a better
understanding of the patch, and if he thinks it's safe to put in, then
that's good enough to go in 7.2.

-- 
Joel

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Thursday 08 July 2010 18:16:48, Jan Kratochvil wrote:

> gdb/
> 2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         Make core files the process_stratum.
>         * corefile.c (core_target): New variable.
>         (core_file_command): Remove variable t, use core_target.
>         * corelow.c (core_ops): Make it static.
>         (init_core_ops): Change to process_stratum.  Initialize CORE_TARGET.
>         * defs.h (make_cleanup_unpush_target): New prototype.
>         * gdbarch.h: Regenerate.
>         * gdbarch.sh (core_pid_to_str): Remove core_stratum from its comment.
>         * gdbcore.h (core_target): New declaration.
>         * inf-ptrace.c (inf_ptrace_create_inferior, inf_ptrace_attach): New
>         variables ops_already_pushed and back_to.  Use push_target,
>         make_cleanup_unpush_target and discard_cleanups calls.
>         * record.c (record_open): Replace core_stratum by a core_bfd check.
>         * target.c (target_is_pushed): New function.
>         (find_core_target): Remove.
>         * target.h (enum strata) <core_stratum>: Remove.
>         (target_is_pushed): New declaration.
>         (find_core_target): Remove declaration.
>         * tracepoint.c (init_tfile_ops) <to_stratum>: Remove comment.
>         * utils.c (do_unpush_target, make_cleanup_unpush_target): New functions.

This is okay, thanks.

> gdb/doc/
> 2010-07-08  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
>         Make core files the process_stratum.
>         * gdb.texinfo (Active Targets): Remove core_stratum.  Include
>         record_stratum example.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Sunday 23 May 2010 19:54:40, Jan Kratochvil wrote:
> On Fri, 21 May 2010 17:06:07 +0200, Pedro Alves wrote:
> > I don't think it makes that much sense nowadays to have a
> > distinct core_stratum vs process_stratum.
> 
> OK, removed core_stratum.

Thanks.

> @@ -194,6 +195,10 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty)
>    if (pid == getpid ())                /* Trying to masturbate?  */
>      error (_("I refuse to debug myself!"));
>  
> +  /* target_pid_to_str already uses the target.  Clear possible core file with
> +     its process_stratum.  */
> +  pop_all_targets_above (ops->to_stratum - 1, 0);
> +

Unfortunately, this would break multiprocess.  For example, you may
be attaching to your second process (e.g., "start", "add-inferior",
"inferior 2", "attach $pid"), and the thread stratum needs to remain
pushed for the first inferior/process.

> +  /* Clear possible core file with its process_stratum.  */
> +  push_target (ops);
> +
>    pid = fork_inferior (exec_file, allargs, env, inf_ptrace_me, NULL,
>                        NULL, NULL);
>  
> -  push_target (ops);

This makes sense, as indeed fork_inferior touches the current
target, even today, but, can you check what happens if fork_inferior
throws an error?  I worry that if, for example, exec fails, we still
end up with the target pushed.  Probably not a big issue for linux-nat.c,
which since it support multi-process, handles being pushed with
inferior_ptid == null_ptid, but other ptrace targets don't.

> On Fri, 21 May 2010 17:02:31 +0200, Joel Brobecker wrote:
> > > +set test "start with core"
> > > +gdb_test_multiple "start" $test {
> > > +    -re {Core file is already loaded.  Unload it[?] [(]y or n[)] } {
> > > +   pass $test
> > > +    }
> > > +}
> > 
> > We should not use the "start" command in testcases, as it does not
> > work with remote targets. I'm afraid we're going to have to rely on
> > the run command instead.

There's some confusion here.  "start" doesn't work with "target remote",
just as much as "run" doesn't work.  In other words, if "run" would
work, so would "start", as the latter is just the former plus an
internal temporary breakpoint on `main'.

> OK, used runto_main now.  Attach test does not run with gdbserver the same way
> gdb.base/attach.exp does not.  Tested this testfile with runtest-gdbserver
> (only on localhost).

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Jan Kratochvil]

On Fri, 21 May 2010 16:47:07 +0200, Mark Kettenis wrote:
> > Date: Fri, 21 May 2010 15:47:19 +0200
> > From: Jan Kratochvil <jan.kratochvil@redhat.com>
> > 
> > Hi,
> > 
> > there is already a protection against loading a core file when a program is
> > running.
> 
> That makes sense.  But what you are suggesting doesn't.
> 
> I often start gdb and load a core file to investigate a problem.  Then
> I set a breakpoint at some point before the crash and run the program
> again.  This used to work just fine.

What do you suggest otherwise?

If core-and-then-run should work then we must make working also
run-and-then-core (fine with that).

With both core file and running process the core file memory gets ignored.
So I find unloading a core file as just making the situation more simple.

(a) Do you find the "Unload it?" question too distrurbing?

(b) Do you find essential to see the core file memory again after the process
    terminates?


Thanks,
Jan

Re: [patch] Forbid run with a core file loaded

[Joel Brobecker]

> (gdb) start
> Starting program: /bin/sleep 
> ^^^^^^^^^^^^^^^^ !!!
> [...]

Hah! I was convinced we would ask confirmation to the user in this case...
I agree we should.

> 2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Forbid run with a core file loaded.
> 	* infcmd.c (run_command_1) <core_bfd>: New.

This looks fine to me (but let's wait for Mark to confirm that there
was a misunderstanding - I'll followup separately).

> gdb/testsuite/
> 2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Forbid run with a core file loaded.
> 	* gdb.base/corefile.exp (load core again, start with core)
> 	(started with core): New tests.

> +set test "start with core"
> +gdb_test_multiple "start" $test {
> +    -re {Core file is already loaded.  Unload it[?] [(]y or n[)] } {
> +	pass $test
> +    }
> +}

We should not use the "start" command in testcases, as it does not
work with remote targets. I'm afraid we're going to have to rely on
the run command instead.

-- 
Joel

Re: [patch] Forbid run with a core file loaded

[Pedro Alves]

On Friday 21 May 2010 14:47:19, Jan Kratochvil wrote:
> Hi,
> 
> there is already a protection against loading a core file when a program is
> running.
> 
> But one still can now run a program when a core file is loaded.  Moreover GDB
> then crashes on `quit'.
> 
> (gdb) file sleep
> [...]
> (gdb) start
> [...]
> (gdb) core-file core.5841 
> A program is being debugged already.  Kill it? (y or n) y
> [...]
> Program terminated with signal 11, Segmentation fault.
> [...]
> (gdb) start
> Starting program: /bin/sleep 
> ^^^^^^^^^^^^^^^^ !!!

That's sort of expected.  From gdb.texinfo:

"There are three classes of targets: processes, core files, and
executable files.  @value{GDBN} can work concurrently on up to three
active targets, one in each class.  This allows you to (for example)
start a process and inspect its activity without abandoning your work on
a core file."

This doesn't, and I think never has worked 100% correctly, except
maybe on single-threaded targets, on older GDBs.

Nowadays, it's really not expected and supported to have the
core_ops target remain on the target stack simultaneously and
below a process_stratum target (linux-nat.c, in your case).

I don't think it makes that much sense nowadays to have a
distinct core_stratum vs process_stratum.  It core_ops was
a process_stratum, pushing linux-nat.c would auto discard
the core, as you can't have two targets of the same stratum
on the stack.  The only main use for that stratum
nowadays is for find_core_target, I think.

> [...]
> (gdb) quit
> A debugging session is active.
> 	Inferior 1 [process 13887] will be killed.
> Quit anyway? (y or n) y
> inferior.c:362: internal-error: find_inferior_pid: Assertion `pid != 0' failed.
> 

> gdb/
> 2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Forbid run with a core file loaded.
> 	* infcmd.c (run_command_1) <core_bfd>: New.

What about "attach"?

> 
> gdb/testsuite/
> 2010-05-21  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Forbid run with a core file loaded.
> 	* gdb.base/corefile.exp (load core again, start with core)
> 	(started with core): New tests.
> 
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -483,6 +483,15 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main)
>  
>    dont_repeat ();
>  
> +  if (core_bfd)
> +    {
> +      if (!from_tty
> +	  || query (_("Core file is already loaded.  Unload it? ")))
> +	core_file_command (NULL, from_tty);
> +      if (core_bfd)
> +	error (_("Core file not unloaded."));
> +    }
> +

I wish this would query a target property instead of poking
core_bfd directly, but I don't see what.  Several targets call
target_preopen to clean up the previous different target, which
does more cleanup than just getting rid of the core target, but
it looks a bit messy to harmonise this.  So I'm fine with
going this route.

"is already loaded" sort of implies (to my ears) that you are
trying to load another core.  "load a core --- what do yo mean
?  I _already_ have one loaded."

Taking from attach_command, how about:

"A core file is being debugged.  Unload it? "

Anyway, that's just a suggestion.  Ignore that one if you
don't like it.

-- 
Pedro Alves

Re: [patch] Forbid run with a core file loaded

[Joel Brobecker]

> Taking from attach_command, how about:
> 
> "A core file is being debugged.  Unload it? "

I like that one.

-- 
Joel