extended logging of ARM memory errors

controlled by preferences file (false by default)

hardware/memory/cartridge/arm/arm.go

@@ -33,7 +33,7 @@ import (
 // it is sometimes convenient to dissassemble every instruction and to print it
 // to stdout for inspection. we most likely need this during the early stages of
 // debugging of a new cartridge type
-const useCartCoProcDisassemblerStdout = false
+const disassembleToStdout = false
 
 // register names.
 const (
@@ -222,13 +222,11 @@ type ARM struct {
 	// error seen during execution
 	executionError error
 
-	// is set to true when an access to memory using a read/write function used
-	// an unrecognised address. when this happens, the address is logged and
-	// the Thumb program aborted (ie returns early)
-	//
-	// note: it is only honoured if abortOnIllegalMem is true
-	memoryError       error
-	memoryErrorDetail error
+	// error accessing memory
+	memoryError error
+
+	// additional error information returned by developer package
+	memoryErrorDev error
 
 	// the speed at which the arm is running at and the required stretching for
 	// access to flash memory. speed is in MHz. Access latency of Flash memory is
@@ -326,7 +324,7 @@ func NewARM(mmap architecture.Map, prefs *preferences.ARMPreferences, mem Shared
 	}
 
 	// disassembly printed to stdout
-	if useCartCoProcDisassemblerStdout {
+	if disassembleToStdout {
 		arm.disasm = &mapper.CartCoProcDisassemblerStdout{}
 	}
 
@@ -607,7 +605,7 @@ func (arm *ARM) Run() (mapper.YieldReason, float32) {
 	arm.continueExecution = true
 	arm.executionError = nil
 	arm.memoryError = nil
-	arm.memoryErrorDetail = nil
+	arm.memoryErrorDev = nil
 
 	// reset disasm notes/flags
 	if arm.disasm != nil {
@@ -811,7 +809,7 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 			arm.state.stackFrame = candidateStackFrame
 		}
 
-		// send disasm information to disassembler
+		// disassemble if appropriate
 		if arm.disasm != nil {
 			if !arm.state.function32bitDecoding {
 				var cached bool
@@ -819,21 +817,10 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 
 				entry, cached = arm.disasmCache[arm.state.instructionPC]
 				if !cached {
-					switch arm.mmap.ARMArchitecture {
-					case architecture.ARM7TDMI:
-						var err error
-						entry, err = arm.disassemble(opcode)
-						if err != nil {
-							panic(err.Error())
-						}
-					case architecture.ARMv7_M:
-						var err error
-						entry, err = arm.disassembleThumb2(opcode)
-						if err != nil {
-							panic(err.Error())
-						}
-					default:
-						panic(fmt.Sprintf("unhandled ARM architecture: %s", arm.mmap.ARMArchitecture))
+					var err error
+					entry, err = arm.disassemble(opcode)
+					if err != nil {
+						panic(err.Error())
 					}
 				}
 
@@ -867,29 +854,12 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 				// update program cycles
 				arm.disasmSummary.add(arm.state.cycleOrder)
 
-				// additional output for disassembler of type CartCoProcDisassemblerStdout type
-				var isDisasmStdout bool
-				if _, isDisasmStdout = arm.disasm.(*mapper.CartCoProcDisassemblerStdout); isDisasmStdout {
-					fmt.Printf("%08x: ", entry.Addr)
-					if entry.Is32bit {
-						fmt.Printf("%04x %04x ", entry.OpcodeHi, entry.Opcode)
-					} else {
-						fmt.Printf("%04x       ", entry.Opcode)
-					}
-				}
-
 				// we always send the instruction to the disasm interface
 				arm.disasm.Step(entry)
 
-				// additional output for disassembler of type CartCoProcDisassemblerStdout type
-				if isDisasmStdout {
-					for i, r := range arm.state.registers {
-						fmt.Printf("\tR%02d: %08x\t", i, r)
-						if (i+1)%4 == 0 {
-							fmt.Println()
-						}
-					}
-					fmt.Println()
+				// print additional information output for stdout
+				if _, ok := arm.disasm.(*mapper.CartCoProcDisassemblerStdout); ok {
+					fmt.Println(arm.disasmVerbose(entry))
 				}
 			}
 		}
@@ -929,9 +899,9 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 
 		// check stack for stack collision
 		if err, detail := arm.stackCollision(stackPointerBeforeExecution); err != nil {
-			logger.Logf("ARM7", err.Error())
-			if arm.memoryErrorDetail != nil {
-				logger.Logf("ARM7", detail.Error())
+			logger.Logf("ARM7: stack collision", err.Error())
+			if detail != nil {
+				logger.Logf("ARM7: stack collision", "%s", detail.Error())
 			}
 
 			if arm.abortOnStackCollision && arm.breakpointsEnabled {
@@ -941,16 +911,24 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 
 		// handle memory access errors
 		if arm.memoryError != nil {
-			// not quiting so we log instead
-			logger.Logf("ARM7", arm.memoryError.Error())
-			if arm.memoryErrorDetail != nil {
-				logger.Logf("ARM7", arm.memoryErrorDetail.Error())
+			logger.Logf("ARM7: memory error", arm.memoryError.Error())
+			if arm.memoryErrorDev != nil {
+				logger.Logf("ARM7: memory error", arm.memoryErrorDev.Error())
+			}
+
+			if arm.prefs.ExtendedMemoryErrorLogging.Get().(bool) {
+				entry, err := arm.disassemble(opcode)
+				if err == nil {
+					logger.Logf("ARM7: memory error", "disassembly: %s", entry.String())
+				}
+
+				logger.Logf("ARM7: memory error", arm.disasmVerbose(entry))
 			}
 
 			// we need to reset the memory error instances so that we don't end
 			// up printing the same message over and over
 			arm.memoryError = nil
-			arm.memoryErrorDetail = nil
+			arm.memoryErrorDev = nil
 
 			if arm.abortOnIllegalMem && arm.breakpointsEnabled {
 				arm.state.interrupt = true
@@ -960,7 +938,7 @@ func (arm *ARM) run() (mapper.YieldReason, float32) {
 
 		// handle execution errors
 		if arm.executionError != nil {
-			logger.Logf("ARM7", arm.executionError.Error())
+			logger.Logf("ARM7: execution error", arm.executionError.Error())
 
 			if arm.breakpointsEnabled {
 				arm.state.interrupt = true

hardware/memory/cartridge/arm/disassembly.go

@@ -19,9 +19,34 @@ import (
 	"encoding/binary"
 	"fmt"
 	"strings"
+
+	"github.com/jetsetilly/gopher2600/hardware/memory/cartridge/arm/architecture"
 )
 
+// disassemble according to the current ARM architecture
 func (arm *ARM) disassemble(opcode uint16) (DisasmEntry, error) {
+	var entry DisasmEntry
+	var err error
+
+	switch arm.mmap.ARMArchitecture {
+	case architecture.ARM7TDMI:
+		entry, err = arm.disassembleARM7TDMI(opcode)
+		if err != nil {
+			return DisasmEntry{}, err
+		}
+	case architecture.ARMv7_M:
+		entry, err = arm.disassembleARM7vM(opcode)
+		if err != nil {
+			return DisasmEntry{}, err
+		}
+	default:
+		panic(fmt.Sprintf("unhandled ARM architecture: %s", arm.mmap.ARMArchitecture))
+	}
+
+	return entry, nil
+}
+
+func (arm *ARM) disassembleARM7TDMI(opcode uint16) (DisasmEntry, error) {
 	arm.decodeOnly = true
 	defer func() {
 		arm.decodeOnly = false
@@ -42,7 +67,7 @@ func (arm *ARM) disassemble(opcode uint16) (DisasmEntry, error) {
 	return *e, nil
 }
 
-func (arm *ARM) disassembleThumb2(opcode uint16) (DisasmEntry, error) {
+func (arm *ARM) disassembleARM7vM(opcode uint16) (DisasmEntry, error) {
 	arm.decodeOnly = true
 	defer func() {
 		arm.decodeOnly = false
@@ -163,7 +188,7 @@ func StaticDisassemble(config StaticDisassembleConfig) error {
 					e.Operand = fmt.Sprintf("%v", r)
 				}
 			}()
-			return arm.disassembleThumb2(opcode)
+			return arm.disassembleARM7vM(opcode)
 		}()
 
 		if err == nil {
@@ -184,3 +209,25 @@ func StaticDisassemble(config StaticDisassembleConfig) error {
 
 	return nil
 }
+
+// disasmVerbose provides more detail for the disasm entry
+func (arm *ARM) disasmVerbose(entry DisasmEntry) string {
+	var s strings.Builder
+
+	s.WriteString(fmt.Sprintf("instruction PC: %08x\n", entry.Addr))
+	if entry.Is32bit {
+		s.WriteString(fmt.Sprintf("opcode: %04x %04x \n", entry.OpcodeHi, entry.Opcode))
+	} else {
+		s.WriteString(fmt.Sprintf("opcode: %04x       \n", entry.Opcode))
+	}
+
+	// register information for verbose output
+	for i, r := range arm.state.registers {
+		s.WriteString(fmt.Sprintf("\tR%02d: %08x", i, r))
+		if (i+1)%4 == 0 {
+			s.WriteString(fmt.Sprintf("\n"))
+		}
+	}
+
+	return s.String()
+}

hardware/memory/cartridge/arm/memory_access.go

@@ -29,11 +29,9 @@ func (arm *ARM) illegalAccess(event string, addr uint32) {
 	}
 
 	detail := arm.dev.IllegalAccess(event, arm.state.instructionPC, addr)
-	if detail == "" {
-		return
+	if detail != "" {
+		arm.memoryErrorDev = fmt.Errorf("%s: %s", event, detail)
 	}
-
-	arm.memoryErrorDetail = fmt.Errorf("%s: %s", event, detail)
 }
 
 // nullAccess is a special condition of illegalAccess()
@@ -49,7 +47,7 @@ func (arm *ARM) nullAccess(event string, addr uint32) {
 		return
 	}
 
-	arm.memoryErrorDetail = fmt.Errorf("%s: %s", event, detail)
+	arm.memoryErrorDev = fmt.Errorf("%s: %s", event, detail)
 }
 
 // imperfect check of whether stack has collided with memtop
@@ -73,7 +71,7 @@ func (arm *ARM) stackCollision(stackPointerBeforeExecution uint32) (err error, d
 	// will no longer be checked for legality
 	arm.stackHasCollided = true
 
-	err = fmt.Errorf("stack: collision with program memory (%08x)", arm.state.registers[rSP])
+	err = fmt.Errorf("collision with program memory (%08x)", arm.state.registers[rSP])
 
 	if arm.dev != nil {
 		return
@@ -84,7 +82,7 @@ func (arm *ARM) stackCollision(stackPointerBeforeExecution uint32) (err error, d
 		return
 	}
 
-	detail = fmt.Errorf("stack: %s", detailStr)
+	detail = fmt.Errorf("%s", detailStr)
 
 	return err, detail
 }

hardware/preferences/arm_preferences.go

@@ -52,6 +52,9 @@ type ARMPreferences struct {
 	// abort Thumb program if stack pointer collides with memory occupied by
 	// program variables
 	AbortOnStackCollision prefs.Bool
+
+	// include disassembly and register details when logging memory errors
+	ExtendedMemoryErrorLogging prefs.Bool
 }
 
 func (p *ARMPreferences) String() string {
@@ -94,6 +97,10 @@ func newARMprefrences() (*ARMPreferences, error) {
 	if err != nil {
 		return nil, err
 	}
+	err = p.dsk.Add("hardware.arm7.extendedMemoryErrorLogging", &p.ExtendedMemoryErrorLogging)
+	if err != nil {
+		return nil, err
+	}
 	err = p.dsk.Load(true)
 	if err != nil {
 		return nil, err
@@ -110,6 +117,7 @@ func (p *ARMPreferences) SetDefaults() {
 	p.Immediate.Set(false)
 	p.MAM.Set(-1)
 	p.AbortOnIllegalMem.Set(false)
+	p.ExtendedMemoryErrorLogging.Set(false)
 }
 
 // Load current arm preference from disk.