- record all line numbers during function generation. This is useful for error reporting and eventually debugging.

- throw a useful exception when a VM abort occurs, the simple enum was incapable of reporting anything more than the barest minimum, which at least for array index out of bounds errors was insufficient.

The current exception mechanism is still insufficient. It really has to report a proper crash location and print a stack trace to the maximum extent possible. Instead it just prints a message and happily goes on. This is not a good solution.

src/doomerrors.h

@@ -56,6 +56,12 @@ public:
 		strncpy (m_Message, message, MAX_ERRORTEXT-1);
 		m_Message[MAX_ERRORTEXT-1] = '\0';
 	}
+	void AppendMessage(const char *message)
+	{
+		size_t len = strlen(m_Message);
+		strncpy(m_Message + len, message, MAX_ERRORTEXT - 1 - len);
+		m_Message[MAX_ERRORTEXT - 1] = '\0';
+	}
 	const char *GetMessage (void) const
 	{
 		if (m_Message[0] != '\0')
@@ -64,7 +70,7 @@ public:
 			return NULL;
 	}
 
-private:
+protected:
 	char m_Message[MAX_ERRORTEXT];
 };
 

src/sc_man.cpp

@@ -1007,7 +1007,7 @@ void FScanner::CheckOpen()
 //==========================================================================
 int FScriptPosition::ErrorCounter;
 int FScriptPosition::WarnCounter;
-bool FScriptPosition::StrictErrors;	// makes all OPTERRPR messages real errors.
+bool FScriptPosition::StrictErrors;	// makes all OPTERROR messages real errors.
 
 FScriptPosition::FScriptPosition(const FScriptPosition &other)
 {

src/scripting/codegeneration/codegen.cpp

@@ -288,6 +288,21 @@ ExpEmit FxExpression::Emit (VMFunctionBuilder *build)
 }
 
 
+//==========================================================================
+//
+// Emits a statement and records its position in the source.
+//
+//==========================================================================
+
+void FxExpression::EmitStatement(VMFunctionBuilder *build)
+{
+	build->BeginStatement(this);
+	ExpEmit exp = Emit(build);
+	exp.Free(build);
+	build->EndStatement();
+}
+
+
 //==========================================================================
 //
 //
@@ -8519,9 +8534,7 @@ ExpEmit FxSequence::Emit(VMFunctionBuilder *build)
 {
 	for (unsigned i = 0; i < Expressions.Size(); ++i)
 	{
-		ExpEmit v = Expressions[i]->Emit(build);
+		Expressions[i]->EmitStatement(build);
-		// Throw away any result. We don't care about it.
-		v.Free(build);
 	}
 	return ExpEmit();
 }
@@ -8817,7 +8830,7 @@ ExpEmit FxSwitchStatement::Emit(VMFunctionBuilder *build)
 			break;
 
 		default:
-			line->Emit(build);
+			line->EmitStatement(build);
 			break;
 		}
 	}
@@ -8979,13 +8992,13 @@ ExpEmit FxIfStatement::Emit(VMFunctionBuilder *build)
 	if (WhenTrue != nullptr)
 	{
 		build->BackpatchListToHere(yes);
-		WhenTrue->Emit(build);
+		WhenTrue->EmitStatement(build);
 	}
 	if (WhenFalse != nullptr)
 	{
 		if (!WhenTrue->CheckReturn()) jumpspot = build->Emit(OP_JMP, 0);	// no need to emit a jump if the block returns.
 		build->BackpatchListToHere(no);
-		WhenFalse->Emit(build);
+		WhenFalse->EmitStatement(build);
 		if (jumpspot != ~0u) build->BackpatchToHere(jumpspot);
 		if (WhenTrue == nullptr) build->BackpatchListToHere(yes);
 	}
@@ -9114,8 +9127,7 @@ ExpEmit FxWhileLoop::Emit(VMFunctionBuilder *build)
 	// Execute the loop's content.
 	if (Code != nullptr)
 	{
-		ExpEmit code = Code->Emit(build);
+		Code->EmitStatement(build);
-		code.Free(build);
 	}
 
 	// Loop back.
