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

[Pedro Alves <pedro@codesourcery.com>]

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