Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 338136

Summary: [debug view] Don't show the inferior process in the debug view
Product: [Tools] CDT Reporter: Marc Khouzam <marc.khouzam>
Component: cdt-debug-dsf-gdbAssignee: Marc Khouzam <marc.khouzam>
Status: RESOLVED FIXED QA Contact: Marc Khouzam <marc.khouzam>
Severity: normal    
Priority: P3 CC: cdtdoug, malaperle, nobody, pawel.1.piech
Version: 8.0Flags: malaperle: review+
Target Milestone: 8.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Bug Depends on:    
Bug Blocks: 237308    
Attachments:
Description Flags
Prototype using a new preference
marc.khouzam: iplog-
Patch to make the right console show
marc.khouzam: iplog-
Patch to make the right console show 2
marc.khouzam: iplog-
Fix for Patch to make the right console show 2
marc.khouzam: iplog-
Some fixes for showing the console
marc.khouzam: iplog-
Patch to never show the inferior in the Debug View marc.khouzam: iplog-

Description Marc Khouzam CLA 2011-02-24 13:53:31 EST 
The value of having the inferior process shown in the debug view as a entry on its own is debatable.  Some vendors have removed it, like TI, and EDC also does not show it.

The only value I see about showing the inferior in the debug view is that it
allow the user to terminate it.  However, for single process debugging, it has
the same effect as terminating the entire session, so it does not add something
useful.

We we deal with multi-process, we may want to allow to terminate a single process.  We can surely achieve this by handling the terminate action properly based on which process (container) is selected in the debug view.  Therefore, we still don't need to show the inferior.

Finally, for attach sessions we already don't show the inferior.

For Post-mortem sessions, we don't show the inferior, but that was kind of a
bug.  I'm not sure yet if we want to show it in that case.  CDI shows it as terminated, which may be informative to the user.  See bug 271795.

Another issue is that the inferior is used to allow the user to make the corresponding console come to the front.  We can still probably do this when selecting a process (container) instead of when selecting the inferior.

I will prototype a solution that does not show the inferior in the debug view
and see how that feels.
Comment 1 Marc Khouzam CLA 2011-02-24 13:55:29 EST 
I am wondering if showing the inferior or now should be a user-preference?
Comment 2 Marc Khouzam CLA 2011-02-24 21:32:10 EST 
Created attachment 189768 [details]
Prototype using a new preference

I'm still not sure we should add a new preference for showing the inferior, but here is a patch that does it.

I haven't found the proper way to refresh the debug view when that preference is set though.  It should be done in
GdbStandardProcessVMNode#fPropertyListener#propertyChange
Comment 3 Marc Khouzam CLA 2011-02-27 00:10:54 EST 
Created attachment 189886 [details]
Patch to make the right console show

Here is the second part of this bug.  It allows to show the right console automatically when we select a stack frame/thread/process in the debug view.  With a single process, there is no noticeable change since we already showed the console of the inferior.  However, with multi-process, we change the selection from one process (or its children) to another, and with this patch, the console will properly match.

This part is valuable even if we decide to still show the inferior entries.

I revisited the investigation I had done for Bug 264895 and after much thought, the solution I found was to use the ConsolePageParticipant contributed by DSF-GDB and to extend it to mimic ProcessConsolePageParticipant from the platform.  Whenever the debug context changes, the ConsolePageParticipant for a DSF-GDB inferior or gdb entry, will check if the new debug context is a DSF-GDB context, and will extract the IMIContainerDMC from it, to be able to find the corresponding console to show.

To do this properly I added a new class called InferiorRuntimeProcess which I use for the inferior runtime process.  That way we can know if a process within a launch is for DSF-GDB or not.

The complexity about this patch is that ConsolePageParticipant affects all consoles (not just DSF-GDB) and that a new debug context could belong to any debug session.  I think I did things cleanly but I'd like another pair of eyes on this.
Comment 4 Marc Khouzam CLA 2011-02-27 00:13:59 EST 
(In reply to comment #3)
> Created attachment 189886 [details]
> Patch to make the right console show

> The complexity about this patch is that ConsolePageParticipant affects all
> consoles (not just DSF-GDB) and that a new debug context could belong to any
> debug session.  I think I did things cleanly but I'd like another pair of eyes
> on this.

Since Marc-Andre is about to be a committer and since he wrote ConsolePageParticipant, maybe he can review the patch?  Being a committer has its 'privileges' ;-)
Comment 5 Marc Khouzam CLA 2011-02-27 00:16:57 EST 
Just a clarification that the two patches currently in this bug are totally independent.
Comment 6 Marc-André Laperle CLA 2011-02-28 02:40:33 EST 
Hi Marc,

