From a8195048731655a422586856d1bc8801d1f0147e Mon Sep 17 00:00:00 2001 From: Jonah Graham Date: Tue, 21 Nov 2017 13:07:27 +0000 Subject: [PATCH] Bug 530377: Fix corrupt bp state & add test for fast bp events. This generally required adding RequestMonitors everywhere possible and then holding up processing future bp events until previous ones were finished. Change-Id: Icc641071249f7f8c619f0592e07772e47645c9db --- .../dsf/mi/service/MIBreakpointsManager.java | 58 +++- .../mi/service/MIBreakpointsSynchronizer.java | 283 +++++++++++++++--- .../gdb/tests/GDBConsoleBreakpointsTest.java | 36 ++- 3 files changed, 321 insertions(+), 56 deletions(-) diff --git a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsManager.java b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsManager.java index af5dcaeae10..adc37c12d70 100644 --- a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsManager.java +++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsManager.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2007, 2016 Wind River and others. + * Copyright (c) 2007, 2018 Wind River and others. * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at @@ -50,7 +50,6 @@ import org.eclipse.cdt.debug.internal.core.breakpoints.BreakpointProblems; import org.eclipse.cdt.dsf.concurrent.CountingRequestMonitor; import org.eclipse.cdt.dsf.concurrent.DataRequestMonitor; import org.eclipse.cdt.dsf.concurrent.DsfRunnable; -import org.eclipse.cdt.dsf.concurrent.ImmediateDataRequestMonitor; import org.eclipse.cdt.dsf.concurrent.ImmediateExecutor; import org.eclipse.cdt.dsf.concurrent.ImmediateRequestMonitor; import org.eclipse.cdt.dsf.concurrent.RequestMonitor; @@ -873,7 +872,6 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo // Update the mappings threadsIDs.remove(breakpoint); fPendingRequests.remove(breakpoint); - rm.done(); } }; @@ -1294,16 +1292,38 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo @ThreadSafe @Override public void breakpointAdded(final IBreakpoint breakpoint) { - breakpointAdded(breakpoint, null); + breakpointAdded(breakpoint, null, new RequestMonitor(getExecutor(), null)); } - - /** - * Extension of {@link #breakpointAdded(IBreakpoint)} - * @param miBpt the MIBreakpoint that initiated the breakpointAdded, or null - * @since 5.3 - */ - @ThreadSafe - public void breakpointAdded(final IBreakpoint breakpoint, MIBreakpoint miBpt) { + + /** + * Extension of {@link #breakpointAdded(IBreakpoint)} + * + * @param miBpt + * the MIBreakpoint that initiated the breakpointAdded, or null + * @since 5.3 + * @deprecated Use + * {@link #breakpointAdded(IBreakpoint, MIBreakpoint, RequestMonitor)} + * instead. See Bug 530377. + */ + @ThreadSafe + @Deprecated + public void breakpointAdded(final IBreakpoint breakpoint, MIBreakpoint miBpt) { + breakpointAdded(breakpoint, miBpt, new RequestMonitor(getExecutor(), null)); + } + + /** + * Extension of {@link #breakpointAdded(IBreakpoint)} that can be monitored for + * completeness with a {@link RequestMonitor}. + * + * @param breakpoint + * the added breakpoint + * @param miBpt + * the MIBreakpoint that initiated the breakpointAdded, or null + * @param rm + * @since 5.5 + */ + @ThreadSafe + public void breakpointAdded(final IBreakpoint breakpoint, MIBreakpoint miBpt, RequestMonitor rm) { if (supportsBreakpoint(breakpoint)) { try { // Retrieve the breakpoint attributes @@ -1315,7 +1335,7 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo // For a new breakpoint, the first thing we do is set the target filter to all existing processes. // We will need this when it is time to install the breakpoint. // We fetch the processes from our IProcess service to be generic (bug 431986) - fProcesses.getProcessesBeingDebugged(fConnection.getContext(), new ImmediateDataRequestMonitor() { + fProcesses.getProcessesBeingDebugged(fConnection.getContext(), new DataRequestMonitor(getExecutor(), rm) { @Override protected void handleCompleted() { if (isSuccess()) { @@ -1341,14 +1361,14 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo } // Now we can install the bp for all target contexts - final CountingRequestMonitor countingRm = new CountingRequestMonitor(getExecutor(), null) { + final CountingRequestMonitor countingRm = new CountingRequestMonitor(getExecutor(), rm) { @Override protected void handleCompleted() { // Log any error when creating the breakpoint if (getStatus().getSeverity() == IStatus.ERROR) { GdbPlugin.getDefault().getLog().log(getStatus()); } - + rm.done(); } }; countingRm.setDoneCount(fPlatformToAttributesMaps.size()); @@ -1368,15 +1388,20 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo countingRm.done(); } } - } }); } }); + + // Normal return case, rm handling passed to runnable + return; } catch (CoreException e) { } catch (RejectedExecutionException e) { } } + + // error/abnormal return case + rm.done(); } /** @@ -1454,6 +1479,7 @@ public class MIBreakpointsManager extends AbstractDsfService implements IBreakpo } } }); + } catch (CoreException e) { } catch (RejectedExecutionException e) { } diff --git a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsSynchronizer.java b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsSynchronizer.java index 576ab6131a6..c232b615c67 100644 --- a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsSynchronizer.java +++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsSynchronizer.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2012, 2016 Mentor Graphics and others. + * Copyright (c) 2012, 2018 Mentor Graphics and others. * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at @@ -11,6 +11,7 @@ * Marc Khouzam (Ericsson) - Update breakpoint handling for GDB >= 7.4 (Bug 389945) * Marc Khouzam (Ericsson) - Support for dynamic printf (Bug 400628) * Jonah Graham (Kichwa Coders) - Bug 317173 - cleanup warnings + * Jonah Graham (Kichwa Coders) - Bug 530377 - Corruption of state due to fast events from GDB *******************************************************************************/ package org.eclipse.cdt.dsf.mi.service; @@ -22,9 +23,11 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Hashtable; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Queue; import java.util.Set; import org.eclipse.cdt.core.IAddress; @@ -80,8 +83,22 @@ import org.eclipse.debug.core.sourcelookup.containers.LocalFileStorage; import org.osgi.framework.BundleContext; /** - * Provides synchronization between breakpoints set from outside of the Eclipse breakpoint - * framework (GDB console, trace files, etc.) and the Breakpoints view. + * Provides synchronization between breakpoints set from outside of the Eclipse + * breakpoint framework (GDB console, trace files, etc.) and the Breakpoints + * view. + *

