SCI: Move bpk/logkernel to main breakpoint infrastructure

This changes the syntax for bpk and logkernel:

Enable breakpoint on kernel call:

bpk FrameOut

Enable logging for kernel call:

bpk FrameOut log
For backward compatibility this has an alias: logkernel FrameOut

Removing a kernel call breakpoint is done with bp_del/bc now.

7 files changed, 111 insertions, 133 deletions

diff --git a/engines/sci/console.cpp b/engines/sci/console.cpp
index 3840e33548..44d2a71683 100644
--- a/engines/sci/console.cpp
+++ b/engines/sci/console.cpp
@@ -3698,27 +3698,17 @@ bool Console::cmdGo(int argc, const char **argv) {
 }
 
 bool Console::cmdLogKernel(int argc, const char **argv) {
-	if (argc < 3) {
+	if (argc != 2) {
 		debugPrintf("Logs calls to specified kernel function.\n");
-		debugPrintf("Usage: %s <kernel function/*> <on/off>\n", argv[0]);
-		debugPrintf("Example: %s StrCpy on\n", argv[0]);
+		debugPrintf("Usage: %s <kernel function/*>\n", argv[0]);
+		debugPrintf("Example: %s StrCpy\n", argv[0]);
+		debugPrintf("This is an alias for: bpk <kernel function> log\n");
 		return true;
 	}
 
-	bool logging;
-	if (strcmp(argv[2], "on") == 0)
-		logging = true;
-	else if (strcmp(argv[2], "off") == 0)
-		logging = false;
-	else {
-		debugPrintf("2nd parameter must be either on or off\n");
-		return true;
-	}
+	const char *bpk_argv[] = { "bpk", argv[1], "log" };
+	cmdBreakpointKernel(3, bpk_argv);
 
-	if (g_sci->getKernel()->debugSetFunction(argv[1], logging, -1))
-		debugPrintf("Logging %s for k%s\n", logging ? "enabled" : "disabled", argv[1]);
-	else
-		debugPrintf("Unknown kernel function %s\n", argv[1]);
 	return true;
 }
 
@@ -3759,6 +3749,9 @@ void Console::printBreakpoint(int index, const Breakpoint &bp) {
 	}
 	case BREAK_ADDRESS:
 		debugPrintf("Execute address %04x:%04x%s\n", PRINT_REG(bp._regAddress), bpaction);
+		break;
+	case BREAK_KERNEL:
+		debugPrintf("Kernel call k%s%s\n", bp._name.c_str(), bpaction);
 	}
 }
 
