[RFC] Don't create inferior in tfile target.

[RFC] Don't create inferior in tfile target.

[Yao Qi]

In ctf target, we don't create inferior when open ctf trace file,
however we do it in tfile target.  After read the code for a while, I
don't see any reason that we need an inferior here.  The code was
added along with the tfile support patch, and was modified once by
patch [1]

I checked out the code on the revision before patch [1] was applied
bd196e7a61b03f2ea7e5dcb0aecbd49d239d6390 and I am able to reproduce
the same internal error, However, I am wondering why do we need
inferior in tfile target.  I removed the code to create inferior in
tfile_open and remove inferior in tfile_close.  The internal error can
be fixed also.  I also checked that 'info threads' and 'info
inferiors' works properly.

I talked with Stan (he wrote tfile supported code) on this, but we were
unable to recall the reason in details on creating inferior.  I post
this patch to remove the code to create inferior in tfile target.
Comments are welcome.

[1] robustify target tfile's open code
http://sourceware.org/ml/gdb-patches/2011-05/msg00477.html

gdb:

2013-05-04  Yao Qi  <yao@codesourcery.com>

	* tracepoint.c (TFILE_PID): Remove.
	(tfile_open): Don't add thread and inferior.
	(tfile_close): Don't set 'inferior_ptid'.  Don't call
	exit_inferior_silent.
	(tfile_thread_alive): Remove.
	(init_tfile_ops): Don't set field 'to_thread_alive' of
	tfile_ops.

gdb/testsuite:

2013-05-04  Yao Qi  <yao@codesourcery.com>

	* gdb.trace/tfile.exp: Test inferior and thread.

---
 gdb/testsuite/gdb.trace/tfile.exp |    3 +++
 gdb/tracepoint.c                  |   19 -------------------
 2 files changed, 3 insertions(+), 19 deletions(-)

diff --git a/gdb/testsuite/gdb.trace/tfile.exp b/gdb/testsuite/gdb.trace/tfile.exp
index 087d207..d7381eb 100644
--- a/gdb/testsuite/gdb.trace/tfile.exp
+++ b/gdb/testsuite/gdb.trace/tfile.exp
@@ -119,6 +119,9 @@ Trace buffer has 256 bytes of 4096 bytes free \\(93% full\\).*
 Not looking at any trace frame.*" \
     "tstatus on error trace file"
 
+gdb_test "info threads" "No threads\..*"
+gdb_test "info inferiors" "\\* 1    <null>\[ \t\]+${binfile}.*"
+
 # Make sure we can reopen without error.
 gdb_test \
     "interpreter-exec mi \"-target-select tfile tfile-basic.tf\"" \
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 9e2c4e3..0124211 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -81,8 +81,6 @@
    large.  (400 - 31)/2 == 184 */
 #define MAX_AGENT_EXPR_LEN	184
 
-#define TFILE_PID (1)
-
 /* A hook used to notify the UI of tracepoint operations.  */
 
 void (*deprecated_trace_find_hook) (char *arg, int from_tty);
@@ -4273,10 +4271,6 @@ tfile_open (char *filename, int from_tty)
       throw_exception (ex);
     }
 
-  inferior_appeared (current_inferior (), TFILE_PID);
-  inferior_ptid = pid_to_ptid (TFILE_PID);
-  add_thread_silent (inferior_ptid);
-
   if (ts->traceframe_count <= 0)
     warning (_("No traceframes present in this file."));
 
@@ -4287,8 +4281,6 @@ tfile_open (char *filename, int from_tty)
   merge_uploaded_trace_state_variables (&uploaded_tsvs);
 
   merge_uploaded_tracepoints (&uploaded_tps);
-
-  post_create_inferior (&tfile_ops, from_tty);
 }
 
 /* Interpret the given line from the definitions part of the trace
@@ -4661,10 +4653,6 @@ tfile_close (void)
   if (trace_fd < 0)
     return;
 
-  pid = ptid_get_pid (inferior_ptid);
-  inferior_ptid = null_ptid;	/* Avoid confusion from thread stuff.  */
-  exit_inferior_silent (pid);
-
   close (trace_fd);
   trace_fd = -1;
   xfree (trace_filename);
