Community
Participate
Working Groups
java.lang.NullPointerException at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager$24$1.run(MIBreakpointsManager.java:1460) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2344) at org.eclipse.cdt.dsf.mi.service.MIBreakpointsManager$24.run(MIBreakpointsManager.java:1502) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) This happens because 'clearBreakpointStatus()' is called after 'stopTrackingBreakpoints()' which clears 'fBreakpointIDs' map. I think a simple null check in 'clearBreakpointStatus()' would be sufficient. If that's OK I'll submit a patch.
(In reply to comment #0) > This happens because 'clearBreakpointStatus()' is called after > 'stopTrackingBreakpoints()' which clears 'fBreakpointIDs' map. I think a simple > null check in 'clearBreakpointStatus()' would be sufficient. > If that's OK I'll submit a patch. I was wondering why this wasn't happening all the time, but I think I figured it out. clearBreakpointStatus() is called for every context in fPlatformBPs, but stopTrackingBreakpoints() clears fPlatformBPs also when it clears fBreakpointIDs, so we shouldn't expect an NPE, except that.... Maybe this is a race condition because clearBreakpointStatus() uses a Job? 1- clearBreakpointStatus() is called, and prepares the Job 2- stopTrackingBreakpoints() gets called and clears fBreakpointIDs 3- the clearBreakpointStatus() Job executes and finds fBreakpointIDs.get() null Makes sense? With that in mind, I believe your fix makes perfect sense.
*** Bug 349283 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > Maybe this is a race condition because clearBreakpointStatus() uses a Job? > 1- clearBreakpointStatus() is called, and prepares the Job > 2- stopTrackingBreakpoints() gets called and clears fBreakpointIDs > 3- the clearBreakpointStatus() Job executes and finds fBreakpointIDs.get() null I think we have a deeper problem in the fact that we access non-synchronized maps from two different threads. I believe fBreakpointIDs is definitely a problem, while fBreakpointActionManager and fBreakpointMarkerProblems are potentially a risk
(In reply to comment #3) > I think we have a deeper problem in the fact that we access non-synchronized > maps from two different threads. > > I believe fBreakpointIDs is definitely a problem, while > fBreakpointActionManager and fBreakpointMarkerProblems are potentially a risk I didn't realize clearBreakpointStatus() is run in a regular job. Can we move it to the executor thread?
(In reply to comment #4) > (In reply to comment #3) > > I think we have a deeper problem in the fact that we access non-synchronized > > maps from two different threads. > > > > I believe fBreakpointIDs is definitely a problem, while > > fBreakpointActionManager and fBreakpointMarkerProblems are potentially a risk > > I didn't realize clearBreakpointStatus() is run in a regular job. Can we move > it to the executor thread? I think the Job is there because it is needed to use IMarker (although I don't know why). We also use a job in addBreakpointProblemMarker() and removeBreakpointProblemMarker(), so I guess it is needed. clearBreakpointStatus() used to do the same, but I had to add a fix to support multiple launches and that is when I started to use fBreakpointIDs, and I missed the synchronization. I think you could have the code using IWorkspaceRunnable use a DsfRunnable and run in the DSF executor instead.
Created attachment 204473 [details] Proposed fix. Run the workspace operation in the executor thread.
Marc, forcing to run the workspace operation in the executor thread seems to work fine. I don't see why it shouldn't work.
(In reply to comment #7) > Marc, forcing to run the workspace operation in the executor thread seems to > work fine. I don't see why it shouldn't work. I'm copying Francois and will talk with him to see if he remembers why the Job is being used. If the change is safe, I guess you don't even need to put it in the ImmediateExecutor, as clearBreakpointStatus() is being called on the Executor already. I'm worried that using the Job was specifically to avoid using the DSF Executor... I'll see with Francois.
(In reply to comment #8) > I'm worried that using the Job was specifically to avoid using the DSF > Executor... I'll see with Francois. It has been a very long time so Francois wasn't sure. He said it might have been because he wanted to avoid the file system access of the IMarker to slow down the Executor work. We think it is safe to run that code in the DSF Executor. If we're wrong, we should see it before it is too late :-)
Is there any interest to migrate DSF-GDB to the BreakpointMediator (Bug 219604)? I'm curious as I'll be adding additional features to breakpoint status reporting.
(In reply to comment #10) > Is there any interest to migrate DSF-GDB to the BreakpointMediator (Bug > 219604)? I'm curious as I'll be adding additional features to breakpoint > status reporting. Bug 219604 (as I'm sure you know). Many features and fixes have gone into the MIBreakpointsManager since the BreakpointMediator2 was 'forked' from it. Doing this migration will require to go over all the commits to the MIBreakpointsManager since then and push them to the mediator. This has value in itself for other people using the mediator. However, it was too much effort for me to invest for what seemed like a very small gain towards DSF-GDB. I'm all for it though, if someone finds it is worth their time.
(In reply to comment #9) > We think it is safe to run that code in the DSF Executor. If we're wrong, we > should see it before it is too late :-) Hi Mikhail, will you be able to commit this fix soon? I'm doing an internal release for Ericsson this week and I would like to include this fix. If you don't have time, do you mind if I commit your patch myself (to master and cdt_8_0)? Thanks
(In reply to comment #12) > > Hi Mikhail, > > will you be able to commit this fix soon? I'm doing an internal release for > Ericsson this week and I would like to include this fix. > > If you don't have time, do you mind if I commit your patch myself (to master > and cdt_8_0)? > > Thanks Marc, can you wait until tomorrow? I'll try to submit a new patch today.
(In reply to comment #13) > Marc, can you wait until tomorrow? I'll try to submit a new patch today. No problem. Thank you
Created attachment 205553 [details] Proposed fix. This patch separates the problem marker related operations and runs them in a separate job. Decrement install count is run in the executor thread.
Marc, can you please review this patch. If you find that it is OK please apply it. I left the problem marker related operations in a job and moved "decrementInstallCount" into the executor thread. I haven't been able to test problem markers though - not sure if it is working. I tried to switch off pending breakpoints but didn't see problem markers at all.
(In reply to comment #16) > Marc, can you please review this patch. If you find that it is OK please apply > it. > I left the problem marker related operations in a job and moved > "decrementInstallCount" into the executor thread. > I haven't been able to test problem markers though - not sure if it is working. > I tried to switch off pending breakpoints but didn't see problem markers at > all. Looks good to me. I'll run some test and try problem markers too. If all goes well I'll commit it. Thanks!
To allow us to see breakpoint problem markers easily I usually modify MIBreakInsert.java like this: @@ -86,6 +86,8 @@ boolean allowPending) { super(ctx, "-break-insert"); //$NON-NLS-1$ + allowPending = false; + // For a tracepoint, force certain parameters to what is allowed if (isTracepoint) { // A tracepoint cannot be temporary
(In reply to comment #18) > To allow us to see breakpoint problem markers easily I usually modify > MIBreakInsert.java like this: > > @@ -86,6 +86,8 @@ > boolean allowPending) { > super(ctx, "-break-insert"); //$NON-NLS-1$ > > + allowPending = false; > + > // For a tracepoint, force certain parameters to what is > allowed > if (isTracepoint) { > // A tracepoint cannot be temporary I changed this flag in the debugger to false but for some reason it didn't work. No problem marker appeared. I just don't have time now to investigate why. BTW, I noticed another issue. 'MIBreakpointsManager.terminated()' clears fPlatformBPs, but leaves fTargetBPs untouched. This causes an error in 'stopTrackingBreakpoints()'. It doesn't affect anything because the error is swallowed in 'GDBProcesses_7_2.eventDispatched(IExitedEvent)'.
(In reply to comment #19) > (In reply to comment #18) > BTW, I noticed another issue. 'MIBreakpointsManager.terminated()' clears > fPlatformBPs, but leaves fTargetBPs untouched. This causes an error in > 'stopTrackingBreakpoints()'. It doesn't affect anything because the error is > swallowed in 'GDBProcesses_7_2.eventDispatched(IExitedEvent)'. I'm not sure what error you are seeing, but it may be one I also saw before. In 'GDBProcesses_7_2.eventDispatched(IExitedEvent)', there is a comment that says: // Ok, no need to report any error because we may have already shutdown. // We need to override handleCompleted to avoid risking having a error // printout in the log which is where 'GDBProcesses_7_2.eventDispatched(IExitedEvent)' swallows errors. The reason is that if we shutdown, we can still get an IExitedDMEvent for the process and we'll try to remove breakpoints, but the ICommandControlShutdownDMEvent that we already got cleared fPlatformBPs as you mentioned. In that case, no need to remove breakpoints again, so the error was not valid. I didn't find a nice way to figure out we were in that situation, which is why I just let the error be swallowed (not much we could have done anyway).
I committed Mikhail's patch to both master and cdt_8_0. Thanks Mikhail!
*** cdt git genie on behalf of Marc Khouzam *** Bug 358754: NPE in MIBreakpointsManager [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=bc856eeea5660db81b3ed19117320282f24ba01b
*** cdt git genie on behalf of Marc Khouzam *** Bug 358754: NPE in MIBreakpointsManager [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=2abc834a65c2dbb8e7d400a57c7476bd5632a575