@@ -9190,8 +9202,7 @@ ExpEmit FxDoWhileLoop::Emit(VMFunctionBuilder *build)
 	codestart = build->GetAddress();
 	if (Code != nullptr)
 	{
-		ExpEmit code = Code->Emit(build);
+		Code->EmitStatement(build);
-		code.Free(build);
 	}
 
 	// Evaluate the condition and execute/break out of the loop.
@@ -9301,8 +9312,7 @@ ExpEmit FxForLoop::Emit(VMFunctionBuilder *build)
 	// Execute the loop's content.
 	if (Code != nullptr)
 	{
-		ExpEmit code = Code->Emit(build);
+		Code->EmitStatement(build);
-		code.Free(build);
 	}
 
 	// Iteration statement.

src/scripting/codegeneration/codegen.h

@@ -328,6 +328,7 @@ public:
 	bool IsObject() const { return ValueType->IsKindOf(RUNTIME_CLASS(PPointer)) && !ValueType->IsKindOf(RUNTIME_CLASS(PClassPointer)) && ValueType != TypeNullPtr && static_cast<PPointer*>(ValueType)->PointedType->IsKindOf(RUNTIME_CLASS(PClass)); }
 
 	virtual ExpEmit Emit(VMFunctionBuilder *build);
+	void EmitStatement(VMFunctionBuilder *build);
 	virtual void EmitCompare(VMFunctionBuilder *build, bool invert, TArray<size_t> &patchspots_yes, TArray<size_t> &patchspots_no);
 
 	FScriptPosition ScriptPosition;

src/scripting/vm/vm.h

@@ -5,6 +5,7 @@
 #include "autosegs.h"
 #include "vectors.h"
 #include "cmdlib.h"
+#include "doomerrors.h"
 
 #define MAX_RETURNS		8	// Maximum number of results a function called by script code can return
 #define MAX_TRY_DEPTH	8	// Maximum number of nested TRYs in a single function
@@ -189,6 +190,13 @@ enum EVMAbortException
 	X_BAD_SELF,
 };
 
