Bugzilla – Full Text Bug Listing
| Summary: | [memory] Memory service does not properly use the MemoryContext to select the proper process | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Pawel Piech <pawel.1.piech> | ||||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | cdtdoug, dd.general-inbox, pawel.1.piech | ||||||||||
| Version: | 0 DD 1.1 | Flags: | cdtdoug:
iplog-
|
||||||||||
| Target Milestone: | DD 1.1 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Marc Khouzam
Created attachment 114416 [details]
Original suggested fix
This patch has AbstractMIControl check if the context of a command is a container, and then choose a threadId belonging to that container.
I don't like this solution anymore :-)
1- they way to find a threadId is basically to get the list of threads within a process. We already have this functionality; the problem is that for AbstractMIcontrol to use it, it needs a synchronous call to get those threadIds. So, this patch introduces a synchronous getExecutionContexts(). This is not nice, I think.
2- This solution adds a --thread to every command that applies to a process. However, I just realized that -exec-continue and -exec-interrupt will eventually have a --thread-group option to continue/interrupt a process. The --thread flag is also valid for those two commands to continue/interrupt a single thread. So, in this case, AbstractMIControl needs to behave differently, sigh...
Mutli-process is still evolving in MI and I'm hoping that these inconsistencies will be fixed. Until then though, I find it quite difficult to reach an elegant solution. Therefore, I will not apply this patch (which I attached for reference), but will instead write a patch that is tailored specifically to the memory commands. Once multi-process extends to other commands, we can revisit this solution.
Wow, that's too bad. At least you're discovering these things early on while they can still be corrected in GDB. Created attachment 114481 [details] Propposed fix This relatively small patch adds the --thread option to the two memory commands only which solved issue number 2 of comment #1. As for issue 1 of comment #1, I couldn't get rid of it just yet ubt with M3 approaching I really didn't like this change in interface. So, I thought I should make the interface change in a provisional interface, like a new IGDBProcesses. To make all this happen, I had to override the MIMemory service with GDBMemory and GDBMemory_7_0. The nice thing about this change is that the only non-provisional file that changes is MIMemory. The JUnit tests pass, and I ran some successful tests on my multi-process GDB. Opinions are appreciated. P.S. I hesitated between two options within this design; the other option I looked at was to instead of giving an ExecutionContext to the Memory MI commands, I would continue to give a MemoryContext, and add a parameter for the threadId. Doing it like that changes both MIDataReadMemory and MIDataWriteMemory, but it allows to keep the methods within the MemoryCache to only accept a MemoryDMContext instead of a generic IDMContext. I didn't choose that option to avoid changing the two MI command classes, but I am still ambivalent about it. Created attachment 114708 [details]
Final fix
This is the fix I committed.
It should be easy to adapt once GDB stabilizes.
Fixed. Pawel, can you verify? Reviewed. Created attachment 114833 [details]
Improved fix
I didn't like the fact that I had to make the MemoryCache visible to fix this bug. I would have preferred to minimize the API changes.
So, this patch simply moves the methods I needed out of the memory cache so they could be overriden more easily. Those methods did not really need to be in the memory cache anyway. In my opinion, the memory cache does too much, which will lead to having to override it, as we had to here. But for the sake of stability, I only moved what I needed out of the memory cache. The memory cache is back to being private, and I was able to get rid of the ackward setMemoryCache() method.
JUnit tests all pass.
Committed.
Pawel, maybe you can have a quick look?
(In reply to comment #7) Looks fine on a quick glance. > ... In my opinion, the memory cache does too much, > which will lead to having to override it, as we had to here.... I agree. DD 1.1 reelased! |