From 3a20e6d394dbb739c904a35777b8a0d5b415dc9e Mon Sep 17 00:00:00 2001 From: steve Date: Wed, 15 Aug 2018 10:13:20 +0100 Subject: [PATCH] o breakpoints - reworked breakpoints so that we can AND conditions together - breakpoints are now checked every video cycle regardless of current step mode - this is required if we want to break on attributes like Horiz Pos (HP) which may jump over values in CPU step mode --- debugger/breakpoints.go | 230 ++++++++++++++++++++++---------- debugger/commands.go | 2 +- debugger/debugger.go | 94 ++++++++----- debugger/print.go | 3 + debugger/traps.go | 20 ++- television/sdltv/guiloop.go | 4 +- television/sdltv/tvinterface.go | 14 +- television/television.go | 6 +- 8 files changed, 257 insertions(+), 116 deletions(-) diff --git a/debugger/breakpoints.go b/debugger/breakpoints.go index dc604bf8..13c93557 100644 --- a/debugger/breakpoints.go +++ b/debugger/breakpoints.go @@ -1,6 +1,6 @@ // breakpoints are used to halt execution when a target is *changed to* a // specific value. compare to traps which are used to halt execution when the -// target *changes* from its current value to any other value. +// target *changes from* its current value *to* any other value. package debugger @@ -10,21 +10,86 @@ import ( "gopher2600/debugger/ui" "gopher2600/errors" "strconv" + "strings" ) -// breakpoints keeps track of all the currently defined breaker +// breakpoints keeps track of all the currently defined breakers type breakpoints struct { dbg *Debugger breaks []breaker - - // ignore certain target values - ignoredBreakerStates map[target]interface{} } // breaker defines a specific break condition type breaker struct { - target target - value interface{} + target target + value interface{} + ignoreValue interface{} + + // basic linked list to implement AND-conditions + next *breaker + prev *breaker +} + +func (bk breaker) String() string { + b := strings.Builder{} + b.WriteString(fmt.Sprintf("%s->%d", bk.target.ShortLabel(), bk.value)) + n := bk.next + for n != nil { + b.WriteString(fmt.Sprintf(" & %s->%d", n.target.ShortLabel(), n.value)) + n = n.next + } + return b.String() +} + +// isSingleton checks if break condition is part of a list (false) or is a +// singleton condition (true) +func (bk breaker) isSinglton() bool { + return bk.next == nil && bk.prev == nil +} + +// breaker.check checks the specific break condition with the current value of +// the break target +func (bk *breaker) check() bool { + currVal := bk.target.Value() + b := currVal == bk.value + + if bk.next == nil { + b = b && currVal != bk.ignoreValue + + // this is either a singleton break or the end of a break-list + // (inList==true). note how we set the ignoreValue in these two + // instances. if it's a singleton break then we always reset the + // ignoreValue. if it's the end of the list we reset the value to nil + // if there is no match + if bk.isSinglton() { + bk.ignoreValue = currVal + } else { + bk.ignoreValue = nil + } + return b + } + + // this breaker is part of list so we need to recurse into the list + b = b && bk.next.check() + if b { + b = b && currVal != bk.ignoreValue + bk.ignoreValue = currVal + } else { + bk.ignoreValue = nil + } + + return b +} + +// add appends a new breaker object to the *end of the list* from the perspective +// of bk +func (bk *breaker) add(nbk *breaker) { + n := &bk.next + for *n != nil { + nbk.prev = *n + *n = (*n).next + } + *n = nbk } // newBreakpoints is the preferred method of initialisation for breakpoins @@ -39,46 +104,18 @@ func (bp *breakpoints) clear() { bp.breaks = make([]breaker, 0, 10) } -// prepareBreakpoints prepares for next breakpoint by storing the current state -// of all Targets. we can then use these stored values to know what to -// ignore. used primarily so that we're not breaking immediately on a previous -// breakstate. -func (bp *breakpoints) prepareBreakpoints() { - bp.ignoredBreakerStates = make(map[target]interface{}, len(bp.breaks)) - for _, b := range bp.breaks { - bp.ignoredBreakerStates[b.target] = b.target.Value() - } -} - -// check compares the current state of the emulation with every break -// condition. it lists every condition that applies, not just the first -// condition it encounters. -func (bp *breakpoints) check() bool { - broken := false +// breakpoints.check compares the current state of the emulation with every +// break condition. it returns a string listing every condition that applies +func (bp *breakpoints) check(previousResult string) string { + checkString := strings.Builder{} + checkString.WriteString(previousResult) for i := range bp.breaks { // check current value of target with the requested value - if bp.breaks[i].target.Value() == bp.breaks[i].value { - // make sure that we're not breaking on an ignore state - bv, prs := bp.ignoredBreakerStates[bp.breaks[i].target] - if !prs || prs && bp.breaks[i].target.Value() != bv { - bp.dbg.print(ui.Feedback, "break on %s=%d", bp.breaks[i].target.ShortLabel(), bp.breaks[i].value) - broken = true - } + if bp.breaks[i].check() { + checkString.WriteString(fmt.Sprintf("break on %s\n", bp.breaks[i])) } } - - // remove ignoreBreakerState if the break target has changed from its - // ignored value - if !broken { - for i := range bp.breaks { - bv, prs := bp.ignoredBreakerStates[bp.breaks[i].target] - if prs && bp.breaks[i].target.Value() != bv { - delete(bp.ignoredBreakerStates, bp.breaks[i].target) - } - } - } - - return broken + return checkString.String() } func (bp breakpoints) list() { @@ -86,68 +123,123 @@ func (bp breakpoints) list() { bp.dbg.print(ui.Feedback, "no breakpoints") } else { for i := range bp.breaks { - bp.dbg.print(ui.Feedback, "%s->%d", bp.breaks[i].target.ShortLabel(), bp.breaks[i].value) + bp.dbg.print(ui.Feedback, "%s", bp.breaks[i]) } } } +// parseBreakpoints consumes tokens and adds new conditions to the list of +// breakpoints. For example: +// +// PC 0xf000 +// adds a new breakpoint to the PC +// +// 0xf000 +// is the same, because we assume a target of PC if none is given +// +// X 10 11 +// adds two new breakpoints to X - we've changed targets so the second value +// is assumed to be for the previously selected target +// +// X 10 11 Y 12 +// add three breakpoints; 2 to X and 1 to Y +// +// SL 100 & HP 0 +// add one AND-condition +// +// SL 100 & HP 0 | X 10 +// add two conditions; one AND-condition and one condition on X +// +// note that this is a very simple parser and we can do unusual things: the & +// and | symbols simply switch "modes", with unusual consequences. for example, +// the last example above could be written: +// +// & SL 100 HP 0 | X 10 +// +// TODO: more sophisticated breakpoints parser func (bp *breakpoints) parseBreakpoint(tokens *input.Tokens) error { - var tgt target - - // resolvedTarget notes whether a target has been used correctly - var resolvedTarget bool + andBreaks := false // default target of CPU PC. meaning that "BREAK n" will cause a breakpoint // being set on the PC. breaking on PC is probably the most common type of // breakpoint. the target will change value when the input string sees // something appropriate - tgt = bp.dbg.vcs.MC.PC + tgt := target(bp.dbg.vcs.MC.PC) // resolvedTarget is true to begin with so that the initial target of PC // can be changed immediately - resolvedTarget = true + resolvedTarget := true + + // we don't add new breakpoints to the main list straight away. we append + // them to newBreaks first and then check that we aren't adding duplicates + newBreaks := make([]breaker, 0, 10) // loop over tokens. if token is a number then add the breakpoint for the // current target. if it is not a number, look for a keyword that changes // the target (or run a BREAK meta-command) // // note that this method of looping allows the user to chain break commands - a, present := tokens.Get() + tok, present := tokens.Get() for present { - val, err := strconv.ParseUint(a, 0, 16) + val, err := strconv.ParseUint(tok, 0, 16) if err == nil { - // check to see if breakpoint already exists - addNewBreak := true - for _, mv := range bp.breaks { - if mv.target == tgt && mv.value == int(val) { - addNewBreak = false - bp.dbg.print(ui.Feedback, "breakpoint already exists") - break // for loop + if andBreaks == true { + if len(newBreaks) == 0 { + newBreaks = append(newBreaks, breaker{target: tgt, value: int(val)}) + } else { + newBreaks[len(newBreaks)-1].add(&breaker{target: tgt, value: int(val)}) } + resolvedTarget = true + } else { + newBreaks = append(newBreaks, breaker{target: tgt, value: int(val)}) + resolvedTarget = true } - if addNewBreak { - bp.breaks = append(bp.breaks, breaker{target: tgt, value: int(val)}) - } - resolvedTarget = true + } else { if !resolvedTarget { return errors.NewGopherError(errors.InputTooFewArgs, fmt.Errorf("need a value to break on (%s)", tgt.Label())) } - tokens.Unget() - tgt, err = parseTarget(bp.dbg, tokens) - if err != nil { - return err + if tok == "&" { + andBreaks = true + } else if tok == "|" { + andBreaks = false + } else { + tokens.Unget() + tgt, err = parseTarget(bp.dbg, tokens) + if err != nil { + return err + } + resolvedTarget = false } - resolvedTarget = false } - a, present = tokens.Get() + tok, present = tokens.Get() } if !resolvedTarget { return errors.NewGopherError(errors.InputTooFewArgs, fmt.Errorf("need a value to break on (%s)", tgt.Label())) } + // don't add breakpoints that already exist (only works correctly with + // singleton breaks currently) + // TODO: fix this so we do not add AND-conditions that already exist + for _, nb := range newBreaks { + if nb.next == nil { + exists := false + for _, ob := range bp.breaks { + if ob.next == nil && ob.target == nb.target && ob.value == nb.value { + bp.dbg.print(ui.Feedback, "breakpoint already exists (%s)", ob) + exists = true + } + } + if !exists { + bp.breaks = append(bp.breaks, nb) + } + } else { + bp.breaks = append(bp.breaks, nb) + } + } + return nil } diff --git a/debugger/commands.go b/debugger/commands.go index 53037435..6ecaeb22 100644 --- a/debugger/commands.go +++ b/debugger/commands.go @@ -113,7 +113,7 @@ var commandTemplate = input.CommandTemplate{ KeywordBall: "", KeywordPlayfield: "", KeywordDisplay: "[|OFF]", - KeywordMouse: "", + KeywordMouse: "[|X|Y]", KeywordScript: "%F", KeywordDisassemble: "", } diff --git a/debugger/debugger.go b/debugger/debugger.go index 253c1943..c999eb14 100644 --- a/debugger/debugger.go +++ b/debugger/debugger.go @@ -36,6 +36,10 @@ type Debugger struct { breakpoints *breakpoints traps *traps + // we accumulate break and trap messsages until we can service them + breakMessages string + trapMessages string + // any error from previous emulation step lastStepError bool @@ -75,11 +79,11 @@ type Debugger struct { // channel for communicating with the debugging loop from other areas of // the emulation, paritcularly from other goroutines. for instance, we use - // the callbackChannel to implement ctrl-c handling, even though all the + // the syncChannel to implement ctrl-c handling, even though all the // code to do it is right here in this very file. we do this to avoid // having to use sync.Mutex. marking critical sections with a mutex is fine // and would definitiely work, it is, frankly, a pain and feels wrong. - callbackChannel chan func() + syncChannel chan func() } // NewDebugger creates and initialises everything required for a new debugging @@ -118,15 +122,18 @@ func NewDebugger() (*Debugger, error) { // make callback channel - buffered because this channel may be used in the // same goroutine as the debuggin loop and we don't want it to deadlock - dbg.callbackChannel = make(chan func(), 2) + dbg.syncChannel = make(chan func(), 2) // register onMouseButton1 callback - err = tv.RegisterCallback(television.ReqOnMouseButton1, dbg.callbackChannel, func() { + err = tv.RegisterCallback(television.ReqOnMouseButton1, dbg.syncChannel, func() { // this callback function may be running inside a different goroutine // so care must be taken not to cause a deadlock - dbg.runUntilHalt = false + hp, _ := dbg.vcs.TV.RequestTVInfo(television.ReqLastMouseX) + sl, _ := dbg.vcs.TV.RequestTVInfo(television.ReqLastMouseY) - // TODO: set breakpoint at mouse coordinates + dbg.parseCommand(fmt.Sprintf("%s sl %s & hp %s", KeywordBreak, sl, hp)) + + // if the emulation is running the new break should cause it to halt }) if err != nil { return nil, err @@ -167,7 +174,7 @@ func (dbg *Debugger) Start(interf ui.UserInterface, filename string, initScript for loop { <-ctrlC - dbg.callbackChannel <- func() { + dbg.syncChannel <- func() { if dbg.runUntilHalt { dbg.runUntilHalt = false } else { @@ -222,11 +229,13 @@ func (dbg *Debugger) loadCartridge(cartridgeFilename string) error { return nil } -// videoCycleCallback() and noVideoCycleCallback() are wrapper functions to be -// used when calling vcs.Step() -- video stepping uses the former and cpu -// stepping uses the latter +// videoCycleCallback() and breakpointCallback() are wrapper functions to be +// used when calling vcs.Step(). stepmode CPU uses breakpointCallback(), +// whereas stepmode VIDEO uses videoCycleCallback() which in turn uses +// breakpointCallback() func (dbg *Debugger) videoCycleCallback(result *result.Instruction) error { + dbg.breakpointCallback(result) dbg.lastResult = result if dbg.commandOnStep != "" { _, err := dbg.parseInput(dbg.commandOnStep) @@ -237,7 +246,9 @@ func (dbg *Debugger) videoCycleCallback(result *result.Instruction) error { return dbg.inputLoop(false) } -func (dbg *Debugger) noVideoCycleCallback(result *result.Instruction) error { +func (dbg *Debugger) breakpointCallback(result *result.Instruction) error { + dbg.breakMessages = dbg.breakpoints.check(dbg.breakMessages) + dbg.trapMessages = dbg.traps.check(dbg.trapMessages) return nil } @@ -259,22 +270,17 @@ func (dbg *Debugger) inputLoop(mainLoop bool) error { return nil } - // check callback channel and run any functions we find in there + // check syncChannel and run any functions we find in there // TODO: not sure if this is the best part of the loop to put this // check. it works for now. select { - case f := <-dbg.callbackChannel: + case f := <-dbg.syncChannel: f() default: } - // check for breakpoints and traps. check() functions echo all the - // conditions that match - if dbg.inputloopNext { - bpCheck := dbg.breakpoints.check() - trCheck := dbg.traps.check() - dbg.inputloopHalt = bpCheck || trCheck || dbg.lastStepError - } + // check for deferred breakpoints and traps + dbg.inputloopHalt = dbg.breakMessages != "" || dbg.trapMessages != "" || dbg.lastStepError // reset last step error dbg.lastStepError = false @@ -289,6 +295,12 @@ func (dbg *Debugger) inputLoop(mainLoop bool) error { } } + // print and reset accumulated break and trap messages + dbg.print(ui.Feedback, dbg.breakMessages) + dbg.print(ui.Feedback, dbg.trapMessages) + dbg.breakMessages = "" + dbg.trapMessages = "" + // expand breakpoint to include step-once/many flag dbg.inputloopHalt = dbg.inputloopHalt || !dbg.runUntilHalt @@ -335,10 +347,7 @@ func (dbg *Debugger) inputLoop(mainLoop bool) error { } // prepare for next loop - // o forget about current break state - // o prepare for matching on next breakpoint dbg.inputloopHalt = false - dbg.breakpoints.prepareBreakpoints() // make sure tv is unpaused if emulation is about to resume if dbg.inputloopNext { @@ -355,7 +364,7 @@ func (dbg *Debugger) inputLoop(mainLoop bool) error { if dbg.inputloopVideoClock { _, dbg.lastResult, err = dbg.vcs.Step(dbg.videoCycleCallback) } else { - _, dbg.lastResult, err = dbg.vcs.Step(dbg.noVideoCycleCallback) + _, dbg.lastResult, err = dbg.vcs.Step(dbg.breakpointCallback) } if err != nil { @@ -503,22 +512,28 @@ func (dbg *Debugger) parseCommand(userInput string) (bool, error) { case KeywordList: list, _ := tokens.Get() - switch strings.ToUpper(list) { + list = strings.ToUpper(list) + switch list { case "BREAKS": dbg.breakpoints.list() case "TRAPS": dbg.traps.list() + default: + return false, fmt.Errorf("unknown list option (%s)", list) } case KeywordClear: clear, _ := tokens.Get() - switch strings.ToUpper(clear) { + clear = strings.ToUpper(clear) + switch clear { case "BREAKS": dbg.breakpoints.clear() dbg.print(ui.Feedback, "breakpoints cleared") case "TRAPS": dbg.traps.clear() dbg.print(ui.Feedback, "traps cleared") + default: + return false, fmt.Errorf("unknown clear option (%s)", clear) } case KeywordOnHalt: @@ -593,7 +608,7 @@ func (dbg *Debugger) parseCommand(userInput string) (bool, error) { } dbg.print(printTag, "%s", dbg.lastResult.GetString(dbg.disasm.Symtable, result.StyleFull)) default: - return false, fmt.Errorf("unknown last request (%s)", option) + return false, fmt.Errorf("unknown last request option (%s)", option) } } @@ -727,7 +742,7 @@ func (dbg *Debugger) parseCommand(userInput string) (bool, error) { } dbg.print(ui.MachineInfo, info) default: - return false, fmt.Errorf("unknown tv info (%s)", option) + return false, fmt.Errorf("unknown info request (%s)", option) } } else { dbg.printMachineInfo(dbg.vcs.TV) @@ -756,9 +771,12 @@ func (dbg *Debugger) parseCommand(userInput string) (bool, error) { visibility := true action, present := tokens.Get() if present { - switch strings.ToUpper(action) { + action = strings.ToUpper(action) + switch action { case "OFF": visibility = false + default: + return false, fmt.Errorf("unknown display action (%s)", action) } } err := dbg.vcs.TV.SetVisibility(visibility) @@ -767,7 +785,23 @@ func (dbg *Debugger) parseCommand(userInput string) (bool, error) { } case KeywordMouse: - info, err := dbg.vcs.TV.RequestTVInfo(television.ReqLastMouse) + req := television.ReqLastMouse + + coord, present := tokens.Get() + + if present { + coord = strings.ToUpper(coord) + switch coord { + case "X": + req = television.ReqLastMouseX + case "Y": + req = television.ReqLastMouseY + default: + return false, fmt.Errorf("unknown mouse option (%s)", coord) + } + } + + info, err := dbg.vcs.TV.RequestTVInfo(req) if err != nil { return false, err } diff --git a/debugger/print.go b/debugger/print.go index 32e13f22..764ffe2b 100644 --- a/debugger/print.go +++ b/debugger/print.go @@ -16,6 +16,9 @@ func (dbg *Debugger) print(pp ui.PrintProfile, s string, a ...interface{}) { // trim *all* trailing newlines - UserPrint() will add newlines if required s = strings.TrimRight(s, "\n") + if s == "" { + return + } dbg.ui.UserPrint(pp, s, a...) } diff --git a/debugger/traps.go b/debugger/traps.go index 20363148..dc944bfa 100644 --- a/debugger/traps.go +++ b/debugger/traps.go @@ -8,6 +8,7 @@ import ( "fmt" "gopher2600/debugger/input" "gopher2600/debugger/ui" + "strings" ) // traps keeps track of all the currently defined trappers @@ -34,21 +35,18 @@ func (tr *traps) clear() { tr.traps = make([]trapper, 0, 10) } -// check compares the current state of the emulation with every trap -// condition. it lists every condition that applies, not just the first -// condition it encounters. -func (tr *traps) check() bool { - trapped := false +// check compares the current state of the emulation with every trap condition. +// it returns a string listing every condition that applies +func (tr *traps) check(previousResult string) string { + checkString := strings.Builder{} + checkString.WriteString(previousResult) for i := range tr.traps { - hasTrapped := tr.traps[i].target.Value() != tr.traps[i].origValue - if hasTrapped { + if tr.traps[i].target.Value() != tr.traps[i].origValue { tr.traps[i].origValue = tr.traps[i].target.Value() - tr.dbg.print(ui.Feedback, "trap on %s", tr.traps[i].target.ShortLabel()) + checkString.WriteString(fmt.Sprintf("trap on %s", tr.traps[i].target.ShortLabel())) } - trapped = hasTrapped || trapped } - - return trapped + return checkString.String() } func (tr traps) list() { diff --git a/television/sdltv/guiloop.go b/television/sdltv/guiloop.go index d223a489..187e13a8 100644 --- a/television/sdltv/guiloop.go +++ b/television/sdltv/guiloop.go @@ -56,7 +56,7 @@ func (tv *SDLTV) guiLoop() { // the opposite of pixelX() and also the scalining applied // by the SDL renderer if tv.scr == tv.dbgScr { - tv.mouseX = int(float32(int(ev.X)-tv.Spec.ClocksPerHblank) / sx) + tv.mouseX = int(float32(ev.X)/sx) - tv.Spec.ClocksPerHblank } else { tv.mouseX = int(float32(ev.X) / sx) } @@ -67,7 +67,7 @@ func (tv *SDLTV) guiLoop() { if tv.scr == tv.dbgScr { tv.mouseY = int(float32(ev.Y) / sy) } else { - tv.mouseY = int(float32(int(ev.Y)+tv.Spec.ScanlinesPerVBlank) / sy) + tv.mouseY = int(float32(ev.Y)/sy) + tv.Spec.ScanlinesPerVBlank } tv.onMouseButton1.dispatch() diff --git a/television/sdltv/tvinterface.go b/television/sdltv/tvinterface.go index beba43f3..f441152a 100644 --- a/television/sdltv/tvinterface.go +++ b/television/sdltv/tvinterface.go @@ -111,7 +111,19 @@ func (tv *SDLTV) RequestTVInfo(request television.TVInfoReq) (string, error) { // (R) tv.mouseX, tv.mouseY tv.guiLoopLock.Lock() defer tv.guiLoopLock.Unlock() - return fmt.Sprintf("mouse: hp=%d, sl=%d\n", tv.mouseX, tv.mouseY), nil + return fmt.Sprintf("mouse: hp=%d, sl=%d", tv.mouseX, tv.mouseY), nil + case television.ReqLastMouseX: + // * CRITICAL SEECTION* + // (R) tv.mouseX + tv.guiLoopLock.Lock() + defer tv.guiLoopLock.Unlock() + return fmt.Sprintf("%d", tv.mouseX), nil + case television.ReqLastMouseY: + // * CRITICAL SEECTION* + // (R) tv.mouseY + tv.guiLoopLock.Lock() + defer tv.guiLoopLock.Unlock() + return fmt.Sprintf("%d", tv.mouseY), nil default: return "", errors.NewGopherError(errors.UnknownTVRequest, request) } diff --git a/television/television.go b/television/television.go index e990d700..7f141301 100644 --- a/television/television.go +++ b/television/television.go @@ -11,8 +11,10 @@ const ( ReqScanline TVStateReq = "SCANLINE" ReqHorizPos TVStateReq = "HORIZPOS" - ReqTVSpec TVInfoReq = "TVSPEC" - ReqLastMouse TVInfoReq = "MOUSE" + ReqTVSpec TVInfoReq = "TVSPEC" + ReqLastMouse TVInfoReq = "MOUSE" + ReqLastMouseX TVInfoReq = "MOUSEX" + ReqLastMouseY TVInfoReq = "MOUSEY" ReqOnWindowClose CallbackReq = "ONWINDOWCLOSE" ReqOnMouseButton1 CallbackReq = "ONMOUSEBUTTON1"