Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ dotnet test tests/Spice86.Tests --filter 'FullyQualifiedName!~SingleStepTest'

> **SingleStepTest exclusion rule**: `SingleStepTest` runs millions of CPU instruction test cases and take an extremely long time to complete. **Always exclude it** using `--filter 'FullyQualifiedName!~SingleStepTest'` unless the change being tested directly touches CPU instruction decoding, execution, or flag handling (e.g. changes to `CfgCpu`, instruction parsers, ALU operations, or flag computation). When in doubt, exclude it.

> **Test output rule (agents only)**: no need to redirect `dotnet test` output to a file. On success the console summary is short (a single `Passed! - Failed: 0, Passed: N ...` line), so run it directly and read the summary. Only capture to a file when a run actually fails and you need the full failure detail.

> **Long-running test rule (Kiro agents only)**: the foreground shell tool (`execute_bash`) enforces a hard wall-clock cap (roughly 25-60s in practice) that fires *before* the `timeout` argument. When it trips it force-returns exit 1 while the detached `dotnet` process keeps running and completes normally, which looks exactly like a crash (abrupt exit 1, no test summary, no dump) but is not one. Any test run expected to exceed ~25s (the full suite is ~2 min; `CfgGraphReloadTest` alone is ~55s) MUST be launched with the background-process tool (which has no such cap), writing a completion marker, then polled with short (<20s) foreground calls until the marker appears. Do not conclude a suite "crashed" from a bare exit 1 with no summary; re-run it in the background and check the real result first. Splitting `--filter` so each foreground run stays under the cap also works.