@@ -5111,12 +5099,6 @@ tfile_has_registers (struct target_ops *ops)
   return traceframe_number != -1;
 }
 
-static int
-tfile_thread_alive (struct target_ops *ops, ptid_t ptid)
-{
-  return 1;
-}
-
 /* Callback for traceframe_walk_blocks.  Builds a traceframe_info
    object for the tfile target's current traceframe.  */
 
@@ -5196,7 +5178,6 @@ init_tfile_ops (void)
   tfile_ops.to_has_stack = tfile_has_stack;
   tfile_ops.to_has_registers = tfile_has_registers;
   tfile_ops.to_traceframe_info = tfile_traceframe_info;
-  tfile_ops.to_thread_alive = tfile_thread_alive;
   tfile_ops.to_magic = OPS_MAGIC;
 }
 
-- 
1.7.7.6

Re: [RFC] Don't create inferior in tfile target.

[Pedro Alves]

On 05/04/2013 02:31 AM, Yao Qi wrote:
> In ctf target, we don't create inferior when open ctf trace file,
> however we do it in tfile target.  After read the code for a while, I
> don't see any reason that we need an inferior here.  The code was
> added along with the tfile support patch, and was modified once by
> patch [1]
> 
> I checked out the code on the revision before patch [1] was applied
> bd196e7a61b03f2ea7e5dcb0aecbd49d239d6390 and I am able to reproduce
> the same internal error, However, I am wondering why do we need
> inferior in tfile target.  I removed the code to create inferior in
> tfile_open and remove inferior in tfile_close.  The internal error can
> be fixed also.  I also checked that 'info threads' and 'info
> inferiors' works properly.
> 
> I talked with Stan (he wrote tfile supported code) on this, but we were
> unable to recall the reason in details on creating inferior.  I post
> this patch to remove the code to create inferior in tfile target.
> Comments are welcome.

I have a very vague recollection that it was I who suggested this
in internal reviews at the time, but GDB was a bit different then,
and I don't recall exactly why.  It's possible you might find that
in CS's archives.

The whole tfile/tracepoints model is weird, in that it's hacked
on, and bypasses the whole target stack model.  You get
things like, while inspecting a traceframe (on targets where
threads are a kernel entity), "info threads" lists you the threads
the live target happens to have at the moment.  Same with shared
libraries, etc.  This could/should probably be revisited once
gdb learns about being connected to multiple targets simultaneously.

I was going to say this would break "detach" with tfile
(detach_command checks for null_ptid).  Except "detach" with tfile
doesn't work already -- with target core,
"detach" unloads the core, and I assumed tfile behaved the same.
I think it'd be reasonable if it did.

You didn't mention it explicitly, so I'll ask.
There are probably more commands that treat null_ptid magically.
Could you audit kill, detach, continue, step, etc.
to see if they'll do something reasonable?  Or rather,
could you audit/grep the tree for null_ptid uses?  E.g.,
I see that dcache.c uses null_ptid as magic number, but
probably that doesn't matter for tfile.  There don't seem
to be that many checks for null_ptid, and many are in run
control code, which obviously doesn't apply, so an audit
seems doable.

Thanks,
-- 
Pedro Alves

Re: [RFC] Don't create inferior in tfile target.

[Stan Shebs]

On 5/15/13 8:33 AM, Pedro Alves wrote:
> On 05/04/2013 02:31 AM, Yao Qi wrote:
>> In ctf target, we don't create inferior when open ctf trace file,
>> however we do it in tfile target.  After read the code for a while, I
>> don't see any reason that we need an inferior here.  The code was
>> added along with the tfile support patch, and was modified once by
>> patch [1]
>>
>> I checked out the code on the revision before patch [1] was applied
>> bd196e7a61b03f2ea7e5dcb0aecbd49d239d6390 and I am able to reproduce
>> the same internal error, However, I am wondering why do we need
>> inferior in tfile target.  I removed the code to create inferior in
>> tfile_open and remove inferior in tfile_close.  The internal error can
>> be fixed also.  I also checked that 'info threads' and 'info
>> inferiors' works properly.
>>
>> I talked with Stan (he wrote tfile supported code) on this, but we were
>> unable to recall the reason in details on creating inferior.  I post
>> this patch to remove the code to create inferior in tfile target.
>> Comments are welcome.
> 
> I have a very vague recollection that it was I who suggested this
> in internal reviews at the time, but GDB was a bit different then,
> and I don't recall exactly why.  It's possible you might find that
> in CS's archives.

