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

Bug 337881

Summary: [Memory Browser] Persist the Go To Address history in the launch configuration
Product: [Tools] CDT Reporter: Alain Lee <a-lee>
Component: cdt-memoryAssignee: Randy Rohrbach <Randy.Rohrbach>
Status: RESOLVED FIXED QA Contact: Ted Williams <ted>
Severity: major    
Priority: P3 CC: cdtdoug, dalexiev, john.cortell, pchuong, Randy.Rohrbach
Version: 8.0Flags: Randy.Rohrbach: review? (john.cortell)
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Patch to persist the Go To Address history in Launch Configuration
none
Patch to persist the Go To Address history in Launch Configuration
none
Slight update from Alaimn's original patch.
none
patch to address John's feedback cdtdoug: iplog+

Description Alain Lee CLA 2011-02-22 13:47:06 EST 
Go To Address history is currently stored in the preference store. It'd be better to persist the Go To Address history to the launch configuration so that each configuration can have its own history.
Comment 1 Alain Lee CLA 2011-02-22 13:50:52 EST 
Created attachment 189526 [details]
Patch to persist the Go To Address history in Launch Configuration

The patch does the following:
- Persist the Go To Address history in the launch configuraiton.
- Added a new interface ITargetLabelProvider to provide the name of the target.
- In the launch configuraiton, there will be one list per target per memory space of the target supports ITargetLabelProvider. 
- Change the history size from 30 to 10
Comment 2 Alain Lee CLA 2011-02-22 13:56:04 EST 
Created attachment 189528 [details]
Patch to persist the Go To Address history in Launch Configuration

Corrected a minor mistake in the previous patch
Comment 3 Randy Rohrbach CLA 2011-02-24 13:07:53 EST 
I am taking this one on Alain's behalf.

Randy
Comment 4 Randy Rohrbach CLA 2011-02-24 13:13:25 EST 
Created attachment 189726 [details]
Slight update from Alaimn's original patch.

Slight updates to Alain's patch. Better error checking in spots and
an increase of the list to 15 from 10.

Randy
Comment 5 Randy Rohrbach CLA 2011-02-24 13:16:07 EST 
I have checked in my modified patch based on Alain's work.

Randy

John - please review as well.
Comment 7 John Cortell CLA 2011-02-25 17:47:26 EST 
I have concerns with the mechanism for persisting the expressions for each process context in the launch configuration. It relies on getting a name/label from the process (target) context via a new interface that is CDI/DSF agnostic. 

Problem is that there is no clear definition of what a "process name" is. Is it merely the PID (or something that incorporates the PID)? If so, then that won't serve the needs of the memory browser since two consecutive debug sessions of the same program could end up with different PIDs, and thus the expressions won't be restored at best (at worst an unrelated set of expressions will be used).

A more suitable handle would be the name of the executable associated with the context. However, the new interface should be named to better reflect that. Instead of ITargetLabelProvider, I think IProgramNameProvider. (Note that "label" implies to me some connection to the UI layer, which is not so here).
Comment 8 Alain Lee CLA 2011-02-25 22:43:12 EST 
(In reply to comment #7)
> I have concerns with the mechanism for persisting the expressions for each
> process context in the launch configuration. It relies on getting a name/label
> from the process (target) context via a new interface that is CDI/DSF agnostic. 
> Problem is that there is no clear definition of what a "process name" is. Is it
> merely the PID (or something that incorporates the PID)? If so, then that won't
> serve the needs of the memory browser since two consecutive debug sessions of
> the same program could end up with different PIDs, and thus the expressions
> won't be restored at best (at worst an unrelated set of expressions will be
> used).
> A more suitable handle would be the name of the executable associated with the
> context. However, the new interface should be named to better reflect that.
> Instead of ITargetLabelProvider, I think IProgramNameProvider. (Note that
> "label" implies to me some connection to the UI layer, which is not so here).

