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

Jan Kratochvil
2010-06-06T19:51:25Z

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;