Community
Participate
Working Groups
This bug affects GdbLaunchDelegate and its base class AbstractCLaunchDelegate2, but the problem applies to our legacy launch delegates also and AbstractCLaunchDelegate Mikhail pointed out in review https://git.eclipse.org/r/#/c/38679/3/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/GdbLaunchDelegate.java : "There is a problem with storing launch related data in the launch delegate. There is only one instance of a launch delegate is created for an Eclipse session. If two launches start in parallel their data will be mixed up. The only reason we haven't seen this issue yet is because we never start two launchers in parallel." We could possibly trigger such a bug when using a LaunchGroup which would launch multiple GdbLaunchDelegate at the same time.
We may not be able to overcome this easily. Launch delegate API does not always pass launch object around which makes it extremly difficult to deal with, since GdbLaunchDelegate is probably an API class on its own we may not be able even to change its methods to pass around an additional object... This situation will be exterimly rare. You need exact same delegate, and launch two diffrent LC at the same time without waiting (And yes you can do it with launch group).
I've posted a patch that removes the one field in GdbLaunchDelegate. I wanted to take advantage of CDT 9.0 to clean that up. It was not trivial, but I believe I got a working solution. The patch breaks the API as expected but I think it is worth it for the sake of correctness.
I've realized that we are safe to use final class fields for our launch delegates. Final fields are specified by the constructor and remain constant for the life of the class, so they are not affected by the issue of parallel launches. We actually use such a field in AbstractCLaunchDelegate2.requireCProject to allow different overriding launch delegates to behave differently. Since each of these overriding delegates has its own instance, this field is safe. I'll make it final for completeness.
New Gerrit change created: https://git.eclipse.org/r/66921
Gerrit change https://git.eclipse.org/r/66800 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=ffffd118900317f1b1c59d955ce7fa2fd4816695
Gerrit change https://git.eclipse.org/r/66921 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=99465c1604a0d9010a310441d830bfbb4b6cfb98
I really appreciate the efforts cleaning this up, but the change breaks the TCF/TE Launcher for CDT, see https://git.eclipse.org/r/#/c/68276/1 We really need TCF to be compatible with both CDT 9.0 and 8.x from single source. Could GdbLaunchDelegate offer a cleanupLaunch() method that can be called like in 8.x (for backward compatibility) while still offering the new feature of passing the Launch around ?
New Gerrit change created: https://git.eclipse.org/r/68295
(In reply to Martin Oberhuber from comment #7) > I really appreciate the efforts cleaning this up, but the change breaks the > TCF/TE Launcher for CDT, see https://git.eclipse.org/r/#/c/68276/1 > > We really need TCF to be compatible with both CDT 9.0 and 8.x from single > source. > > Could GdbLaunchDelegate offer a cleanupLaunch() method that can be called > like in 8.x (for backward compatibility) while still offering the new > feature of passing the Launch around ? The change in GdbLaunchDelegate needs to be resolved in either CDT side or TCF side. To this end I provide two possible implementations. 1) Add cleanupLaunch() back in to CDT, but deprecated and never called by CDT. This is https://git.eclipse.org/r/68295 2) Copy cleanupLaunch into TCF, but deprecate it until TCF no longer wants to support CDT < 9.0 at which point it should be removed and replaced with the new API in CDT. This is https://git.eclipse.org/r/68276 Someone needs to decide which one of these options is the best fit going forward.
> Someone needs to decide which one of these options is the best fit going > forward. It was decided a while ago to go with option 2: > 2) Copy cleanupLaunch into TCF, but deprecate it until TCF no longer wants > to support CDT < 9.0 at which point it should be removed and replaced with > the new API in CDT. This is https://git.eclipse.org/r/68276
I added an additional entry to the API changes in N&N for this change: # '''GdbLaunchDelegate.getLaunch()''' no longer initializes the launch. Initializing the launch needs to be done in '''launchDebugSession''' if overridden by calling '''launch.initialize()'''. It is an interesting one because this is a case where API tooling does not tell you anything. But if you override launchDebugSession but not getLaunch the the launch in CDT 8.x was initialized but in CDT 9.x is not initialized.
(In reply to Jonah Graham from comment #11) > It is an interesting one because this is a case where API tooling does not > tell you anything. But if you override launchDebugSession but not getLaunch > the the launch in CDT 8.x was initialized but in CDT 9.x is not initialized. These changes probably happen more often that they should. They are hard to track as you mention. They need to be caught in code reviews. That being said, if we considered code behaviour as API, it would make it very hard to fix some bugs. I personally have always worried about such changes, but no one has spoken out against them, so I've grown hopeful that extenders do sufficient testing to catch major behavioural changes such as this one. Thanks for documenting, it will help.
