Skip to content

Emit EXT_STMT after each pipe stage, and attach the TMP var that holds the intermediary result #19377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 12, 2025

Conversation

derickr
Copy link
Member

@derickr derickr commented Aug 5, 2025

The following script:

<?php
function testPipes()
{
	$result = "Hello World"
		|> htmlentities(...)
		|> strtolower(...)
		|> str_split(...)
		|> fn($x) => array_map(strtoupper(...), $x)
		|> fn($x) => array_filter($x, fn($v) => $v != 'O');
	return $result;
}

$result = testPipes();
var_dump($result);

Creates the following opcodes for the testPipes() function (without this patch):

line      #* E I O op                               fetch          ext  return  operands
-----------------------------------------------------------------------------------------
    5     0  E >   EXT_STMT                                                     
          1        INIT_FCALL                                                   'htmlentities'
    4     2        SEND_VAL                                                     'Hello+World'
    5     3        DO_ICALL                                             $2      
    4     4        QM_ASSIGN                                            ~3      $2
    6     5        INIT_FCALL                                                   'strtolower'
    4     6        SEND_VAL                                                     ~3
    6     7        DO_ICALL                                             $4      
    4     8        QM_ASSIGN                                            ~5      $4
    7     9        INIT_FCALL                                                   'str_split'
    4    10        SEND_VAL                                                     ~5
    7    11        DO_ICALL                                             $6      
    4    12        QM_ASSIGN                                            ~7      $6
    8    13        DECLARE_LAMBDA_FUNCTION                              ~8      [0]
         14        BIND_LEXICAL                                                 ~8, !1
    9    15        INIT_DYNAMIC_CALL                                            ~8
    4    16        SEND_VAL_EX                                                  ~7
    9    17        DO_FCALL                                          0  $9      
    4    18        ASSIGN                                                       !0, $9
   10    19        EXT_STMT                                                     
         20      > RETURN                                                       !0
   11    21*       EXT_STMT                                                     
         22*     > RETURN                                                       null

If users set a breakpoint on line 4 $result = "Hello World", then Xdebug realises this and moves it to line 5. This is because Xdebug only breaks on EXT_STMT lines, and is clever enough to adjust the breakpoint.

However, if you then would use "step over" then it would immediately go to line 10, as that's where the next EXT_STMT op is created. This means it is not possible to inspect the intermediary values being passed from pipe element to pipe element.

This patch changes the opcode generation to add additional EXT_STMT opcodes in between each state:

line      #* E I O op                               fetch          ext  return  operands
-----------------------------------------------------------------------------------------
    5     0  E >   EXT_STMT                                                     
          1        INIT_FCALL                                                   'htmlentities'
    4     2        SEND_VAL                                                     'Hello+World'
    5     3        DO_ICALL                                             $2      
          4        EXT_STMT                                                     $2
          5        QM_ASSIGN                                            ~3      $2
    6     6        INIT_FCALL                                                   'strtolower'
    5     7        SEND_VAL                                                     ~3
    6     8        DO_ICALL                                             $4      
          9        EXT_STMT                                                     $4
         10        QM_ASSIGN                                            ~5      $4
    7    11        INIT_FCALL                                                   'str_split'
    6    12        SEND_VAL                                                     ~5
    7    13        DO_ICALL                                             $6      
         14        EXT_STMT                                                     $6
         15        QM_ASSIGN                                            ~7      $6
    8    16        DECLARE_LAMBDA_FUNCTION                              ~8      [0]
         17        BIND_LEXICAL                                                 ~8, !1
    9    18        INIT_DYNAMIC_CALL                                            ~8
    7    19        SEND_VAL_EX                                                  ~7
    9    20        DO_FCALL                                          0  $9      
    4    21        EXT_STMT                                                     $9
         22        ASSIGN                                                       !0, $9
   10    23        EXT_STMT                                                     
         24      > RETURN                                                       !0
   11    25*       EXT_STMT                                                     
         26*     > RETURN                                                       null

The EXT_STMT in op 4, 9, 14, and 21 are new, and also have as their op1 value the intermediate pipe value.
This allows Xdebug to break here, and show the contents of this hinted variable.

This however does not work (yet) for the closure wrapped stages as these are handled differently. Ideally they all have a similar DECLARE_LAMBDA_FUNCTION/BIND_LEXICAL/INIT_DYNAMIC_CALL set, but instead PHP creates nested closures instead:

