Skip to content

don't call vivifier macros Perl_error_log/Perl_debug_log in a loop #23356

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

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

Conversation

bulk88
Copy link
Contributor

@bulk88 bulk88 commented Jun 5, 2025

  • macros Perl_error_log/Perl_debug_log internally call exported function PerlIO_stderr(my_perl) which may or may not call Newxz() or calloc(). A naive person would think these macros are PL_something my_perl->Isomething vars or they are C image globals vars. These 2 macros are not simple data var derefs, but are in fact vivifing function calls. Probably 20-30 years, before PerlIO was invented, I will guess Perl_error_log/Perl_debug_log where just "#define Perl_error_log 2".
  • Fix this by caching the PerlIO * ptrs to C autos.
  • Why the PerlIO API is NULL ptr, and why these vivifier function calls need to be called everywhere in all of core, at all PL_phases of execution, is beyond the scope of this commit.

  • This set of changes does not require a perldelta entry.

Verified

This commit was signed with the committer’s verified signature.
- macros Perl_error_log/Perl_debug_log internally call exported
  function PerlIO_stderr(my_perl) which may or may not call Newxz() or
  calloc(). A naive person would think these macros are PL_something
  my_perl->Isomething vars or they are C image globals vars. These 2 macros
  are not simple data var derefs, but are in fact vivifing function calls.
  Probably 20-30 years, before PerlIO was invented, I will guess
  Perl_error_log/Perl_debug_log where just "#define  Perl_error_log  2".
- Fix this by caching the PerlIO * ptrs to C autos.
- Why the PerlIO API is NULL ptr, and why these vivifier function calls
  need to be called everywhere in all of core, at all PL_phases of
  execution, is beyond the scope of this commit.
Copy link
Contributor

@khwilliamson khwilliamson left a comment

Choose a reason for hiding this comment

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

It seems to me that a more general solution would be better. Would making these inline functions be sufficient?

Also, I will forcefully object to committing anything that doesn't have a readable commit message, as this one isn't. There need to be paragraphs, and not run-on sentences, and proper English grammar.

This sounds like stream-of-contagiousness writing. It is unfair to code reviewers and people reading the commit in the future to not communicate in clear prose.

@bulk88
Copy link
Contributor Author

bulk88 commented Jul 11, 2025

It seems to me that a more general solution would be better. Would making these inline functions be sufficient?

malloc() and calloc() can not be inlined. Therefore macro token Perl_error_log and Perl_debug_log can not be inlined.

Also, I will forcefully object to committing anything that doesn't have a readable commit message, as this one isn't. There need to be paragraphs, and not run-on sentences, and proper English grammar.

This sounds like stream-of-contagiousness writing. It is unfair to code reviewers and people reading the commit in the future to not communicate in clear prose.

Then go read the code then without looking at the commit message. If someone don't understand the C lang, or intentionally doesn't want to learn about how a certain XS/C Perl API actually works, because its TMI for them and a waste of productivity, I can't help them.


PERL_ARGS_ASSERT_DUMP_SUB_PERL;

cv = isGV_with_GP(gv) ? GvCV(gv) : CV_FROM_REF((SV*)gv);
cv = (is_gv = cBOOL(isGV_with_GP(gv))) ? GvCV(gv) : CV_FROM_REF((SV*)gv);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not fond of this sort of assignment inside assignment, why not split it up in two statements to make things easier to read?

@Leont
Copy link
Contributor

Leont commented Jul 12, 2025

Why the PerlIO API is NULL ptr, and why these vivifier function calls need to be called everywhere in all of core, at all PL_phases of execution, is beyond the scope of this commit.

I can't help wondering if that wouldn't be the better fix than this, but I also don't know why these are lazily vivified.

@thesamesam
Copy link

malloc() and calloc() can not be inlined. Therefore macro token Perl_error_log and Perl_debug_log can not be inlined.

How did you conclude this?

@iabyn
Copy link
Contributor

iabyn commented Jul 13, 2025 via email

@bulk88
Copy link
Contributor Author

bulk88 commented Jul 13, 2025

malloc() and calloc() can not be inlined. Therefore macro token Perl_error_log and Perl_debug_log can not be inlined.

How did you conclude this?

perl5/perlio.c

Line 5273 in 9ef5300