+class CVMAbortException : public CDoomError
+{
+public:
+	static FString stacktrace;
+	CVMAbortException(EVMAbortException reason, const char *moreinfo, ...);
+};
+
 enum EVMOpMode
 {
 	MODE_ASHIFT		= 0,
@@ -813,6 +821,7 @@ public:
 	const VM_ATAG *KonstATags() const { return (VM_UBYTE *)(KonstA + NumKonstA); }
 
 	VMOP *Code;
+	FString SourceFileName;
 	int *KonstD;
 	double *KonstF;
 	FString *KonstS;

src/scripting/vm/vmbuilder.cpp

@@ -75,12 +75,38 @@ VMFunctionBuilder::~VMFunctionBuilder()
 
 //==========================================================================
 //
-// VMFunctionBuilder :: MakeFunction
+// VMFunctionBuilder :: BeginStatement
 //
-// Creates a new VMScriptFunction out of the data passed to this class.
+// Records the start of a new statement.
 //
 //==========================================================================
 
+void VMFunctionBuilder::BeginStatement(FxExpression *stmt)
+{
+	// pop empty statement records.
+	while (LineNumbers.Size() > 0 && LineNumbers.Last().InstructionIndex == Code.Size()) LineNumbers.Pop();
+	// only add a new entry if the line number differs.
+	if (LineNumbers.Size() == 0 || stmt->ScriptPosition.ScriptLine != LineNumbers.Last().LineNumber)
+	{
+		StatementInfo si = { (uint16_t)Code.Size(), (uint16_t)stmt->ScriptPosition.ScriptLine };
+		LineNumbers.Push(si);
+	}
+	StatementStack.Push(stmt);
+}
+
+void VMFunctionBuilder::EndStatement()
+{
+	// pop empty statement records.
+	while (LineNumbers.Size() > 0 && LineNumbers.Last().InstructionIndex == Code.Size()) LineNumbers.Pop();
+	StatementStack.Pop();
+	// Re-enter the previous statement.
+	if (StatementStack.Size() > 0)
+	{
+		StatementInfo si = { (uint16_t)Code.Size(), (uint16_t)StatementStack.Last()->ScriptPosition.ScriptLine };
+		LineNumbers.Push(si);
+	}
+}
+
 void VMFunctionBuilder::MakeFunction(VMScriptFunction *func)
 {
 	func->Alloc(Code.Size(), IntConstantList.Size(), FloatConstantList.Size(), StringConstantList.Size(), AddressConstantList.Size());
@@ -880,7 +906,10 @@ void FFunctionBuildList::Build()
 			// Emit code
 			try
 			{
+				sfunc->SourceFileName = item.Code->ScriptPosition.FileName;	// remember the file name for printing error messages if something goes wrong in the VM.
+				buildit.BeginStatement(item.Code);
 				item.Code->Emit(&buildit);
+				buildit.EndStatement();
 				buildit.MakeFunction(sfunc);
 				sfunc->NumArgs = 0;
 				// NumArgs for the VMFunction must be the amount of stack elements, which can differ from the amount of logical function arguments if vectors are in the list.

src/scripting/vm/vmbuilder.h

@@ -4,6 +4,7 @@
 #include "dobject.h"
 
 class VMFunctionBuilder;
+class FxExpression;
 
 struct ExpEmit
 {
@@ -42,6 +43,8 @@ public:
 	VMFunctionBuilder(int numimplicits);
 	~VMFunctionBuilder();
 
+	void BeginStatement(FxExpression *stmt);
+	void EndStatement();
 	void MakeFunction(VMScriptFunction *func);
 
 	// Returns the constant register holding the value.
@@ -97,6 +100,15 @@ private:
 		VM_ATAG Tag;
 	};
 
+	struct StatementInfo
+	{
+		uint16_t InstructionIndex;
+		uint16_t LineNumber;
+	};
+
+	TArray<StatementInfo> LineNumbers;
+	TArray<FxExpression *> StatementStack;
+
 	TArray<int> IntConstantList;
 	TArray<double> FloatConstantList;
 	TArray<void *> AddressConstantList;

src/scripting/vm/vmexec.cpp

@@ -82,8 +82,6 @@
 #define ASSERTKA(x)		assert(sfunc != NULL && (unsigned)(x) < sfunc->NumKonstA)
 #define ASSERTKS(x)		assert(sfunc != NULL && (unsigned)(x) < sfunc->NumKonstS)
 
-#define THROW(x)		throw(EVMAbortException(x))
-
 #define CMPJMP(test) \
 	if ((test) == (a & CMP_CHECK)) { \
 		assert(pc[1].op == OP_JMP); \
@@ -93,7 +91,7 @@
 	}
 
 #define GETADDR(a,o,x) \
-	if (a == NULL) { THROW(x); } \
+	if (a == NULL) { throw CVMAbortException(x, nullptr); } \
 	ptr = (VM_SBYTE *)a + o
 
 static const VM_UWORD ZapTable[16] =
@@ -231,4 +229,55 @@ void VMFillParams(VMValue *params, VMFrame *callee, int numparam)
 
 void NullParam(const char *varname)
 {
-}
+	throw CVMAbortException(X_READ_NIL, "In function parameter %s", varname);
+}
+
+FString CVMAbortException::stacktrace;
+
+CVMAbortException::CVMAbortException(EVMAbortException reason, const char *moreinfo, ...)
+{
+	SetMessage("VM execution aborted: ");
+	switch (reason)
+	{
+	case X_READ_NIL:
+		AppendMessage("tried to read from address zero.");
+		break;
+
+	case X_WRITE_NIL:
+		AppendMessage("tried to write to address zero.");
+		break;
+
+	case X_TOO_MANY_TRIES:
+		AppendMessage("too many try-catch blocks.");
+		break;
+
+	case X_ARRAY_OUT_OF_BOUNDS:
+		AppendMessage("array access out of bounds.");
+		break;
+
+	case X_DIVISION_BY_ZERO:
+		AppendMessage("division by zero.");
+		break;
+
+	case X_BAD_SELF:
+		AppendMessage("invalid self pointer.");
+		break;
+
+	default:
+	{
+		size_t len = strlen(m_Message);
+		mysnprintf(m_Message + len, MAX_ERRORTEXT - len, "Unknown reason %d", reason);
+		break;
+	}
+	}
+	if (moreinfo != nullptr)
+	{
+		AppendMessage(" ");
+		va_list ap;
+		va_start(ap, moreinfo);
+		size_t len = strlen(m_Message);
+		myvsnprintf(m_Message + len, MAX_ERRORTEXT - len, moreinfo, ap);
+		va_end(ap);
+	}
+}
+

src/scripting/vm/vmexec.h

@@ -704,7 +704,7 @@ begin:
 		assert(try_depth < MAX_TRY_DEPTH);
 		if (try_depth >= MAX_TRY_DEPTH)
 		{
-			THROW(X_TOO_MANY_TRIES);
+			throw CVMAbortException(X_TOO_MANY_TRIES, nullptr);
 		}
 		assert((pc + JMPOFS(pc) + 1)->op == OP_CATCH);
 		exception_frames[try_depth++] = pc + JMPOFS(pc) + 1;
@@ -727,7 +727,7 @@ begin:
 		}
 		else
 		{
-			THROW(BC);
+			throw CVMAbortException(EVMAbortException(BC), nullptr);
 		}
 		NEXTOP;
 	OP(CATCH):
@@ -736,13 +736,10 @@ begin:
 		assert(0);
 		NEXTOP;
 
-		// Fixme: This really needs to throw something more informative than a number. Printing the message here instead of passing it to the exception is not sufficient.
 	OP(BOUND):
 		if (reg.d[a] >= BC)
 		{
-			assert(false);
+			throw CVMAbortException(X_ARRAY_OUT_OF_BOUNDS, "Max.index = %u, current index = %u\n", BC, reg.d[a]);
-			Printf("Array access out of bounds: Max. index = %u, current index = %u\n", BC, reg.d[a]);
-			THROW(X_ARRAY_OUT_OF_BOUNDS);
 		}
 		NEXTOP;
 
@@ -750,9 +747,7 @@ begin:
 		ASSERTKD(BC);
 		if (reg.d[a] >= konstd[BC])
 		{
-			assert(false);
+			throw CVMAbortException(X_ARRAY_OUT_OF_BOUNDS, "Max.index = %u, current index = %u\n", konstd[BC], reg.d[a]);
-			Printf("Array access out of bounds: Max. index = %u, current index = %u\n", konstd[BC], reg.d[a]);
-			THROW(X_ARRAY_OUT_OF_BOUNDS);
 		}
 		NEXTOP;
 
@@ -760,9 +755,7 @@ begin:
 		ASSERTD(B);
 		if (reg.d[a] >= reg.d[B])
 		{
-			assert(false);
+			throw CVMAbortException(X_ARRAY_OUT_OF_BOUNDS, "Max.index = %u, current index = %u\n", reg.d[B], reg.d[a]);
-			Printf("Array access out of bounds: Max. index = %u, current index = %u\n", reg.d[B], reg.d[a]);
-			THROW(X_ARRAY_OUT_OF_BOUNDS);
 		}
 		NEXTOP;
 
@@ -909,7 +902,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = reg.d[B] / reg.d[C];
 		NEXTOP;
@@ -917,7 +910,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTKD(C);
 		if (konstd[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = reg.d[B] / konstd[C];
 		NEXTOP;
@@ -925,7 +918,7 @@ begin:
 		ASSERTD(a); ASSERTKD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = konstd[B] / reg.d[C];
 		NEXTOP;
@@ -934,7 +927,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)reg.d[B] / (unsigned)reg.d[C]);
 		NEXTOP;
@@ -942,7 +935,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTKD(C);
 		if (konstd[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)reg.d[B] / (unsigned)konstd[C]);
 		NEXTOP;
@@ -950,7 +943,7 @@ begin:
 		ASSERTD(a); ASSERTKD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)konstd[B] / (unsigned)reg.d[C]);
 		NEXTOP;