One thing that is missing when not showing the inferior is the exit value of the process. I wanted to try EDC to look at what they do there, but couldn't make it work (on Linux). If there is some way the exit value can still be shown, then there would be no loss of functionality and I don't think a preference would be needed.

About the second patch. This is the first time I try multi-process so bear with me. I attached gdb to 2 processes (I had to run as root) but didn't get a console for the inferiors. Then I tried without attaching the first process, paused and then attached a second process. I only got one console, for the first process. I don't know the state of multi-process... Did you make this patch in preparation of having multiple inferior consoles in multi-process or is this supposed to already work?

In ConsolePageParticipant:

if (launch.getProcesses()[0] instanceof GDBProcess) {
    return launch.getProcesses()[0];
}

Is it safe to assume that getProcesses() will return at least one process when getConsoleProcess returns non-null? Or could this throw an ArrayIndexOutOfBoundsException?

You might want to update the javadoc for this class to something like
"A console page participant for the gdb tracing console and gdb CLI console and the inferior console"
and update the year in the copyright ;)

Otherwise, it looks good, but I'll wait for your comments.
Comment 7 Marc Khouzam CLA 2011-02-28 11:09:30 EST 
(In reply to comment #6)
> Hi Marc,
> 
> One thing that is missing when not showing the inferior is the exit value of
> the process. I wanted to try EDC to look at what they do there, but couldn't
> make it work (on Linux). 

I couldn't either.  I think it would be easier to try on Windows.  I'll try it from home.

> If there is some way the exit value can still be
> shown, then there would be no loss of functionality and I don't think a
> preference would be needed.

That is a good point.  In the last multicore debug meeting we discussed what to show when a process terminates, and people felt that, like threads, when a process terminates, it should simply disappear (this is the process entry that has threads under it, not the inferior entry that we talking about never showing).  So, I'm not sure where we would put the exit value of the inferior.  We could put it in the title of the corresponding console, but that is a little hidden...  I'll have to think about it.

BTW, I'm also leaning towards not having a preference.  It just adds complexity for the user.

> About the second patch. This is the first time I try multi-process so bear with
> me. I attached gdb to 2 processes (I had to run as root) but didn't get a
> console for the inferiors. Then I tried without attaching the first process,
> paused and then attached a second process. I only got one console, for the
> first process. I don't know the state of multi-process... Did you make this
> patch in preparation of having multiple inferior consoles in multi-process or
> is this supposed to already work?

I'm sorry, I should have explained how to try it.
Currently, in CDT you can attach to multiple processes, but attached processes don't have a console.  To get a console, you need a new process, which you can only create one of :-O.  CDT does support creating multiple ones but there is no UI to do it.  We haven't worked out the UI details about how to do that yet.  I test things using a little plugin that adds a menu action to create a new process.  I have posted it to Bug 237308 comment 15.
You need GDB 7.2.


> In ConsolePageParticipant:
> 
> if (launch.getProcesses()[0] instanceof GDBProcess) {
>     return launch.getProcesses()[0];
> }
> 
> Is it safe to assume that getProcesses() will return at least one process when
> getConsoleProcess returns non-null? Or could this throw an
> ArrayIndexOutOfBoundsException?

For DSF-GDB it is safe for now since we know we show the GDB process.  However, it wouldn't be very future-proof, so I'll add a check.

> You might want to update the javadoc for this class to something like
> "A console page participant for the gdb tracing console and gdb CLI console and
> the inferior console"
> and update the year in the copyright ;)

Will do.

> Otherwise, it looks good, but I'll wait for your comments.

Thanks.  If you are able to try it out, let me know if you find anything else.
Comment 8 Marc Khouzam CLA 2011-02-28 11:28:25 EST 
Created attachment 189966 [details]
Patch to make the right console show 2

