A real breakpoint makes entries in the breakpoint filter for all offsets at the given line. Various flow control commands will also use the breakpoint filter to add and remove temporary "internal breakpoints" required during the flow operation. Ensure that we never remove a breakpoint filter entry if there was already one there due to a breakpoint (or really any other reason).
When the client sends a list of breakpoints over to the server, the logic that applies the breakpoints as soon as a file is loaded is broken. It stops looking at breakpoints as soon as it finds the first one. It should carry on and look at all of them.
Because stronger types are better types, and this will make
future refactoring easier. I considered trying to purge the Opcode
type from the codebase too but that would be a much bigger project.
Currently a breakpoint specified as relativefilepath:lineno will match only if the relative file path is a simple file name or if sandbox root + relative file path is an exact match. This is now generalized so that matching occurs if the relative file path is a suffix (path) of the absolute path of the execution location.
The debugger client now accepts feedback from the the debugger server about whether a breakpoint can be hit, absent further loads of files, classes or functions.
Cleanup a lot of hangs with either the debugger client or server in a variety of error conditions, mostly related to communication errors or the client or server exiting unexpectedly. One of the biggest fixes is that all cases where the client was left in a state where Ctrl-C wouldn't work have been fixed.
Remove lots of little snippets of dead code. If you see a function (or small set of functions/fields) deleted then it was actually dead.
I debated whether to keep throwing DebuggerClientExitException on the server, and I decided to keep it. I think it's reasonable that if you've got the server stopped and you quit the debugger that the request gets terminated rather than continuing to run.
I also considered a big change to the way Ctrl-C works, but ended up staying with what was there with just a bit of cleanup. We need to guard against people banging on Ctrl-C, which is a reasonable behavior, and I think it feels pretty reasonable with the updated message.
Finally, added many comments about how this stuff works.
We had two similar-but-different functions for getting a notification from the VM about an exception. Cleaned that up by using the proper one for a thrown exception where appropriate, and moving the old one into a hook (like the other VM->debugger hooks) specifically for error messages.
There were multiple issues with flow control when exceptions occur. Fixed these by ditching the reliance on the exception thrown interrupt and introduce an exception handler interrupt, which indicates control is about to pass to a catch clause. This gives us much better insight into how execution is flowing and how we might need to adjust an in-flight stepping operation.
Add reasonable behavior for stepping within continuations (generators). Stepping over a yield now does what one would expect. When the generator is driven from a C++ extension like ASIO, the next logical execution point is after the yield statement, and that's where we'll stop now. When driven from PHP, say in a loop calling send(), the next execution point is in fact the call site of send(), so we go there. Stepping off the end of the generator function, goes to the caller of send(), or the caller of the C++ extension. Stepping _out_ of a generator driven by a C++ extension ensures that we go to the caller and not back into the generator again. The logic for both cases is exactly the same. The difference comes from the fact that we don't actually debug C++ extensions.
This also fixes a long-standing problem where breakpoints would interfere with control flow cmds on the same source line. This caused funny behavior, like taking multiple steps to get off of a breakpoint.
We had a lot of odd behavior with both Next and Out. Previously the debugger would interpret the world until it saw that it was no longer on a specific line, or at a specific stack depth. I changed it recently to let the program run normally, and use an internal breakpoint to control step outs. Next's become like a step out temporarily when you descend into a function and need to get back out to the original line, so some bugs with out showed as bugs with Next, too.
Specifically, any time more PHP code was executed as a "side-effect" of a non-fcall instruction, step out would get lost. In these cases the stack trace gives us the offset of the instruction causing the PHP to run, not the instruction that control will return to as is the case with a Fcall. A breakpoint set there would get missed. This changes the step out logic to recognize such cases (via the fact that a nested VM state was pushed to execute the code) and step out more intelligently. We look at the instruction in question, and determine where execution may go, which might be multiple places. I also made a small change to ensure that we don't stop in generated functions when stepping out, which cleans up the iterator experience quite a bit, and sets us up for proper generator stepping, which will come next.
runtime/eval is a relic of a bygone era. As long as we're cleaning up
our directory structure, let's move FileRepository (the only remaining
thing in runtime/eval/runtime) to where it makes sense.
runtime/eval still contains the debugger, which would probably make more
sense as runtime/debugger, but I don't want to throw a wrench in the
works for @mikemag and @hermanv unnecessarily.
This adds a new trace macro to allow tracing to the ring buffer in release builds. It also performs normal tracing, too, like TRACE(n, ...). Converted a number of trace/log messages in the debugger to the new macro so we have this data when we get a core file. I've converted things that we've found useful lately, but this will be adjusted over time quite a lot as we discover new things that help us find problems more quickly, or find messages that turn our to be useless or spammy.
I was learning from @jdelong and he said that you should use
double quotes for local includes and angle brackets for library
includes. I asked why our code was the way it was, and he said he wanted
to clean it up. I beat him to it :)
Conflicts:
hphp/runtime/base/server/admin_request_handler.cpp
hphp/runtime/vm/named_entity.h
Added comments to every method in cmd_breakpoint.cpp. Also renamed some methods to make their intent more obvious. Moved a few implementation methods from the public to protected space.
This improves both Next and Out to avoid interpreting and stepping everything between when they start and finish. Out now lets the program run free until a pseudo-breakpoint at the return site it hit. Next continues to single-step the source line being stepped over, but now lets the program run free under calls made from that line.
The logic in Next regarding "calls made from that line" is extremely generic. We don't look at, say, call opcodes and decide to do something special. Rather, when we find we're off the original source line and a frame deeper we setup a "step out" operation much like the Out command then let the program run free. When we reach our return point, we continue stepping like normal. This accounts for not just calls, but iterators, and anything else that causes more PHP to run under the original source line.
This change moves the flow control logic down in to the respective cmds: Next, Step, Out, Continue. These cmds get a crack at executing at various points in the interrupt/command processing path. These cmds now own setting up the last location filter, whether they need VM interrupts, and whether they're done or not.
The separation between these classes was a vestige of days gone by. Combined them. I ran into issue with having the proxy split up in particular when working on stepping, so doing this now as a separate diff to keep things cleaner.
Add a lot of comments to the debugger based on my current understanding of it. These may change in the future as we learn more, but they're helpful right now.
Also moved a few small things around in the code to clarify their purpose or scope. I.e., making a few things private, renaming a few functions, etc. No real logic changes, though. Also minor dead code removal. Also a few lint errors.
This change is mostly for FB internal organizational reasons.
Building is not effected beyond the fact that the target now
lands in hphp/hhvm/hhvm rather than src/hhvm/hhvm.
Mike Magruder