@@ -3996,27 +3989,68 @@ bool Console::cmdBreakpointWrite(int argc, const char **argv) {
 }
 
 bool Console::cmdBreakpointKernel(int argc, const char **argv) {
-	if (argc < 3) {
+	if (argc < 2 || argc > 3) {
 		debugPrintf("Sets a breakpoint on execution of a kernel function.\n");
-		debugPrintf("Usage: %s <name> <on/off>\n", argv[0]);
-		debugPrintf("Example: %s DrawPic on\n", argv[0]);
+		debugPrintf("Usage: %s <name> [<action>]\n", argv[0]);
+		debugPrintf("Example: %s DrawPic\n", argv[0]);
+		debugPrintf("         %s DrawPic log\n", argv[0]);
+		debugPrintf("See bp_action usage for possible actions.\n");
 		return true;
 	}
 
-	bool breakpoint;
-	if (strcmp(argv[2], "on") == 0)
-		breakpoint = true;
-	else if (strcmp(argv[2], "off") == 0)
-		breakpoint = false;
-	else {
-		debugPrintf("2nd parameter must be either on or off\n");
+	BreakpointAction action = BREAK_BREAK;
+	if (argc == 3) {
+		if (!stringToBreakpointAction(argv[2], action)) {
+			debugPrintf("Invalid breakpoint action %s.\n", argv[2]);
+			debugPrintf("See bp_action usage for possible actions.\n");
+			return true;
+		}
+	}
+
+	// Check if any kernel functions match, to catch typos
+	Common::String name = argv[1];
+	bool wildcard = name.lastChar() == '*';
+	Common::String prefix = name;
+	if (wildcard)
+		prefix.deleteLastChar();
+	bool found = false;
+	const Kernel::KernelFunctionArray &kernelFuncs = _engine->getKernel()->_kernelFuncs;
+	for (uint id = 0; id < kernelFuncs.size() && !found; id++) {
+		if (kernelFuncs[id].name) {
+			const KernelSubFunction *kernelSubCall = kernelFuncs[id].subFunctions;
+			if (!kernelSubCall) {
+				Common::String kname = kernelFuncs[id].name;
+				if (name == kname || (wildcard && kname.hasPrefix(prefix)))
+					found = true;
+			} else {
+				uint kernelSubCallCount = kernelFuncs[id].subFunctionCount;
+				for (uint subId = 0; subId < kernelSubCallCount; subId++) {
+					if (kernelSubCall->name) {
+						Common::String kname = kernelSubCall->name;
+						if (name == kname || (wildcard && kname.hasPrefix(prefix)))
+							found = true;
+					}
+					kernelSubCall++;
+				}
+			}
+		}
+	}
+	if (!found) {
+		debugPrintf("No kernel functions match %s.\n", name.c_str());
 		return true;
 	}
 
-	if (g_sci->getKernel()->debugSetFunction(argv[1], -1, breakpoint))
-		debugPrintf("Breakpoint %s for k%s\n", (breakpoint ? "enabled" : "disabled"), argv[1]);
-	else
-		debugPrintf("Unknown kernel function %s\n", argv[1]);
+	Breakpoint bp;
+	bp._type = BREAK_KERNEL;
+	bp._name = name;
+	bp._action = action;
+
+	_debugState._breakpoints.push_back(bp);
+
+	if (action != BREAK_NONE)
+		_debugState._activeBreakpointTypes |= BREAK_KERNEL;
+
+	printBreakpoint(_debugState._breakpoints.size() - 1, bp);
 
 	return true;
 }
diff --git a/engines/sci/debug.h b/engines/sci/debug.h
index afcac19f04..cd1d8095f9 100644
--- a/engines/sci/debug.h
+++ b/engines/sci/debug.h
@@ -31,7 +31,7 @@ namespace Sci {
 // These types are used both as identifiers and as elements of bitfields
 enum BreakpointType {
 	/**
-	 * Break when a selector is executed. Data contains (char *) selector name
+	 * Break when a selector is executed. Data contains selector name
 	 * (in the format Object::Method)
 	 */
 	BREAK_SELECTOREXEC  = 1 << 0, // break when a function selector is executed
@@ -39,11 +39,12 @@ enum BreakpointType {
 	BREAK_SELECTORWRITE = 1 << 2, // break when a variable selector is written
 
 	/**
-	 * Break when an exported function is called. Data contains
+	 * Break when an exported function is called. _address contains
 	 * script_no << 16 | export_no.
 	 */
 	BREAK_EXPORT        = 1 << 3,
-	BREAK_ADDRESS       = 1 << 4  // break when pc is at this address
+	BREAK_ADDRESS       = 1 << 4, // break when pc is at _regAddress
+	BREAK_KERNEL        = 1 << 5  // break on named kernel call
 };
 
 enum BreakpointAction {
diff --git a/engines/sci/engine/kernel.cpp b/engines/sci/engine/kernel.cpp
index efa10c337c..539475c464 100644
--- a/engines/sci/engine/kernel.cpp
+++ b/engines/sci/engine/kernel.cpp
@@ -598,7 +598,6 @@ void Kernel::mapFunctions() {
 		_kernelFuncs[id].workarounds = NULL;
 		_kernelFuncs[id].subFunctions = NULL;
 		_kernelFuncs[id].subFunctionCount = 0;
-		_kernelFuncs[id].debugLogging = false;
 		if (kernelName.empty()) {
 			// No name was given -> must be an unknown opcode
 			warning("Kernel function %x unknown", id);
@@ -715,85 +714,6 @@ void Kernel::mapFunctions() {
 	return;
 }
 
-bool Kernel::debugSetFunction(const char *kernelName, int logging, int breakpoint) {
-	if (strcmp(kernelName, "*")) {
-		for (uint id = 0; id < _kernelFuncs.size(); id++) {
-			if (_kernelFuncs[id].name) {
-				if (strcmp(kernelName, _kernelFuncs[id].name) == 0) {
-					if (_kernelFuncs[id].subFunctions) {
-						// sub-functions available and main name matched, in that case set logging of all sub-functions
-						KernelSubFunction *kernelSubCall = _kernelFuncs[id].subFunctions;
-						uint kernelSubCallCount = _kernelFuncs[id].subFunctionCount;
-						for (uint subId = 0; subId < kernelSubCallCount; subId++) {
-							if (kernelSubCall->function) {
-								if (logging != -1)
-									kernelSubCall->debugLogging = logging == 1 ? true : false;
-								if (breakpoint != -1)
-									kernelSubCall->debugBreakpoint = breakpoint == 1 ? true : false;
-							}
-							kernelSubCall++;
-						}
-						return true;
-					}
-					// function name matched, set for this one and exit
-					if (logging != -1)
-						_kernelFuncs[id].debugLogging = logging == 1 ? true : false;
-					if (breakpoint != -1)
-						_kernelFuncs[id].debugBreakpoint = breakpoint == 1 ? true : false;
-					return true;
-				} else {
-					// main name was not matched
-					if (_kernelFuncs[id].subFunctions) {
-						// Sub-Functions available
-						KernelSubFunction *kernelSubCall = _kernelFuncs[id].subFunctions;
-						uint kernelSubCallCount = _kernelFuncs[id].subFunctionCount;
-						for (uint subId = 0; subId < kernelSubCallCount; subId++) {
-							if (kernelSubCall->function) {
-								if (strcmp(kernelName, kernelSubCall->name) == 0) {
-									// sub-function name matched, set for this one and exit
-									if (logging != -1)
-										kernelSubCall->debugLogging = logging == 1 ? true : false;
-									if (breakpoint != -1)
-										kernelSubCall->debugBreakpoint = breakpoint == 1 ? true : false;
-									return true;
-								}
-							}
-							kernelSubCall++;
-						}
-					}
-				}
-			}
-		}
-		return false;
-	}
-	// Set debugLogging for all calls
-	for (uint id = 0; id < _kernelFuncs.size(); id++) {
-		if (_kernelFuncs[id].name) {
-			if (!_kernelFuncs[id].subFunctions) {
-				// No sub-functions, enable actual kernel function
-				if (logging != -1)
-					_kernelFuncs[id].debugLogging = logging == 1 ? true : false;
-				if (breakpoint != -1)
-					_kernelFuncs[id].debugBreakpoint = breakpoint == 1 ? true : false;
-			} else {
-				// Sub-Functions available, enable those too
-				KernelSubFunction *kernelSubCall = _kernelFuncs[id].subFunctions;
-				uint kernelSubCallCount = _kernelFuncs[id].subFunctionCount;
-				for (uint subId = 0; subId < kernelSubCallCount; subId++) {
-					if (kernelSubCall->function) {
-						if (logging != -1)
-							kernelSubCall->debugLogging = logging == 1 ? true : false;
-						if (breakpoint != -1)
-							kernelSubCall->debugBreakpoint = breakpoint == 1 ? true : false;
-					}
-					kernelSubCall++;
-				}
-			}
-		}
-	}
-	return true;
-}
-
 #ifdef ENABLE_SCI32
 enum {
 	kKernelEntriesSci2 = 0x8b,
diff --git a/engines/sci/engine/kernel.h b/engines/sci/engine/kernel.h
index bddfab531f..d1dce37e48 100644
--- a/engines/sci/engine/kernel.h
+++ b/engines/sci/engine/kernel.h
@@ -143,8 +143,6 @@ struct KernelFunction {
 	const SciWorkaroundEntry *workarounds;
 	KernelSubFunction *subFunctions;
 	uint16 subFunctionCount;
-	bool debugLogging;
-	bool debugBreakpoint;
 };
 
 class Kernel {
@@ -231,11 +229,6 @@ public:
 	 */
 	void loadKernelNames(GameFeatures *features);
 
-	/**
-	 * Sets debug flags for a kernel function
-	 */
-	bool debugSetFunction(const char *kernelName, int logging, int breakpoint);
-
 private:
 	/**
 	 * Loads the kernel selector names.
diff --git a/engines/sci/engine/scriptdebug.cpp b/engines/sci/engine/scriptdebug.cpp
index e2522414e9..9f1b536fec 100644
--- a/engines/sci/engine/scriptdebug.cpp
+++ b/engines/sci/engine/scriptdebug.cpp
@@ -779,6 +779,44 @@ bool SciEngine::checkAddressBreakpoint(const reg32_t &address) {
 	return found;
 }
 
+bool SciEngine::checkKernelBreakpoint(const Common::String &name) {
+	bool found = false;
+
+	if (_debugState._activeBreakpointTypes & BREAK_KERNEL) {
+
+		Common::List<Breakpoint>::const_iterator bp;
+		for (bp = _debugState._breakpoints.begin(); bp != _debugState._breakpoints.end(); ++bp) {
+			if (bp->_action == BREAK_NONE)
+				continue;
+			if (bp->_type != BREAK_KERNEL)
+				continue;
+
+			bool wildcard = bp->_name.lastChar() == '*';
+			Common::String prefix = bp->_name;
+			if (wildcard)
+				prefix.deleteLastChar();
+			if (bp->_name == name || (wildcard && name.hasPrefix(prefix))) {
+				if (bp->_action == BREAK_BREAK) {
+					if (!found)
+						_console->debugPrintf("Break on k%s\n", name.c_str());
+					_debugState.debugging = true;
+					_debugState.breakpointWasHit = true;
+				} else if (bp->_action == BREAK_BACKTRACE) {
+					if (!found)
+						_console->debugPrintf("Break on k%s\n", name.c_str());
+					logBacktrace();
+				} else if (bp->_action == BREAK_INSPECT) {
+					// Ignoring this mode, to make it identical to BREAK_LOG
+				}
+				found = true;
+			}
+		}
+	}
+
+	return found;
+}
+
+
 void debugSelectorCall(reg_t send_obj, Selector selector, int argc, StackPtr argp, ObjVarRef &varp, reg_t funcp, SegManager *segMan, SelectorType selectorType) {
 	int activeBreakpointTypes = g_sci->_debugState._activeBreakpointTypes;
 	const char *objectName = segMan->getObjectName(send_obj);
diff --git a/engines/sci/engine/vm.cpp b/engines/sci/engine/vm.cpp
index 1e088d6d7c..b169e0ec11 100644
--- a/engines/sci/engine/vm.cpp
+++ b/engines/sci/engine/vm.cpp
@@ -296,7 +296,8 @@ ExecStack *send_selector(EngineState *s, reg_t send_obj, reg_t work_obj, StackPt
 			sp = CALL_SP_CARRY; // Destroy sp, as it will be carried over
 		}
 
-		if (activeBreakpointTypes || DebugMan.isDebugChannelEnabled(kDebugLevelScripts))
+		if ((activeBreakpointTypes & (BREAK_SELECTOREXEC | BREAK_SELECTORREAD | BREAK_SELECTORWRITE))
+		     || DebugMan.isDebugChannelEnabled(kDebugLevelScripts))
 			debugSelectorCall(send_obj, selector, argc, argp, varp, funcp, s->_segMan, selectorType);
 
 		assert(argp[0].toUint16() == argc); // The first argument is argc
@@ -375,13 +376,8 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) {
 		addKernelCallToExecStack(s, kernelCallNr, -1, argc, argv);
 		s->r_acc = kernelCall.function(s, argc, argv);
 
-		if (kernelCall.debugLogging)
+		if (g_sci->checkKernelBreakpoint(kernelCall.name))
 			logKernelCall(&kernelCall, NULL, s, argc, argv, s->r_acc);
-		if (kernelCall.debugBreakpoint) {
-			debugN("Break on k%s\n", kernelCall.name);
-			g_sci->_debugState.debugging = true;
-			g_sci->_debugState.breakpointWasHit = true;
-		}
 	} else {
 		// Sub-functions available, check signature and call that one directly
 		if (argc < 1)
@@ -447,13 +443,8 @@ static void callKernelFunc(EngineState *s, int kernelCallNr, int argc) {
 		addKernelCallToExecStack(s, kernelCallNr, subId, argc, argv);
 		s->r_acc = kernelSubCall.function(s, argc, argv);
 
-		if (kernelSubCall.debugLogging)
+		if (g_sci->checkKernelBreakpoint(kernelSubCall.name))
 			logKernelCall(&kernelCall, &kernelSubCall, s, argc, argv, s->r_acc);
-		if (kernelSubCall.debugBreakpoint) {
-			debugN("Break on k%s\n", kernelSubCall.name);
-			g_sci->_debugState.debugging = true;
-			g_sci->_debugState.breakpointWasHit = true;
-		}
 	}
 
 	// Remove callk stack frame again, if there's still an execution stack
diff --git a/engines/sci/sci.h b/engines/sci/sci.h
index 3139f1583b..8109317cdf 100644
--- a/engines/sci/sci.h
+++ b/engines/sci/sci.h
@@ -316,6 +316,7 @@ public:
 	bool checkAddressBreakpoint(const reg32_t &address);
 
 public:
+	bool checkKernelBreakpoint(const Common::String &name);
 
 	/**
 	 * Processes a multilanguage string based on the current language settings and