Updated based on Marc-Andre's comments.
Comment 9 Marc-André Laperle CLA 2011-03-01 01:38:29 EST 
(In reply to comment #7)
> (In reply to comment #6)
> I couldn't either.  I think it would be easier to try on Windows.  I'll try it
> from home.

I just tried on Windows, EDC doesn't seem to show the exit value. (BTW, I like that they use the command prompt, it probably avoids the console problems on Windows)

> showing).  So, I'm not sure where we would put the exit value of the inferior. 
> We could put it in the title of the corresponding console, but that is a little
> hidden...  I'll have to think about it.

I think in the console title would be good enough. It would also be nice to have signals like SIGSEGV (bug 325521).
 
>  I test things using a little plugin that adds a menu action to create a new
> process.  I have posted it to Bug 237308 comment 15.
> You need GDB 7.2.

Thank you, it was very useful for testing.

> Thanks.  If you are able to try it out, let me know if you find anything else.

Looks good to me!
Comment 10 Marc Khouzam CLA 2011-03-07 06:35:21 EST 
Created attachment 190539 [details]
Fix for Patch to make the right console show 2

I found a bug in the previous patch for GDB < 7.0.  I forgot to set the empty groupId as a launch attribute, so the console wouldn't show.

This patch fixes it and is updated for HEAD.

Committed to HEAD.
Comment 11 CDT Genie CLA 2011-03-07 07:23:22 EST 
*** cdt cvs genie on behalf of mkhouzam ***
Bug 338136: Show the proper inferior console when selecting a container in the debug view.  This support is for multi-process.

[*] ConsolePageParticipant.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/console/ConsolePageParticipant.java?root=Tools_Project&r1=1.1&r2=1.2

[+] InferiorRuntimeProcess.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/InferiorRuntimeProcess.java?root=Tools_Project&revision=1.1&view=markup
[*] GdbLaunch.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/GdbLaunch.java?root=Tools_Project&r1=1.13&r2=1.14

[+] IGdbDebugConstants.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/IGdbDebugConstants.java?root=Tools_Project&revision=1.1&view=markup

[*] StartOrRestartProcessSequence_7_0.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/StartOrRestartProcessSequence_7_0.java?root=Tools_Project&r1=1.5&r2=1.6
[*] GDBProcesses.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.17&r2=1.18
Comment 12 Marc Khouzam CLA 2011-03-07 10:51:07 EST 
Created attachment 190561 [details]
Some fixes for showing the console

I noticed that the solution I committed had some issues:
1- in GDB 7.0 and 7.1, the inferior console would not be shown properly.  This is because the groupIdAttribute is not being set properly when we create the inferior.  The simple solution, since we know that for 7.0 and 7.1 we are dealing with single process, is to return the first (and only) inferior.
2- For attach session where there are no inferiors, we have to return the GDB console.

Committed to HEAD.
Comment 13 Marc Khouzam CLA 2011-03-07 11:16:14 EST 
Marc-Andre, can you have a look a the last changes?  (I can set review flag for you!  Welcome!)
Comment 14 CDT Genie CLA 2011-03-07 11:23:27 EST 
*** cdt cvs genie on behalf of mkhouzam ***
Bug 338136: Show the proper inferior console when selecting a container in the debug view.  This support is for multi-process.

[*] ConsolePageParticipant.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/console/ConsolePageParticipant.java?root=Tools_Project&r1=1.2&r2=1.3
Comment 15 Marc Khouzam CLA 2011-03-08 15:56:58 EST 
Created attachment 190695 [details]
Patch to never show the inferior in the Debug View

I've committed this patch to HEAD after we agreed to do so at the Multicore Debug conference call.  It no longer shows the inferior in the debug view but does not add a preference because it didn't seem worth the confusion.

(In reply to comment #9)
> I just tried on Windows, EDC doesn't seem to show the exit value. (BTW, I like
> that they use the command prompt, it probably avoids the console problems on
> Windows)
> 
> I think in the console title would be good enough. It would also be nice to
> have signals like SIGSEGV (bug 325521).

I have created Bug 339280 about this issue because I'm not sure using the console is the right thing to do because there is no console for attached or remote processes.
Comment 16 Marc Khouzam CLA 2011-03-08 15:57:19 EST 
Fixed
Comment 18 Marc-André Laperle CLA 2011-03-09 00:14:11 EST 
Looks good. Just a nitpick, there is one instance of launch.getProcesses() that could be replaced by the processes variable (line 160). Not really worth to commit just for that though.

While testing with gdb 6.8, I noticed the instruction pointer often not being removed after the launch was terminated. It seems to be OK with 7.0.1, 7.1 and 7.2. Is there a bug for that?
Comment 19 Marc Khouzam CLA 2011-03-09 10:54:24 EST 
(In reply to comment #18)
> Looks good. Just a nitpick, there is one instance of launch.getProcesses() that
> could be replaced by the processes variable (line 160). Not really worth to
> commit just for that though.

Fixed.  Thanks.

> While testing with gdb 6.8, I noticed the instruction pointer often not being
> removed after the launch was terminated. It seems to be OK with 7.0.1, 7.1 and
> 7.2. Is there a bug for that?

I see it too, when selecting the launch node and pressing terminate.  I have opened bug 339379 about it.

Thanks again!
Comment 20 CDT Genie CLA 2011-03-09 11:23:35 EST 
*** cdt cvs genie on behalf of mkhouzam ***
Bug 338136: Don't show the inferior process in the debug view

[*] ConsolePageParticipant.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/console/ConsolePageParticipant.java?root=Tools_Project&r1=1.3&r2=1.4