Fixed issue #1016: Support for pause-execution by robberphex · Pull Request #477 · xdebug/xdebug

robberphex · 2019-06-10T10:13:28Z

Fix https://bugs.xdebug.org/view.php?id=1016.

cmdloop support block and non-block mode
call cmdloop pre-statement.
call cmdloop when breakpoint hits.

derickr

I've left a whole bunch of comments, they mostly come down to:

use constants, and refactor into helper functions
add explanations why you've made specific changes (as comments on the PR)
watch your spaces

In addition, please name the PR (and commits) according to precedence:
1a2b402

robberphex · 2019-06-11T01:18:44Z

I've left a whole bunch of comments, they mostly come down to:
* use constants, and refactor into helper functions

* add explanations why you've made specific changes (as comments on the PR)

* watch your spaces
In addition, please name the PR (and commits) according to precedence:
1a2b402

Thanks for your patience. I fixed those problems. Please review it.

derickr · 2019-06-15T16:05:13Z

It's going to take me some time to get to this, as I'll be away from the "office" over the next few weeks. I am intending to get this patch into v3.0, but as it's quite a big and scary change, it's unlikely I will want to put this in the upcoming PHP 7.4 compat release (v2.8).

In any case, right now there is a merge conflict, so if you could rebase (on master) and resolve that, that'd be great.

robberphex · 2019-11-07T16:48:09Z

@derickr
Hi, I've resolved conflict, could you please review this pr?

Pause-execution is really necessary for investigating what's happening for user's program. And this feature-request have existed for almost six years(2013-2019). It's time to get it done.

And if you have any question about this pr, feel free to comment.

robberphex · 2019-12-03T15:42:12Z

@derickr
After your mergings/commits, I have rebase-ed to latest master branch.

This PR is ready to merge, could you please to review this?

robberphex · 2019-12-04T02:34:37Z

I saw the PR, but it's extensive, and I've no idea how to end user test this because there is no IDE yet that supports it. This code hasn't really changed in 15 years so I'm going to be really sure it works before I merge it.

@derickr
Thanks. If this PR is merged, I will send feedback to PHPStorm to support this feature.

And, I forked vscode-php-debug to support this feature: robberphex/vscode-php-debug@3b611f2 (https://marketplace.visualstudio.com/items?itemName=robberphex.php-debug)

I think this PR is work well.
If you have any new idea, please commit here.

derickr · 2019-12-10T10:20:05Z

I'm going to have to spend some proper time understanding this new code, as with a PR you're not only asking me to merge it, but also maintain the new code. I don't know or use vscode, nor would I know how to build it. I think I would like that we walk through your changes in an audio/video call at some point (in the new year).

For now, would you please refrain from rebasing and force-pushing this (unless there is a merge conflict), as it takes CI on AppVeyor about 3 hours to build...which stops me from merging other PRs.

cheers,
Derick

derickr · 2020-01-13T08:36:25Z

 		}
+	}

+	if(xdebug_is_debug_connection_active_for_current_pid()) {


Add a space between if and ( please.

derickr · 2020-01-13T08:43:29Z

 }

-static int xdebug_dbgp_cmdloop(xdebug_con *context, int bail)
+int xdebug_dbgp_cmdloop(xdebug_con *context, int block, int bail TSRMLS_DC)


TSRMLS_DC/TSRMLS_CC and friends no longer mean anything in PHP 7, so please don't re-add them.

derickr · 2020-01-13T08:44:54Z

 }

-static void send_message_ex(xdebug_con *context, xdebug_xml_node *message, int stage)
+static int xdebug_is_paused(int status) {


I would probably rename this function to is_debugger_paused. There is no need to prefix with xdebug_ for static functions, and just is_paused doesn't really mean anything without the context of the debugger.

Please also add a new line before { please.

derickr · 2020-01-13T08:46:21Z

 #define REASON_EXCEPTION  3

+#define XDEBUG_CMDLOOP_BLOCK                         1
+#define XDEBUG_CMDLOOP_NONBLOCK                      0


Can you swap them so they count up like all the others? :-)

derickr · 2020-01-13T08:47:07Z

+$debugClient->sendCommand($conn, 'run', 1);
+
+$debugClient->sendCommand($conn, 'status', 2);
+$debugClient->doRead($conn, (string)2);


Why the (string) cast here instead of just using "2"?

derickr · 2020-01-13T08:49:47Z

 		xdebug_xml_node_dtor(response);

-		xdebug_dbgp_cmdloop(context, 0);
+		xdebug_dbgp_cmdloop(context, XDEBUG_CMDLOOP_BLOCK, 0 TSRMLS_CC);


The TSRMC_CC should go here, and I think it would also make sense to add a constant for the 0/1 here (for example XDEBUG_CMDLOOP_BAIL for the invocation on line 2717)

derickr · 2020-01-13T08:51:13Z

-
-				ret = -1;
-			}
+			ret = command->handler((xdebug_xml_node**) &retval, context, args TSRMLS_CC);