+ * Bug 530377: Prior to fixing 530377, events that arrived from GDB faster than + * DSF/Eclipse fully processed them could cause the state within the + * synchronizer and manager to become corrupt. This would happen because it + * takes multiple DSF stages to complete handling 1 event, so the handling of + * the next event would become intermingled. That violated many assumptions in + * the code that the code run in the respective RequestMonitor would be on the + * same state. This is an unsuprising assumption based on the general idea of + * DSF as not requiring the normal synchronization primitives as everything is + * single-threaded. To resolve this problem, there is some code + * {@link #queueEvent(BreakpointEvent)} that ensures each event is fully + * processed before the next event starts processing. + * * @since 4.2 */ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMIBreakpointsTrackingListener { @@ -122,6 +139,32 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI */ private Map> fPendingModifications; + /** + * Class to store an event that needs to be performed by the synchronizer + * + * @see MIBreakpointsSynchronizer class documentation for design comments + */ + private static class BreakpointEvent { + MIBreakpoint created; + MIBreakpoint modified; + String deleted; + } + + /** + * List of events that are queued, waiting to be processed. + * + * @see MIBreakpointsSynchronizer class documentation for design comments + */ + private Queue fBreakpointEvents = new LinkedList<>(); + + /** + * True if the delayed events processing task is idle. If idle, a new event + * should trigger restarting the processing. + * + * @see MIBreakpointsSynchronizer class documentation for design comments + */ + private boolean fEventsIdle = true; + public MIBreakpointsSynchronizer(DsfSession session) { super(session); fTrackedTargets = new HashSet(); @@ -172,6 +215,7 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI fCreatedTargetBreakpoints.clear(); fDeletedTargetBreakpoints.clear(); fPendingModifications.clear(); + fBreakpointEvents.clear(); getSession().removeServiceEventListener(this); MIBreakpointsManager bm = getBreakpointsManager(); if (bm != null) { @@ -217,18 +261,68 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI return fBreakpointsManager; } + /** + * Queue (and potentially start processing) breakpoint events from GDB. + * + * @param event + * from GDB that needs to be processed once the synchronizer is idle + * and has completed the previous event. + */ + private void queueEvent(BreakpointEvent event) { + fBreakpointEvents.add(event); + if (fEventsIdle) { + runNextEvent(); + } + } + + private void runNextEvent() { + fEventsIdle = false; + BreakpointEvent event = fBreakpointEvents.poll(); + if (event == null) { + fEventsIdle = true; + return; + } + + RequestMonitor rm = new RequestMonitor(getExecutor(), null) { + @Override + protected void handleCompleted() { + runNextEvent(); + super.handleCompleted(); + } + }; + + if (event.created != null) { + doTargetBreakpointCreated(event.created, rm); + } else if (event.deleted != null) { + doTargetBreakpointDeleted(event.deleted, rm); + } else if (event.modified != null) { + doTargetBreakpointModified(event.modified, rm); + } else { + rm.done(); + } + } + public void targetBreakpointCreated(final MIBreakpoint miBpt) { + BreakpointEvent event = new BreakpointEvent(); + event.created = miBpt; + queueEvent(event); + } + + private void doTargetBreakpointCreated(final MIBreakpoint miBpt, RequestMonitor rm) { if (isCatchpoint(miBpt)) { + rm.done(); return; } MIBreakpoints breakpointsService = getBreakpointsService(); final MIBreakpointsManager bm = getBreakpointsManager(); if (breakpointsService == null || bm == null) { + rm.done(); return; } final IBreakpointsTargetDMContext bpTargetDMC = getBreakpointsTargetContext(miBpt); if (bpTargetDMC == null) { + rm.done(); return; } @@ -253,7 +347,7 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI getSource( bpTargetDMC, debuggerPath, - new DataRequestMonitor(getExecutor(), null) { + new DataRequestMonitor(getExecutor(), rm) { @Override @ConfinedToDsfExecutor( "fExecutor" ) protected void handleSuccess() { @@ -273,6 +367,9 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI if (isThreadSpecific) { setThreadSpecificBreakpoint(plBpt, miBpt); } + doTargetBreakpointCreatedSync(miBpt, bpTargetDMC, plBpt); + delayDone(100, rm); + return; } else { // The corresponding platform breakpoint already exists. @@ -286,37 +383,85 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI if (isThreadSpecific) { setThreadSpecificBreakpoint(plBpt, miBpt); } - bm.breakpointAdded(plBpt, miBpt); + + ICBreakpoint plBpt2 = plBpt; + bm.breakpointAdded(plBpt2, miBpt, new RequestMonitor(getExecutor(), rm) { + @Override + protected void handleCompleted() { + doTargetBreakpointCreatedSync(miBpt, bpTargetDMC, plBpt2); + rm.done(); + } + }); + return; + } else { + doTargetBreakpointCreatedSync(miBpt, bpTargetDMC, plBpt); + rm.done(); + return; } } - // Make sure the platform breakpoint's parameters are synchronized - // with the target breakpoint. - Map map = fPendingModifications.get(bpTargetDMC); - if (map != null) { - MIBreakpoint mod = map.remove(miBpt.getNumber()); - if (mod != null) { - targetBreakpointModified(bpTargetDMC, plBpt, mod); - } - } - else { - targetBreakpointModified(bpTargetDMC, plBpt, miBpt); - } } catch(CoreException e) { GdbPlugin.log(getStatus()); } - super.handleSuccess(); + rm.done(); + return; } + }); } + private void doTargetBreakpointCreatedSync(final MIBreakpoint miBpt, + final IBreakpointsTargetDMContext bpTargetDMC, ICBreakpoint plBpt) { + // Make sure the platform breakpoint's parameters are synchronized + // with the target breakpoint. + Map map = fPendingModifications.get(bpTargetDMC); + if (map != null) { + MIBreakpoint mod = map.remove(miBpt.getNumber()); + if (mod != null) { + targetBreakpointModified(bpTargetDMC, plBpt, mod); + } + } + else { + targetBreakpointModified(bpTargetDMC, plBpt, miBpt); + } + }; + + /** + * Some operations that are passed to platform require a number or delays before + * they complete. The reason is that the platform code will retrigger DSF code + * and continue updating state. Ideally there would be completion monitors for + * the platform operations, but that is not available. Use this method to delay + * calling .done() until at least delayExecutorCycles cycles of the executor + * have run. + * + * @param delayExecutorCycles + * @param rm + */ + private void delayDone(int delayExecutorCycles, RequestMonitor rm) { + getExecutor().execute(() -> { + int remaining = delayExecutorCycles - 1; + if (remaining < 0) { + rm.done(); + } else { + delayDone(remaining, rm); + } + }); + } + /** * @since 5.0 */ public void targetBreakpointDeleted(final String id) { + BreakpointEvent event = new BreakpointEvent(); + event.deleted = id; + queueEvent(event); + } + + private void doTargetBreakpointDeleted(final String id, RequestMonitor rm) { MIBreakpoints breakpointsService = getBreakpointsService(); final MIBreakpointsManager bm = getBreakpointsManager(); if (breakpointsService == null || bm == null) { + rm.done(); return; } final IBreakpointsTargetDMContext bpTargetDMC = breakpointsService.getBreakpointTargetContext(id); @@ -325,15 +470,17 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI new MIBreakpointDMContext(breakpointsService, new IDMContext[] { bpTargetDMC }, id); breakpointsService.getBreakpointDMData( bpDMC, - new DataRequestMonitor(getExecutor(), null) { + new DataRequestMonitor(getExecutor(), rm) { @Override @ConfinedToDsfExecutor( "fExecutor" ) protected void handleSuccess() { if (!(getData() instanceof MIBreakpointDMData)) { + rm.done(); return; } MIBreakpointDMData data = (MIBreakpointDMData)getData(); if (MIBreakpoints.CATCHPOINT.equals(data.getBreakpointType())) { + rm.done(); return; } @@ -353,11 +500,13 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI IMIProcesses processes = getServicesTracker().getService(IMIProcesses.class); if (processes == null) { + rm.done(); return; } IContainerDMContext contDMC = processes.createContainerContextFromThreadId(getCommandControl().getContext(), data.getThreadId()); if (contDMC == null) { + rm.done(); return; } @@ -371,59 +520,117 @@ public class MIBreakpointsSynchronizer extends AbstractDsfService implements IMI } if (!list.isEmpty()) { bpExtension.setThreadFilters(list.toArray(new IExecutionDMContext[list.size()])); + rm.done(); + return; } else { - bm.uninstallBreakpoint(bpTargetDMC, (ICBreakpoint)plBpt, new RequestMonitor(getExecutor(), null)); + bm.uninstallBreakpoint(bpTargetDMC, (ICBreakpoint)plBpt, new RequestMonitor(getExecutor(), rm)); + return; } } else { - bm.uninstallBreakpoint(bpTargetDMC, (ICBreakpoint)plBpt, new RequestMonitor(getExecutor(), null)); + bm.uninstallBreakpoint(bpTargetDMC, (ICBreakpoint)plBpt, new RequestMonitor(getExecutor(), rm)); + return; } } catch(CoreException e) { GdbPlugin.log(e.getStatus()); } } + rm.done(); } }); + } else { + rm.done(); } } public void targetBreakpointModified(final MIBreakpoint miBpt) { - if (isCatchpoint(miBpt)) { - return; - } + BreakpointEvent event = new BreakpointEvent(); + event.modified = miBpt; + queueEvent(event); + } + + /** + * Find the platform breakpoint, returning it, if it exists via the DRM. If the + * drm's data is null, it has not been found. + */ + private void findPlatformBreakpoint(final MIBreakpoint miBpt, DataRequestMonitor drm) { MIBreakpoints breakpointsService = getBreakpointsService(); final MIBreakpointsManager bm = getBreakpointsManager(); if (breakpointsService != null && bm != null) { final IBreakpointsTargetDMContext bpTargetDMC = getBreakpointsTargetContext(miBpt); if (bpTargetDMC == null) { + drm.done((IBreakpoint)null); return; } final Map contextBreakpoints = breakpointsService.getBreakpointMap(bpTargetDMC); if (contextBreakpoints == null) { + drm.done((IBreakpoint)null); return; } IBreakpoint b = bm.findPlatformBreakpoint( - new MIBreakpointDMContext(breakpointsService, new IDMContext[] { bpTargetDMC }, miBpt.getNumber())); - if (!(b instanceof ICBreakpoint)) { - // Platform breakpoint hasn't been created yet. Store the latest - // modification data, it will be picked up later. - Map map = fPendingModifications.get(bpTargetDMC); - if (map == null) { - map = new HashMap(); - fPendingModifications.put(bpTargetDMC, map); - } - map.put(miBpt.getNumber(), miBpt); - } - else { - ICBreakpoint plBpt = (ICBreakpoint)b; - targetBreakpointModified(bpTargetDMC, plBpt, miBpt); + new MIBreakpointDMContext(breakpointsService, new IDMContext[] { bpTargetDMC }, miBpt.getNumber())); + if (b != null) { + drm.done(b); + } else { + // Convert the debug info file path into the file path in the local file system + String debuggerPath = getFileName(miBpt); + getSource(bpTargetDMC, debuggerPath, new DataRequestMonitor(getExecutor(), drm) { + @Override + @ConfinedToDsfExecutor("fExecutor") + protected void handleSuccess() { + String fileName = getData(); + if (fileName == null) { + fileName = getFileName(miBpt); + } + // Try to find matching platform breakpoint + ICBreakpoint plBpt = getPlatformBreakpoint(miBpt, fileName); + drm.done(plBpt); + } + }); } + } else { + drm.done((ICBreakpoint) null); } } + private void doTargetBreakpointModified(final MIBreakpoint miBpt, RequestMonitor rm) { + if (isCatchpoint(miBpt)) { + rm.done(); + return; + } + + findPlatformBreakpoint(miBpt, new DataRequestMonitor(getExecutor(), rm) { + @Override + protected void handleSuccess() { + IBreakpointsTargetDMContext bpTargetDMC = getBreakpointsTargetContext(miBpt); + if (bpTargetDMC == null) { + rm.done(); + return; + } + + IBreakpoint breakpoint = getData(); + if (!(breakpoint instanceof ICBreakpoint)) { + // Platform breakpoint hasn't been created yet. Store the latest + // modification data, it will be picked up later. + Map map = fPendingModifications.get(bpTargetDMC); + if (map == null) { + map = new HashMap(); + fPendingModifications.put(bpTargetDMC, map); + } + map.put(miBpt.getNumber(), miBpt); + rm.done(); + } else { + ICBreakpoint plBpt = (ICBreakpoint) breakpoint; + targetBreakpointModified(bpTargetDMC, plBpt, miBpt); + delayDone(100, rm); + } + } + }); + } + private void targetBreakpointModified( IBreakpointsTargetDMContext bpTargetDMC, ICBreakpoint plBpt, diff --git a/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/GDBConsoleBreakpointsTest.java b/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/GDBConsoleBreakpointsTest.java index ed0b738ee9e..785a7a72ded 100644 --- a/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/GDBConsoleBreakpointsTest.java +++ b/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/GDBConsoleBreakpointsTest.java @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2012, 2016 Mentor Graphics and others. + * Copyright (c) 2012, 2018 Mentor Graphics and others. * All rights reserved. This program and the accompanying materials * are made available under the terms of the Eclipse Public License v1.0 * which accompanies this distribution, and is available at @@ -13,6 +13,7 @@ package org.eclipse.cdt.tests.dsf.gdb.tests; import static org.junit.Assert.assertEquals; +import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -446,7 +447,38 @@ public class GDBConsoleBreakpointsTest extends BaseParametrizedTestCase { waitForBreakpointEvent(IBreakpointsRemovedEvent.class); assertEquals(0, getTargetBreakpoints().length); } - + + /** + * Bug 530377 + */ + @Test + public void testFastEvents() throws Throwable { + List bps = getPlatformFunctionBreakpoints(); + assertEquals(0, bps.size()); + + java.nio.file.Path tempFile = Files.createTempFile("testFastEvents", "gdb"); + try { + + StringBuilder sb = new StringBuilder(); + for (int bpId = 2; bpId < 1000; bpId++) { + sb.append(String.format("break %s\n", FUNCTION_VALID)); + sb.append(String.format("delete %s\n", bpId)); + } + Files.write(tempFile, sb.toString().getBytes("UTF-8")); + queueConsoleCommand("source " + tempFile.toString()); + } finally { + Files.delete(tempFile); + } + + bps = getPlatformFunctionBreakpoints(); + assertEquals(1, bps.size()); + + IBreakpoint breakpoint = bps.get(0); + + CBreakpoint cBreakpoint = ((CBreakpoint) breakpoint); + waitForInstallCountChange(cBreakpoint, 0); + breakpoint.delete(); + } @DsfServiceEventHandler public void eventDispatched(IBreakpointsChangedEvent e) {