Commits

spencercw committed 907360a

#7 Implement processing of type chains so variable types are now properly displayed in the variables window.

CDB parsing diagnostics also improved.

  • Participants
  • Parent commits 6108ddd

Comments (0)

Files changed (4)

File gb_debugger_msvs/properties/variable_property.cpp

 		attributes_ |= DBG_ATTRIB_STORAGE_NONE;
 	}
 
-	// Map the type to a string
-	const CdbFile::TypeEntry &typeEntry = variable->type->chain[0];
-	switch (typeEntry.type)
-	{
-	case CdbFile::TypeEntry::LONG:
-		if (typeEntry.sign == CdbFile::TypeEntry::UNSIGNED)
-		{
-			type_ = L"unsigned long";
-		}
-		else
-		{
-			type_ = L"long";
-		}
-		break;
-
-	case CdbFile::TypeEntry::INT:
-		if (typeEntry.sign == CdbFile::TypeEntry::UNSIGNED)
-		{
-			type_ = L"unsigned int";
-		}
-		else
-		{
-			type_ = L"int";
-		}
-		break;
-
-	case CdbFile::TypeEntry::CHAR:
-		if (typeEntry.sign == CdbFile::TypeEntry::UNSIGNED)
-		{
-			type_ = L"unsigned char";
-		}
-		else
-		{
-			type_ = L"char";
-		}
-		break;
-
-	case CdbFile::TypeEntry::SHORT:
-		if (typeEntry.sign == CdbFile::TypeEntry::UNSIGNED)
-		{
-			type_ = L"unsigned short";
-		}
-		else
-		{
-			type_ = L"short";
-		}
-		break;
-
-	case CdbFile::TypeEntry::T_VOID:
-		type_ = L"void";
-		break;
-
-	case CdbFile::TypeEntry::FLOAT:
-		type_ = L"float";
-		break;
-	}
-
 	if (!GetValue(frame_, variable, value_, size_, &error_))
 	{
 		attributes_ |= DBG_ATTRIB_VALUE_ERROR;
 	}
 	if (fields & DEBUGPROP_INFO_TYPE)
 	{
+		CComBSTR type = variable_->type->typeStr.c_str();
 		propertyInfo->dwFields |= DEBUGPROP_INFO_TYPE;
-		propertyInfo->bstrType = SysAllocString(type_.c_str());
+		propertyInfo->bstrType = type;
+		type.Detach();
 	}
 	if (fields & DEBUGPROP_INFO_VALUE)
 	{

File gb_debugger_msvs/properties/variable_property.h

 	boost::shared_ptr<CdbFile::Symbol> variable_;
 
 	std::wstring name_;
-	std::wstring type_;
 	std::wstring error_;
 	uint32_t value_;
 	unsigned size_;

File gb_emulator/include/gb_emulator/cdb_file.h

 		NO_SUCH_FILE,         //!< The given file does not exist.
 		OPEN_FAILED,          //!< Failed to open the given file.
 		READ_ERROR,           //!< There was an error reading the file.
-		BAD_RECORD_TYPE,      //!< A record with an unknown type was seen in the file.
 		INVALID_LINE,         //!< An invalid line was read from the file.
 		DATA_INCONSISTENCY    //!< There is an inconsistency in the data.
 	};
 	};
 
 	//! Struct representing the type of a symbol.
-	struct TypeChain
+	struct Type
 	{
 		//! Size of the type in bytes.
 		unsigned size;
 		//! List of type entries that represent the type.
 		std::vector<TypeEntry> chain;
+		//! The type chain formatted as a C type string.
+		std::string typeStr;
 	};
 
 	//! Struct containing details about a file.
 		//! Name of the symbol.
 		std::string name;
 		//! Type of the symbol.
-		boost::shared_ptr<TypeChain> type;
+		boost::shared_ptr<Type> type;
 		//! Symbol scope level.
 		unsigned level;
 		//! Symbol scope block.
 		unsigned line) const;
 
 private:
