Debugger breakpoints: wrong bank #512
Comments
|
Also related to #217. |
|
We could use the already existing conditional breakpoints (e.g. generally or via config or in case of a multi bank ROM). Currently all normal breakpoints (and traps) only check for the PC. This is done via a 64k bitset for speed reasons. But maybe this is not that relevant anymore? |
|
Keep using the 64k bitset for initial check, but once a breakpoint is discovered, then check if it is the correct bank and if not, ignore...? |
|
I've though about this in the past, but never had time to revisit it. It also needs work in the UI, since clicking an address in the disassembly right now creates a naive breakpoint (ie, one that only looks at address, not bank). There needs to be a way to graphically set the kind you want (address AND bank). And perhaps have them be different colours/shapes too, to indicate which is active. I agree with @thrust26 that the easiest way with our current infrastructure is to just use a CBP. So the former looks like this: So the infrastructure is there, just with a little more typing. Presenting a choice to the user in UI mode would then allow the code to 'write' that breakpoint for us. In the end, I think that's the best approach. |
I thought about that too. A problem are multiple breakpoints at the same address but different banks. We would have to add a counter for each address. Else deleting one breakpoint would affect the other ones too. Maybe something for later, if the new solution turns out to be too slow. |
|
I still think a CBP is what we need. The bitset is for a simple use-case: unconditional breakpoint. As soon as we add an extra condition (ie, a corresponding bank), then it becomes a conditional breakpoint, hence CBP. The infrastructure is already there; there's no need to complicate the bitset approach to do something it wasn't designed to do. Just use a CBP and be done with it. Also, they're not that slow. |
|
In any event, the UI work still needs to be done no matter what approach we take. It should be easy to just point and click, and add the different types of BP without having to type anything. But until that happens, the typing approach works right now. |
|
Agreed. I will have a look. |
|
Well, I didn't mean to imply that it had to be you, or even that we need to do this before 6.1. I just wanted to point out that this is more of a UI issue than anything else. The bankend can already do it; we just need to extend the functionality to the frontend. |
|
If we completely remove the unconditional breakpoints, we have no UI issue anymore. |
|
There are two issues with going that route:
|
|
I defer to the experience of people actually using Stella for real 2600 development; you and @andrew-davie. |
|
I have been following the conversation. I agree with the proposed solution regarding using existing infrastructure (i.e., CBP) to implement the breakpoints with bank. That makes perfect sense to me. I don’t really see how anyone would want a breakpoint WITHOUT a bank, so I’d probably drop that capability.
… On 20 Aug 2019, at 12:32 am, Stephen Anthony ***@***.***> wrote:
I defer to the experience of people actually using Stella for real 2600 development; you and @andrew-davie <https://github.com/andrew-davie>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#512?email_source=notifications&email_token=AD5JZC2QTLTJS6VVFUMX4JDQFKVGVA5CNFSM4IK37LZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TEKPY#issuecomment-522601791>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5JZCZYBHFJIXH4GWLIJ3LQFKVGVANCNFSM4IK37LZA>.
|
|
And one could still define a conditional breakpoint manually, without providing the bank as argument. |
|
So, when I’m in the UI and stepping through… and I double-click on the column for breakpoints, it can put in an automatic “banked” breakpoint for me (as a CBP), because it knows the bank it’s in, and of course the PC. No need for a menu or anything. As to using the command-line, if I type “break label” then perhaps it could add a CPB breakpoint for EVERY bank at that label address. Then I get the original behaviour. If that’s not what I wanted I can disable each breakpoint that I didn’t want via UI as I hit it.
… On 20 Aug 2019, at 12:30 am, Stephen Anthony ***@***.***> wrote:
There are two issues with going that route:
the unconditional BP are faster; but maybe that doesn't matter so much anymore?
more importantly, what if someone want to set a BP and not consider banks? Again, maybe that isn't a use case we need to worry about??
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#512?email_source=notifications&email_token=AD5JZCZ3BDRHT5TPILHLAHDQFKVB7A5CNFSM4IK37LZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4TEGEY#issuecomment-522601235>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5JZC7VL7O2SFVMX5NSUO3QFKVB7ANCNFSM4IK37LZA>.
|
This is probably the most elegant solution. If we make all BP conditional, then the whole problem goes away (UI issues included). Then the only potential issue is that CBP are slower. But we can test that right now, before any work is started. I don't think it will be an issue on any modern computer, and even if it slows down slightly, that's to be expected when enabling debugging features. |
|
@andrew-davie Yup. And you can type Though removing break points added without bank by clicking in the breakpoint column needs some thought.:
What's better? |
|
This address mirroring in the 2600 really complicates things, doesn't it? 😈 |
|
@sa666666 You won't be surprised if I tell you that I already started? I love challenges! 😄 One more thought: "break label" could alternatively automatically add the current bank. Then the problem described above is gone. |
Sigh. I used to feel that way too, and I miss it. I'm just so overloaded with stuff now that I'm getting a little burnt out. Hopefully I can get back to the 'challenging' stuff soon too. Multi-bank disassembly is one that I really want to work on ... |
|
When I am back at work next Tuesday, I will feel more like you do now. Multi-bank disassembly will become very challenging, DiStella was never meant for that and the code shows. I happily leave that task for up. 😈 BTW: Is there an issue for this? |
|
Yes, #284. In fact you created it! |
|
I just checked the maximum frame rates with multiple conditional breakpoints. The slowdown is quite noticeable on my notebook (i7-7820HQ):
So for slower machines we should consider some optimizations. |
|
I don't think that's actually that bad. How many times would one have 20 BP at a time? And even if the machine is 10x slower and gets 60fps max, now maybe it only gets 40fps max (assuming this scales linearly). Of course I'm all for efficiency, but at the same time, if you're doing heavy debugging then you have to accept more CPU usage and potential slowdown. |
|
Yea, the single thread performance of my Z8350 based mini notebook is only about 5x slower than the i7. So that should be sufficient. And I did some minor optimizations (e.g. no bank check for non-bankswitched ROMs). |
|
You can probably also extend that optimization to ROMs that support only one bank but still have bankswitching. Some of the more complicated schemes only report one bank, since it's much too complicated otherwise. In those cases the bank check is also redundant. |
|
But couldn't their banks share the same addresses? |
|
@andrew-davie But here is another thing:
Problems:
The root problem are the upper three bits of the address. In reality they do not exist. But we developers got used to them and also used and still use them to workaround our tools' (e.g. DiStella) flaws. BTW: If we decide for one thing or the other we should make this consistent for the debugger. |
|
I'd mask bits 15-13. If I understand correctly, this is option 3 😏 |
|
Correct. I am leaning towards it too. But I am not exactly happy with it. |
|
I went for option 3 and also reimplemented the breakpoints which now use a hash map. So there are no slowdowns, even with a LOT of breakpoints. As of now -1 for the bank enables a bp in all banks. This may change. |
|
Thank you, I will try and test soon. |
|
Agreed. IMO So I need a good name for putting a breakpoint in all banks. Then you will also be able to type just |
This is NOT my preferred course of action, as I truly believe if I say "break address" then I should get a breakpoint no matter what bank happens to be switched in when the CPU hits that address. As stated earlier, if I want to break in a specific bank and only that bank, then I should specify it. Having said that, and hopefully making it totally clear I do not think this is a good solution... how about using "breakall" as the command to implement it the way you want? |
Following that argument, To me, |
I believe "break" should maintain its existing behaviour and break at every bank with the given address. Yes. And if you don't want that behaviour, then specify the bank you want. Or, disable the breaks as they trigger in the banks you don't want them, via UI click. So no, "breakall" is not the solution I prefer. It is an ugly hack/compromise to get around the issues you are having with parsing numbers/text. I prefer "break" to do what it has always done - break at every bank with the given address. And if that's not what the user wants, with the extra 'bank' option, you get the choice to limit it to one bank. THAT's what I prefer. |
|
To me we fixed a long standing bug with I suppose this is one of those cases where we both cannot agree, except that we agree on that. 😄 |
|
I'm not really wanting to argue this much more. But I will emphasise what I think are the important points regarding using 'break' in the debugger with a bankswitched game. Of prime importance is that it is possible, even probable that the person doing the debugging has absolutely no idea of the actual bank in which code is living. All they have is a label (say, the entry to a subroutine). So, let's say they want to put a breakpoint at the entry to the subroutine 'DrawTheScreen'. How do they do that in the new system via your method? They have to use 'breakall' - in other words, put a breakpoint in every single bank so that the 'DrawTheScreen' PC address will be caught. In other words, what I expect to be the DEFAULT behaviour for most breakpoints usage when debugging (because you almost certainly do not know the bank), is now to use 'breakall'. You have therefore changed usage from 'break' (which used to do this - that is, effectively put breaks in all banks) to a new one 'breakall'. What has that actually achieved? Nothing but confusing existing users. I think it's extremely unlikely and very rare that anyone will actually type the bank number when entering a breakpoint. After all, there is no bank number available in the symbol table, unless you specifically add a label for it to tie label/bank together somehow. So to me, what you are coding for is the least used case of all. In fact the most used case is the previous behaviour (break on all banks), and the whole discussion was to allow those extra bank breakpoints to be removed if required. And that lead to having the CBPs added, and thus easy removal of the breaks that were not wanted. In summary, then, I think your use-case is not well considered. The previous behaviour was the most likely scenario (and not, IMHO, a bug). It only needed the change to CPB to allow individual ones to be removed as required. Perhaps we just use the debugger totally differently. In any case, hopefully I'll fall silent now and you can implement it however you think is best. I appreciate all your hard work on this! |
|
Here is another use-case, which I have just encountered. Your game crashes. Stella halts, and you can see addresses on the stack. F809. You want to put a breakpoint on that location - but you don't, of course, know which bank it was from. The most common requirement here would be to want a break in EVERY bank so you could find out where it came from. |
|
@andrew-davie The problems we are facing are a direct result of multiple complexities:
Previously the When designing a UI, the complexity should be mitigated by the design. Making the UI consistent is a major point here. Keeping it backward compatible is another one. Setting and clearing a breakpoint via I then added the option to break in all banks to suit your needs (even though you are not happy with it). But by now I realized that this is not sufficient and your suggestion too. When you are searching for a label and its bank, the upper 3 bits usually (depending on coding style) become important. When you use the upper 3 bits to differentiate banks and there are no more than 8 banks, each label becomes totally unique. And even for more banks, they are still quite unique. So there is no need to set breakpoints in all banks (if you assume that the PC is set correctly, see 4.). Instead the break command should use all 16 bits in this case. Therefore, depending on the situation, it makes sense to search for 13 or 16 bit addresses. I do not like to handle this via extra parameter as it adds extra complexity to the UI. Therefore we need a new command for the latter (e.g. So that's my plan, the best solution I found by now. But I am open for more arguments and suggestions as long as my points (especially UI consistency) given above are considered too. |
|
Any more feedback on this one? Else I will close it in some days. |
|
Initial test has me confused. I have 3E bankswitching, and put a breakpoint on a label I KNOW is in bank $F. "Reset". That's the start vector entry point to the game. So, 'breaklabel Restart' and then 'run'. It breaks, but instead of in bank $F it's showing me a correct disassembly in bank $B. I switch the bank to anything via the manual bank-select droplist in the 3E tab, and nothing changes. I think the issue here is that the fixed bank ($F) is NOT one of the selectable banks - that is, the disassembly is showing the non-bankswitched code, but the title of it is as if it's a bank. Perhaps it would be useful to change the wording to include bank # ($F000-$F800) + fixed ($f800-$FFFF) or at least something. I know, this was my misunderstanding, but still... Back to 'breaklabel'... it worked as advertised for a label in the fixed bank. |
|
I have to check the first case. Actually I am not quite clear what happens. Does it break at the wrong place or is the disassembly shown just the wrong one? Or is just the bank number display "wrong"? "breaklabel" will of course only work correctly if the origin on assembly matches the one on execution. There will be cases were this problem circumvented and maybe there are other cases where this turns out difficult to impossible. The complexity strikes again here and probably we have to find a good compromise here. E.g. with 1K slice sizes, code with was assembled in slice 3, could go to slice 0, 1, 2 or 3 (even changing and/or multiple ones). There is no way for Stella to know this. In your case, a correct ORG should fix the problem. |
|
The first case: It broke in the correct place, say $FC2F. That's in bank $F (fixed). |
|
Yes, that's irritating. For a general solution, we would have to either list all slices (only two in your case). IMO that's probably irritating as well. Or we detect the bank from the address and react accordingly. So when it breaks in the first slice(s), display "bank #N" from that slice, else we display e.g. "bank #15" or "fixed bank". |
|
How about put a little colour box next to the bank # display in the title, and use that colour as the background for the displayed bank ONLY. Thus, if you see a background of a different colour, you know it's NOT the bank displayed. Also, when you click anywhere in the disassembly window, the bank # should update to show correctly what bank is currently being seen at that point/line? |
|
That would probably require a lot of work. The disassembler and the breakpoint handling are in very different places of the code. Making them interact would need quite some refactoring. |
But the UI already has the ability to know what address a click is. You can set/clear breakpoints on any line, and the current PC line is highlighted... so clearly the address of each line is available. Given that, it would be pretty straightforward in the draw of the disassembly to change the background colour based on address on that line...? |
|
Yes, that would be possible. But that wouldn't be sufficient. The root problem is, that there is no interface in the |
|
@andrew-davie |
TY! |
|
I suppose this is as good as it gets for now. |
It seems that when you have a breakpoint, only the PC address is used/recorded for the breakpoint. If I am stepping through code and I click in the breakpoint column to add a breakpoint, and then continue - the breakpoint can be (is!) triggered by access to the PC address from a different bank. This is not good behaviour, because I don't want to break on code in a different bank - I want to break on code in the bank I set the breakpoint in!
In other words, breakpoints should keep track of the bank they belong to. Yes, this isn't possible to know if you set a breakpoint via label ("break label"), but in that case you would want to break when PC = label in ANY bank. But when you do it manually, you're stepping through code, you most definitely do NOT want it to break whenever PC=addr. You want it to break ONLY when PC==addr AND bank=bank that was switched in when breakpoint was set.
In other words, I think breakpoints also need a bank field. Normally this should mean "any bank" but in the case where set via step-through and set-breakpoint-here type usage, then the bank should be constrained to the one you are currently in.
The text was updated successfully, but these errors were encountered: