Skip to content
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.

Sign up for GitHub

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

Jump to bottom

signatures: generate only one debug breakable line for signatures #22610

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from
Open

signatures: generate only one debug breakable line for signatures #22610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions lib/perl5db.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3662,6 +3662,35 @@ EOS
$wrapper->contents_like(qr/print "2\\n"/, "break immediately after defining problem");
}

{
my $wrapper = DebugWrap->new(
{
cmds =>
[
"s",
"s",
"s",
"s",
"s",
"s",
"s",
"q",
],
prog => \<<'EOS',
use v5.40.0;
sub four ($x1, $x2, $x3, $x4) { # start sub four
print $x1, $x2, $x3, $x4;
}
four(1,2,3,4);
EOS
Copy link
Contributor

Choose a reason for hiding this comment

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

If I save this heredoc as a separate program, what differences should I be able to observe in the behavior of the debugger between, say, perl-5.40.0 and blead+this patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you single-step (s) in the debugger, you should see a lot of invisible statements being executed on the sub four($x1, $x2, $x3, $x4) { line; i.e. you have to repeat s a lot to make the debugger advance into the subroutine.

I haven't tried it, but with this patch I'd assume the debugger only stops once and immediately proceeds to the print in the subroutine body when you hit s.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you single-step (s) in the debugger, you should see a lot of invisible statements being executed on the sub four($x1, $x2, $x3, $x4) { line; i.e. you have to repeat s a lot to make the debugger advance into the subroutine.

I haven't tried it, but with this patch I'd assume the debugger only stops once and immediately proceeds to the print in the subroutine body when you hit s.

Thanks. In perl-5.40.0, It took me 8 s steps before the print statement executed. In the p.r., it only took 3 s steps. LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Step through the code with "s", pre-patch this steps 6 times on the sub prolog, with patch it steps once

}
);
my $content = $wrapper->_contents;
my $count = () = $content =~ /start sub four/g;
is($count, 1, "expect to step on sub four entry once")
or diag $content;
}

done_testing();

END {
Expand Down
3 changes: 2 additions & 1 deletion op.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8896,7 +8896,8 @@ Perl_newSTATEOP(pTHX_ I32 flags, char *label, OP *o)
flags &= ~SVf_UTF8;

NewOp(1101, cop, 1, COP);
if (PERLDB_LINE && CopLINE(PL_curcop) && PL_curstash != PL_debstash) {
if (PERLDB_LINE && CopLINE(PL_curcop) && PL_curstash != PL_debstash &&
!(flags & OPf_FORCE_NEXTSTATE)) {
OpTYPE_set(cop, OP_DBSTATE);
}
else {
Expand Down
9 changes: 9 additions & 0 deletions op.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,15 @@ Deprecated. Use C<GIMME_V> instead.
op_convert_list to set op_folded. */
#define OPf_FOLDED (1<<16)

/* In debugging mode newSTATEOP() normally makes an OP_DBSTATE, set
this to force creation of an OP_NEXTSTATE (which isn't
steppable/breakable) instead.

This is useful if you need to intro_my() but not make a breakable
op.
*/
#define OPf_FORCE_NEXTSTATE (1 << 17)

/* old names; don't use in new code, but don't break them, either */
#define OPf_LIST OPf_WANT_LIST
#define OPf_KNOW OPf_WANT
Expand Down
8 changes: 4 additions & 4 deletions perly.act
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion perly.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion perly.tab
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions perly.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ sigslurpelem: sigslurpsigil sigvarname sigdefault/* def only to catch errors */
yyerror("A slurpy parameter may not have "
"a default value");

$$ = var ? newSTATEOP(0, NULL, var) : NULL;
$$ = var ? newSTATEOP(OPf_FORCE_NEXTSTATE, NULL, var) : NULL;
}
;

Expand Down Expand Up @@ -913,7 +913,7 @@ sigscalarelem:
"follows optional parameter");
}

$$ = var ? newSTATEOP(0, NULL, var) : NULL;
$$ = var ? newSTATEOP(OPf_FORCE_NEXTSTATE, NULL, var) : NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that in the debugger it now won't be single-steppable even if there is a defaulting expression. It's hard to predict what users might want here, but maybe it's best to keep the DBSTATE if there is a defop? So maybe

defop ? 0 : OPf_FORCE_NEXTSTATE

for the flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would get annoying in the same way as in #22602 if you had more than a couple of default valued arguments.

I think the ideal experience would be to make the debugger display each argument at the signature step points as you step through the signature.

Maybe these breakpoints could be optional, but it would need to be controllable at runtime (ie. need a flag on the dbstate), and I expect discoverability of such an option would be bad.

I'm not sure of a good approach for the debugger to get the variable set by the argelem.

}
;

Expand Down Expand Up @@ -985,7 +985,7 @@ subsigguts:
(UNOP_AUX_item *)aux);
sigops = op_prepend_elem(OP_LINESEQ, check, sigops);
sigops = op_prepend_elem(OP_LINESEQ,
newSTATEOP(0, NULL, NULL),
newSTATEOP(OPf_FORCE_NEXTSTATE, NULL, NULL),
sigops);
/* a nextstate at the end handles context
* correctly for an empty sub body */
Expand Down
Loading