From 6de9f5288e678d920bb16386ecd81f2835734ac0 Mon Sep 17 00:00:00 2001 From: Andrew Eidsness Date: Wed, 22 Jan 2014 11:18:13 -0500 Subject: [PATCH] Bug 426238: Update head of external references list when needed When a PDOMName is deleted and that name is the head of an external references list, the list's head must be changed. The Qt plugin is the only user of the external reference list. One case is the link from a SIGNAL or SLOT expansion to the C++ method binding for the corresponding function. In this case, the problem will appear when all of the following are true: 1) The file containing the SIGNAL/SLOT expansion is changed and the index updated 2) The corresponding function is declared in a different file 3) The the function is the first entry in the external references list When #2 is false, the binding (and the entire list) is removed as part of updating the file containing the name. When #3 is false, the list is updated with existing code using the PDOMName's {next|prev}InBinding pointers. Change-Id: I1e27c7c2356ca1fb68f57d69c40728289288ed66 Signed-off-by: Andrew Eidsness Reviewed-on: https://git.eclipse.org/r/20972 Tested-by: Hudson CI Reviewed-by: Doug Schaefer IP-Clean: Doug Schaefer --- .../internal/pdom/tests/PDOMNameTests.java | 79 +++++++++++++++--- .../cdt/internal/core/pdom/db/Database.java | 2 +- .../pdom/db/PDOMExternalReferencesList.java | 80 +++++++++++++++---- .../internal/core/pdom/dom/PDOMBinding.java | 14 +++- .../cdt/internal/core/pdom/dom/PDOMName.java | 2 +- 5 files changed, 145 insertions(+), 32 deletions(-) diff --git a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/pdom/tests/PDOMNameTests.java b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/pdom/tests/PDOMNameTests.java index a01b54d87c9..9dcde135f75 100644 --- a/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/pdom/tests/PDOMNameTests.java +++ b/core/org.eclipse.cdt.core.tests/parser/org/eclipse/cdt/internal/pdom/tests/PDOMNameTests.java @@ -69,14 +69,12 @@ public class PDOMNameTests extends BaseTestCase { assertTrue(bindings[0] instanceof PDOMBinding); PDOMBinding binding_c = (PDOMBinding) bindings[0]; - PDOMName name_c = binding_c.getFirstReference(); - assertNotNull(name_c); - assertSame(binding_c.getLinkage(), name_c.getLinkage()); + PDOMName name_c1 = binding_c.getFirstReference(); + assertNotNull(name_c1); + assertSame(binding_c.getLinkage(), name_c1.getLinkage()); // Check that the external references list is currently empty. - IPDOMIterator extNames = binding_cpp.getExternalReferences(); - assertNotNull(extNames); - assertFalse(extNames.hasNext()); + assertEquals(0, countExternalReferences(binding_cpp)); // Make sure the C++ binding and the C name are in different linkages, then add the name // as an external reference of the binding. The case we're setting up is: @@ -86,13 +84,13 @@ public class PDOMNameTests extends BaseTestCase { // We can then test the following (see reference numbers below): // 1) Getting the C name as an external reference of the C++ binding // 2) Loading the C++ binding from the C name - assertNotSame(binding_cpp.getLinkage(), name_c.getLinkage()); - name_c.setBinding(binding_cpp); - binding_cpp.addReference(name_c); + assertNotSame(binding_cpp.getLinkage(), name_c1.getLinkage()); + name_c1.setBinding(binding_cpp); + binding_cpp.addReference(name_c1); // Make sure there is an external reference, then retrieve it. Then make sure there // aren't anymore external references. - extNames = binding_cpp.getExternalReferences(); + IPDOMIterator extNames = binding_cpp.getExternalReferences(); assertNotNull(extNames); assertTrue(extNames.hasNext()); PDOMName extRef = extNames.next(); @@ -102,8 +100,8 @@ public class PDOMNameTests extends BaseTestCase { // 1) Check that the external reference is the same as the C name that was added, that the // external reference does not have the same linkage as the binding, and that it does // have the same linkage as the initial name. - assertSame(name_c.getLinkage(), extRef.getLinkage()); - assertEquals(name_c.getRecord(), extRef.getRecord()); + assertSame(name_c1.getLinkage(), extRef.getLinkage()); + assertEquals(name_c1.getRecord(), extRef.getRecord()); assertNotSame(binding_cpp.getLinkage(), extRef.getLinkage()); assertSame(binding_c.getLinkage(), extRef.getLinkage()); @@ -113,8 +111,65 @@ public class PDOMNameTests extends BaseTestCase { assertEquals(binding_cpp.getRecord(), extBinding.getRecord()); assertEquals(binding_cpp.getNodeType(), extBinding.getNodeType()); assertTrue(binding_cpp.getClass() == extBinding.getClass()); + + // Bug 426238: When names are deleted they need to be removed from the external references + // list. This test puts 3 names into the list and then removes the 2nd, last, + // and then first. + + // Add more external references and then check removals. + PDOMName name_c2 = name_c1.getNextInFile(); + assertNotNull(name_c2); + PDOMName name_c3 = name_c2.getNextInFile(); + name_c2.setBinding(binding_cpp); + binding_cpp.addReference(name_c2); + name_c3.setBinding(binding_cpp); + binding_cpp.addReference(name_c3); + + // Collect the 3 names from the external reference list. + extNames = binding_cpp.getExternalReferences(); + assertNotNull(extNames); + assertTrue(extNames.hasNext()); + PDOMName extRef_1 = extNames.next(); + assertNotNull(extRef); + assertTrue(extNames.hasNext()); + PDOMName extRef_2 = extNames.next(); + assertTrue(extNames.hasNext()); + PDOMName extRef_3 = extNames.next(); + assertFalse(extNames.hasNext()); + + // Check that the list is ordered as expected (reversed during insertion). + assertEquals(name_c3.getRecord(), extRef_1.getRecord()); + assertEquals(name_c2.getRecord(), extRef_2.getRecord()); + assertEquals(name_c1.getRecord(), extRef_3.getRecord()); + + // 3) Check deleting of middle entry. + extRef_2.delete(); + assertEquals(2, countExternalReferences(binding_cpp)); + + // 4) Make sure extref head is updated when there is a next name. + extRef_1.delete(); + assertEquals(1, countExternalReferences(binding_cpp)); + extNames = binding_cpp.getExternalReferences(); + assertNotNull(extNames); + assertTrue(extNames.hasNext()); + assertEquals(extRef_3.getRecord(), extNames.next().getRecord()); + assertFalse(extNames.hasNext()); + + // 5) Check deleting of first entry. + extRef_3.delete(); + assertEquals(0, countExternalReferences(binding_cpp)); + } finally { pdom.releaseWriteLock(); } } + + private static int countExternalReferences(PDOMBinding binding) throws Exception { + IPDOMIterator extRefs = binding.getExternalReferences(); + assertNotNull(extRefs); + int count = 0; + for( ; extRefs.hasNext(); extRefs.next()) + ++count; + return count; + } } diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/Database.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/Database.java index 4954161c346..eb73d630605 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/Database.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/Database.java @@ -456,7 +456,7 @@ public class Database { if (blocksize < 0) { // Already freed. throw new CoreException(new Status(IStatus.ERROR, CCorePlugin.PLUGIN_ID, 0, - "Already freed", new Exception())); //$NON-NLS-1$ + "Already freed record " + offset, new Exception())); //$NON-NLS-1$ } addBlock(chunk, blocksize, block); freed += blocksize; diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/PDOMExternalReferencesList.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/PDOMExternalReferencesList.java index c0d08a0a9e3..891fe19f6cd 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/PDOMExternalReferencesList.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/db/PDOMExternalReferencesList.java @@ -18,25 +18,42 @@ import org.eclipse.core.runtime.CoreException; /** * A utility class for storing a list of external references. An external reference is - * a PDOMName that references a PDOMBinding in a different linkage. - *