We've poked around those, haven't found anything specific.  At the time
we were also working on multi-process support and other ideas, it might
simply be that it seemed like it was going to be required in the near
future. (Wouldn't be the first time that Stan added gratuitous
future-anticipating code, ahem.)

> The whole tfile/tracepoints model is weird, in that it's hacked
> on, and bypasses the whole target stack model.  You get
> things like, while inspecting a traceframe (on targets where
> threads are a kernel entity), "info threads" lists you the threads
> the live target happens to have at the moment.  Same with shared
> libraries, etc.  This could/should probably be revisited once
> gdb learns about being connected to multiple targets simultaneously.

Heh, yeah, we've certainly discussed that ourselves, and I remember
multiple discussions with Andrew, Michael, etc on the subject as well.

I take the view that the target stack is a flawed concept, in that it
makes sense for the simple exec/process/core trio, and breaks down for
just about every other situation.  It has needed hacks and workarounds
ever since it was introduced ("strata", etc).  It probably wouldn't even
be that hard to replace with a functional model where you pass an
inheritance chain to a manipulating function and get back a new chain
that may be arbitrarily different ("push" and "pop" being two common
cases); the amount of code involved is less than has been changed in
other recent restructuring projects.

Stan
stan@codesourcery.com

Re: [RFC] Don't create inferior in tfile target.

[Yao Qi]

On 05/15/2013 11:33 PM, Pedro Alves wrote:
> I have a very vague recollection that it was I who suggested this
> in internal reviews at the time, but GDB was a bit different then,
> and I don't recall exactly why.  It's possible you might find that
> in CS's archives.

I went through the CS's archives (in a range of 3 years), but didn't 
find anything related to it.

> I was going to say this would break "detach" with tfile
> (detach_command checks for null_ptid).  Except "detach" with tfile
> doesn't work already -- with target core,

Yes, this patch changes the behaviour of "detach" on tfile, without this 
patch, there is no output after command 'detach'.

(gdb) detach

However, with this patch, GDB prints something,

(gdb) detach
The program is not being run.

> "detach" unloads the core, and I assumed tfile behaved the same.
> I think it'd be reasonable if it did.

'detach' uploads the core, but I don't think 'detach' should behave the 
same in tfile target.  The GDB manual says about 'detach' command:

"When you have finished debugging the attached process, you can use the 
detach command to release it from gdb control. Detaching the process 
continues its execution. After the detach command, that process and gdb 
become completely independent once more, and you are ready to attach 
another process or start one with run."

User can use 'detach' command to release the process from gdb control. 
Command 'detach' applies to core file, because the pid of the process 
which core file is dumped is stored in core file, so GDB can 'emulate' 
the pid of the core file.  However, the tfile and ctf doesn't save the 
pid in it (unnecessary to do so on the other hand), so command 'detach' 
doesn't apply to tfile.  The output "The program is not being run." of
command 'detach' on tfile is reasonable to me.
>
> You didn't mention it explicitly, so I'll ask.
> There are probably more commands that treat null_ptid magically.
> Could you audit kill, detach, continue, step, etc.
> to see if they'll do something reasonable?  Or rather,

The commands 'step', 'continue' behave the same, while command 'kill' 
behaves differently.

Without this patch,

(gdb) kill
Kill the program being debugged? (y or n) y
You can't do that without a process to debug.

With this patch,

(gdb) kill
The program is not being run.

According to the GDB manual, 'kill' command is "Kill the child process 
in which your program is running under gdb", we don't have a child 
process at all on tfile, IMO, the later is more reasonable.

The output of 'info threads' on tfile changes also,

without this patch:
(gdb) info threads
   Id   Target Id         Frame
* 1    process 1         No registers.

with this patch:
(gdb) info threads
No threads.

I feel the latter is more reasonable.

Without this patch command 'thread name' print nothing, but with this 
patch, command 'thread name' prints "No thread selected".

