Bug 313767 - [cdi][memory] restoring memory monitor from previous session fails
Summary: [cdi][memory] restoring memory monitor from previous session fails
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 7.0   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-20 12:54 EDT by John Cortell CLA
Modified: 2020-09-04 15:20 EDT (History)
1 user (show)

See Also:


Attachments
Fix (7.54 KB, patch)
2010-06-02 11:07 EDT, John Cortell CLA
john.cortell: iplog-
Details | Diff
Updated patch. Listens for all events in order to establish a memory context. (9.84 KB, patch)
2010-06-02 12:54 EDT, John Cortell CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Cortell CLA 2010-05-20 12:54:53 EDT
This is related to the Memory view. The following works for CDI-GDB. It does not work for DSF-GDB.

1. Launch DSF-GDB debug session. Stops on main()
2. Open the Memory view and create a memory monitor using some valid address (e.g., 0x4012D0, in a MinGW wizard created C project)
Memory appears in the <Hex> rendering fine
3. Terminate session and launch a new one.
4. Open the memory view. The monitor is restored but you have to now choose a rendering. Choose Hex.
All memory shows as ??????

I've reproduced this with Galileo (CDT 6.0.2) and with HEAD CDT 7.0
Comment 1 John Cortell CLA 2010-06-01 18:13:41 EDT
This problem happens only with gdb >= 7.0

The root of the problem lies in the fact that we call DsfMemoryBlockRetrieval.initialize(IMemoryDMContext) with a process context that is created generically using MIProcesses.UNIQUE_GROUP_ID. See GdbLaunch.initializeControl(). That non-valid context (for our gdb 7.0 support) is used to create the DsfMemoryBlocks. Getting bytes from that block later on fails; see GDBMemory_7_0.readMemoryBlock(IDMContext, IAddress, long, int, int, DataRequestMonitor<MemoryByte[]>). We get an empty array of execution contexts. 

Note that we don't have a more reasonable context at the point that we call DsfMemoryBlockRetrieval.initialize() since the session is just getting under way. So, we need to figure out a way to defer establishing the exact context until later. The memory blocks are created at the outset of the session, but blocks aren't queried for their bytes until much later.
Comment 2 Marc Khouzam CLA 2010-06-02 08:37:54 EDT
Nice find.

It was a mistake of mine to have MIProcesses.UNIQUE_GROUP_ID be public, it should have been protected.  It should be possible to always rely on IProcesses.getProcessesBeingDebugged(), instead of building our own IProcessDMC or IContainterDMC, which is prone to error, like this bug shows.
Comment 3 John Cortell CLA 2010-06-02 11:07:21 EDT
Created attachment 170812 [details]
Fix

Marc, please review. Will hold off on committing.
Comment 4 Marc Khouzam CLA 2010-06-02 11:59:49 EDT
(In reply to comment #3)
> Created an attachment (id=170812) [details]
> Fix
> 
> Marc, please review. Will hold off on committing.

I guess in the short term, we should be able to restore monitor for single process debugging when using gdb >= 7.0.  This fix seems ok.  

I'd like to try a session that does not stop on main and has a sleep at the start.  That will keep the DsfMemoryBlock context null for a while, and we can see what happens when we look at a restored monitor.

When we start looking at multi-process, restoring the memory monitors will assign them to the first process to stop, which may not be right.  I don't see an obvious better way of handling this though.  I was thinking that a DsfMemoryBlock could always use the currently selected context, like for variables and expressions, but I'm not sure that makes sense, since some memory addresses may not be valid for a different context...
Comment 5 John Cortell CLA 2010-06-02 12:20:47 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > Created an attachment (id=170812) [details] [details]
> > Fix
> > 
> > Marc, please review. Will hold off on committing.
> 
> I guess in the short term, we should be able to restore monitor for single
> process debugging when using gdb >= 7.0.  This fix seems ok.  
> 
> I'd like to try a session that does not stop on main and has a sleep at the
> start.  That will keep the DsfMemoryBlock context null for a while, and we can
> see what happens when we look at a restored monitor.

You know, I could have sworn the memory view went blank (didn't show monitors) while the target is running, but I see that's not the case. OK. I'll look at establishing the context from *any* session event that provides it. 


> When we start looking at multi-process, restoring the memory monitors will
> assign them to the first process to stop, which may not be right.  I don't see
> an obvious better way of handling this though.  I was thinking that a
> DsfMemoryBlock could always use the currently selected context, like for
> variables and expressions, but I'm not sure that makes sense, since some memory
> addresses may not be valid for a different context...

Yes. The persisting of monitors is tricky for this very reason.
Comment 6 John Cortell CLA 2010-06-02 12:54:11 EDT
Created attachment 170837 [details]
Updated patch. Listens for all events in order to establish a memory context.

We now listen for all events not just suspended events. The heavier-weight listener unregisters itself once its mission is accomplished.
Comment 7 Marc Khouzam CLA 2010-06-02 15:34:04 EDT
(In reply to comment #6)
> Created an attachment (id=170837) [details]
> Updated patch. Listens for all events in order to establish a memory context.
> 
> We now listen for all events not just suspended events. The heavier-weight
> listener unregisters itself once its mission is accomplished.

Looks ok.  However, if it was me, I'd play it safe and not commit for Helios.  It's not a very nice bug, but no one noticed until now.  I'm not a heavy user of the memory view, so maybe it is more annoying than I think?

To be honest, the whole recreation of memory monitors with DSF kind of bothers me as I'm looking at it.  But it's too late to have a stab at it now.
Comment 8 John Cortell CLA 2010-06-02 15:38:08 EDT
(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=170837) [details] [details]
> > Updated patch. Listens for all events in order to establish a memory context.
> > 
> > We now listen for all events not just suspended events. The heavier-weight
> > listener unregisters itself once its mission is accomplished.
> 
> Looks ok.  However, if it was me, I'd play it safe and not commit for Helios. 
> It's not a very nice bug, but no one noticed until now.  I'm not a heavy user
> of the memory view, so maybe it is more annoying than I think?
> 
> To be honest, the whole recreation of memory monitors with DSF kind of bothers
> me as I'm looking at it.  But it's too late to have a stab at it now.

Fair enough. I'll apply it after Helios. If you see a better long term solution, feel free to replace it.