This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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;
 
 
 /* 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);
 }
 
 
--- 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;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]