@@ -959,7 +952,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = reg.d[B] % reg.d[C];
 		NEXTOP;
@@ -967,7 +960,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTKD(C);
 		if (konstd[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = reg.d[B] % konstd[C];
 		NEXTOP;
@@ -975,7 +968,7 @@ begin:
 		ASSERTD(a); ASSERTKD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = konstd[B] % reg.d[C];
 		NEXTOP;
@@ -984,7 +977,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)reg.d[B] % (unsigned)reg.d[C]);
 		NEXTOP;
@@ -992,7 +985,7 @@ begin:
 		ASSERTD(a); ASSERTD(B); ASSERTKD(C);
 		if (konstd[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)reg.d[B] % (unsigned)konstd[C]);
 		NEXTOP;
@@ -1000,7 +993,7 @@ begin:
 		ASSERTD(a); ASSERTKD(B); ASSERTD(C);
 		if (reg.d[C] == 0)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.d[a] = int((unsigned)konstd[B] % (unsigned)reg.d[C]);
 		NEXTOP;
@@ -1178,7 +1171,7 @@ begin:
 		ASSERTF(a); ASSERTF(B); ASSERTF(C);
 		if (reg.f[C] == 0.)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.f[a] = reg.f[B] / reg.f[C];
 		NEXTOP;
@@ -1186,7 +1179,7 @@ begin:
 		ASSERTF(a); ASSERTF(B); ASSERTKF(C);
 		if (konstf[C] == 0.)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.f[a] = reg.f[B] / konstf[C];
 		NEXTOP;