function name:  {closure:testPipes():8}
compiled vars:  !0 = $x, !1 = $v
line      #* E I O op                               fetch          ext  return  operands
-----------------------------------------------------------------------------------------
    8     0  E >   RECV                                                 !0      
          1        BIND_STATIC                                                  !1
          2        EXT_STMT                                                     
          3        INIT_FCALL                                                   'array_map'
          4        INIT_FCALL                                                   'strtoupper'
          5        CALLABLE_CONVERT                                     ~2      
          6        SEND_VAL                                                     ~2
          7        SEND_VAR                                                     !0
          8        DO_ICALL                                             $3      
          9        QM_ASSIGN                                            ~4      $3
    9    10        DECLARE_LAMBDA_FUNCTION                              ~5      [0]
         11        BIND_LEXICAL                                                 ~5, !1
         12        INIT_DYNAMIC_CALL                                            ~5
    8    13        SEND_VAL_EX                                                  ~4
    9    14        DO_FCALL                                          0  $6      
         15        EXT_STMT                                                     $6
         16      > RETURN                                                       $6
         17*       EXT_STMT                                                     
         18*     > RETURN                                                       null

function name:  {closure:{closure:testPipes():8}:9}
compiled vars:  !0 = $x, !1 = $v
line      #* E I O op                               fetch          ext  return  operands
-----------------------------------------------------------------------------------------
          0  E >   RECV                                                 !0      
          1        BIND_STATIC                                                  !1
          2        EXT_STMT                                                     
          3        INIT_FCALL                                                   'array_filter'
          4        SEND_VAR                                                     !0
          5        DECLARE_LAMBDA_FUNCTION                              ~2      [0]
          6        SEND_VAL                                                     ~2
          7        DO_ICALL                                             $3      
          8      > RETURN                                                       $3
          9*       EXT_STMT                                                     
         10*     > RETURN                                                       null

I can currently work around this by always breaking upon the return of a user-land closure, but this is not ideal. Instead, I would like to have an EXT_STMT in the original function just like in between htmlentities/strtolower/str_split).

@derickr derickr requested a review from iluuu1994 August 5, 2025 16:48
@derickr derickr self-assigned this Aug 5, 2025
@derickr derickr marked this pull request as ready for review August 6, 2025 16:31
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, but unless you disable optimizations you'll need some opcache changes.

  • You'll need to add EXT_STMT to keeps_op1_alive().
  • You'll probably need to create a USE for op1 in Zend/Optimizer/zend_ssa.c. Otherwise, if the pipe is optimized away (might happen for CTE-calls) you'll end up with a dead variable as an input.

I'm also confused by this remark:

This however does not work (yet) for the closure wrapped stages as these are handled differently. Ideally they all have a similar DECLARE_LAMBDA_FUNCTION/BIND_LEXICAL/INIT_DYNAMIC_CALL set, but instead PHP creates nested closures instead:

The EXT_STMT call is there after dynamic calls. Does this not suffice? To enter the body of the closure, I presume you'd need step into rather than step over. I don't know exactly how Xdebug handles this, but it seems like that should work.

@derickr derickr requested a review from dstogov as a code owner August 11, 2025 15:30
This example would leak:

    function x() {
        return range(0, 10);
    }
    function test($a, $b) {
        $a |> $b;
    }
    test(42, x(...));

because we were missing a FREE in test(). This requires two fixes, namely one in
zend_do_free() to skip over the EXT_STMT usage, and another in
zend_optimize_block() to avoid removing the FREE opcode by removing the result
def of the declaring opcode.
@derickr
Copy link
Member Author

derickr commented Aug 12, 2025

To keep a record of what the (unrelated) issue is once this patch get merged, and copied from @TimWolla on Slack:

From what I see arrow functions gobble up everything everything to the right of them, which means that:

|> fn($x) => array_map(strtoupper(...), $x)
|> fn($x) => array_filter($x, fn($v) => $v != 'O');

is being parsed as:

    |> fn($x) => (
        array_map(strtoupper(...), $x)
            |> fn($x) => (
                array_filter($x, fn($v) => $v != 'O')
            )
    )
;

@derickr derickr merged commit 4d6dde5 into php:master Aug 12, 2025
9 checks passed
@derickr derickr deleted the pipes-debug-improvements branch August 12, 2025 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants