Bugzilla – Full Text Bug Listing
| Summary: | [launch] Process list shown during attach launch with gdb 6.8 has bogus entries | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | John Cortell <john.cortell> | ||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | John Cortell <john.cortell> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | marc.khouzam, pawel.1.piech | ||||||||
| Version: | 7.0 | Flags: | marc.khouzam:
review+
|
||||||||
| Target Milestone: | 7.0 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
John Cortell
Created attachment 166519 [details]
Fix
Committed to HEAD. Marc, please review. !^%#$&%$!@#^$ ProcessList.getProcessList() does the work again every time?!?! When you see an interface "IProcessList.getProcessList()" doesn't it give the impression is just a simple getter? That method should have been called "generateProcessList()". Anyway. Great find John! So, with this new information, couldn't we only request the IProcessList once and call getProcessList() each time we want a new list, instead of fetching IProcessList again? Not a big deal though. Also, you'll need to apply the same fix to you path of bug 310914 where you have one instance of calling getProcessList() in a loop. (In reply to comment #3) > Also, you'll need to apply the same fix to you path of bug 310914 where you > have one instance of calling getProcessList() in a loop. Sorry, I was wrong about that. The line for (IProcessInfo procInfo : list.getProcessList()) { only makes the call once. (In reply to comment #3) > !^%#$&%$!@#^$ > ProcessList.getProcessList() does the work again every time?!?! > > When you see an interface "IProcessList.getProcessList()" doesn't it give the > impression is just a simple getter? That method should have been called > "generateProcessList()". The interface was poorly named. No doubt about it. > So, with this new information, couldn't we only request the IProcessList once > and call getProcessList() each time we want a new list, instead of fetching > IProcessList again? Not a big deal though. Hell, why isn't CCorePlugin.getProcessList() caching the instance? I guess since finding the provider is based on an extension point, and since plugins are loaded dynamically, in theory there could be use case where we'd want to not assume that looking for it once is sufficient. I don't know. I have a feeling this is called so infrequently that it may not be worth risking a regression, even if it is unlikely. (In reply to comment #5) > Hell, why isn't CCorePlugin.getProcessList() caching the instance? I guess > since finding the provider is based on an extension point, and since plugins > are loaded dynamically, in theory there could be use case where we'd want to > not assume that looking for it once is sufficient. I don't know. I have a > feeling this is called so infrequently that it may not be worth risking a > regression, even if it is unlikely. Agreed. Safer is better. Created attachment 166544 [details] Temporarily revert second half of fix (In reply to comment #0) > The second problem is that GDBProcesses.getExecutionData() ends up assuming > that if the PID isn't found in its map, it should just use the binary filename. > But that assumes the caller has requested the data for the inferior, which is > not necessarily the case. The fix for that part does not work because of Bug 305385 I had to commit the attached patch to make things work again. (In reply to comment #7) > Created an attachment (id=166544) [details] > Temporarily revert second half of fix > > (In reply to comment #0) > > > The second problem is that GDBProcesses.getExecutionData() ends up assuming > > that if the PID isn't found in its map, it should just use the binary filename. > > But that assumes the caller has requested the data for the inferior, which is > > not necessarily the case. > > The fix for that part does not work because of Bug 305385 > > I had to commit the attached patch to make things work again. The inferior pid not being available is turning out to be quite a pain. There are two interrupt scenarios that are blocked with DSF-GDB because of it. And I don't like that this method you've reverted returns the project binary name no matter what unrecognized PID you give it. That can be pretty misleading. At least if it returns "unknown", it's not so bad. Can't the hack be moved higher up? (In reply to comment #8) > The inferior pid not being available is turning out to be quite a pain. There > are two interrupt scenarios that are blocked with DSF-GDB because of it. And I > don't like that this method you've reverted returns the project binary name no > matter what unrecognized PID you give it. That can be pretty misleading. At > least if it returns "unknown", it's not so bad. Can't the hack be moved higher > up? what do you mean by higher up? (In reply to comment #9) > (In reply to comment #8) > > > The inferior pid not being available is turning out to be quite a pain. There > > are two interrupt scenarios that are blocked with DSF-GDB because of it. And I > > don't like that this method you've reverted returns the project binary name no > > matter what unrecognized PID you give it. That can be pretty misleading. At > > least if it returns "unknown", it's not so bad. Can't the hack be moved higher > > up? > > what do you mean by higher up? The caller (or somewhere up the callstack). If he gets an unknown process name, and he knows its the inferior, he should use the binary name. Of course, we are talking about a hack, here. I just think the hack where it is now is dangerous. Again, it tripped me up. I was seeing three 'sleep' processes and scratching my head. If I had seen a sleep process and two "unknown" ones, I would have done less scratching :-) Created attachment 166771 [details] Maybe better temporary fix How about this fix until bug 305385 is resolved? I didn't commit it yet. (In reply to comment #11) > Created an attachment (id=166771) [details] > Maybe better temporary fix > > How about this fix until bug 305385 is resolved? > I didn't commit it yet. OK. I'm going to try to get to the bottom of this inferior PID thing this week if you don't have time. I know I punted last time but maybe I'll see things clearer this time around. (In reply to comment #11) > Created an attachment (id=166771) [details] > Maybe better temporary fix > > How about this fix until bug 305385 is resolved? > I didn't commit it yet. Mark, now that we have a patch for bug 305385, can you validate that this temporary fix is no longer necessary and back it out? (In reply to comment #13) > (In reply to comment #11) > > Created an attachment (id=166771) [details] [details] > > Maybe better temporary fix > > > > How about this fix until bug 305385 is resolved? > > I didn't commit it yet. > > Mark, now that we have a patch for bug 305385, can you validate that this > temporary fix is no longer necessary and back it out? We've had to pull out the fix for bug 305385 and it has been shelved due to low-value but high-risk. *** cdt cvs genie on behalf of jcortell *** Bug 311059: Process list shown during attach launch with gdb 6.8 has bogus entries [*] GDBProcesses.java 1.4 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.3&r2=1.4 *** cdt cvs genie on behalf of mkhouzam *** Bug 311059: workaround for another bug (305385) which is not fixed yet. [*] GDBProcesses.java 1.5 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.4&r2=1.5 |