I don't quite understand why you can get away with removing all this code :-) Especially the checks for POST_MORTEM and the command->flags should be checked.

Could this then be the (only) place where it needs checking for "is paused" and whether this command can support a "paused" state as I described near context_get

Also: no TSRMLS_CC, please.

derickr · 2020-01-13T08:56:13Z


+	if (!xdebug_is_paused(XG_DBG(status))) {
+		RETURN_RESULT(XG_DBG(status), XG_DBG(reason), XDEBUG_ERROR_COMMAND_UNAVAILABLE);
+	}


Is there a reason why this command can't be run when the debugger is not paused? I also see this being done for most (if not all) commands, so would it instead not be nicer to add whether a command can be run when not paused into the structure list at https://github.com/xdebug/xdebug/pull/477/files#diff-ec26437df5ade1abeb51e1c4cd21aa2bR219, and then make this check once before the command handler gets called (at https://github.com/xdebug/xdebug/pull/477/files#diff-ec26437df5ade1abeb51e1c4cd21aa2bR2354 )?

derickr · 2020-01-13T08:58:10Z

 	}

-	xdebug_debugger_error_cb(error_filename, error_lineno, type, error_type_str, buffer);
+	xdebug_debugger_error_cb(error_filename, error_lineno, type, error_type_str, buffer, block);


As block here can't really have a different value than XDEBUG_CMDLOOP_NONBLOCK, I think I would prefer there not to be a variable for it.