- * External references are stored in a singly linked list with one node for each linkage. - * Each node stores a list of PDOMNames. The PDOMName.BindingList is used to store the - * list. - *

- * Each node has three fields: - * { - * INT_SIZE linkageId; - * PTR_SIZE nextNode; - * PTR_SIZE nameListHead; - * } - *

- * An iterator is provided to examine all names in the list. An iterator is needed (instead - * of a simple list) so that we have a chance to move to the next linkage's node when we get - * to the end of a name list. + * a PDOMName that references a PDOMBinding in a different linkage. This list can be + * examined using {@link #getIterator()}. */ public class PDOMExternalReferencesList { + /* + * This data structure is a list of lists. The inner lists are names that should that + * should be created in the same linkage. The outer list collects the heads of the + * inner lists. + * + * This example shows this storage structure for three names in two linkages. + * + * NameA2 + * ^ + * | + * NameA1 NameB + * ^ ^ + * | | + * record --> LinkageA --> LinkageB + * + * NameA1 and NameA2 should both be created in LinkageA, NameB should be created in LinkageB. + * + * The interface to this class flattens this storage structure so it appears as a simple + * list that can be iterated over. + * + * Continuing with the same example, the iterator behaves as though the list were: + * + * { NameA1, NameA2, NameB } + * + * This class mostly doesn't care about the inner lists. They are implemented using the + * #getNextInBinding attribute of PDOMName. + * + * This class implements the outer list as a singly linked list of "nodes". Each node stores + * the linkageId, the record of the first PDOMName in the list, and the record of the next node. + */ + private final PDOM pdom; private final long record; @@ -71,7 +88,7 @@ public class PDOMExternalReferencesList { // needed. long lastAddr = record; long nodeRec = 0; - while((nodeRec = pdom.getDB().getRecPtr(lastAddr)) != 0) { + while ((nodeRec = pdom.getDB().getRecPtr(lastAddr)) != 0) { // Each node looks like { int linkageID; recPtr nextNode; recPtr headOfList; } int linkageID = pdom.getDB().getInt(nodeRec); if (linkageID == nameLinkageID) @@ -106,6 +123,35 @@ public class PDOMExternalReferencesList { pdom.getDB().putRecPtr(nodeRec + Database.INT_SIZE + Database.PTR_SIZE, name.getRecord()); } + /** + * This function must be used during PDOMName deletion when the list head is being deleted. + */ + public void setFirstReference(PDOMLinkage linkage, PDOMName name) throws CoreException { + // Find the head of this linkage's list. + PDOMName head = null; + Iterator iterator = new Iterator(record); + while (head == null) { + if (iterator.next == null) + break; + + if (!linkage.equals(iterator.next.getLinkage())) { + iterator.next = iterator.advance(); + } else { + head = iterator.next; + } + } + + // If there isn't already a node for this name's linkage then the new name can be inserted + // in the normal way. + if (head == null) { + add(name); + } else { + // Otherwise update the node's head field to point to the given name. + long nextNameRec = iterator.node + Database.INT_SIZE + Database.PTR_SIZE; + pdom.getDB().putRecPtr(nextNameRec, name == null ? 0 : name.getRecord()); + } + } + private class Iterator implements IPDOMIterator { private long nodeAddr; private long node; diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMBinding.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMBinding.java index cb5198df1dc..167eb221ede 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMBinding.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMBinding.java @@ -184,7 +184,19 @@ public abstract class PDOMBinding extends PDOMNamedNode implements IPDOMBinding return new PDOMExternalReferencesList(getPDOM(), record + FIRST_EXTREF_OFFSET).getIterator(); } - public void setFirstReference(PDOMName name) throws CoreException { + /** + * In most cases the linkage can be found from the linkage of the name. However, when the + * list is being cleared (there is no next), the linkage must be passed in. + */ + public void setFirstReference(PDOMLinkage linkage, PDOMName name) throws CoreException { + if (linkage.equals(getLinkage())) { + setFirstReference(name); + } else { + new PDOMExternalReferencesList(getPDOM(), record + FIRST_EXTREF_OFFSET).setFirstReference(linkage, name); + } + } + + private void setFirstReference(PDOMName name) throws CoreException { // This needs to filter between the local and external lists because it can be used in // contexts that don't know which type of list they are iterating over. E.g., this is // used when deleting names from a PDOMFile. diff --git a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMName.java b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMName.java index 8fbb35c83cc..0e63b910929 100644 --- a/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMName.java +++ b/core/org.eclipse.cdt.core/parser/org/eclipse/cdt/internal/core/pdom/dom/PDOMName.java @@ -387,7 +387,7 @@ public final class PDOMName implements IIndexFragmentName, IASTFileLocation { getBinding().setFirstDefinition(nextName); break; case IS_REFERENCE: - getBinding().setFirstReference(nextName); + getBinding().setFirstReference(getLinkage(), nextName); break; } }