GT-3347 string / charsequences code review fixes

Fixed problem with picking between ArrayStringable elements and char
sequence for the string layout.

Ghidra/Features/Base/src/main/java/ghidra/app/plugin/core/string/StringTableProvider.java

@@ -568,8 +568,9 @@ public class StringTableProvider extends ComponentProviderAdapter implements Dom
 			FoundString foundString = stringModel.getRowObject(table.getSelectedRow());
 			MemBuffer membuf =
 				new DumbMemBufferImpl(currentProgram.getMemory(), foundString.getAddress());
-			StringDataInstance stringInstance = new StringDataInstance(foundString.getDataType(),
+			StringDataInstance stringInstance =
-				SettingsImpl.NO_SETTINGS, membuf, foundString.getLength());
+				StringDataInstance.getStringDataInstance(foundString.getDataType(), membuf,
+					SettingsImpl.NO_SETTINGS, foundString.getLength());
 			if (charOffset != 0) {
 				stringInstance = stringInstance.getCharOffcut(charOffset);
 			}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/AbstractIntegerDataType.java

@@ -297,7 +297,7 @@ public abstract class AbstractIntegerDataType extends BuiltIn implements ArraySt
 	public String getArrayDefaultLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options) {
 		if (hasStringValue(settings) && buf.isInitializedMemory()) {
-			return new StringDataInstance(this, settings, buf, len).getLabel(
+			return new StringDataInstance(this, settings, buf, len, true).getLabel(
 				AbstractStringDataType.DEFAULT_ABBREV_PREFIX + "_",
 				AbstractStringDataType.DEFAULT_LABEL_PREFIX, AbstractStringDataType.DEFAULT_LABEL,
 				options);
@@ -309,7 +309,7 @@ public abstract class AbstractIntegerDataType extends BuiltIn implements ArraySt
 	public String getArrayDefaultOffcutLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options, int offcutOffset) {
 		if (hasStringValue(settings) && buf.isInitializedMemory()) {
-			return new StringDataInstance(this, settings, buf, len).getOffcutLabelString(
+			return new StringDataInstance(this, settings, buf, len, true).getOffcutLabelString(
 				AbstractStringDataType.DEFAULT_ABBREV_PREFIX + "_",
 				AbstractStringDataType.DEFAULT_LABEL_PREFIX, AbstractStringDataType.DEFAULT_LABEL,
 				options, offcutOffset);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Array.java

@@ -105,9 +105,9 @@ public interface Array extends DataType {
 		ArrayStringable stringableElementType = ArrayStringable.getArrayStringable(getDataType());
 		String value =
 			(stringableElementType != null && stringableElementType.hasStringValue(settings))
-				? new StringDataInstance(stringableElementType, settings, buf,
+					? new StringDataInstance(stringableElementType, settings, buf, length,
-					length).getStringRepresentation()
+						true).getStringRepresentation()
-				: null;
+					: null;
 		return (value != null) ? value : "";
 	}
 

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/ArrayStringable.java

@@ -47,7 +47,7 @@ public interface ArrayStringable extends DataType {
 	 */
 	public default String getArrayString(MemBuffer buf, Settings settings, int length) {
 		if (hasStringValue(settings) && buf.isInitializedMemory()) {
-			return new StringDataInstance(this, settings, buf, length).getStringValue();
+			return new StringDataInstance(this, settings, buf, length, true).getStringValue();
 		}
 		return null;
 	}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/StringDataInstance.java

@@ -65,6 +65,12 @@ public class StringDataInstance {
 		return false;
 	}
 
+	/**
+	 * Returns true if the {@link Data} instance is one of the many 'char' data types.
+	 * 
+	 * @param data {@link Data} instance to test, null ok
+	 * @return boolean true if char data 
+	 */
 	public static boolean isChar(Data data) {
 		if (data == null) {
 			return false;
@@ -112,7 +118,7 @@ public class StringDataInstance {
 			ArrayStringable arrayStringable =
 				ArrayStringable.getArrayStringable(((Array) dt).getDataType());
 			if (arrayStringable != null && arrayStringable.hasStringValue(data)) {
-				return new StringDataInstance(arrayStringable, data, data, data.getLength());
+				return new StringDataInstance(arrayStringable, data, data, data.getLength(), true);
 			}
 		}
 		return NULL_INSTANCE;
@@ -133,13 +139,15 @@ public class StringDataInstance {
 		if (dataType instanceof AbstractStringDataType) {
 			return ((AbstractStringDataType) dataType).getStringDataInstance(buf, settings, length);
 		}
-		if (dataType instanceof Array) {
+		boolean isArray = dataType instanceof Array;
+		if (isArray) {
 			dataType = ArrayStringable.getArrayStringable(((Array) dataType).getDataType());
 		}
 		if (dataType instanceof ArrayStringable &&
 			((ArrayStringable) dataType).hasStringValue(settings) && buf.isInitializedMemory()) {
 
-			return new StringDataInstance(dataType, settings, buf, length);
+			// this could be either a charsequence or an array of char elements
+			return new StringDataInstance(dataType, settings, buf, length, isArray);
 		}
 		return NULL_INSTANCE;
 	}
@@ -207,6 +215,26 @@ public class StringDataInstance {
 	 * of the containing field of the data instance.
 	 */
 	public StringDataInstance(DataType dataType, Settings settings, MemBuffer buf, int length) {
+		this(dataType, settings, buf, length, false);
+	}
+
+	/**
+	 * Creates a string instance using the data in the {@link MemBuffer} and the settings
+	 * pulled from the {@link AbstractStringDataType string data type}.
+	 * 
+	 * @param dataType {@link DataType} of the string, either a {@link AbstractStringDataType} derived type
+	 * or an {@link ArrayStringable} element-of-char-array type. 
+	 * @param settings {@link Settings} attached to the data location.
+	 * @param buf {@link MemBuffer} containing the data.
+	 * @param length Length passed from the caller to the datatype.  -1 indicates a 'probe'
+	 * trying to detect the length of an unknown string, otherwise it will be the length
+	 * of the containing field of the data instance.
+	 * @param isArrayElement boolean flag, true indicates that the specified dataType is an
+	 * element in an array (ie. char[] vs. just a plain char), causing the string layout
+	 * to be forced to {@link StringLayoutEnum#NULL_TERMINATED_BOUNDED}
+	 */
+	public StringDataInstance(DataType dataType, Settings settings, MemBuffer buf, int length,
+			boolean isArrayElement) {
 		settings = (settings == null) ? SettingsImpl.NO_SETTINGS : settings;
 		this.buf = buf;
 		this.charsetName = getCharsetNameFromDataTypeOrSettings(dataType, settings);
@@ -215,7 +243,9 @@ public class StringDataInstance {
 		this.paddedCharSize = (dataType instanceof ArrayStringable) && (charSize == 1) //
 				? getDataOrganization(dataType).getCharSize()
 				: charSize;
-		this.stringLayout = getLayoutFromDataType(dataType);
+		this.stringLayout = isArrayElement //
+				? StringLayoutEnum.NULL_TERMINATED_BOUNDED
+				: getLayoutFromDataType(dataType);
 		this.showTranslation = TRANSLATION.isShowTranslated(settings);
 		this.translatedValue = TRANSLATION.getTranslatedValue(settings);
 		this.renderSetting = RENDER.getEnumValue(settings);
@@ -300,11 +330,6 @@ public class StringDataInstance {
 		return length >= 0 && stringLayout.isFixedLen();
 	}
 
-	public boolean isPascal() {
-		return stringLayout == StringLayoutEnum.PASCAL_255 ||
-			stringLayout == StringLayoutEnum.PASCAL_64k;
-	}
-
 	/**
 	 * Returns the length of this string's data, in bytes.
 	 *
@@ -595,11 +620,6 @@ public class StringDataInstance {
 		return result;
 	}
 
-	private byte[] convertStringToBytes(String s, AdjustedCharsetInfo aci) {
-		Charset cs = Charset.isSupported(aci.charsetName) ? Charset.forName(aci.charsetName) : null;
-		return (cs != null) ? s.getBytes(cs) : null;
-	}
-
 	private static DataConverter getDataConverter(Endian endian) {
 		return endian == Endian.BIG ? BigEndianDataConverter.INSTANCE
 				: LittleEndianDataConverter.INSTANCE;

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/WideChar16DataType.java

@@ -77,7 +77,7 @@ public class WideChar16DataType extends BuiltIn implements ArrayStringable, Data
 	@Override
 	public Object getValue(MemBuffer buf, Settings settings, int length) {
 		try {
-			return new Character((char) buf.getUnsignedShort(0));
+			return Character.valueOf((char) buf.getUnsignedShort(0));
 		}
 		catch (MemoryAccessException e) {
 			// ignore
@@ -94,7 +94,7 @@ public class WideChar16DataType extends BuiltIn implements ArrayStringable, Data
 	public String getDefaultLabelPrefix(MemBuffer buf, Settings settings, int length,
 			DataTypeDisplayOptions options) {
 
-		StringBuffer strBuf = new StringBuffer();
+		StringBuilder strBuf = new StringBuilder();
 		strBuf.append("WCHAR16_");
 		try {
 			int val = buf.getUnsignedShort(0);
@@ -125,7 +125,7 @@ public class WideChar16DataType extends BuiltIn implements ArrayStringable, Data
 	@Override
 	public String getArrayDefaultLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options) {
-		return new StringDataInstance(this, settings, buf, len).getLabel(
+		return new StringDataInstance(this, settings, buf, len, true).getLabel(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options);
@@ -134,7 +134,7 @@ public class WideChar16DataType extends BuiltIn implements ArrayStringable, Data
 	@Override
 	public String getArrayDefaultOffcutLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options, int offcutOffset) {
-		return new StringDataInstance(this, settings, buf, len).getOffcutLabelString(
+		return new StringDataInstance(this, settings, buf, len, true).getOffcutLabelString(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options, offcutOffset);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/WideChar32DataType.java

@@ -96,7 +96,7 @@ public class WideChar32DataType extends BuiltIn implements ArrayStringable, Data
 	public String getDefaultLabelPrefix(MemBuffer buf, Settings settings, int length,
 			DataTypeDisplayOptions options) {
 
-		StringBuffer strBuf = new StringBuffer();
+		StringBuilder strBuf = new StringBuilder();
 		strBuf.append("WCHAR32_");
 		try {
 			int val = buf.getInt(0);
@@ -127,7 +127,7 @@ public class WideChar32DataType extends BuiltIn implements ArrayStringable, Data
 	@Override
 	public String getArrayDefaultLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options) {
-		return new StringDataInstance(this, settings, buf, len).getLabel(
+		return new StringDataInstance(this, settings, buf, len, true).getLabel(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options);
@@ -136,7 +136,7 @@ public class WideChar32DataType extends BuiltIn implements ArrayStringable, Data
 	@Override
 	public String getArrayDefaultOffcutLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options, int offcutOffset) {
-		return new StringDataInstance(this, settings, buf, len).getOffcutLabelString(
+		return new StringDataInstance(this, settings, buf, len, true).getOffcutLabelString(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options, offcutOffset);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/WideCharDataType.java

@@ -83,7 +83,7 @@ public class WideCharDataType extends BuiltIn implements ArrayStringable, DataTy
 		try {
 			switch (getLength()) {
 				case 2:
-					return new Character((char) buf.getShort(0));
+					return Character.valueOf((char) buf.getShort(0));
 				case 4:
 					return new Scalar(32, buf.getInt(0), true);
 			}
@@ -114,7 +114,7 @@ public class WideCharDataType extends BuiltIn implements ArrayStringable, DataTy
 			return "WCHAR_??";
 		}
 
-		StringBuffer strBuf = new StringBuffer();
+		StringBuilder strBuf = new StringBuilder();
 		strBuf.append("WCHAR_");
 		try {
 			int val = (int) buf.getVarLengthUnsignedInt(0, length);
@@ -151,7 +151,7 @@ public class WideCharDataType extends BuiltIn implements ArrayStringable, DataTy
 	@Override
 	public String getArrayDefaultLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options) {
-		return new StringDataInstance(this, settings, buf, len).getLabel(
+		return new StringDataInstance(this, settings, buf, len, true).getLabel(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options);
@@ -160,7 +160,7 @@ public class WideCharDataType extends BuiltIn implements ArrayStringable, DataTy
 	@Override
 	public String getArrayDefaultOffcutLabelPrefix(MemBuffer buf, Settings settings, int len,
 			DataTypeDisplayOptions options, int offcutOffset) {
-		return new StringDataInstance(this, settings, buf, len).getOffcutLabelString(
+		return new StringDataInstance(this, settings, buf, len, true).getOffcutLabelString(
 			AbstractStringDataType.DEFAULT_UNICODE_ABBREV_PREFIX + "_",
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL_PREFIX,
 			AbstractStringDataType.DEFAULT_UNICODE_LABEL, options, offcutOffset);