Perl_PerlIO_stderr(pTHX)

  5271: 
  5272: PerlIO *
  5273: Perl_PerlIO_stderr(pTHX)
  5274: {
000007FEAE0FF03C 40 53                push        rbx  
000007FEAE0FF03E 48 83 EC 20          sub         rsp,20h  
  5275:     if (!PL_perlio) {
000007FEAE0FF042 48 83 B9 90 0D 00 00 00 cmp         qword ptr [rcx+0D90h],0  
000007FEAE0FF04A 48 8B D9             mov         rbx,rcx  
000007FEAE0FF04D 75 05                jne         Perl_PerlIO_stderr+18h (07FEAE0FF054h)  
  5276:         PerlIO_stdstreams(aTHX);
000007FEAE0FF04F E8 FC BA FF FF       call        PerlIO_stdstreams (07FEAE0FAB50h)  
  5277:     }
  5278:     return &PL_perlio[3].next;
000007FEAE0FF054 48 8B 83 90 0D 00 00 mov         rax,qword ptr [rbx+0D90h]  
000007FEAE0FF05B 48 83 C0 78          add         rax,78h  
  5279: }
000007FEAE0FF05F 48 83 C4 20          add         rsp,20h  
000007FEAE0FF063 5B                   pop         rbx  
000007FEAE0FF064 C3                   ret  

What is there to inline here? How would changing this func from exported func to static inline remove the call PerlIO_stdstreams (07FEAE0FAB50h) branch? I don't see any problems with what what was abstracted at which function. PerlIO_stderr() abstracts out 2 U32 constants, 1 U8 constant, and 1 branch, from 69 different callsites inside the libperl file, And I am not counting any XS modules either.

How is a C compiler supposed to prove that PerlIO_printf() and every function call inside of it, including all OS syscalls, can not write to the 8 bytes long PerlIOl * PL_perlio data var while PerlIO_printf() was executed? It can't.

Since PerlIO_stdstreams(aTHX); can not be removed or optimized out, or nobody (human) knows if it is safe to "remove it", according to Perl's private API and public API. The next best thing to do is cache the retval ptr of PerlIO_stderr() for limited periods of time, in caller functions, where it is easy to determine, the caller frame is not trying dynamic dangerous or very risky things like call_sv() and perl_clone_using(), which is what this PR does.

  1536: void
  1537: PerlIO_stdstreams(pTHX)
  1538: {
000007FEAE0FAB50 40 53                push        rbx  
000007FEAE0FAB52 48 83 EC 20          sub         rsp,20h  
  1539:     if (!PL_perlio) {
000007FEAE0FAB56 48 83 B9 90 0D 00 00 00 cmp         qword ptr [rcx+0D90h],0  
000007FEAE0FAB5E 48 8B D9             mov         rbx,rcx  
000007FEAE0FAB61 75 44                jne         PerlIO_stdstreams+57h (07FEAE0FABA7h)  
  1540:         PerlIO_init_table(aTHX);
000007FEAE0FAB63 BA 28 00 00 00       mov         edx,28h  
000007FEAE0FAB68 8D 4A 18             lea         ecx,[rdx+18h]  
000007FEAE0FAB6B E8 20 44 FF FF       call        Perl_safesyscalloc (07FEAE0EEF90h)  
  1541:         PerlIO_fdopen(0, "Ir" PERLIO_STDTEXT);
000007FEAE0FAB70 48 8D 15 FD 03 1C 00 lea         rdx,[string "Irt" (07FEAE2BAF74h)]  
  1540:         PerlIO_init_table(aTHX);
000007FEAE0FAB77 48 89 83 90 0D 00 00 mov         qword ptr [rbx+0D90h],rax  
  1541:         PerlIO_fdopen(0, "Ir" PERLIO_STDTEXT);
000007FEAE0FAB7E 33 C9                xor         ecx,ecx  
000007FEAE0FAB80 E8 FF 44 00 00       call        PerlIO_fdopen (07FEAE0FF084h)  
  1542:         PerlIO_fdopen(1, "Iw" PERLIO_STDTEXT);
000007FEAE0FAB85 48 8D 15 EC 03 1C 00 lea         rdx,[string "Iwt" (07FEAE2BAF78h)]  
000007FEAE0FAB8C B9 01 00 00 00       mov         ecx,1  
000007FEAE0FAB91 E8 EE 44 00 00       call        PerlIO_fdopen (07FEAE0FF084h)  
  1543:         PerlIO_fdopen(2, "Iw" PERLIO_STDTEXT);
000007FEAE0FAB96 48 8D 15 DB 03 1C 00 lea         rdx,[string "Iwt" (07FEAE2BAF78h)]  
000007FEAE0FAB9D B9 02 00 00 00       mov         ecx,2  
000007FEAE0FABA2 E8 DD 44 00 00       call        PerlIO_fdopen (07FEAE0FF084h)  
  1544:     }
  1545: }