### Debugging Workflow
- **GDB Integration**: Server runs on port 10000 by default (`--GdbPort 10000`)
- Use `--Debug` to pause at startup for breakpoint setup
Expand Down Expand Up @@ -117,9 +121,10 @@ Variants: `MemoryBasedDataStructureWithCsBaseAddress`, `MemoryBasedDataStructure
- **No scripts outside the project** - do NOT create temporary scripts (Python, Bash, PowerShell, Node, etc.) anywhere on the filesystem, including `/tmp`, the user's home directory, or any location outside the workspace. Do not create throwaway helper scripts inside the workspace either. Use the available tools (file editing, grep, search, terminal one-liners) directly. If a multi-step computation is truly needed, run it as an inline shell one-liner in the terminal without writing a file.
- **Use `tmp/` for temporary files** - if a temporary file must be written (e.g., captured command output, intermediate data), place it inside the `tmp/` folder at the root of the repository. Never write to `/tmp` or any path outside the workspace.
- **Avoid complexity** - keep cyclomatic complexity low, prefer simple, linear code over nested conditionals
- **No optional parameters** - avoid nullable or optional parameters in new code
- **No optional parameters** - avoid optional/defaulted method parameters in new code. Prefer non-nullable types; a field or dependency should be nullable only when its absence is a real, distinct state (e.g. a feature disabled by a flag). When a nullable feature-gate would otherwise force `?.`/`is not null` guards across many call sites, prefer a null-object (no-op) implementation or unconditional construction over a nullable reference.
- **No complex ternary expressions** - avoid nested, chained, or multi-line ternaries; simple single-line ternaries with short operands are allowed (e.g. `int x = a > b ? a : b;`), but non-trivial conditions or non-trivial branches must use explicit `if/else`
- **Minimal comments** - write self-documenting code with clear names; avoid obvious comments
- **No references to plan/spec sections in code** - never point comments, XML docs, or test names at sections of a plan, spec, or implementation-tracking document (e.g. "Phase 5", "Gap D", "Rule 4", "section S6.1.1", "Test 18", "implementation plan"). Those documents are temporary or drift over time, leaving dangling pointers that mean nothing to a future reader. Describe the actual behavior, invariant, or scenario directly instead.
- **Test before submit** - always run tests after code changes to verify functionality
- **Rebuild and verify** - For any task that changes code or tests, rebuild the project and run the full test suite; do not stop until all tests are green.
- **Concise documentation** - XML docs should be precise and complete but not verbose; avoid excessive remarks
Expand Down
13 changes: 13 additions & 0 deletions src/Spice86.Core/CLI/Configuration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -357,4 +357,17 @@ public sealed class Configuration : CommandSettings {
[CommandOption("--AllowIvtAddress0")]
public bool AllowIvtAddress0 { get; init; }

/// <summary>
/// When true (default), the speculative CFG explorer is enabled: after each newly-observed
/// instruction is parsed, the explorer performs a recursive-descent static decode of all
/// statically-reachable successors that have not yet been observed, marking the resulting nodes
/// as speculative. This allows the C# code generator to emit real branches for unobserved
/// conditional arms instead of always falling back to <c>FailAsUntested</c>.
/// When false, no speculative nodes, edges, or index entries are created; the graph content is
/// byte-identical to the pre-feature baseline.
/// </summary>
[CommandOption("--EnableSpeculativeCfgExploration <ENABLESPECULATIVECFGEXPLORATION>")]
[DefaultValue(true)]
public bool EnableSpeculativeCfgExploration { get; init; } = true;

}
5 changes: 3 additions & 2 deletions src/Spice86.Core/Emulator/CPU/CfgCpu/CfgCpu.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ public CfgCpu(IMemory memory, State state, IOPortDispatcher ioPortDispatcher, Ca
DualPic dualPic, EmulatorBreakpointsManager emulatorBreakpointsManager,
IPauseHandler pauseHandler,
FunctionCatalogue functionCatalogue,
bool useCodeOverride, bool failOnInvalidOpcode, bool allowIvtAddress0, ILoggerService loggerService, CfgNodeExecutionCompiler executionCompiler, SequentialIdAllocator idAllocator, CpuHeavyLogger? cpuHeavyLogger = null) {
bool useCodeOverride, bool failOnInvalidOpcode, bool allowIvtAddress0, bool enableSpeculativeExploration, ILoggerService loggerService, CfgNodeExecutionCompiler executionCompiler, SequentialIdAllocator idAllocator, CpuHeavyLogger? cpuHeavyLogger = null) {
_loggerService = loggerService;
_state = state;
_dualPic = dualPic;
_cpuHeavyLogger = cpuHeavyLogger;
_emulatorBreakpointsManager = emulatorBreakpointsManager;
_pauseHandler = pauseHandler;
CfgNodeFeeder = new(memory, state, emulatorBreakpointsManager, _replacerRegistry, executionCompiler, idAllocator);
CfgNodeFeeder = new(memory, state, emulatorBreakpointsManager, _replacerRegistry, executionCompiler, idAllocator, enableSpeculativeExploration);
_executionContextManager = new(memory, state, CfgNodeFeeder, _replacerRegistry, functionCatalogue, useCodeOverride, loggerService, cpuHeavyLogger);
_instructionExecutionHelper = new(state, memory, ioPortDispatcher, callbackHandler, emulatorBreakpointsManager, _executionContextManager, failOnInvalidOpcode, allowIvtAddress0, loggerService);
}
Expand All @@ -61,6 +61,7 @@ public CfgCpu(IMemory memory, State state, IOPortDispatcher ioPortDispatcher, Ca
public FunctionHandler FunctionHandlerInUse => ExecutionContextManager.CurrentExecutionContext.FunctionHandler;
public bool IsInitialExecutionContext => ExecutionContextManager.CurrentExecutionContext.Depth == 0;
private ExecutionContext CurrentExecutionContext => _executionContextManager.CurrentExecutionContext;

public ICfgNode ToExecute() {
return CfgNodeFeeder.GetLinkedCfgNodeToExecute(CurrentExecutionContext);
}
Expand Down
70 changes: 67 additions & 3 deletions src/Spice86.Core/Emulator/CPU/CfgCpu/ControlFlowGraph/CfgBlock.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ public sealed class CfgBlock : CfgNode {
/// </summary>
private int _nonLiveCounter;

/// <summary>
/// Number of contained nodes whose <see cref="ICfgNode.IsSpeculative"/> is currently <c>true</c>.
/// <see cref="IsSpeculative"/> is <c>(_speculativeCounter &gt; 0)</c>.
/// </summary>
private int _speculativeCounter;

private BlockNode? _cachedDisplayAst;
private bool _isDisplayAstStale = true;

Expand Down Expand Up @@ -50,6 +56,13 @@ public CfgBlock(int id, ICfgNode entry)
/// </remarks>
public override bool IsLive => _nonLiveCounter == 0;

/// <inheritdoc />
/// <remarks>
/// O(1). The block is speculative iff at least one contained instruction is speculative;
/// computed from the maintained <see cref="_speculativeCounter"/> without iterating.
/// </remarks>
public override bool IsSpeculative => _speculativeCounter > 0;

/// <inheritdoc />
/// <remarks>
/// Always <c>null</c>: a <see cref="CfgBlock"/> is itself the container, not contained in one.
Expand Down Expand Up @@ -140,19 +153,22 @@ public override IVisitableAstNode DisplayAst {

/// <summary>
/// Appends <paramref name="node"/> to the end of the block, making it the new
/// <see cref="Terminator"/>. Updates the non-live counter from the node's live state.
/// <see cref="Terminator"/>. Updates the non-live and speculative counters from the node's state.
/// </summary>
internal void Append(ICfgNode node) {
_instructions.Add(node);
if (!node.IsLive) {
_nonLiveCounter++;
}
if (node.IsSpeculative) {
_speculativeCounter++;
}
_isDisplayAstStale = true;
}

/// <summary>
/// Replaces the node at <paramref name="index"/> with <paramref name="newNode"/>, preserving
/// order. Adjusts the non-live counter to reflect the new node's live state.
/// order. Adjusts the non-live and speculative counters to reflect the new node's state.
/// </summary>
internal void ReplaceInPlace(int index, ICfgNode newNode) {
ICfgNode old = _instructions[index];
Expand All @@ -163,18 +179,46 @@ internal void ReplaceInPlace(int index, ICfgNode newNode) {
if (!newNode.IsLive) {
_nonLiveCounter++;
}
if (old.IsSpeculative) {
_speculativeCounter--;
}
if (newNode.IsSpeculative) {
_speculativeCounter++;
}
_isDisplayAstStale = true;
}

/// <summary>
/// Removes <paramref name="node"/> from the block by value, maintaining the non-live and
/// speculative counters via O(1) incremental decrement (mirrors <see cref="ReplaceInPlace"/>).
/// Returns <c>false</c> when the node is not part of the block. Tolerates transient
/// non-contiguity: it is a batch-internal primitive for removing a contiguous suffix of
/// nodes one at a time.
/// </summary>
internal bool Remove(ICfgNode node) {
if (!_instructions.Remove(node)) {
return false;
}
if (!node.IsLive) {
_nonLiveCounter--;
}
if (node.IsSpeculative) {
_speculativeCounter--;
}
_isDisplayAstStale = true;
return true;
}

/// <summary>
/// Removes nodes from <paramref name="splitIndex"/> through the end and returns them as a
/// new list. The non-live counter is recomputed for the surviving prefix.
/// new list. The non-live and speculative counters are recomputed for the surviving prefix.
/// </summary>
internal List<ICfgNode> SliceFrom(int splitIndex) {
int tailLength = _instructions.Count - splitIndex;
List<ICfgNode> tail = _instructions.GetRange(splitIndex, tailLength);
_instructions.RemoveRange(splitIndex, tailLength);
RecountNonLiveFromInstructions();
RecountSpeculativeFromInstructions();
_isDisplayAstStale = true;
return tail;
}
Expand All @@ -191,6 +235,18 @@ internal void OnContainedInstructionLiveChanged(bool nowLive) {
}
}

/// <summary>
/// Counter choke-point invoked by <see cref="CfgInstruction.SetSpeculative"/> exactly once per
/// actual <see cref="ICfgNode.IsSpeculative"/> transition of a contained instruction.
/// </summary>
internal void OnContainedInstructionSpeculativeChanged(bool nowSpeculative) {
if (nowSpeculative) {
_speculativeCounter++;
} else {
_speculativeCounter--;
}
}

/// <summary>
/// Recomputes the non-live counter from scratch by iterating the contained instructions.
/// Used after a split to re-base the counter on the new instruction set.
Expand All @@ -199,6 +255,14 @@ internal void RecountNonLiveFromInstructions() {
_nonLiveCounter = _instructions.Count(node => !node.IsLive);
}

/// <summary>
/// Recomputes the speculative counter from scratch by iterating the contained instructions.
/// Used after a split to re-base the counter on the new instruction set.
/// </summary>
internal void RecountSpeculativeFromInstructions() {
_speculativeCounter = _instructions.Count(node => node.IsSpeculative);
}

/// <summary>
/// Returns the index of <paramref name="node"/> in the contained list, or <c>-1</c> if
/// the node is not part of this block.
Expand Down
15 changes: 15 additions & 0 deletions src/Spice86.Core/Emulator/CPU/CfgCpu/ControlFlowGraph/CfgNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,21 @@ public CfgNodeExecutionAction<InstructionExecutionHelper> CompiledExecution {
}

public abstract bool IsLive { get; }

/// <summary>
/// Default is <c>false</c>. <see cref="ParsedInstruction.CfgInstruction"/> overrides this to
/// expose its stored speculative flag, maintained by <see cref="SetSpeculative"/> calls.
/// <see cref="SelectorNode"/> and <see cref="CfgBlock"/> keep the default <c>false</c>.
/// </summary>
public virtual bool IsSpeculative => false;

/// <summary>
/// No-op default: <see cref="SelectorNode"/> and <see cref="CfgBlock"/> have no speculative
/// state. <see cref="ParsedInstruction.CfgInstruction"/> overrides this to maintain its stored
/// flag and notify its containing block.
/// </summary>
public virtual void SetSpeculative(bool isSpeculative) {
}

public abstract void UpdateSuccessorCache();
public abstract ICfgNode? GetNextSuccessor(InstructionExecutionHelper helper);
Expand Down
18 changes: 18 additions & 0 deletions src/Spice86.Core/Emulator/CPU/CfgCpu/ControlFlowGraph/ICfgNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,24 @@ public interface ICfgNode : IEquatable<ICfgNode> {
/// </summary>
bool IsLive { get; }

/// <summary>
/// Returns whether the node is speculative.
/// A speculative node was decoded by the static explorer from memory bytes but has never been
/// executed and confirmed. It is a separate axis from <see cref="IsLive"/>: a speculative node
/// is always non-live, but a non-live observed node (e.g. SMC-evicted) is not speculative.
/// Speculative nodes never enter <see cref="Feeder.CurrentInstructions"/>,
/// <see cref="Feeder.PreviousInstructions"/>, or a <see cref="ParsedInstruction.SelfModifying.SelectorNode"/>.
/// </summary>
bool IsSpeculative { get; }

/// <summary>
/// Updates the speculative provenance of this node. On an actual transition, keeps the
/// containing <see cref="CfgBlock"/>'s speculative counter in sync. Setting speculative also
/// forces the node non-live. No-op for node kinds that have no speculative state
/// (<see cref="ParsedInstruction.SelfModifying.SelectorNode"/>, <see cref="CfgBlock"/>).
/// </summary>
void SetSpeculative(bool isSpeculative);

/// <summary>
/// True when the node execution can lead to going back to previous execution context if the next to execute is the correct address
/// </summary>
Expand Down
54 changes: 54 additions & 0 deletions src/Spice86.Core/Emulator/CPU/CfgCpu/ExecutionContextManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
using Spice86.Core.Emulator.Memory;
using Spice86.Shared.Emulator.Memory;
using Spice86.Shared.Interfaces;
using Spice86.Shared.Utils;

using System.Linq;

public class ExecutionContextManager : InstructionReplacer, IClearable {
private readonly ILoggerService _loggerService;
Expand Down Expand Up @@ -121,6 +124,39 @@
nodes.Add(toExecute);
}

/// <summary>
/// Registers <paramref name="node"/> as a CFG entry point so it is treated as a generation root by
/// the graph exporter and the function partitioner. Used to seed emulator-installed hardware
/// interrupt handlers: these fire on external events with nondeterministic timing and may never be
/// reached from the program's observed entry points during discovery, yet must still become
/// generated overrides so generated code can service the interrupt.
/// </summary>
/// <param name="node">The handler entry instruction to register.</param>
public void RegisterEntryPoint(CfgInstruction node) {
if (!ExecutionContextEntryPoints.TryGetValue(node.Address, out ISet<CfgInstruction>? nodes)) {
nodes = new HashSet<CfgInstruction>();
ExecutionContextEntryPoints.Add(node.Address, nodes);
}
nodes.Add(node);
}

/// <summary>
/// Seeds the given known-safe handler entry addresses for speculative exploration and registers
/// each decoded handler entry node as a CFG entry point. No-op per handler when speculative
/// exploration is disabled (no seeded node is produced).
/// </summary>
/// <param name="handlerAddresses">Entry addresses of emulator-installed interrupt handlers.</param>
public void SeedKnownSafeHandlersAndRegisterEntryPoints(IReadOnlyList<SegmentedAddress> handlerAddresses) {
_cfgNodeFeeder.SeedKnownSafeHandlers(handlerAddresses);
foreach (SegmentedAddress handlerAddress in handlerAddresses) {
CfgInstruction? entryNode = _cfgNodeFeeder.NodeIndex.GetAtAddress(handlerAddress)
.FirstOrDefault(node => node.ContainingBlock is not null);
if (entryNode is not null) {
RegisterEntryPoint(entryNode);
}
}

Check notice

Code scanning / CodeQL

Missed opportunity to use Select Note

This foreach loop immediately
maps its iteration variable to another variable
- consider mapping the sequence explicitly using '.Select(...)'.
Comment on lines +151 to +157
}

public override void ReplaceInstruction(CfgInstruction oldInstruction, CfgInstruction newInstruction) {
if (ExecutionContextEntryPoints.TryGetValue(newInstruction.Address, out ISet<CfgInstruction>? entriesAtAddress)
&& entriesAtAddress.Remove(oldInstruction)) {
Expand All @@ -132,6 +168,24 @@
}
}

/// <summary>
/// Handles removal fan-out: drops <paramref name="instruction"/> from the
/// entry-point set at its address so a removed generation root cannot linger as a
/// detached, de-indexed dead root. Removes the address key when its set empties.
/// </summary>
/// <remarks>
/// Deliberately does NOT clear a context's <see cref="ExecutionContext.NodeToExecuteNextAccordingToGraph"/>
/// that still points at the removed node, unlike <see cref="ReplaceInstruction"/> which repoints it.
/// Replacement has a live successor to point at; removal does not. The feeder's
/// reconcile-with-memory path relies on the stale pointer surviving the sweep: a non-live graph
/// node whose address still matches memory but is no longer indexed is how it detects a swept
/// speculative node and routes to the live memory node. Nulling it here makes that node null and
/// trips the address-mismatch guard instead.
/// </remarks>
public override void RemoveInstruction(CfgInstruction instruction) {
DictionaryUtils.RemoveFromCollection(ExecutionContextEntryPoints, instruction.Address, instruction);
}

private static void UpdateNodeToExecuteIfStale(ExecutionContext context,
CfgInstruction oldInstruction, CfgInstruction newInstruction) {
if (oldInstruction.Equals(context.NodeToExecuteNextAccordingToGraph)) {
Expand Down
Loading
Loading