GP-1103 fixed issues related to moving memory blocks with pinned symbols in source or destination.

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDB.java

@@ -134,22 +134,38 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 			oldAddr, addr);
 	}
 
-	protected void move(Address oldBase, Address newBase) {
+	/**
+	 * 	low level record adjustment to move a symbol. Used only when moving a memory block or
+	 *  changing the image base.
+	 *  
+	 * @param newAddress the new address for the symbol
+	 * @param newName the new name for the symbol (or null if the name should stay the same)
+	 * @param newNamespace the new namespace for the symbol (or null if it should stay the same)
+	 * @param newSource 
+	 */
+	protected void moveLowLevel(Address newAddress, String newName, Namespace newNamespace,
+			SourceType newSource, boolean pinned) {
 		lock.acquire();
 		try {
 			checkDeleted();
-			// check if this symbol is in the address space that was moved.
-			if (!oldBase.getAddressSpace().equals(address.getAddressSpace())) {
-				return;
-			}
-			ProgramDB program = symbolMgr.getProgram();
 			Address oldAddress = address;
-			long diff = address.subtract(oldBase);
-			address = newBase.addWrap(diff);
 			record.setLongValue(SymbolDatabaseAdapter.SYMBOL_ADDR_COL,
-				program.getAddressMap().getKey(address, true));
+				symbolMgr.getAddressMap().getKey(newAddress, true));
+			if (newName != null) {
+				record.setString(SymbolDatabaseAdapter.SYMBOL_NAME_COL, newName);
+				if (newName.length() == 0) {
+					setSourceFlagBit(SourceType.DEFAULT);
+				}
+			}
+			if (newNamespace != null) {
+				record.setLongValue(SymbolDatabaseAdapter.SYMBOL_PARENT_COL, newNamespace.getID());
+			}
+			if (newSource != null) {
+				setSourceFlagBit(newSource);
+			}
+			updatePinnedFlag(pinned);
 			updateRecord();
-			symbolMgr.moveLabelHistory(oldAddress, address);
+			setInvalid();
 		}
 		finally {
 			lock.release();
@@ -406,12 +422,7 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 				throw new IllegalArgumentException(msg);
 			}
 			if (record != null) {
-				byte flags = record.getByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL);
-				byte clearBits = SymbolDatabaseAdapter.SYMBOL_SOURCE_BITS;
-				byte setBits = (byte) newSource.ordinal();
-				flags &= ~clearBits;
-				flags |= setBits;
-				record.setByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL, flags);
+				setSourceFlagBit(newSource);
 				updateRecord();
 				symbolMgr.symbolSourceChanged(this);
 			}
@@ -421,6 +432,15 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 		}
 	}
 
+	private void setSourceFlagBit(SourceType newSource) {
+		byte flags = record.getByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL);
+		byte clearBits = SymbolDatabaseAdapter.SYMBOL_SOURCE_BITS;
+		byte setBits = (byte) newSource.ordinal();
+		flags &= ~clearBits;
+		flags |= setBits;
+		record.setByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL, flags);
+	}
+
 	@Override
 	public SourceType getSource() {
 		lock.acquire();
@@ -461,7 +481,7 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 
 	@Override
 	public void setPinned(boolean pinned) {
-		throw new UnsupportedOperationException("Only Code Symbols may be pinned.");
+		throw new UnsupportedOperationException("Only Code and Function Symbols may be pinned.");
 	}
 
 	protected void doSetPinned(boolean pinned) {
@@ -472,14 +492,7 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 				return;
 			}
 			if (record != null) {
-				byte flags = record.getByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL);
-				if (pinned) {
-					flags |= SymbolDatabaseAdapter.SYMBOL_PINNED_FLAG;
-				}
-				else {
-					flags &= ~SymbolDatabaseAdapter.SYMBOL_PINNED_FLAG;
-				}
-				record.setByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL, flags);
+				updatePinnedFlag(pinned);
 				updateRecord();
 				symbolMgr.symbolAnchoredFlagChanged(this);
 			}
@@ -489,6 +502,17 @@ public abstract class SymbolDB extends DatabaseObject implements Symbol {
 		}
 	}
 
