Skip to content

Commit

Permalink
fix: No crashes, but phantom breakpoints still persist
Browse files Browse the repository at this point in the history
  • Loading branch information
@maximilien-noal
maximilien-noal committed Dec 9, 2024
1 parent 18c488e commit 9f945f7
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 69 deletions.
55 changes: 39 additions & 16 deletions src/Spice86/ViewModels/BreakpointViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,49 @@ namespace Spice86.ViewModels;
using Spice86.Models.Debugging;

public partial class BreakpointViewModel : ViewModelBase {
private readonly BreakPoint _breakPoint;
private readonly EmulatorBreakpointsManager _emulatorBreakpointsManager;
private readonly Action _onReached;

public BreakpointViewModel(EmulatorBreakpointsManager emulatorBreakpointsManager, AddressBreakPoint breakPoint) {
_breakPoint = breakPoint;
public BreakpointViewModel(EmulatorBreakpointsManager emulatorBreakpointsManager,
long address,
BreakPointType type,
bool isRemovedOnTrigger,
Action onReached) {
_emulatorBreakpointsManager = emulatorBreakpointsManager;
IsEnabled = true;
Address = breakPoint.Address;
Address = address;
Type = type;
IsRemovedOnTrigger = isRemovedOnTrigger;
if(IsRemovedOnTrigger) {
_onReached = () => {
Reached?.Invoke();
onReached();
};
} else {
_onReached = onReached;
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity Note

Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
Enable();
}

public BreakPointType Type => _breakPoint.BreakPointType;
internal event Action? Reached;

public BreakPointType Type { get; }

//Can't get out of sync since GDB can't be used at the same time as the internal debugger
private bool _isEnabled;

public bool IsEnabled {
get => _isEnabled;
set {
if(SetProperty(ref _isEnabled, value)) {
if (value) {
Enable();
} else {
Disable();
}
if (value) {
Enable();
} else {
Disable();
}
SetProperty(ref _isEnabled, value);
}
}

public bool IsRemovedOnTrigger => _breakPoint.IsRemovedOnTrigger;
public bool IsRemovedOnTrigger { get; }

public long Address { get; }

Expand All @@ -51,19 +64,29 @@ public void Toggle() {
[ObservableProperty]
private string? _comment;

private AddressBreakPoint GenerateBreakPoint() => new(
Type, Address,
(_) => _onReached(), IsRemovedOnTrigger);

public void Enable() {
_emulatorBreakpointsManager.ToggleBreakPoint(_breakPoint, on: true);
if (IsEnabled) {
return;
}
_emulatorBreakpointsManager.ToggleBreakPoint(GenerateBreakPoint(), on: true);
_isEnabled = true;
OnPropertyChanged(nameof(IsEnabled));
}

public void Disable() {
_emulatorBreakpointsManager.ToggleBreakPoint(_breakPoint, on: false);
if (!IsEnabled) {
return;
}
_emulatorBreakpointsManager.ToggleBreakPoint(GenerateBreakPoint(), on: false);
_isEnabled = false;
OnPropertyChanged(nameof(IsEnabled));
}

internal bool IsFor(CpuInstructionInfo instructionInfo) {
return _breakPoint is AddressBreakPoint addressBreakPoint && addressBreakPoint.Address == instructionInfo.Address;
return Address == instructionInfo.Address;
}
}
29 changes: 18 additions & 11 deletions src/Spice86/ViewModels/BreakpointsViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,36 +42,43 @@ private void ToggleSelectedBreakpoint() {
[NotifyCanExecuteChangedFor(nameof(ToggleSelectedBreakpointCommand))]
private BreakpointViewModel? _selectedBreakpoint;

internal void AddAddressBreakpoint(AddressBreakPoint addressBreakPoint) {
var breakpointViewModel = new BreakpointViewModel( _emulatorBreakpointsManager, addressBreakPoint);
internal void AddAddressBreakpoint(
long address,
BreakPointType type,
bool isRemovedOnTrigger,
Action onReached) {
var breakpointViewModel = new BreakpointViewModel(
_emulatorBreakpointsManager,
address, type, isRemovedOnTrigger, onReached);
Breakpoints.Add(breakpointViewModel);
if (isRemovedOnTrigger) {
breakpointViewModel.Reached += () => DeleteBreakpoint(breakpointViewModel);
}
SelectedBreakpoint = breakpointViewModel;
}

internal BreakpointViewModel? GetBreakpoint(CpuInstructionInfo instructionInfo) {
return Breakpoints.FirstOrDefault(x => x.IsFor(instructionInfo));
}

private bool RemoveBreakpointCanExecute() => SelectedBreakpoint is not null;


[RelayCommand(CanExecute = nameof(RemoveBreakpointCanExecute))]
private void RemoveBreakpoint() {
if (SelectedBreakpoint is not null) {
DeleteBreakpoint(SelectedBreakpoint);
BreakpointDeleted?.Invoke();
}
DeleteBreakpoint(SelectedBreakpoint);
}

internal void RemoveBreakpoint(BreakpointViewModel vm) {
internal void RemoveBreakpointInternal(BreakpointViewModel vm) {
DeleteBreakpoint(vm);
}

internal BreakpointViewModel? GetBreakpoint(CpuInstructionInfo instructionInfo) {
return Breakpoints.FirstOrDefault(x => x.IsFor(instructionInfo));
}

private void DeleteBreakpoint(BreakpointViewModel? breakpoint) {
if (breakpoint is null) {
return;
}
breakpoint.Disable();
Breakpoints.Remove(breakpoint);
BreakpointDeleted?.Invoke();
}
}
2 changes: 1 addition & 1 deletion src/Spice86/ViewModels/DebugWindowViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public DebugWindowViewModel(IInstructionExecutor cpu, State cpuState, Stack stac
pauseHandler.Resumed += () => uiDispatcher.Post(() => IsPaused = false);
DisassemblyViewModel disassemblyVm = new(
cpu, memory, cpuState, functionsInformation.ToDictionary(x => x.Key.ToPhysical(), x => x.Value),
BreakpointsViewModel, emulatorBreakpointsManager, pauseHandler,
BreakpointsViewModel, pauseHandler,
uiDispatcher, messenger, textClipboard);
DisassemblyViewModels.Add(disassemblyVm);
PaletteViewModel = new(argbPalette, uiDispatcher);
Expand Down
73 changes: 37 additions & 36 deletions src/Spice86/ViewModels/DisassemblyViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,13 @@ public partial class DisassemblyViewModel : ViewModelWithErrorDialog {
private readonly IPauseHandler _pauseHandler;
private readonly IInstructionExecutor _cpu;
private readonly BreakpointsViewModel _breakpointsViewModel;
private readonly EmulatorBreakpointsManager _emulatorBreakpointsManager;
private readonly IDictionary<uint, FunctionInformation> _functionsInformation;
private readonly InstructionsDecoder _instructionsDecoder;

public DisassemblyViewModel(
IInstructionExecutor cpu, IMemory memory, State state,
IDictionary<uint, FunctionInformation> functionsInformation,
BreakpointsViewModel breakpointsViewModel, EmulatorBreakpointsManager emulatorBreakpointsManager,
BreakpointsViewModel breakpointsViewModel,
IPauseHandler pauseHandler, IUIDispatcher uiDispatcher,
IMessenger messenger, ITextClipboard textClipboard, bool canCloseTab = false)
: base(uiDispatcher, textClipboard) {
Expand All @@ -44,9 +43,8 @@ public DisassemblyViewModel(
.Select(x => new FunctionInfo() {
Name = x.Value.Name,
Address = x.Key,
}).OrderBy(x => x.Address));
}).OrderBy(x => x.Address));
AreFunctionInformationProvided = functionsInformation.Count > 0;
_emulatorBreakpointsManager = emulatorBreakpointsManager;
_breakpointsViewModel = breakpointsViewModel;
_messenger = messenger;
_memory = memory;
Expand All @@ -55,13 +53,20 @@ public DisassemblyViewModel(
_instructionsDecoder = new(memory, state, functionsInformation, breakpointsViewModel);
IsPaused = pauseHandler.IsPaused;
pauseHandler.Pausing += OnPausing;
pauseHandler.Resumed += () => _uiDispatcher.Post(() => IsPaused = false);
pauseHandler.Resumed += OnResumed;
CanCloseTab = canCloseTab;
breakpointsViewModel.BreakpointDeleted += UpdateDisassemblyInternal;
breakpointsViewModel.BreakpointDisabled += UpdateDisassemblyInternal;
breakpointsViewModel.BreakpointEnabled += UpdateDisassemblyInternal;
}

private void OnResumed() {
_uiDispatcher.Post(() => {
IsPaused = false;
Instructions.Clear();
});
}

[ObservableProperty]
private bool _areFunctionInformationProvided;

Expand All @@ -74,7 +79,7 @@ public DisassemblyViewModel(
private void OnPausing() {
_uiDispatcher.Post(() => {
IsPaused = true;
if(Instructions.Count == 0 && GoToCsIpCommand.CanExecute(null)) {
if (Instructions.Count == 0 && GoToCsIpCommand.CanExecute(null)) {
GoToCsIpCommand.Execute(null);
}
});
Expand Down Expand Up @@ -117,12 +122,14 @@ private void ConfirmCreateExecutionBreakpoint() {
CreatingExecutionBreakpoint = false;
if (!string.IsNullOrWhiteSpace(BreakpointAddress) &&
TryParseMemoryAddress(BreakpointAddress, out ulong? breakpointAddressValue)) {
AddressBreakPoint addressBreakPoint = new(BreakPointType.EXECUTION,
(long)breakpointAddressValue.Value, (breakpoint) => {
RequestPause(breakpoint, breakpointAddressValue.Value);
UpdateDisassemblyInternal();
}, false);
_breakpointsViewModel.AddAddressBreakpoint(addressBreakPoint);
_breakpointsViewModel.AddAddressBreakpoint(
(long)breakpointAddressValue.Value,

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
breakpointAddressValue
Loading
may be null at this access because it has a nullable type.
BreakPointType.EXECUTION,
isRemovedOnTrigger: false,
() => {
RequestPause((long)breakpointAddressValue.Value);

Check warning

Code scanning / CodeQL

Dereferenced variable may be null Warning

Variable
breakpointAddressValue
Loading
may be null at this access because it has a nullable type.
UpdateDisassemblyInternal();
});
}
}

Expand Down Expand Up @@ -159,19 +166,16 @@ private void UpdateHeader(uint? address) {

[RelayCommand(CanExecute = nameof(IsPaused))]
private void StepOver() {
if(SelectedInstruction is null) {
if (SelectedInstruction is null) {
return;
}
long nextInstructionAddressInListing = SelectedInstruction.Address + SelectedInstruction.Length;
AddressBreakPoint addressBreakpoint = new AddressBreakPoint(
BreakPointType.EXECUTION,
nextInstructionAddressInListing,
(breakpoint) => {
RequestPause(breakpoint, (uint)nextInstructionAddressInListing);
_breakpointsViewModel.AddAddressBreakpoint(
nextInstructionAddressInListing,
BreakPointType.EXECUTION, isRemovedOnTrigger: true, () => {
RequestPause((uint)nextInstructionAddressInListing);
_uiDispatcher.Post(() => GoToCsIpCommand.Execute(null));
},
isRemovedOnTrigger: true);
_emulatorBreakpointsManager.ToggleBreakPoint(addressBreakpoint, on: true);
});
_pauseHandler.Resume();
}

Expand Down Expand Up @@ -205,8 +209,8 @@ private async Task StepInto() {
[RelayCommand(CanExecute = nameof(IsPaused))]
private void NewDisassemblyView() {
DisassemblyViewModel disassemblyViewModel = new(
_cpu, _memory, _state, _functionsInformation,
_breakpointsViewModel, _emulatorBreakpointsManager, _pauseHandler, _uiDispatcher, _messenger,
_cpu, _memory, _state, _functionsInformation,
_breakpointsViewModel, _pauseHandler, _uiDispatcher, _messenger,
_textClipboard, canCloseTab: true) {
IsPaused = IsPaused
};
Expand All @@ -221,7 +225,7 @@ private async Task GoToCsIp() {

[RelayCommand(CanExecute = nameof(IsPaused))]
private async Task GoToFunction(object? parameter) {
if(parameter is FunctionInfo functionInfo) {
if (parameter is FunctionInfo functionInfo) {
await GoToAddress(functionInfo.Address);
}
}
Expand Down Expand Up @@ -293,8 +297,8 @@ private async Task CopyLine() {
}
}

private void RequestPause(BreakPoint breakPoint, ulong address) {
string message = $"{breakPoint.BreakPointType} breakpoint was reached at address {address}.";
private void RequestPause(long address) {
string message = $"Execution breakpoint was reached at address {address}.";
_pauseHandler.RequestPause(message);
_uiDispatcher.Post(() => {
_messenger.Send(new StatusMessage(DateTime.Now, this, message));
Expand Down Expand Up @@ -327,27 +331,24 @@ private void RemoveExecutionBreakpointHere() {
if (SelectedInstruction?.Breakpoint is null) {
return;
}
_breakpointsViewModel.RemoveBreakpoint(SelectedInstruction.Breakpoint);
_breakpointsViewModel.RemoveBreakpointInternal(SelectedInstruction.Breakpoint);
SelectedInstruction.Breakpoint = _breakpointsViewModel.GetBreakpoint(SelectedInstruction);
}

private bool CreateExecutionBreakpointHereCanExecute() =>
SelectedInstruction?.Breakpoint is not { Type: BreakPointType.EXECUTION };

[RelayCommand(CanExecute = nameof(CreateExecutionBreakpointHereCanExecute))]
private void CreateExecutionBreakpointHere() {
if (SelectedInstruction is null) {
return;
}
AddressBreakPoint breakPoint = new(
BreakPointType.EXECUTION,
SelectedInstruction.Address,
(breakpoint) => {
RequestPause(breakpoint, SelectedInstruction.Address);
long address = SelectedInstruction.Address;
_breakpointsViewModel.AddAddressBreakpoint(address,
BreakPointType.EXECUTION, isRemovedOnTrigger: false, () => {
RequestPause(address);
UpdateDisassemblyInternal();
},
isRemovedOnTrigger: false);
_breakpointsViewModel.AddAddressBreakpoint(breakPoint);
});
SelectedInstruction.Breakpoint = _breakpointsViewModel.GetBreakpoint(SelectedInstruction);
}
}
12 changes: 7 additions & 5 deletions src/Spice86/ViewModels/MemoryViewModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ private void BeginCreateMemoryBreakpoint() {

public BreakPointType[] BreakpointTypes => [BreakPointType.ACCESS, BreakPointType.WRITE, BreakPointType.READ];

private void OnBreakPointReached(BreakPoint breakPoint) {
string message = $"{breakPoint.BreakPointType} breakpoint was reached.";
private void OnBreakPointReached() {
string message = $"Memory breakpoint was reached.";
_pauseHandler.RequestPause(message);
_uiDispatcher.Post(() => {
_messenger.Send(new StatusMessage(DateTime.Now, this, message));
Expand Down Expand Up @@ -186,9 +186,11 @@ private void ConfirmCreateMemoryBreakpoint() {
}

private void CreateMemoryAddressBreakpoint(ulong breakpointAddressValue) {
AddressBreakPoint addressBreakPoint = new(SelectedBreakpointType,
(long)breakpointAddressValue, OnBreakPointReached, false);
_breakpointsViewModel.AddAddressBreakpoint(addressBreakPoint);
_breakpointsViewModel.AddAddressBreakpoint(
(long)breakpointAddressValue,
SelectedBreakpointType,
isRemovedOnTrigger: false,
OnBreakPointReached);
}

[RelayCommand]
Expand Down

0 comments on commit 9f945f7

Please sign in to comment.