This definitely cannot be PID and it does not necessary to be the process name/program name. Currently, any contexts that can adapt to the same MemoryBlockRetrieval share the same memory rendering. Hence, these contexts should share the same history. These contexts must provide the same label to identify the history. This label must be unique within the launch configuration and must stay the same when the configuration is re-launched. For our product, we get this label through the ThreadDMContext and this label is displayed in the Debug View. Any suggestion how this can be reflected in the interface?
Comment 9 John Cortell CLA 2011-02-26 08:08:29 EST 
> This definitely cannot be PID and it does not necessary to be the process
> name/program name. Currently, any contexts that can adapt to the same
> MemoryBlockRetrieval share the same memory rendering. Hence, these contexts
> should share the same history. These contexts must provide the same label to
> identify the history. This label must be unique within the launch configuration
> and must stay the same when the configuration is re-launched. For our product,
> we get this label through the ThreadDMContext and this label is displayed in
> the Debug View. Any suggestion how this can be reflected in the interface?

That is a complex relationship to describe. The closest tangible thing to that is the program name. That will meet the requirement the vast majority of the time, I believe. In the edge cases where it doesn't, those sessions will share a single history list, which isn't a real problem, IMO.
Comment 10 Alain Lee CLA 2011-02-28 10:39:16 EST 
(In reply to comment #9)
> That is a complex relationship to describe. The closest tangible thing to that
> is the program name. That will meet the requirement the vast majority of the
> time, I believe. In the edge cases where it doesn't, those sessions will share
> a single history list, which isn't a real problem, IMO.

This is up to the debugger to provide the unique id for persistence in the launch configuration. This can be anything that is unique. The Pin And Clone feature will utilize this new interface as well. Should we ask in the forum to see what the community thinks what this new interface should be called and what APIs should be implemented.
Comment 11 John Cortell CLA 2011-03-01 11:23:33 EST 
(In reply to comment #10)
> This can be anything that is unique. 

Not true. A GUID is unique. If an implementation generates a GUID for every process launch, that meets the requirement you just stated, but fails miserably at meeting the needs of the feature. Basically, you're asking an implementation to come up with an identifier that uniquely represents the process being debugged in the scope of all other processes debugged under the same launch, with the requirement that a subsequent launch of the same program uses that same identifier. We can try to reduce that long-winded description to some new term (good luck), but why when there's a well known concept (program name) that fits the bill 95% of the time. Again, for the edge cases where it doesn't, we're not looking at significant issue.


> Should we ask in the forum to
> see what the community thinks what this new interface should be called and what
> APIs should be implemented.

Sure.
Comment 12 Patrick Chuong CLA 2011-03-01 13:09:00 EST 
Hi John, it would be useful if this interface can be use to uniquely identify the pinned context for pin&clone. In the gdb case, I use the thread id. However, for our debugger, I use a different method to identify the context that was pinned to. I am hopping that this interface can be generic that is re-useable for what you guys are doing and as well as the pin&clone usecase. Perhaps something similar to IMemento.
Comment 13 John Cortell CLA 2011-03-01 13:55:57 EST 
(In reply to comment #12)
> Hi John, it would be useful if this interface can be use to uniquely identify
> the pinned context for pin&clone. In the gdb case, I use the thread id.
> However, for our debugger, I use a different method to identify the context
> that was pinned to. I am hopping that this interface can be generic that is
> re-useable for what you guys are doing and as well as the pin&clone usecase.
> Perhaps something similar to IMemento.

OK. So the pin-n-clone would need this for all type of contexts (process, threads, frames, whatever). In that case, how about IReocurringDebugContext, with a method called getContextID(). The javadoc for the interface would say something like:

"This interface should be implemented by context objects that can provide an identifier that is unique among other contexts of the same type in a debug session, but which will be reused if the underlying thing appears in a future debug session. For example, the executable name can typically be used to identify that the process context in two successive debug sessions represent the same program. Where threads are programatically given meaningful names, the thread name can be used to detect that we're dealing with the same thread in successive debug sessions. Implementations should stay away from using identifiers that are assigned by the underlying runtime system (e.g., PIDs and TIDs), since such IDs are often assigned and reused in non-deterministic ways."
Comment 14 John Cortell CLA 2011-03-01 13:59:07 EST 
Um. IRecurringDebugContext
Comment 15 Dobrin Alexiev CLA 2011-03-02 10:18:53 EST 
(In reply to comment #14)
> Um. IRecurringDebugContext

Most probably I'll need something similar to the grouping feature. When I restart the debug session I'd like to recreate the same debug view hierarchy.
Comment 16 Alain Lee CLA 2011-03-02 10:22:23 EST 
(In reply to comment #13)
> OK. So the pin-n-clone would need this for all type of contexts (process,
> threads, frames, whatever). In that case, how about IReocurringDebugContext,
> with a method called getContextID(). The javadoc for the interface would say
> something like:
> "This interface should be implemented by context objects that can provide an
> identifier that is unique among other contexts of the same type in a debug
> session, but which will be reused if the underlying thing appears in a future
> debug session. For example, the executable name can typically be used to
> identify that the process context in two successive debug sessions represent
> the same program. Where threads are programatically given meaningful names, the
> thread name can be used to detect that we're dealing with the same thread in
> successive debug sessions. Implementations should stay away from using
> identifiers that are assigned by the underlying runtime system (e.g., PIDs and
> TIDs), since such IDs are often assigned and reused in non-deterministic ways."

I am working on the patch with this change.
Comment 17 Alain Lee CLA 2011-03-03 10:57:36 EST 
Created attachment 190279 [details]
patch to address John's feedback

John/Randy,

Please review the changes.

ITargetLabelProvider can be removed from CVS
Comment 18 Patrick Chuong CLA 2011-03-03 11:49:41 EST 
Does it make sense to return an Object or an interface i.e IContext for getContextId? For instance, I can return IExecutionContext or integer, instead of String.
Comment 19 John Cortell CLA 2011-03-03 18:05:53 EST 
(In reply to comment #18)
> Does it make sense to return an Object or an interface i.e IContext for
> getContextId? For instance, I can return IExecutionContext or integer, instead
> of String.

Why would an implementation return an execution context? This interface is not about getting a context from a context. It's about getting a simple identifier that allows the context to be recognized if it shows up again in the future. I guess I'm not really following you. As for an integer, I don't think asking the implementation to stringify whatever identifier it natively uses is unreasonable.
Comment 20 John Cortell CLA 2011-03-03 18:06:08 EST 
(In reply to comment #17)
> Created attachment 190279 [details]
> patch to address John's feedback
> 
> John/Randy,
> 
> Please review the changes.
> 
> ITargetLabelProvider can be removed from CVS

Randy, I've got this.
Comment 21 John Cortell CLA 2011-03-04 11:54:51 EST 
I committed a number of changes. Some were tweaks of Alain's patch. Some are tweaks to the changes in the most recent commits (result of code review requested in comment # 5). None of these changes affect behavior; they are simply code improvements.

- the 'clear history' action wipes out all the expression history in the active context's launch configuration. It's debatable whether that's the desirable behavior or simply the behavior the implementation happens to provide. I happen to think it's the right thing to do. That said, the associated methods take a superfluous memory space ID.

- misspelling in the delimiter sequence

- no documentation of new methods

- methods that take a memory space take the ID as the first parameter, which is awkward given that the memory space param is optional and the others are not

- new interface should be IRecurringDebugContext, not IReocurringDebugContext. See my duh moment in comment # 14

- improve method names for clarity

- format of new code is not in keeping with standard practices; no precedence to follow in this plugin since code is inconsistent

- @SuppressWarnings can be avoided

- launch configuration exceptions are eaten instead of reported in error log

- unnecessary use of trim() on strings that don't come from user

- use of StringTokenizer.nextElement() instead of nextToken(); latter doe not require a cast

- methods create and pass empty string to indicate no memory space; null is more efficient

- use SelectionAdapter instead of SelectionListener to reduce clutter (updated some pre-existing code, too)
Comment 24 CDT Genie CLA 2011-03-04 13:23:08 EST 
*** cdt cvs genie on behalf of jcortell ***
Bug 337881: [Memory Browser] Persist the Go To Address history in the launch configuration.

[+] IRecurringDebugContext.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/model/provisional/IRecurringDebugContext.java?root=Tools_Project&revision=1.1&view=markup
Comment 25 CDT Genie CLA 2011-03-07 14:23:32 EST 
*** cdt cvs genie on behalf of jcortell ***
Bug 337881: [Memory Browser] Persist the Go To Address history in the launch configuration (removed obsolete file)

[-] ITargetLabelProvider.java  http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/model/provisional/ITargetLabelProvider.java?root=Tools_Project&view=markup