nil protection on disassembly definition

.gitignore

@@ -29,3 +29,4 @@ armcode.map
 *.jpg
 *.bin
 *.macro
+go.work

debugger/deeppoke.go

@@ -147,6 +147,10 @@ func (dbg *Debugger) searchDeepPoke(searchState *rewind.State, searchAddr uint16
 			return poking, nil
 		}
 
+		if searchState.CPU.LastResult.Defn == nil {
+			return deepPoking{}, fmt.Errorf("unexpected CPU result with a nil definition")
+		}
+
 		// writes to a register can happen from another register, and immediate
 		// value, or an address in memory (most probably from the cartridge or
 		// VCS RAM)

debugger/halt_targets.go

@@ -248,7 +248,7 @@ func parseTarget(dbg *Debugger, tokens *commandline.Tokens) (*target, error) {
 					trg = &target{
 						label: "Instruction Effect",
 						value: func() targetValue {
-							if !dbg.vcs.CPU.LastResult.Final {
+							if !dbg.vcs.CPU.LastResult.Final || dbg.vcs.CPU.LastResult.Defn == nil {
 								return -1
 							}
 							return int(dbg.vcs.CPU.LastResult.Defn.Effect)

disassembly/disassembly.go

@@ -25,7 +25,6 @@ import (
 	"github.com/jetsetilly/gopher2600/hardware"
 	"github.com/jetsetilly/gopher2600/hardware/cpu"
 	"github.com/jetsetilly/gopher2600/hardware/cpu/execution"
-	"github.com/jetsetilly/gopher2600/hardware/cpu/instructions"
 	"github.com/jetsetilly/gopher2600/hardware/memory/cartridge/mapper"
 	"github.com/jetsetilly/gopher2600/hardware/memory/cpubus"
 	"github.com/jetsetilly/gopher2600/hardware/memory/memorymap"
@@ -299,11 +298,8 @@ func (dsm *Disassembly) FormatResult(bank mapper.BankInfo, result execution.Resu
 	// address of instruction
 	e.Address = fmt.Sprintf("$%04x", result.Address)
 
-	// protect against empty definitions. we shouldn't hit this condition from
-	// the disassembly package itself, but it is possible to get it from ad-hoc
-	// formatting from GUI interfaces (see CPU window in sdlimgui)
+	// if definition is nil then set the operator field to ??? and return with no further formatting
 	if result.Defn == nil {
-		e.Result.Defn = &instructions.Definition{}
 		e.Operator = "???"
 		return e
 	}

disassembly/entry.go

@@ -107,11 +107,12 @@ func (e *Entry) updateExecutionEntry(result execution.Result) {
 // the number of cycles in the definition. for executed branch instructions this
 // will always be the case.
 func (e *Entry) Cycles() string {
+	// the Defn field may be unassigned
+	if e.Result.Defn == nil {
+		return "?"
+	}
+
 	if e.Level < EntryLevelExecuted {
-		// the Defn field may be unassigned
-		if e.Result.Defn == nil {
-			return "-"
-		}
 		return e.Result.Defn.Cycles.Formatted
 	}
 
@@ -137,7 +138,7 @@ func (e *Entry) Notes() string {
 
 	s := strings.Builder{}
 
-	if e.Result.Defn.IsBranch() {
+	if e.Result.Defn != nil && e.Result.Defn.IsBranch() {
 		if e.Result.BranchSuccess {
 			s.WriteString("branch succeeded ")
 		} else {

disassembly/preferences.go

@@ -119,7 +119,7 @@ func (dsm *Disassembly) setCartMirror() {
 
 			// branch instructions need special handling because for readability we
 			// translate the offset to an absolute address, which has changed.
-			if e.Result.Defn.IsBranch() {
+			if e.Result.Defn != nil && e.Result.Defn.IsBranch() {
 				// mask off bits that indicate the cartridge/segment origin and reset
 				// them with the chosen origin
 				a := e.Result.Address&memorymap.CartridgeBits | dsm.Prefs.mirrorOrigin

gui/sdlimgui/win_disasm.go

@@ -751,7 +751,9 @@ func (win *winDisasm) drawEntry(currBank mapper.BankInfo, e *disassembly.Entry,
 		imgui.PushStyleColor(imgui.StyleColorText, win.img.cols.DisasmCycles)
 		defer imgui.PopStyleColor()
 	}
-	imgui.Text(e.Result.Defn.Cycles.Formatted)
+	if e.Result.Defn != nil {
+		imgui.Text(e.Result.Defn.Cycles.Formatted)
+	}
 
 	// notes column
 	imgui.TableNextColumn()

hardware/cpu/execution/result.go

@@ -35,6 +35,11 @@ import (
 // Defn of nil means the opcode hasn't even been decoded.
 type Result struct {
 	// a reference to the instruction definition
+	//
+	// * this field can be nil and should be checked before referencing. note
+	// that we could make this a non-pointer field but we would still need to
+	// check if the definition is valid which would require a flag in the
+	// Definition type. a pointer check is just as good
 	Defn *instructions.Definition
 
 	// the number of bytes read during instruction decode. if this value is

hardware/cpu/execution/validity.go

@@ -20,6 +20,10 @@ import "fmt"
 // IsValid checks whether the instance of Result contains information
 // consistent with the instruction definition.
 func (r Result) IsValid() error {
+	if r.Defn == nil {
+		return fmt.Errorf("cpu: execution result has no instruction definition")
+	}
+
 	if !r.Final {
 		return fmt.Errorf("cpu: execution not finalised (bad opcode?)")
 	}