+	enum Diagnostic
+	{
+		WARN,
+		ERR
+	};
+
 	bool valid_;
 	std::vector<std::string> modules_;
 	std::map<std::string, boost::shared_ptr<Symbol> > symbols_;
 	std::map<uint64_t, boost::shared_ptr<Line> > asmLines_;
 
 	// Cache of type strings to their associated TypeCache.
-	std::map<std::string, boost::shared_ptr<TypeChain> > typeCache_;
+	std::map<std::string, boost::shared_ptr<Type> > typeCache_;
 
 	// Cache of encoded filename strings to their associated File.
 	std::map<std::string, boost::shared_ptr<File> > filenameCache_;
 
 	ParseError parseSymbol(const std::string &line, bool isFunction,
-		boost::shared_ptr<Symbol> &symbol);
+		boost::shared_ptr<Symbol> &symbol, const boost::filesystem::path &path, unsigned lineNo);
 	ParseError parseSymbolScope(
 		const std::string &scopeStr,
 		const boost::ssub_match &file,
 		const boost::ssub_match &function,
 		SymbolScope &scope,
 		boost::shared_ptr<File> &fileOut,
-		std::string &functionOut);
+		std::string &functionOut,
+		const boost::filesystem::path &path, unsigned lineNo);
 	ParseError parseType(const std::string &typeStr,
-		boost::shared_ptr<TypeChain> &typeChain);
-	ParseError parseLineRecord(const std::string &line, bool isCLine);
+		boost::shared_ptr<Type> &typeChain, const boost::filesystem::path &path, unsigned lineNo);
+	ParseError formatTypeChain(boost::shared_ptr<Type> typeChain,
+		const boost::filesystem::path &path, unsigned lineNo);
+	ParseError parseLineRecord(const std::string &line, bool isCLine,
+		const boost::filesystem::path &path, unsigned lineNo);
 
 	// Decodes the given filename and returns the associated File. This will create a new File
 	// record if none already exists.
-	boost::shared_ptr<File> decodeFilename(const std::string &filename);
+	boost::shared_ptr<File> decodeFilename(const std::string &filename,
+		const boost::filesystem::path &path, unsigned lineNo);
 
 	// Finds the function which covers the given address, if any.
 	boost::shared_ptr<Symbol> findFunction(uint64_t address) const;
+
+	// Prints a diagnostic message
+	void diagnostic(Diagnostic type, const boost::filesystem::path &file, unsigned line,
+		const std::string &message);
 };
 
 #endif

File gb_emulator/src/cdb_file.cpp

 #include <gb_emulator/cdb_file.h>
 
 #include <iomanip>
+#include <iostream>
 #include <sstream>
 
 #include <boost/algorithm/string/classification.hpp>
 using boost::lexical_cast;
 using boost::shared_ptr;
 using boost::token_compress_on;
+using std::clog;
 using std::hex;
 using std::ios_base;
 using std::istream;
 using std::istringstream;
 using std::make_pair;
 using std::map;
+using std::ostringstream;
 using std::pair;
 using std::string;
 using std::vector;
 	// Open the file
 	if (!fs::exists(path))
 	{
+		diagnostic(ERR, path, 0, "file does not exist");
 		return NO_SUCH_FILE;
 	}
 	fs::ifstream f(path, ios_base::in | ios_base::binary);
 	if (!f)
 	{
+		diagnostic(ERR, path, 0, "failed to open file for reading");
 		return OPEN_FAILED;
 	}
 
 	////////////////////////////////////////////////////////////////////////////////////////////////
 	// Parse the file
 
+	
 	string line;
