Skip to content

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

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