derickr · 2020-01-13T09:17:20Z

 	if (!CMD_OPTION_SET('f')) {
-		if ((fse = xdebug_get_stack_tail())) {
+		if (!xdebug_is_paused(XG_DBG(status))) {
+			RETURN_RESULT(XG_DBG(status), XG_DBG(reason), XDEBUG_ERROR_COMMAND_UNAVAILABLE);


You mention in your document

"And, we cannot execute … source command without file option."

Why is this exactly?

derickr

A few more questions - sorry :-)

derickr · 2020-01-15T09:41:26Z

-	if (xdebug_is_debug_connection_active_for_current_pid() && XG_DBG(breakpoints_allowed)) {
-		if (!handle_breakpoints(fse, breakpoint_type)) {
-			xdebug_mark_debug_connection_not_active();
+	int block = XDEBUG_CMDLOOP_NONBLOCK;


block here is still defined as a variable, but only used with this value in line 461. No need for this extra variable.

derickr · 2020-01-15T09:42:29Z

-	DBGP_FUNC_ENTRY(breakpoint_update, XDEBUG_DBGP_NONE)
+	DBGP_FUNC_ENTRY(break)
+	DBGP_FUNC_ENTRY(breakpoint_get)
+	DBGP_FUNC_ENTRY(breakpoint_list)


I see that you removed thee NONE/POST_MORTEM flag. Where is this now checked?

derickr · 2020-01-15T09:44:16Z

-			} else {
-				error = xdebug_xml_node_init("error");
-				xdebug_xml_add_attribute_ex(error, "code", xdebug_sprintf("%lu", XDEBUG_ERROR_COMMAND_UNAVAILABLE), 0, 1);
-				ADD_REASON_MESSAGE(XDEBUG_ERROR_COMMAND_UNAVAILABLE);


The whole check for availability seems gone now. Some of this (such as the lastcmd stuft) is now done in every handler, but why do it there when it can be done once here? I am probably misunderstanding so I am hoping you can explain this to me when we chat about it.

derickr · 2020-01-15T09:45:31Z



-static char* xdebug_fd_read_line_delim(int socketfd, fd_buf *context, int type, unsigned char delim, int *length)
+int xdebug_fd_read_line_delim(int socketfd, fd_buf *context, int type, int block, unsigned char delim, char **cmd, int *length)


You're introducing the block argument here, but I can't see it being used in the function.

derickr

I've checked what the protocol says about which commands you can run in the "running" state, it says in the 5th paragraph for 6.2:

If the debugger engine supports asynchronous commands, the IDE may also send commands while the debugger engine is in a 'run' state. These commands should be limited to commands such as the 'break' or 'status' commands for performance reasons, but this protocol does not impose such limitations.

Although other commands are allowed, I think we should stick to just allowing break and status.

robberphex · 2020-02-23T03:38:18Z

@derickr Hi, I've fixed tests. Could you pleas review this again? Thanks a lot.

robberphex · 2020-03-02T09:28:41Z

Since this is merged, so close this PR.

robberphex force-pushed the bug0001016 branch from 6bb9b57 to 666d20d Compare June 10, 2019 10:35

robberphex changed the title ~~Bug0001016~~ Bug0001016: Support for pause-execution Jun 10, 2019

derickr requested changes Jun 10, 2019

View reviewed changes

robberphex force-pushed the bug0001016 branch 6 times, most recently from bd9061f to 5db727b Compare June 10, 2019 16:34

robberphex changed the title ~~Bug0001016: Support for pause-execution~~ issue #1016: Support for pause-execution Jun 10, 2019

robberphex changed the title ~~issue #1016: Support for pause-execution~~ Issue #1016: Support for pause-execution Jun 10, 2019

robberphex force-pushed the bug0001016 branch 7 times, most recently from 7df226d to f654587 Compare June 14, 2019 00:34

robberphex force-pushed the bug0001016 branch from f654587 to 1bb5f7b Compare June 17, 2019 04:21

robberphex mentioned this pull request Jun 17, 2019

Get his Pull requests robberphex/vscode-php-debug#2

Open

robberphex force-pushed the bug0001016 branch from 1bb5f7b to 1ec3f10 Compare June 26, 2019 06:25

robberphex force-pushed the bug0001016 branch from 1ec3f10 to 47db039 Compare September 3, 2019 10:52

robberphex force-pushed the bug0001016 branch 3 times, most recently from 7317300 to 01718da Compare November 7, 2019 16:19

robberphex requested a review from derickr November 7, 2019 16:41

robberphex force-pushed the bug0001016 branch from 01718da to 38d4ed2 Compare November 8, 2019 06:34

robberphex force-pushed the bug0001016 branch from 38d4ed2 to 35c2a44 Compare November 8, 2019 06:37

robberphex force-pushed the bug0001016 branch 4 times, most recently from e97b91b to 932b56c Compare December 1, 2019 15:58

robberphex changed the title ~~Issue #1016: Support for pause-execution~~ Fixed issue #1016: Support for pause-execution Dec 1, 2019

robberphex force-pushed the bug0001016 branch 2 times, most recently from 0e9e65b to 2032214 Compare December 3, 2019 12:07

robberphex force-pushed the bug0001016 branch 4 times, most recently from ea067e2 to dace0ad Compare December 10, 2019 08:55

derickr reviewed Jan 13, 2020

View reviewed changes

robberphex force-pushed the bug0001016 branch 2 times, most recently from 862d70e to ff014fd Compare January 15, 2020 03:50

derickr reviewed Jan 15, 2020

View reviewed changes

robberphex force-pushed the bug0001016 branch from ff014fd to 3e80c6d Compare January 15, 2020 10:22

derickr reviewed Jan 15, 2020

View reviewed changes

robberphex force-pushed the bug0001016 branch from 3e80c6d to f881fac Compare February 15, 2020 01:38

Fixed issue xdebug#1016: Support for pause-execution

f38a21d

robberphex force-pushed the bug0001016 branch from f881fac to f38a21d Compare February 15, 2020 01:54

robberphex requested a review from derickr February 15, 2020 03:48

derickr added a commit that referenced this pull request Feb 28, 2020

Merged pull request #477

957893c

robberphex closed this Mar 2, 2020

zobo mentioned this pull request Feb 2, 2021

Implement pause feature xdebug/vscode-php-debug#489

Closed



		static char* xdebug_fd_read_line_delim(int socketfd, fd_buf context, int type, unsigned char delim, int length)
		int xdebug_fd_read_line_delim(int socketfd, fd_buf context, int type, int block, unsigned char delim, char cmd, int length)

Uh oh!

Uh oh!

Conversation

robberphex commented Jun 10, 2019

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robberphex commented Jun 11, 2019

Uh oh!

derickr commented Jun 15, 2019

Uh oh!

robberphex commented Nov 7, 2019

Uh oh!

robberphex commented Dec 3, 2019

Uh oh!

robberphex commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derickr commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

robberphex commented Feb 23, 2020

Uh oh!

robberphex commented Mar 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robberphex commented Dec 4, 2019 •

edited

Loading