Bugzilla – Full Text Bug Listing
| Summary: | [mac] Need infrastructure to properly support Mac's version of GDB for DSF-GDB | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||
| Priority: | P3 | CC: | cdtdoug, ken.ryall, malaperle, mike.jackson, pawel.1.piech, tobias.hahn | ||||||||||||||||
| Version: | 6.0 | Flags: | pawel.1.piech:
review+
|
||||||||||||||||
| Target Milestone: | 7.0 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 250037 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Marc Khouzam
Created attachment 155504 [details] Mac OS service infrastructure This patch adds proper parsing of Apple's GDB version number to DSF-GDB. Based on that version, different services are instantiated tailored for Mac OS. As a proof of concept, I've also included a fix for bug 250037. To avoid duplicating too much code right away, I've only created services needed for the first bug fix (bug 250037). So, I've copied MIExpressions and MIVariableManager. I've also created MacOSMIVarUpdate and MacOSMIVarUpdateInfo, specific for Mac. Unlike CDI, we don't use a command factory, instead, we modify our specific service to use those new commands. So, the new MacOSGDBVariableManager uses those new classes. Note that I moved away from using different services for GDB and for generic MI. Pawel mentioned he kind of regretted that decision. So, Mac services will always be under org.eclipse.cdt.dsf.gdb.service.macos (there will not be a org.eclipse.cdt.dsf.mi.service.macos) Can someone with a Mac confirm that this patch fixes bug 250037, which will confirm that this new infrastructure works for Mac OS. Oh, I should mention that our approach to using different service version does create a *lot* of code duplication. We made this decision early on to favor stability. It requires a little more work when fixing bugs (each version must be fixed), but it gives complete independence between the versions, which keeps things more stable. Pawel, can you glance over this patch? (In reply to comment #1) > Can someone with a Mac confirm that this patch fixes bug 250037, which will > confirm that this new infrastructure works for Mac OS. I'd like to test this but I think I am getting confused about "how" to test. I have a C++ project that I am using but which "debugger" should I be using? I have "GDB Debugger, GDB/MI, gdbdebugger server". There seem to be 2 choices in the "GDB Command set" (Standard and Standard (Mac OS) ). And the Protocol drop downs have "mi, mi1, mi2". So out of all of that what should I be selecting to start testing this new infrastructure code. Thanks for all for working on this. (In reply to comment #1) > Can someone with a Mac confirm that this patch fixes bug 250037, which will > confirm that this new infrastructure works for Mac OS. It works for me on 10.6.2 with gdb-1346. (In reply to comment #3) > I'd like to test this but I think I am getting confused about "how" to test. You have to change the launcher. Open your debug configuration, in the main tab, at the bottom you can click on Select other and select GDB (DSF). Ah. thanks for the help. I switched everything over to DSF launcher and what I noticed was that the launch was quick (bug 294538), threads are properly shown (269838) and values seem to update correctly (bug 250037). Note that I have applied all the patches from all the bugs and hopefully I have setup things correctly so that I am not giving false positives. Is it possible to build without the deprecated CDI stuff? (In reply to comment #5) > Ah. thanks for the help. I switched everything over to DSF launcher and what I > noticed was that the launch was quick (bug 294538), threads are properly shown > (269838) and values seem to update correctly (bug 250037). Note that I have > applied all the patches from all the bugs and hopefully I have setup things > correctly so that I am not giving false positives. Is it possible to build > without the deprecated CDI stuff? I only have this patch applied and I get similar results. Threads are properly shown but the current thread is not selected because info threads is used so bug 269838 will also need a fix for DSF. Bug 265483 is also present in DSF. At least now it will be easier to make patches. Thanks Marc! Created attachment 155544 [details] Patch with extension APIs in MI expressions service. (In reply to comment #1) > Created an attachment (id=155504) [details] > Mac OS service infrastructure The changes make sense to me. The difficult part is how to override the services without so much code duplication. This patch adds a little bit of API to the variable manager to avoid overriding so much of it. On the other hand it would further restrict what kind of changes could be done to the variable manager without breaking breaking API compatibility. Created attachment 155563 [details] Improved API extensions for MIExpressions (In reply to comment #7) > The changes make sense to me. The difficult part is how to override the > services without so much code duplication. This patch adds a little bit of API > to the variable manager to avoid overriding so much of it. On the other hand > it would further restrict what kind of changes could be done to the variable > manager without breaking breaking API compatibility. Thanks for the patch. After a quick exchange with Pawel, he convinced me that the MIExpressions service is stable enough that it would be worth extending it instead of copying it. So, I took Pawel's patch and fixed a couple of things to complete what he was pointing at. Pawel, how does it look to you? Basically, the infrastructure to support Mac OS stays the same, but we don't copy the Expressions service, we extend it instead. The same could be done for other services in the future. Marc-Andre and/or Mike, can you try out this patch to see that the -var-update problem of bug 250037 is still fixed with this change. Created attachment 155564 [details]
Proper Improved API extensions for MIExpressions
Sorry, the previous patch had some test code left in it.
This patch is the right one.
Created attachment 155570 [details] Proper Improved API extensions for MIExpressions with separate factory (In reply to comment #8) > Marc-Andre and/or Mike, can you try out this patch to see that the -var-update > problem of bug 250037 is still fixed with this change. This patch still works for me. I made a new patch from this one with a separate factory for Mac OS. What do you think? Created attachment 155584 [details] Tweaked separate factory patch (In reply to comment #10) > I made a new patch from this one with a separate > factory for Mac OS. What do you think? I like it a lot! The problem though is that if someone adds a new service to the base GdbDebugServicesFactory, it will automatically get added to MacOS. This is normally a good thing, but in our case, it is a problem. For example, I will soon add: @Override protected IBreakpoints createTraceControlService(DsfSession session) { if (GDB_7_0_VERSION.compareTo(fVersion) <= 0) { return new GDBTraceControl_7_0(session); } return null; } The problem is that the Apple GDB version will be compared with GDB_7_0_VERSION and may make it look like the Apple GDB has the GDB 7.0 feature (e.g., if the version is 969, it will trigger the 7.0 code!). So, we would always have to update the MacOSGdbDebugServicesFactory class to make sure this does not happen, which someone is bound to forget to do. But I really liked your idea still... So, if we realize that the real problem is that we are comparing apples with oranges, meaning the FSF GDB version with the Apple versioning scheme, then we can fix this. The patch I attached does this. It also allowed me to only override the creation of the Expressions service for MacOS. Marc-Andre, let me know what you think, and if it still works for Mac. Thanks. (In reply to comment #8) > Pawel, how does it look to you? +1 (In reply to comment #11) > Created an attachment (id=155584) [details] > Tweaked separate factory patch > > (In reply to comment #10) > > I made a new patch from this one with a separate > > factory for Mac OS. What do you think? > > I like it a lot! > > The problem though is that if someone adds a new service to the base > GdbDebugServicesFactory, it will automatically get added to MacOS. This is > normally a good thing, but in our case, it is a problem. For example, I will > soon add: > > @Override > protected IBreakpoints createTraceControlService(DsfSession session) { > if (GDB_7_0_VERSION.compareTo(fVersion) <= 0) { > return new GDBTraceControl_7_0(session); > } > return null; > } > > The problem is that the Apple GDB version will be compared with GDB_7_0_VERSION > and may make it look like the Apple GDB has the GDB 7.0 feature (e.g., if the > version is 969, it will trigger the 7.0 code!). So, we would always have to > update the MacOSGdbDebugServicesFactory class to make sure this does not > happen, which someone is bound to forget to do. I agree. > Marc-Andre, let me know what you think, and if it still works for Mac. Thanks. It still works. This path looks good. If Apple ever updates the standard gdb version and adds functionalities, we'll just check for the corresponding Apple version and return the proper services. I think this is ready to go. (In reply to comment #13) > It still works. This path looks good. If Apple ever updates the standard gdb > version and adds functionalities, we'll just check for the corresponding Apple > version and return the proper services. Sharp. I knew someone was going to bring that up ;-) I hardcoded the 6.3.50.... version because it seemed to not be going to change any time soon, so I was too lazy/busy to spend time parsing it. But, the day it does change, we can parse it in LaunchUtils and pass it to the MacOSGdbDebugServicesFactory > I think this is ready to go. I'll try to get to it over the weekend Created attachment 155648 [details] Patch using both version numbers (In reply to comment #14) > I hardcoded the 6.3.50.... version because it seemed to not be going to change > any time soon, so I was too lazy/busy to spend time parsing it. But, the day > it does change, we can parse it in LaunchUtils and pass it to the > MacOSGdbDebugServicesFactory I'm currently neither lazy nor busy so here's a new patch :-) (In reply to comment #15) > Created an attachment (id=155648) [details] > Patch using both version numbers > > (In reply to comment #14) > > I hardcoded the 6.3.50.... version because it seemed to not be going to change > > any time soon, so I was too lazy/busy to spend time parsing it. But, the day > > it does change, we can parse it in LaunchUtils and pass it to the > > MacOSGdbDebugServicesFactory > > I'm currently neither lazy nor busy so here's a new patch :-) Thanks! I've committed this patch as is, except for changing MACOS_GDB_PREFIX to MACOS_GDB_MARKER |