> could you audit/grep the tree for null_ptid uses?  E.g.,
> I see that dcache.c uses null_ptid as magic number, but
> probably that doesn't matter for tfile.  There don't seem
> to be that many checks for null_ptid, and many are in run
> control code, which obviously doesn't apply, so an audit
> seems doable.

I examined the null_ptid usages, looks the usages in target.c are not 
affected by this change.  I also tried commands 'info frame' and 
'maintenance print registers', the output is the same.

In short, this patch changes the output of some commands, but the 
changes are reasonable to me.  WDYT?

-- 
Yao (齐尧)

Re: [RFC] Don't create inferior in tfile target.

[Pedro Alves]

On 05/24/2013 09:07 AM, Yao Qi wrote:

>> "detach" unloads the core, and I assumed tfile behaved the same.
>> I think it'd be reasonable if it did.
> 
> 'detach' uploads the core, but I don't think 'detach' should behave the same in tfile target.  The GDB manual says about 'detach' command:
> 
> "When you have finished debugging the attached process, you can use the detach command to release it from gdb control. Detaching the process continues its execution. After the detach command, that process and gdb become completely independent once more, and you are ready to attach another process or start one with run."
> 

> User can use 'detach' command to release the process from gdb control. Command 'detach' applies to core file, because the pid of the process which core file is dumped is stored in core file, so GDB can 'emulate' the pid of the core file.  However, the tfile and ctf doesn't save the pid in it (unnecessary to do so on the other hand), so command 'detach' doesn't apply to tfile.  The output "The program is not being run." of
> command 'detach' on tfile is reasonable to me.

I think that section is assuming native debugging.

When talking about remote debugging, it also says:

When you have finished debugging the remote program, you can use the
@code{detach} command to release it from @value{GDBN} control.
Detaching from the target normally resumes its execution, but the results
will depend on your particular remote stub.  After the @code{detach}
command, @value{GDBN} is free to connect to another target.
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Another way to look at it is that "detach" literally detaches
the core, IOW, let's go of it.  Just like you can detach a
remote bare metal target, even if it has no concept of a pid.

And we have also:

@table @code
@kindex detach inferiors @var{infno}@dots{}
@item detach inferior @var{infno}@dots{}
Detach from the inferior or inferiors identified by @value{GDBN}
inferior number(s) @var{infno}@dots{}.  Note that the inferior's entry
still stays on the list of inferiors shown by @code{info inferiors},
but its Description will show @samp{<null>}.

Anyway...

> (gdb) kill
> Kill the program being debugged? (y or n) y
> You can't do that without a process to debug.
> 
> With this patch,
> 
> (gdb) kill
> The program is not being run.
> 
> According to the GDB manual, 'kill' command is "Kill the child process in which your program is running under gdb", we don't have a child process at all on tfile, IMO, the later is more reasonable.

Agreed.

> 
> The output of 'info threads' on tfile changes also,
> 
> without this patch:
> (gdb) info threads
>   Id   Target Id         Frame
> * 1    process 1         No registers.
> 
> with this patch:
> (gdb) info threads
> No threads.
> 
> I feel the latter is more reasonable.

No strong opinion here.  As long as this doesn't confuse
frontends.  Did you try Eclipse?

>> could you audit/grep the tree for null_ptid uses?  E.g.,
>> I see that dcache.c uses null_ptid as magic number, but
>> probably that doesn't matter for tfile.  There don't seem
>> to be that many checks for null_ptid, and many are in run
>> control code, which obviously doesn't apply, so an audit
>> seems doable.
> 
> I examined the null_ptid usages, looks the usages in target.c are not affected by this change.  I also tried commands 'info frame' and 'maintenance print registers', the output is the same.
> 
> In short, this patch changes the output of some commands, but the changes are reasonable to me.  WDYT?

Okay, let's go with it.  Patch is OK.

-- 
Pedro Alves

Re: [RFC] Don't create inferior in tfile target.

[Yao Qi]

On 05/24/2013 06:42 PM, Pedro Alves wrote:
> Okay, let's go with it.  Patch is OK.

Thanks for the review.  Patch is committed.

-- 
Yao (齐尧)