labath added a comment. In https://reviews.llvm.org/D24610#554587, @omjavaid wrote:
> Give this approach a rethink I dont see a lot of problems with this final > implementation unless it fails on other architectures. > We are already hacking our way to have these byte selection watchpoints > working in existing code. New code seems to be improving the hack in my > opinion. - I don't believe that the presence of one hack justifies the addition of another. If anything, then it's the opposite. - The fact that we have to fiddle with the byte selects to get the kernel to accept our watchpoints could be thought of as a "hack", but I think there it's justified, as otherwise we would be unable to watch anything that isn't dword-aligned. And most importantly, the issue is contained within the SetHardwareWatchpoint function -- noone outside of that needs to know about the fact that we are doing something funny with byte-selects. This is not the case here, as I explain below. > Can you think of any scenarios which might fail for this approach? I can. Try the following sequence of commands: - position the PC on an instruction that will write to address A (aligned) - set a byte watchpoint on A+0 - set a byte watchpoint on A+1 - clear the second watchpoint - single-step What will happen with your patch applied? (*) Nothing, as the watchpoint will not get hit because you disabled it. Now this is a pretty dubious example, but I think it demonstrates, the problems I have with this solution very nicely. **You are lying to the client about which watchpoints are enabled**. The client asked you to remove just one watchpoint, so as far as he is concerned the other one is still active. But you have disabled the other one as well. And now you make this a feature, as without it single stepping a multi-watchpoint will not work. And you are here relying on the fact that the client implements the step-over-watchpoint as a simple $z2, $s, $Z2 packet sequence. If the client decides to do anything more complicated, say evaluate an expression to get some state before the update, then your feature will break, as an intermediate $c, will reinstate your hidden watchpoint (in fact, this is maybe possible already, if a conditional breakpoint and a watchpoint are hit simultaneously). And you don't even fix the step-over-instruction-triggering-multiple-watchpoints problem definitively - this can still happen e.g., if you have a watch on 0x1000 and 0x1008, and a single instruction triggers both.(**) This is why I think this change is bad, as it is making an invisible line between the lowest levels of the server and the highest levels of the client, which now have to be kept in sync, otherwise things will unexpectedly break. I don't believe that having the ability to watch more than 4 locations at once is worth it, particularly when it can be easily worked around by the user (just set one big watchpoint instead of multiple small ones). Do you have a specific use case where this would be necessary/useful? I personally have never used more than one watchpoint per debug session in my life, and I expect this to be true for most people (I think the typical use case is "who is corrupting my variable?"), So, I believe that it is better to have support for fewer watchpoints, but have it working well, than the opposite. (*) Now when I tried this out on lldb without this change, it still did not work as expected - for some reason, after hitting and stepping over the watchpoint, lldb decided to to issue $c, and lost control of the inferior. I think that tracking this issue and fixing it would have more impact than adding the multi-watchpoint support (and it will reduce the number of corner cases, where we are wrong rather than increasing it). (**) Another issue whose solving would have more impact than this. https://reviews.llvm.org/D24610 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
