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 <eclipse@jfront.com>
Reviewed-on: https://git.eclipse.org/r/20972
Tested-by: Hudson CI
Reviewed-by: Doug Schaefer <dschaefer@qnx.com>
IP-Clean: Doug Schaefer <dschaefer@qnx.com>

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();
+			PDOMName name_c1 = binding_c.getFirstReference();
-			assertNotNull(name_c);
+			assertNotNull(name_c1);
-			assertSame(binding_c.getLinkage(), name_c.getLinkage());
+			assertSame(binding_c.getLinkage(), name_c1.getLinkage());
 
 			// Check that the external references list is currently empty.
-			IPDOMIterator<PDOMName> extNames = binding_cpp.getExternalReferences();
+			assertEquals(0, countExternalReferences(binding_cpp));
-			assertNotNull(extNames);
-			assertFalse(extNames.hasNext());
 
 			// 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());
+			assertNotSame(binding_cpp.getLinkage(), name_c1.getLinkage());
-			name_c.setBinding(binding_cpp);
+			name_c1.setBinding(binding_cpp);
-			binding_cpp.addReference(name_c);
+			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<PDOMName> 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());
+			assertSame(name_c1.getLinkage(), extRef.getLinkage());
-			assertEquals(name_c.getRecord(), extRef.getRecord());
+			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<PDOMName> extRefs = binding.getExternalReferences();
+		assertNotNull(extRefs);
+		int count = 0;
+		for( ; extRefs.hasNext(); extRefs.next())
+			++count;
+		return count;
+	}
 }

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;

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.
+ * a PDOMName that references a PDOMBinding in a different linkage.  This list can be
- * <p>
+ * examined using {@link #getIterator()}.
- * 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.
- * <p>
- * Each node has three fields:
- * {
- *     INT_SIZE linkageId;
- *     PTR_SIZE nextNode;
- *     PTR_SIZE nameListHead;
- * }
- * <p>
- * 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.
  */
 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<PDOMName> {
 		private long nodeAddr;
 		private long node;

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.

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;
 			}
 		}