Skip to content

eval: ensure debugging saved lines have an IV part #23171

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 4 commits into from
Apr 19, 2025

Conversation

tonycoz
Copy link
Contributor

@tonycoz tonycoz commented Apr 2, 2025

perldebguts documents that the lines stored in @{"_<$filename"}
arrays have a numeric value in addition to the text of the source,
ensure that is true for evals.

Non-zero IV values indicate the lines are breakable (they represent
the address of the COP for that line)


  • This set of changes requires a perldelta entry, and it is included.

@bulk88
Copy link
Contributor

bulk88 commented Apr 5, 2025

        SV * const tmpstr = newSV_type(SVt_PVMG);
        t = (const char *)memchr(s, '\n', send - s);
        if (t)
            t++;
        else
            t = send;

        sv_setpvn_fresh(tmpstr, s, t - s);
        /* not breakable until we compile a COP for it */
        SvIV_set(tmpstr, 0);

Improvement, why is this code using SVt_PVMG and not the smaller PVIV body struct?

git blame shows Larry typed in "PVMG" for unknown reasons

STATIC void
S_save_lines(pTHX_ AV *array, SV *sv)
{
    const char *s = SvPVX_const(sv);
    const char * const send = SvPVX_const(sv) + SvCUR(sv);
    I32 line = 1;

    while (s && s < send) {
	const char *t;
	SV * const tmpstr = newSV(0);

	sv_upgrade(tmpstr, SVt_PVMG);
	t = strchr(s, '\n');
	if (t)
	    t++;
Revision: a0d0e21ea6ea90a22318550944fe6cb09ae10cda
Author: Larry Wall <[email protected]>
Date: 10/17/1994 7:00:00 PM
Message:perl 5.000

tonycoz added 4 commits April 7, 2025 11:06
perldebguts documents that the lines stored in @{"_<$filename"}
arrays have a numeric value in addition to the text of the source,
ensure that is true for evals.

Non-zero IV values indicate the lines are breakable (they represent
the address of the COP for that line)

Fixes Perl#23151
These were saved as PVMG but as bulk88 suggested in
Perl#23171 (comment)
we only need PVIV, since the source lines don't need magic,
aren't blessed and store an integer, not an NV.

So create them as PVIV instead.

If it turns out we do need PVMG instead for some future use, simply
remove the test added here, it's simply to confirm we don't need
PVMG here.
@tonycoz tonycoz force-pushed the 23151-eval-dblines branch from 7d8e243 to 3b40a0a Compare April 7, 2025 01:07
@tonycoz
Copy link
Contributor Author

tonycoz commented Apr 7, 2025

Improvement, why is this code using SVt_PVMG and not the smaller PVIV body struct?

Good point, I've added changing from PVMG to PVIV.

@tonycoz
Copy link
Contributor Author

tonycoz commented Apr 8, 2025

Fixes #23151

else {
sv = *av_fetch(av, 0, 1);
SvUPGRADE(sv, SVt_PVMG);
SvUPGRADE(sv, SVt_PVIV);
}
if (!SvPOK(sv)) SvPVCLEAR(sv);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unroll SvUPGRADE(), add {} to SvPVCLEAR(sv);, add 2 goto skipping if(!SvPOK(sv)) test and jump right into the branch, if was < SVt_PV or we did a newSV_type(SVt_PVIV);. We know the SV* is undef and/or doesn't have POK_on, so we can skip the if(!SvPOK(sv)) conditional jump CPU opcode/opcodes.

@mauke mauke merged commit a707dec into Perl:blead Apr 19, 2025
33 checks passed
mauke pushed a commit that referenced this pull request Apr 19, 2025
These were saved as PVMG but as bulk88 suggested in
#23171 (comment)
we only need PVIV, since the source lines don't need magic,
aren't blessed and store an integer, not an NV.

So create them as PVIV instead.

If it turns out we do need PVMG instead for some future use, simply
remove the test added here, it's simply to confirm we don't need
PVMG here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants