From a0163ff2f6b01e9fb0002dec9917eaf2fa6e7a79 Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Thu, 8 Apr 2021 13:31:51 -0400 Subject: [PATCH] GP-0: Fixed DefaultDebuggerObjectModelTest --- .../java/ghidra/dbg/DebuggerObjectModel.java | 1 + .../agent/AbstractDebuggerObjectModel.java | 89 ++++++++++++------- .../ghidra/dbg/agent/DefaultTargetObject.java | 8 +- .../agent/DefaultDebuggerObjectModelTest.java | 83 +++++++++++------ 4 files changed, 116 insertions(+), 65 deletions(-) diff --git a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/DebuggerObjectModel.java b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/DebuggerObjectModel.java index a287c57227..b8806d47b5 100644 --- a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/DebuggerObjectModel.java +++ b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/DebuggerObjectModel.java @@ -471,6 +471,7 @@ public interface DebuggerObjectModel { /** * @see #fetchModelObject(List) */ + @Deprecated public default CompletableFuture fetchModelObject(String... path) { return fetchModelObject(List.of(path)); } diff --git a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/AbstractDebuggerObjectModel.java b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/AbstractDebuggerObjectModel.java index 99720da2b0..f71d9679fd 100644 --- a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/AbstractDebuggerObjectModel.java +++ b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/AbstractDebuggerObjectModel.java @@ -31,6 +31,7 @@ import ghidra.util.datastruct.ListenerSet; public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectModel { public final Object lock = new Object(); + public final Object cbLock = new Object(); protected final ExecutorService clientExecutor = Executors.newSingleThreadExecutor(new BasicThreadFactory.Builder() .namingPattern(getClass().getSimpleName() + "-thread-%d") @@ -40,10 +41,12 @@ public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectMo protected SpiTargetObject root; protected boolean rootAdded; + protected boolean cbRootAdded; protected CompletableFuture completedRoot = new CompletableFuture<>(); // Remember the order of creation events protected final Map, SpiTargetObject> creationLog = new LinkedHashMap<>(); + protected final Map, SpiTargetObject> cbCreationLog = new LinkedHashMap<>(); protected void objectCreated(SpiTargetObject object) { synchronized (lock) { @@ -55,6 +58,14 @@ public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectMo this.root = object; } } + CompletableFuture.runAsync(() -> { + synchronized (cbLock) { + cbCreationLog.put(object.getPath(), object); + } + }, clientExecutor).exceptionally(ex -> { + Msg.error(this, "Error updating objectCreated before callback"); + return null; + }); } protected void objectInvalidated(TargetObject object) { @@ -65,11 +76,20 @@ public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectMo assert root == this.root; synchronized (lock) { rootAdded = true; - root.getSchema() - .validateTypeAndInterfaces(root, null, null, root.enforcesStrictSchema()); - this.completedRoot.completeAsync(() -> root, clientExecutor); - listeners.fire.rootAdded(root); } + root.getSchema() + .validateTypeAndInterfaces(root, null, null, root.enforcesStrictSchema()); + CompletableFuture.runAsync(() -> { + synchronized (cbLock) { + cbRootAdded = true; + } + completedRoot.complete(root); + }, clientExecutor).exceptionally(ex -> { + Msg.error(this, "Error updating rootAdded before callback"); + return null; + }); + this.completedRoot.completeAsync(() -> root, clientExecutor); + listeners.fire.rootAdded(root); } @Override @@ -84,27 +104,25 @@ public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectMo } } - protected void onClientExecutor(DebuggerModelListener listener, Runnable r) { - CompletableFuture.runAsync(r, clientExecutor).exceptionally(t -> { + protected void replayed(DebuggerModelListener listener, Runnable r) { + try { + r.run(); + } + catch (Throwable t) { Msg.error(this, "Listener " + listener + " caused unexpected exception", t); - return null; - }); + } } protected void replayTreeEvents(DebuggerModelListener listener) { - if (root == null) { - assert creationLog.isEmpty(); - return; - } - for (SpiTargetObject object : creationLog.values()) { - onClientExecutor(listener, () -> listener.created(object)); + for (SpiTargetObject object : cbCreationLog.values()) { + replayed(listener, () -> listener.created(object)); } Set visited = new HashSet<>(); - for (SpiTargetObject object : creationLog.values()) { + for (SpiTargetObject object : cbCreationLog.values()) { replayAddEvents(listener, object, visited); } - if (rootAdded) { - onClientExecutor(listener, () -> listener.rootAdded(root)); + if (cbRootAdded) { + replayed(listener, () -> listener.rootAdded(root)); } } @@ -113,41 +131,44 @@ public abstract class AbstractDebuggerObjectModel implements SpiDebuggerObjectMo if (!visited.add(object)) { return; } - Map cachedAttributes = object.getCachedAttributes(); - for (Object val : cachedAttributes.values()) { + Map cbAttributes = object.getCallbackAttributes(); + for (Object val : cbAttributes.values()) { if (!(val instanceof TargetObject)) { continue; } assert val instanceof SpiTargetObject; replayAddEvents(listener, (SpiTargetObject) val, visited); } - if (!cachedAttributes.isEmpty()) { - onClientExecutor(listener, - () -> listener.attributesChanged(object, List.of(), cachedAttributes)); + if (!cbAttributes.isEmpty()) { + replayed(listener, + () -> listener.attributesChanged(object, List.of(), cbAttributes)); } - Map cachedElements = object.getCachedElements(); - for (TargetObject elem : cachedElements.values()) { + Map cbElements = object.getCallbackElements(); + for (TargetObject elem : cbElements.values()) { assert elem instanceof SpiTargetObject; replayAddEvents(listener, (SpiTargetObject) elem, visited); } - if (!cachedElements.isEmpty()) { - onClientExecutor(listener, - () -> listener.elementsChanged(object, List.of(), cachedElements)); + if (!cbElements.isEmpty()) { + replayed(listener, + () -> listener.elementsChanged(object, List.of(), cbElements)); } } @Override public void addModelListener(DebuggerModelListener listener, boolean replay) { - try { - synchronized (lock) { - if (replay) { + if (replay) { + synchronized (cbLock) { + CompletableFuture.runAsync(() -> { replayTreeEvents(listener); - } - listeners.add(listener); + listeners.add(listener); + }, clientExecutor).exceptionally(ex -> { + listener.catastrophic(ex); + return null; + }); } } - catch (Throwable ex) { - listener.catastrophic(ex); + else { + listeners.add(listener); } } diff --git a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/DefaultTargetObject.java b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/DefaultTargetObject.java index b1f654f967..99830d463d 100644 --- a/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/DefaultTargetObject.java +++ b/Ghidra/Debug/Framework-Debugging/src/main/java/ghidra/dbg/agent/DefaultTargetObject.java @@ -310,7 +310,9 @@ public class DefaultTargetObject private void updateCallbackElements(Delta delta) { CompletableFuture.runAsync(() -> { - delta.apply(this.cbElements, Delta.SAME); + synchronized (model.cbLock) { + delta.apply(this.cbElements, Delta.SAME); + } }, model.clientExecutor).exceptionally(ex -> { Msg.error(this, "Error updating elements before callback"); return null; @@ -489,7 +491,9 @@ public class DefaultTargetObject private void updateCallbackAttributes(Delta delta) { CompletableFuture.runAsync(() -> { - delta.apply(this.cbAttributes, Delta.EQUAL); + synchronized (model.cbLock) { + delta.apply(this.cbAttributes, Delta.EQUAL); + } }, model.clientExecutor).exceptionally(ex -> { Msg.error(this, "Error updating elements before callback"); return null; diff --git a/Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/agent/DefaultDebuggerObjectModelTest.java b/Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/agent/DefaultDebuggerObjectModelTest.java index 25850a3c1d..5591ff1367 100644 --- a/Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/agent/DefaultDebuggerObjectModelTest.java +++ b/Ghidra/Debug/Framework-Debugging/src/test/java/ghidra/dbg/agent/DefaultDebuggerObjectModelTest.java @@ -15,7 +15,7 @@ */ package ghidra.dbg.agent; -import static ghidra.lifecycle.Unfinished.*; +import static ghidra.lifecycle.Unfinished.TODO; import static org.junit.Assert.*; import java.util.*; @@ -34,6 +34,7 @@ import ghidra.dbg.testutil.*; import ghidra.dbg.testutil.AttributesChangedListener.AttributesChangedInvocation; import ghidra.dbg.testutil.ElementsChangedListener.ElementsChangedInvocation; import ghidra.dbg.testutil.InvalidatedListener.InvalidatedInvocation; +import ghidra.dbg.util.PathUtils; import ghidra.program.model.address.AddressFactory; import ghidra.program.model.address.AddressSpace; @@ -116,7 +117,7 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { @Test public void testGetModelObjectLen0() throws Throwable { - assertEquals(model.root, waitOn(model.fetchModelObject())); + assertEquals(model.root, waitOn(model.fetchModelValue())); } @Test @@ -124,7 +125,7 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { FakeTargetObject a = new FakeTargetObject(model, model.root, "A"); model.root.changeAttributes(List.of(), Map.of("A", a), "Test"); - assertEquals(a, waitOn(model.fetchModelObject("A"))); + assertEquals(a, waitOn(model.fetchModelValue("A"))); } @Test @@ -132,7 +133,7 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { FakeTargetObject a = new FakeTargetObject(model, model.root, "A"); model.root.changeAttributes(List.of(), Map.of("A", a), "Test"); - assertEquals(null, waitOn(model.fetchModelObject("NoA"))); + assertEquals(null, waitOn(model.fetchModelValue("NoA"))); } @Test @@ -143,7 +144,7 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { FakeTargetObject b = new FakeTargetObject(model, a, "[B]"); a.changeElements(List.of(), List.of(b), "Test"); - assertEquals(b, waitOn(model.fetchModelObject("A", "[B]"))); + assertEquals(b, waitOn(model.fetchModelValue("A", "[B]"))); } @Test @@ -154,31 +155,36 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { FakeTargetObject b = new FakeTargetObject(model, a, "[B]"); a.changeElements(List.of(), List.of(b), "Test"); - assertEquals(null, waitOn(model.fetchModelObject("NoA", "[B]"))); - assertEquals(null, waitOn(model.fetchModelObject("NoA", "[NoB]"))); - assertEquals(null, waitOn(model.fetchModelObject("A", "[NoB]"))); + assertEquals(null, waitOn(model.fetchModelValue("NoA", "[B]"))); + assertEquals(null, waitOn(model.fetchModelValue("NoA", "[NoB]"))); + assertEquals(null, waitOn(model.fetchModelValue("A", "[NoB]"))); } @Test public void testElementReplacement() throws Throwable { ElementsChangedListener elemL = new ElementsChangedListener(); InvalidatedListener invL = new InvalidatedListener(); + model.addModelListener(elemL); + model.addModelListener(invL); + TargetObjectAddedWaiter waiter = new TargetObjectAddedWaiter(model); FakeTargetObject fakeA = new FakeTargetObject(model, model.root, "[A]"); model.root.setElements(List.of(fakeA), "Init"); - - model.root.addListener(elemL); - fakeA.addListener(invL); + assertEquals(fakeA, waitOn(waiter.wait(PathUtils.parse("[A]")))); + waitOn(model.flushEvents()); + elemL.clear(); + invL.clear(); PhonyTargetObject phonyA = new PhonyTargetObject(model, model.root, "[A]"); // mere creation causes removal of old - waitOn(elemL.count.waitValue(1)); + waitOn(elemL.count.waitUntil(c -> c >= 1)); ElementsChangedInvocation changed1 = Unique.assertOne(elemL.invocations); assertSame(model.root, changed1.parent); assertEquals(Set.of("A"), changed1.removed); assertTrue(changed1.added.isEmpty()); - waitOn(invL.count.waitValue(1)); + + waitOn(invL.count.waitUntil(c -> c >= 1)); InvalidatedInvocation invalidated = Unique.assertOne(invL.invocations); assertSame(fakeA, invalidated.object); @@ -186,38 +192,45 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { invL.clear(); model.root.setElements(List.of(phonyA), "Replace"); - assertSame(phonyA, waitOn(model.fetchModelObject("[A]"))); + assertSame(phonyA, waitOn(model.fetchModelValue("[A]"))); assertFalse(fakeA.isValid()); + waitOn(elemL.count.waitUntil(c -> c >= 1)); ElementsChangedInvocation changed2 = Unique.assertOne(elemL.invocations); assertSame(model.root, changed2.parent); assertSame(phonyA, Unique.assertOne(changed2.added.values())); assertTrue(changed2.removed.isEmpty()); + waiter.close(); } @Test public void testAttributeReplacement() throws Throwable { AttributesChangedListener attrL = new AttributesChangedListener(); + model.addModelListener(attrL); String str1 = new String("EqualStrings"); String str2 = new String("EqualStrings"); model.root.setAttributes(Map.of("a", str1), "Init"); - model.root.addListener(attrL); + waitOn(model.flushEvents()); - // Note: mere object creation will cause "prior removal" - // We'll do this test just with primitives - // Should not cause replacement, since they're equal + /** + * Note: mere object creation will cause "prior removal," so we'll do this test just with + * primitives. Should not cause replacement, since str1 and str2 are equal. + */ + attrL.clear(); model.root.setAttributes(Map.of("a", str2), "Replace"); - waitOn(model.clientExecutor); + waitOn(model.flushEvents()); + // Verify str1 was not replaced with str2 assertSame(str1, waitOn(model.fetchModelValue("a"))); assertEquals(0, attrL.invocations.size()); // Now, with prior removal // TODO: Should I permit custom equality check? + attrL.clear(); model.root.setAttributes(Map.of(), "Clear"); model.root.setAttributes(Map.of("a", str2), "Replace"); - waitOn(model.clientExecutor); + waitOn(model.flushEvents()); assertEquals(2, attrL.invocations.size()); AttributesChangedInvocation changed = attrL.invocations.get(0); @@ -233,6 +246,7 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { @Test public void testInvalidation() throws Throwable { InvalidatedListener invL = new InvalidatedListener(); + model.addModelListener(invL); FakeTargetObject fakeA = new FakeTargetObject(model, model.root, "A"); model.root.setAttributes(Map.of("A", fakeA), "Init"); @@ -240,10 +254,8 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { FakeTargetObject fakeA1 = new FakeTargetObject(model, fakeA, "[1]"); FakeTargetObject fakeA2 = new FakeTargetObject(model, fakeA, "[2]"); fakeA.setElements(List.of(fakeA1, fakeA2), "Init"); - - fakeA.addListener(invL); - fakeA1.addListener(invL); - fakeA2.addListener(invL); + waitOn(model.flushEvents()); + invL.clear(); model.root.setAttributes(Map.of(), "Clear"); @@ -276,6 +288,11 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { } } + @Override + public void rootAdded(TargetObject root) { + record.add(new ImmutablePair<>("rootAdded", root)); + } + @Override public void registersUpdated(TargetObject bank, Map updates) { record.add(new ImmutablePair<>("registersUpdated", bank)); @@ -287,6 +304,8 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { EventRecordingListener listener = new EventRecordingListener(); model.addModelListener(listener, false); waitOn(model.clientExecutor); + waitOn(model.fetchModelRoot()); + listener.record.clear(); FakeTargetObject fakeA = new FakeTargetObject(model, model.root, "A"); FakeTargetRegisterBank fakeA1rb = new FakeTargetRegisterBank(model, fakeA, "[1]"); @@ -296,10 +315,12 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { waitOn(model.clientExecutor); - assertEquals(List.of(new ImmutablePair<>("created", fakeA), + assertEquals(List.of( + new ImmutablePair<>("created", fakeA), new ImmutablePair<>("created", fakeA1rb), new ImmutablePair<>("registersUpdated", fakeA1rb), - new ImmutablePair<>("addedElem", fakeA1rb), new ImmutablePair<>("addedAttr", fakeA)), + new ImmutablePair<>("addedElem", fakeA1rb), + new ImmutablePair<>("addedAttr", fakeA)), listener.record); } @@ -316,9 +337,13 @@ public class DefaultDebuggerObjectModelTest implements AsyncTestUtils { waitOn(model.clientExecutor); - assertEquals(List.of(new ImmutablePair<>("created", model.root), - new ImmutablePair<>("created", fakeA), new ImmutablePair<>("created", fakeA1rb), - new ImmutablePair<>("addedElem", fakeA1rb), new ImmutablePair<>("addedAttr", fakeA)), + assertEquals(List.of( + new ImmutablePair<>("created", model.root), + new ImmutablePair<>("created", fakeA), + new ImmutablePair<>("created", fakeA1rb), + new ImmutablePair<>("addedElem", fakeA1rb), + new ImmutablePair<>("addedAttr", fakeA), + new ImmutablePair<>("rootAdded", model.root)), listener.record); } }