@@ -1194,7 +1187,7 @@ begin:
 		ASSERTF(a); ASSERTKF(B); ASSERTF(C);
 		if (reg.f[C] == 0.)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.f[a] = konstf[B] / reg.f[C];
 		NEXTOP;
@@ -1205,7 +1198,7 @@ begin:
 	Do_MODF:
 		if (fc == 0.)
 		{
-			THROW(X_DIVISION_BY_ZERO);
+			throw CVMAbortException(X_DIVISION_BY_ZERO, nullptr);
 		}
 		reg.f[a] = luai_nummod(fb, fc);
 		NEXTOP;

src/scripting/vm/vmframe.cpp

@@ -453,7 +453,7 @@ int VMFrameStack::Call(VMFunction *func, VMValue *params, int numparams, VMRetur
 		}
 		throw;
 	}
-	catch (EVMAbortException exception)
+	catch (CVMAbortException &exception)
 	{
 		if (allocated)
 		{
@@ -464,35 +464,7 @@ int VMFrameStack::Call(VMFunction *func, VMValue *params, int numparams, VMRetur
 			*trap = nullptr;
 		}
 
-		Printf("VM execution aborted: ");
+		Printf("%s\n", exception.GetMessage());
-		switch (exception)
-		{
-		case X_READ_NIL:
-			Printf("tried to read from address zero.");
-			break;
-
-		case X_WRITE_NIL:
-			Printf("tried to write to address zero.");
-			break;
-
-		case X_TOO_MANY_TRIES:
-			Printf("too many try-catch blocks.");
-			break;
-
-		case X_ARRAY_OUT_OF_BOUNDS:
-			Printf("array access out of bounds.");
-			break;
-
-		case X_DIVISION_BY_ZERO:
-			Printf("division by zero.");
-			break;
-
-		case X_BAD_SELF:
-			Printf("invalid self pointer.");
-			break;
-		}
-		Printf("\n");
-
 		return -1;
 	}
 	catch (...)