000007FEAE0FABA7 48 83 C4 20          add         rsp,20h  
000007FEAE0FABAB 5B                   pop         rbx  
000007FEAE0FABAC C3                   ret  

how is the Perl_safesyscalloc() function from libc supposed to get inlined,into libperl and then get const folded/CSEd away?

@bulk88
Copy link
Contributor Author

bulk88 commented Jul 13, 2025

Why the PerlIO API is NULL ptr, and why these vivifier function calls need to be called everywhere in all of core, at all PL_phases of execution, is beyond the scope of this commit.

I can't help wondering if that wouldn't be the better fix than this, but I also don't know why these are lazily vivified.

I wish that too. But I've stared at PerlIO_stderr() for many years. IDK what P5P private code, or CPAN XS code, is allowed to free the PL_perlio malloc block at what phases in the process's lifetime.

Here is the ctor after I set a BP.

>	perl543.dll!Perl_PerlIO_stderr(interpreter * my_perl) Line 5275	C
 	perl543.dll!S_init_predump_symbols(interpreter * my_perl) Line 4725	C
 	perl543.dll!S_parse_body(interpreter * my_perl, char * * env, void(*)(interpreter *) xsinit) Line 2653	C
 	perl543.dll!perl_parse(interpreter * my_perl, void(*)(interpreter *) xsinit, int argc, char * * argv, char * * env) Line 1934	C
 	perl543.dll!RunPerl(int argc, char * * argv, char * * env) Line 209	C++
 	[Inline Frame] perl.exe!invoke_main() Line 78	C++
 	perl.exe!__scrt_common_main_seh() Line 288	C++
 	kernel32.dll!BaseThreadInitThunk�()	Unknown
 	ntdll.dll!RtlUserThreadStart�()	Unknown

perl5/perl.c

Line 4688 in 9ef5300

IoOFP(io) = IoIFP(io) = PerlIO_stderr();

    io = GvIOp(tmpgv);
    IoTYPE(io) = IoTYPE_WRONLY;
    IoOFP(io) = IoIFP(io) = PerlIO_stdout();
    setdefout(tmpgv);
    tmpgv = gv_fetchpvs("stdout", GV_ADD|GV_NOTQUAL, SVt_PV);
    GvMULTI_on(tmpgv);
    GvIOp(tmpgv) = MUTABLE_IO(SvREFCNT_inc_simple(io));

    PL_stderrgv = gv_fetchpvs("STDERR", GV_ADD|GV_NOTQUAL, SVt_PVIO);
    GvMULTI_on(PL_stderrgv);
    io = GvIOp(PL_stderrgv);
    IoTYPE(io) = IoTYPE_WRONLY;
    IoOFP(io) = IoIFP(io) = PerlIO_stderr();
    tmpgv = gv_fetchpvs("stderr", GV_ADD|GV_NOTQUAL, SVt_PV);
    GvMULTI_on(tmpgv);
    GvIOp(tmpgv) = MUTABLE_IO(SvREFCNT_inc_simple(io));

    PL_statname = newSVpvs("");		/* last filename we did stat on */
}

But IDK, to positively say in the future, "the code is wrong and not following Perl 5's C API, there is no bug inside the Interp, the Reporter's code defective and buggy, ticket rejected". if one day, some small piece of code, regardless of its author or origins or maintainer, replaces PL_perlio with NULL and frees PerlIO *s representing FD 0, FD 1 and FD 2, then does a Safefree() on the pointer that used to be inside PL_perlio.

Or uses SAVESTACK() to local()ize PL_perlio. temporarily to NULL ptr.

So IDK enough about PerlIO's design requirements, to just delete that NULL check and the fast path/slow path branches that happen inside Perl_PerlIO_stderr(pTHX) and say defect fixed, and later on say, if some bug report comes in the future, that that bug report is invalid, the interp is correct. Case closed.

Caching the pointer in caller frames, where manual screening shows the callers aren't doing risk/dangerous things like call_sv() and perl_clone_used() is the next best thing.

@khwilliamson
Copy link
Contributor

As a contributor to the Perl5 project, I believe I have the responsibility to not deliberately change anything that isn't beneficial to the project

As a person who has commit rights, I am confident that I have the responsibility to not commit something that I believe isn't beneficial, and I have the responsibility to object to another committer adding something non-beneficial.

Commit messages are important. @bulk88 may not think so. But I do, and so do many more people. It is part of my responsibility to try to allow only good commit messages into blead. A commit message that wastes people's time should not be allowed in.

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.

None yet

5 participants