-	while (getLine(f, line))
+	for (unsigned lineNo = 1; getLine(f, line); ++lineNo)
 	{
 		// Ignore empty lines
 		if (line.empty())
 				boost::smatch results;
 				if (!regex_match(line, results, regex))
 				{
+					diagnostic(ERR, path, lineNo, "module record does not match specification");
 					return INVALID_LINE;
 				}
 				modules_.push_back(results[1].str());
 		case 'F':
 			{
 				shared_ptr<Symbol> function;
-				ParseError result = parseSymbol(line, true, function);
+				ParseError result = parseSymbol(line, true, function, path, lineNo);
 				if (result)
 				{
 					return result;
 		case 'S':
 			{
 				shared_ptr<Symbol> symbol;
-				ParseError result = parseSymbol(line, false, symbol);
+				ParseError result = parseSymbol(line, false, symbol, path, lineNo);
 				if (result)
 				{
 					return result;
 		case 'L':
 			if (line.length() < 3 || line[1] != ':')
 			{
+				diagnostic(ERR, path, lineNo, "linker record does not match specification");
 				return INVALID_LINE;
 			}
 			switch (line[2])
 					boost::smatch results;
 					if (!regex_match(line, results, regex))
 					{
+						diagnostic(ERR, path, lineNo, "linker record does not match specification");
 						return INVALID_LINE;
 					}
 
 					SymbolAddress symAddr;
 					ParseError result = parseSymbolScope(results[1], results[2], results[3],
-						symAddr.scope, symAddr.file, symAddr.function);
+						symAddr.scope, symAddr.file, symAddr.function, path, lineNo);
 					if (result)
 					{
 						return result;
 			// ASM line
 			case 'A':
 				{
-					ParseError result = parseLineRecord(line, false);
+					ParseError result = parseLineRecord(line, false, path, lineNo);
 					if (result)
 					{
 						return result;
 			// C line
 			case 'C':
 				{
-					ParseError result = parseLineRecord(line, true);
+					ParseError result = parseLineRecord(line, true, path, lineNo);
 					if (result)
 					{
 						return result;
 				break;
 
 			default:
-				return INVALID_LINE;
+				diagnostic(WARN, path, lineNo, "unrecognised linker record type");
 			}
 			break;
 
 		default:
-			return BAD_RECORD_TYPE;
+			diagnostic(WARN, path, lineNo, "unrecognised record type");
 		}
 	}
 
 	if (!f.eof())
 	{
+		diagnostic(ERR, path, 0, "an error occurred whilst reading the file");
 		return READ_ERROR;
 	}
 
 		map<string, shared_ptr<Symbol> >::iterator symbol = symbols_.find((*function)->name);
 		if (symbol == symbols_.end())
 		{
+			diagnostic(ERR, path, 0, "function record '" + (*function)->name +
+				"' does not have associated symbol");
 			return DATA_INCONSISTENCY;
 		}
 
 		    symbol->second->block     != (*function)->block ||
 		    symbol->second->onStack   != (*function)->onStack)
 		{
+			diagnostic(ERR, path, 0, "symbol record data for function '" + (*function)->name +
+				"' does not match the data in the function record");
 			return DATA_INCONSISTENCY;
 		}
 
 		map<string, shared_ptr<Symbol> >::iterator symbol = symbols_.find(symAddr->name);
 		if (symbol == symbols_.end())
 		{
+			diagnostic(WARN, path, 0, "ignoring symbol start address record for '" +
+				symAddr->name + "' because it has no associated symbol record");
 			continue;
 		}
 
 		    symbol->second->level     != symAddr->level ||
 		    symbol->second->block     != symAddr->block)
 		{
+			diagnostic(ERR, path, 0, "symbol record data for '" + symAddr->name +
+				"' does not match the data in the symbol start address record");
 			return DATA_INCONSISTENCY;
 		}
 
 		map<string, shared_ptr<Symbol> >::iterator symbol = symbols_.find(symAddr->name);
 		if (symbol == symbols_.end())
 		{
+			diagnostic(WARN, path, 0, "ignoring symbol end address record for '" +
+				symAddr->name + "' because it has no associated symbol record");
 			continue;
 		}
 		if (symbol->second->scope     != symAddr->scope ||
 		    symbol->second->level     != symAddr->level ||
 		    symbol->second->block     != symAddr->block)
 		{
+			diagnostic(ERR, path, 0, "symbol record data for '" + symAddr->name +
+				"' does not match the data in the symbol end address record");
 			return DATA_INCONSISTENCY;
 		}
 		symbol->second->endAddressValid = true;
 		if (function == symbols_.end())
 		{
 			// Local with no function?
+			diagnostic(ERR, path, 0, "variable '" + (*symbol)->name + "' refers to function '" +
+				(*symbol)->function + "', but no symbol record exists for this function");
 			return DATA_INCONSISTENCY;
 		}
 
 			if (!function)
 			{
 				// Source code line outside a function?
+				fs::path filename = line->second->file->filename.filename();
+				ostringstream oss;
+				oss << "no function record found for " << filename.string() << ":" << line->second->line;
+				diagnostic(ERR, path, 0, oss.str());
 				return DATA_INCONSISTENCY;
 			}
 
 }
 
 CdbFile::ParseError CdbFile::parseSymbol(const string &line, bool isFunction,
-	shared_ptr<CdbFile::Symbol> &symbol)
+	shared_ptr<CdbFile::Symbol> &symbol, const fs::path &path, unsigned lineNo)
 {
 	static const boost::regex symbolRegex(
 		"S:"
 	boost::smatch results;
 	if (!regex_match(line, results, regex))
 	{
+		diagnostic(ERR, path, lineNo, "symbol record does not match specification");
 		return INVALID_LINE;
 	}
 
 	symbol.reset(new Symbol());
 	ParseError result = parseSymbolScope(results[1], results[2], results[3],
-		symbol->scope, symbol->file, symbol->function);
+		symbol->scope, symbol->file, symbol->function, path, lineNo);
 	if (result)
 	{
 		return result;
 	symbol->level = lexical_cast<unsigned>(results[5]);
 	symbol->block = lexical_cast<unsigned>(results[6]);
 
-	result = parseType(results[7], symbol->type);
+	result = parseType(results[7], symbol->type, path, lineNo);
 	if (result)
 	{
 		return result;
 	if (isFunction &&
 		(symbol->type->chain.size() < 2 || symbol->type->chain[0].type != TypeEntry::FUNCTION))
 	{
+		diagnostic(ERR, path, lineNo, "function type must start with a function followed by at "
+			"least one further entry");
 		return INVALID_LINE;
 	}
 
 	const boost::ssub_match &function,
 	CdbFile::SymbolScope &scope,
 	shared_ptr<File> &fileOut,
-	string &functionOut)
+	string &functionOut,
+	const fs::path &path, unsigned lineNo)
 {
 	switch (scopeStr[0])
 	{
 		{
 			if (!file.matched)
 			{
+				diagnostic(ERR, path, lineNo, "filename missing from symbol scope");
 				return INVALID_LINE;
 			}
 			scope = FILE;
-			fileOut = decodeFilename(file.str());
+			fileOut = decodeFilename(file.str(), path, lineNo);
 			functionOut.clear();
 		}
 		break;
 		{
 			if (!function.matched)
 			{
+				diagnostic(ERR, path, lineNo, "function name missing from symbol scope");
 				return INVALID_LINE;
 			}
 			scope = FUNCTION;
 		break;
 
 	default:
-		return INVALID_LINE;
+		assert(!"all scope types should have been handled");
 	}
 
 	return PARSE_OK;
 }
 
-CdbFile::ParseError CdbFile::parseType(const string &typeStr,
-	shared_ptr<CdbFile::TypeChain> &typeChain)
+CdbFile::ParseError CdbFile::parseType(const string &typeStr, shared_ptr<CdbFile::Type> &type,
+	const fs::path &path, unsigned lineNo)
 {
 	// See if we have already parsed this type
-	map<string, shared_ptr<TypeChain> >::const_iterator cachedType = typeCache_.find(typeStr);
+	map<string, shared_ptr<Type> >::const_iterator cachedType = typeCache_.find(typeStr);
 	if (cachedType != typeCache_.end())
 	{
-		typeChain = cachedType->second;
+		type = cachedType->second;
 		return PARSE_OK;
 	}
-	shared_ptr<TypeChain> type(new TypeChain());
+	shared_ptr<Type> newType(new Type());
 
 	// Get the size of the type and split out the repeated components
 	static const boost::regex typeRegex("\\{([0-9]+)\\}(.+?)");
 	boost::smatch results;
 	if (!regex_match(typeStr, results, typeRegex))
 	{
+		diagnostic(ERR, path, lineNo, "type record does not match specification");
 		return INVALID_LINE;
 	}
-	type->size = lexical_cast<unsigned>(results[1]);
+	newType->size = lexical_cast<unsigned>(results[1]);
 
 	// Split the components
 	string typeData = results[2].str();
 	// Parse each component
 	static const boost::regex componentRegex(
 		"((DA)([0-9]+)|(DF)|(DG)|(DC)|(DX)|(DD)|(DP)|(DI)|(SL)|(SI)|(SC)|"
-		"(SS)|(SV)|(SF)|(ST)(.+?)|(SX)|(SB)([0-9]+))(:[US])?");
+		"(SS)|(SV)|(SF)|(ST)(.+?)|(SX)|(SB)([0-9]+)\\$([0-9]+))(:[US])?");
 	for (vector<string>::const_iterator component = components.begin(), end = components.end();
 		component != end; ++component)
 	{
 		}
 
 		// Extract the signedness
-		boost::ssub_match sign = componentResults[22];
+		boost::ssub_match sign = componentResults[23];
 		if (!sign.matched)
 		{
 			entry.sign = TypeEntry::NONE;
 			assert(!"all sign types should have been handled");
 		}
 
-		type->chain.push_back(entry);
+		newType->chain.push_back(entry);
 	}
 
-	typeCache_.insert(make_pair(typeStr, type));
-	typeChain = type;
+	// Format the type chain into a string
+	ParseError result = formatTypeChain(newType, path, lineNo);
+	if (result != PARSE_OK)
+	{
+		return result;
+	}
+
+	typeCache_.insert(make_pair(typeStr, newType));
+	type = newType;
 	return PARSE_OK;
 }
 
-CdbFile::ParseError CdbFile::parseLineRecord(const string &line, bool isCLine)
+CdbFile::ParseError CdbFile::parseLineRecord(const string &line, bool isCLine, const fs::path &path,
+	unsigned lineNo)
 {
 	static const boost::regex cLineRegex(
 		"L:C"
 	boost::smatch results;
 	if (!regex_match(line, results, regex))
 	{
+		diagnostic(ERR, path, lineNo, "linker line record does not match specification");
 		return INVALID_LINE;
 	}
 
 	shared_ptr<Line> lineData(new Line());
-	lineData->file = decodeFilename(results[1]);
+	lineData->file = decodeFilename(results[1], path, lineNo);
 	lineData->line = lexical_cast<unsigned>(results[2]);
 	if (lineData->line == 0)
 	{
+		diagnostic(ERR, path, lineNo, "line number must be greater than zero");
 		return INVALID_LINE;
 	}
 	--lineData->line;
 	return PARSE_OK;
 }
 
-shared_ptr<CdbFile::File> CdbFile::decodeFilename(const string &filename)
+shared_ptr<CdbFile::File> CdbFile::decodeFilename(const string &filename, const fs::path &path,
+	unsigned lineNo)
 {
 	// The standard CDB format only includes the filename of the source file; not the full path.
 	// This is an issue because it makes it impossible to automatically locate source files
 	// error. If so just use the input string.
 	if (state != NORMAL || !ok)
 	{
+		diagnostic(WARN, path, lineNo, "decoding error on filename; using filename string "
+			"verbatim");
 		file->filename = filename;
 	}
 	else
 	return shared_ptr<CdbFile::Symbol>();
 }
 
+void CdbFile::diagnostic(CdbFile::Diagnostic type, const fs::path &file, unsigned line,
+	const string &message)
+{
+	clog << file.filename().string() << ":";
+	if (line)
+	{
+		clog << line << ":";
+	}
+
+	switch (type)
+	{
+	case WARN:
+		clog << " warning: ";
+		break;
+
+	case ERR:
+		clog << " error: ";
+		break;
+
+	default:
+		assert(!"all diagnostic types should have been handled");
+	}
+	clog << message << "\n";
+}
+
 bool CdbFile::findSource(uint64_t address, CdbFile::SourceLocation &sourceLocation) const
 {
 	if (!valid_)
 		return shared_ptr<Line>();
 	}
 }
+
+static void formatPrimitive(vector<CdbFile::TypeEntry>::const_iterator entry, const string &name,
+	string &str)
+{
+	if (entry->sign == CdbFile::TypeEntry::UNSIGNED)
+	{
+		str = "unsigned " + name + str;
+	}
+	else
+	{
+		str = name + str;
+	}
+}
+
+CdbFile::ParseError CdbFile::formatTypeChain(shared_ptr<Type> type, const fs::path &path,
+	unsigned lineNo)
+{
+	if (type->chain.empty())
+	{
+		diagnostic(ERR, path, lineNo, "empty type chain");
+		return INVALID_LINE;
+	}
+
+	enum State
+	{
+		NORMAL,
+		CODE_POINTER,
+		BASIC_TYPE
+	};
+
+	State state = NORMAL;
+	for (vector<TypeEntry>::const_iterator entry = type->chain.begin(), end = type->chain.end();
+		entry != end; ++entry)
+	{
+		switch (state)
+		{
+		case NORMAL:
+			switch (entry->type)
+			{
+			case TypeEntry::ARRAY:
+				{
+					ostringstream oss;
+					oss << "[" << entry->count << "]";
+					type->typeStr += oss.str();
+				}
+				break;
+
+			case TypeEntry::FUNCTION:
+				type->typeStr = "() " + type->typeStr;
+				break;
+
+			case TypeEntry::CODE_POINTER:
+				state = CODE_POINTER;
+				break;
+
+			case TypeEntry::GENERIC_POINTER:
+			case TypeEntry::EXTERNAL_RAM_POINTER:
+			case TypeEntry::INTERNAL_RAM_POINTER:
+			case TypeEntry::PAGED_POINTER:
+			case TypeEntry::UPPER_128_BYTE_POINTER:
+				type->typeStr = "* " + type->typeStr;
+				break;
+
+			case TypeEntry::LONG:
+				formatPrimitive(entry, "long ", type->typeStr);
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::INT:
+				formatPrimitive(entry, "int ", type->typeStr);
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::CHAR:
+				formatPrimitive(entry, "char ", type->typeStr);
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::SHORT:
+				formatPrimitive(entry, "short ", type->typeStr);
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::T_VOID:
+				type->typeStr = "void " + type->typeStr;
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::FLOAT:
+				type->typeStr = "float " + type->typeStr;
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::STRUCT:
+				type->typeStr = "struct " + entry->name + " " + type->typeStr;
+				state = BASIC_TYPE;
+				break;
+
+			case TypeEntry::SBIT:
+				diagnostic(WARN, path, lineNo, "sbits not yet handled in type chain formatting");
+				break;
+
+			case TypeEntry::BIT_FIELD:
+				diagnostic(WARN, path, lineNo, "bit fields not yet handled in type chain "
+					"formatting");
+				break;
+
+			default:
+				assert(!"all types should have been handled");
+			}
+			break;
+
+		case CODE_POINTER:
+			if (entry->type != TypeEntry::FUNCTION)
+			{
+				diagnostic(ERR, path, lineNo, "code pointer is not followed by a function");
+				return INVALID_LINE;
+			}
+			type->typeStr = "()* " + type->typeStr;
+			state = NORMAL;
+			break;
+
+		case BASIC_TYPE:
+			diagnostic(ERR, path, lineNo, "a basic type (a primitive or struct) may only appear "
+				"at the end of a type chain");
+			return INVALID_LINE;
+
+		default:
+			assert(!"all states should have been handled");
+		}
+	}
+
+	if (state != BASIC_TYPE)
+	{
+		diagnostic(ERR, path, lineNo, "the type chain does not end with a basic type (i.e., a "
+			"primitive or struct)");
+		return INVALID_LINE;
+	}
+
+	// Remove the trailing space if necessary
+	assert(!type->typeStr.empty());
+	if (*(type->typeStr.end() - 1) == ' ')
+	{
+		type->typeStr.resize(type->typeStr.length() - 1);
+	}
+
+	return PARSE_OK;
+}