+	private void updatePinnedFlag(boolean pinned) {
+		byte flags = record.getByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL);
+		if (pinned) {
+			flags |= SymbolDatabaseAdapter.SYMBOL_PINNED_FLAG;
+		}
+		else {
+			flags &= ~SymbolDatabaseAdapter.SYMBOL_PINNED_FLAG;
+		}
+		record.setByteValue(SymbolDatabaseAdapter.SYMBOL_FLAGS_COL, flags);
+	}
+
 	@Override
 	public void setName(String newName, SourceType source)
 			throws DuplicateNameException, InvalidInputException {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDatabaseAdapter.java

@@ -215,19 +215,6 @@ abstract class SymbolDatabaseAdapter {
 	 */
 	abstract void moveAddress(Address oldAddr, Address newAddr) throws IOException;
 
-	/**
-	 * Update the addresses in all records to reflect the movement of a memory block.
-	 * @param fromAddr minimum address of the original block to be moved
-	 * @param toAddr the new minimum address after the block move
-	 * @param length the number of bytes in the memory block being moved
-	 * @param monitor progress monitor
-	 * @return returns the set of addresses where symbols where not moved because they were anchored
-	 * @throws CancelledException
-	 * @throws IOException
-	 */
-	abstract void moveAddressRange(Address fromAddr, Address toAddr, long length,
-			TaskMonitor monitor) throws CancelledException, IOException;
-
 	/**
 	 * Delete all records which contain addresses within the specified range
 	 * @param startAddr minimum address in range

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDatabaseAdapterV0.java

@@ -188,21 +188,11 @@ class SymbolDatabaseAdapterV0 extends SymbolDatabaseAdapter {
 				V0_SYMBOL_ADDR_COL, addrMap, start, end, forward)));
 	}
 
-	void deleteExternalEntries(Address start, Address end) {
-		throw new UnsupportedOperationException();
-	}
-
 	@Override
 	void moveAddress(Address oldAddr, Address newAddr) throws IOException {
 		throw new UnsupportedOperationException();
 	}
 
-	@Override
-	void moveAddressRange(Address fromAddr, Address toAddr, long length, TaskMonitor monitor)
-			throws CancelledException, IOException {
-		throw new UnsupportedOperationException();
-	}
-
 	@Override
 	Set<Address> deleteAddressRange(Address startAddr, Address endAddr, TaskMonitor monitor)
 			throws CancelledException, IOException {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDatabaseAdapterV1.java

@@ -22,7 +22,8 @@ import java.io.IOException;
 import java.util.Set;
 
 import db.*;
-import ghidra.program.database.map.*;
+import ghidra.program.database.map.AddressIndexPrimaryKeyIterator;
+import ghidra.program.database.map.AddressMap;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.address.AddressSpace;
 import ghidra.program.model.symbol.*;
@@ -181,22 +182,11 @@ class SymbolDatabaseAdapterV1 extends SymbolDatabaseAdapter {
 		return new V1ConvertedRecordIterator(symbolTable.indexIterator(V1_SYMBOL_NAME_COL));
 	}
 
-	void deleteExternalEntries(Address start, Address end) throws IOException {
-		AddressRecordDeleter.deleteRecords(symbolTable, V1_SYMBOL_ADDR_COL, addrMap, start, end,
-			null);
-	}
-
 	@Override
 	void moveAddress(Address oldAddr, Address newAddr) throws IOException {
 		throw new UnsupportedOperationException();
 	}
 
-	@Override
-	void moveAddressRange(Address fromAddr, Address toAddr, long length, TaskMonitor monitor)
-			throws CancelledException, IOException {
-		throw new UnsupportedOperationException();
-	}
-
 	@Override
 	Set<Address> deleteAddressRange(Address startAddr, Address endAddr, TaskMonitor monitor)
 			throws CancelledException, IOException {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolDatabaseAdapterV2.java

@@ -21,7 +21,6 @@ import java.util.Set;
 
 import db.*;
 import ghidra.program.database.map.*;
-import ghidra.program.database.util.DatabaseTableUtils;
 import ghidra.program.database.util.RecordFilter;
 import ghidra.program.model.address.Address;
 import ghidra.program.model.address.AddressSpace;
@@ -271,10 +270,6 @@ class SymbolDatabaseAdapterV2 extends SymbolDatabaseAdapter {
 			SYMBOL_ADDR_COL, addrMap, start, end, forward));
 	}
 
-	void deleteExternalEntries(Address start, Address end) throws IOException {
-		AddressRecordDeleter.deleteRecords(symbolTable, SYMBOL_ADDR_COL, addrMap, start, end, null);
-	}
-
 	@Override
 	void moveAddress(Address oldAddr, Address newAddr) throws IOException {
 		LongField oldKey = new LongField(addrMap.getKey(oldAddr, false));
@@ -287,14 +282,6 @@ class SymbolDatabaseAdapterV2 extends SymbolDatabaseAdapter {
 		}
 	}
 
-	@Override
-	void moveAddressRange(Address fromAddr, Address toAddr, long length, TaskMonitor monitor)
-			throws CancelledException, IOException {
-
-		DatabaseTableUtils.updateIndexedAddressField(symbolTable, SYMBOL_ADDR_COL, addrMap,
-			fromAddr, toAddr, length, null, monitor);
-	}
-
 	@Override
 	Set<Address> deleteAddressRange(Address startAddr, Address endAddr, TaskMonitor monitor)
 			throws CancelledException, IOException {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/symbol/SymbolManager.java

@@ -2144,21 +2144,171 @@ public class SymbolManager implements SymbolTable, ManagerDB {
 	@Override
 	public void moveAddressRange(Address fromAddr, Address toAddr, long length, TaskMonitor monitor)
 			throws CancelledException {
+		if (fromAddr.equals(toAddr)) {
+			return;
+		}
+		Set<Address> primaryFixups = new HashSet<>();
 		lock.acquire();
 		try {
 			invalidateCache(true);
-			adapter.moveAddressRange(fromAddr, toAddr, length, monitor);
-			historyAdapter.moveAddressRange(fromAddr, toAddr, length, addrMap, monitor);
-			fixupPinnedSymbols(toAddr, fromAddr, toAddr, toAddr.add(length - 1));
-		}
-		catch (IOException e) {
-			program.dbError(e);
+			Address lastAddress = fromAddr.add(length-1);
+			AddressRange range = new AddressRangeImpl(fromAddr, lastAddress);
+
+			// in order to handle overlapping ranges, need to iterate in the correct direction
+			SymbolIterator symbolIterator = (fromAddr.compareTo(toAddr) > 0) ? 
+						getSymbolIterator(fromAddr, true) :
+						getSymbolIterator(lastAddress, false);
+			
+			for (Symbol symbol : symbolIterator) {
+				if (!range.contains(symbol.getAddress())) {
+					break;
+				}
+				Address newAddress = toAddr.add(symbol.getAddress().subtract(fromAddr));
+
+				// any address that has symbols added or removed may have a corrupted primary (too many or non-existent)
+				primaryFixups.add(symbol.getAddress());
+				primaryFixups.add(newAddress);
+				
+				moveSymbolForMemoryBlockMove((SymbolDB) symbol, newAddress);
+			}
+			// go back and make sure there is a valid primary symbol at touched addressess
+			fixupPrimarySymbols(primaryFixups);
 		}
 		finally {
 			lock.release();
 		}
 	}
 
+	// This method is specifically for moving symbols in the context of a memory block move
+	// Since this is a memory block move, the destination range can not contain any functions, but
+	// it may contain labels so we may have to deal with collisions. The other complication we have
+	// to deal with is functions in the moved block that are currently pinned.
+	private void moveSymbolForMemoryBlockMove(SymbolDB symbol, Address newAddress) {
+		Address oldAddress = symbol.getAddress();
+
+		// If the symbol is not pinned go ahead and move it. Only wrinkle is that there may
+		// be a matching symbol at the destination. In that unlikely event, remove it, but preserve
+		// its pinned status
+		if (!symbol.isPinned()) {
+			// if we conflict with a symbol at the destination, delete it (it can't be a function)
+			// and retain its pinned status if it had one.
+			boolean shouldPin = deleteMatchingSymbolAndCheckPinnedStatus(symbol, newAddress);
+			symbol.moveLowLevel(newAddress, null, null, null, shouldPin);
+			moveLabelHistory(oldAddress, newAddress);
+			return;
+		}
+
+		// the other complicated case is a pinned function symbol. In this case we need to move
+		// the function symbol, but create a replacement label in the pinned source location. Also,
+		// if there is a primary symbol at the destination, we need to remove it and set the
+		// function's symbol to that name.
+		if (symbol.getSymbolType() == SymbolType.FUNCTION) {
+			String originalName = symbol.getName();
+			Namespace originalNamespace = symbol.getParentNamespace();
+			SourceType originalSource = symbol.getSource();
+			String newName = "";
+			Namespace newNamespace = namespaceMgr.getGlobalNamespace();
+			SourceType newSource = SourceType.DEFAULT;
+			boolean newPinned = false;
+			// see if there is a symbol at the destination
+			Symbol destinationPrimary = getPrimarySymbol(newAddress);
+
+			// so there is a symbol at the destination, steal its name and namespace and delete it.
+			if (destinationPrimary != null) {
+				newName = destinationPrimary.getName();
+				newNamespace = destinationPrimary.getParentNamespace();  // use the destination's namespace
+				newSource = destinationPrimary.getSource();
+				newPinned = destinationPrimary.isPinned();
+				destinationPrimary.delete();
+			}
+			try {
+				// so move the symbol to the new address and update namespace, source, and pinned state
+				symbol.moveLowLevel(newAddress, newName, newNamespace, newSource, newPinned);
+
+				// create a pinned label to replace the pinned function symbol at the source.
+				Symbol newSymbol =
+					createLabel(oldAddress, originalName, originalNamespace, originalSource);
+				newSymbol.setPinned(true);
+			}
+			catch (InvalidInputException e) {
+				// can't happen - name was already valid
+			}
+		}
+	}
+
+	// Checks if there is a matching symbol at the given address and deletes it. Also returns if
+	// the deleted symbol was pinned which we will preserve on the moved symbol
+	private boolean deleteMatchingSymbolAndCheckPinnedStatus(SymbolDB symbol, Address address) {
+		boolean isPinned = false;
+		Symbol match = getSymbol(symbol.getName(), address, symbol.getParentNamespace());
+		if (match != null) {
+			isPinned = match.isPinned();
+			match.delete();
+		}
+		return isPinned;
+	}
+
+	/**
+	 * Checks to make sure there is a single valid primary symbol at each address
+	 * @param set the set of addresses that may have to be fixed up
+	 */
+	private void fixupPrimarySymbols(Set<Address> set) {
+		for (Address address : set) {
+			Symbol[] symbols = getSymbols(address);
+
+			// check if there is a valid consistent primary state amongst all the symbols at an address.
+			if (hasValidPrimary(symbols)) {
+				continue;
+			}
+
+			// otherwise, set them all to non-primary, find the best symbol to make primary and
+			// make that one the primary symbol
+			setAllSymbolsToNonPrimary(symbols);
+			Symbol bestPrimary = findBestPrimary(symbols);
+			bestPrimary.setPrimary();
+		}
+	}
+
+	private void setAllSymbolsToNonPrimary(Symbol[] symbols) {
+		for (Symbol symbol : symbols) {
+			if (symbol instanceof CodeSymbol) {
+				((CodeSymbol) symbol).setPrimary(false);
+			}
+		}
+	}
+
+	private Symbol findBestPrimary(Symbol[] symbols) {
+		// first see if see if there is a function, if so that has to be the primary symbol
+		for (Symbol symbol : symbols) {
+			if (symbol.getSymbolType() == SymbolType.FUNCTION) {
+				return symbol;
+			}
+		}
+
+		// else just pick one. This is such a rare case, it isn't worth doing something more clever
+		return symbols[0];
+	}
+
+	/**
+	 * Checks if the givens symbols from the same address have exactly one primary symbol amongst them
+	 * @param symbols the array of symbols at a an address
+	 * @return true if there is exactly one primary symbol at the address (also true if no symbols at address)
+	 */
+	private boolean hasValidPrimary(Symbol[] symbols) {
+		if (symbols.length == 0) {
+			return true;
+		}
+		if (!symbols[0].isPrimary()) {
+			return false;
+		}
+		for (int i = 1; i < symbols.length; i++) {
+			if (symbols[i].isPrimary()) {
+				return false;
+			}
+		}
+		return getPrimarySymbol(symbols[0].getAddress()) == symbols[0];
+	}
+
 	@Override
 	public void deleteAddressRange(Address startAddr, Address endAddr, TaskMonitor monitor)
 			throws CancelledException {
@@ -2176,43 +2326,109 @@ public class SymbolManager implements SymbolTable, ManagerDB {
 		}
 	}
 
-	public void imageBaseChanged(Address oldBase, Address base) {
-		fixupPinnedSymbols(base, oldBase, program.getMinAddress(), program.getMaxAddress());
+	public void imageBaseChanged(Address oldBase, Address newBase) {
+		AddressSpace space = newBase.getAddressSpace();
+		fixupPinnedSymbolsAfterRebase(oldBase, newBase, space.getMinAddress(),
+			space.getMaxAddress());
 	}
 
-	private void fixupPinnedSymbols(Address currentBase, Address newBase, Address minAddr,
+	private void fixupPinnedSymbolsAfterRebase(Address oldBase, Address base, Address minAddr,
 			Address maxAddr) {
-		List<SymbolDB> fixupSymbols = new ArrayList<>();
+
+		List<SymbolDB> fixupPinnedSymbols = findPinnedSymbols(minAddr, maxAddr);
+
+		Set<Address> primaryFixups = new HashSet<>();
+		for (SymbolDB symbol : fixupPinnedSymbols) {
+			Address currentAddress = symbol.getAddress();
+			Address beforeBaseChangeAddress = oldBase.add(currentAddress.subtract(base));
+			primaryFixups.add(currentAddress);
+			primaryFixups.add(beforeBaseChangeAddress);
+
+			// see if there is a name collision for the pinned symbol we are about to move back
+			Symbol match =
+				getSymbol(symbol.getName(), beforeBaseChangeAddress, symbol.getParentNamespace());
+
+			if (symbol.getSymbolType() == SymbolType.FUNCTION) {
+				fixupPinnedFunctionSymbolAfterRebase(symbol, beforeBaseChangeAddress, match);
+			}
+			else {
+				fixupPinnedLabelSymbolAfterRebase(symbol, beforeBaseChangeAddress, match);
+			}
+		}
+		fixupPrimarySymbols(primaryFixups);
+	}
+
+	private void fixupPinnedLabelSymbolAfterRebase(SymbolDB symbol, Address newAddress,
+			Symbol match) {
+		if (match != null) {
+			match.setPinned(true);
+			symbol.delete();
+		}
+		else {
+			symbol.moveLowLevel(newAddress, null, null, null, true);
+		}
+	}
+
+	private void fixupPinnedFunctionSymbolAfterRebase(SymbolDB symbol, Address newAddress,
+			Symbol match) {
+		// since we are a function, we are not moving the symbol, so we must either pin a
+		// matching symbol at the destination or create a new pinned label at the destination.
+		if (match != null) {
+			match.setPinned(true);
+		}
+		else {
+			try {
+				Symbol newLabel =
+					createLabel(newAddress, symbol.getName(), symbol.getParentNamespace(),
+					symbol.getSource());
+				newLabel.setPinned(true);
+			}
+			catch (InvalidInputException e) {
+				// can't happen, we already know it is valid
+			}
+		}
+
+		// unpin the function symbol since it is not moving.
+		symbol.setPinned(false);
+
+		// now, either set the function to default or find another symbol to promote.
+		String newName = "";
+		Namespace newNamespace = namespaceMgr.getGlobalNamespace();
+		SourceType newSource = SourceType.DEFAULT;
+		Symbol symbolToPromote = findSymbolToPromote(symbol.getAddress());
+		if (symbolToPromote != null) {
+			newName = symbolToPromote.getName();
+			newNamespace = symbolToPromote.getParentNamespace();
+			newSource = symbolToPromote.getSource();
+			symbolToPromote.delete();
+		}
+		try {
+			symbol.setNameAndNamespace(newName, newNamespace, newSource);
+		}
+		catch (DuplicateNameException | InvalidInputException | CircularDependencyException e) {
+			// can't happen
+		}
+	}
+
+	private List<SymbolDB> findPinnedSymbols(Address minAddr, Address maxAddr) {
+		List<SymbolDB> pinnedSymbols = new ArrayList<>();
 		for (Symbol symbol : getSymbolIterator(minAddr, true)) {
 			if (symbol.getAddress().compareTo(maxAddr) > 0) {
 				break;
 			}
 			if (symbol.isPinned()) {
-				fixupSymbols.add((SymbolDB) symbol);
-			}
-		}
-		for (SymbolDB symbol : fixupSymbols) {
-			if (symbol.getSymbolType() == SymbolType.FUNCTION) {
-				String name = symbol.getName();
-				SourceType source = symbol.getSource();
-				try {
-					symbol.setPinned(false);
-					symbol.setName("", SourceType.DEFAULT);
-					Address symbolAddr =
-						newBase.addNoWrap(symbol.getAddress().subtract(currentBase));
-					Symbol newSymbol = createLabel(symbolAddr, name, source);
-					newSymbol.setPinned(true);
-					moveLabelHistory(symbol.getAddress(), newSymbol.getAddress());
-				}
-				catch (Exception e) {
-					throw new AssertException("Should not get exception here.", e);
-				}
-			}
-			else {
-				symbol.move(currentBase, newBase);
+				pinnedSymbols.add((SymbolDB) symbol);
 			}
 		}
+		return pinnedSymbols;
+	}
 
+	private Symbol findSymbolToPromote(Address address) {
+		Symbol[] symbols = getSymbols(address);
+		if (symbols.length > 1) {
+			return symbols[1];
+		}
+		return null;
 	}
 
 	void moveLabelHistory(Address oldAddress, Address address) {