hv.c: Macro to check an HV as being %ENV hash

[leonerd]

Previous code would invoke an entire mg_find() call just to check if the given HV happens to have PERL_MAGIC_env. This hardcodes knowledge of the PERL_MAGIC_env type in a lot of places that don't really care about it.

[leonerd]

Huh wow, Windows actually isn't happy with this. I wonder why... It builds OK but then fails tests.

[hvds]

Previous code would invoke an entire mg_find() call just to check if the given HV happens to have PERL_MAGIC_env. This is a more expensive lookup than simply comparing the pointer to the HV slot of the *ENV glob. In addition, it hardcodes in knowledge of the PERL_MAGIC_env type in a lot of places that don't really care about it.

Do we have tests for appropriate behaviour of modifications to either the replacement hash or the original hash during the scope of a local *ENV = { ... }? It feels like this would change behaviour under those circumstances, possibly for the worse.

[tonycoz]

backtrace

>	perl541.dll!Perl_hv_common(interpreter * my_perl, hv * hv, sv * keysv, const char * key, unsigned __int64 klen, int flags, int action, sv * val, unsigned long hash) Line 609	C
 	perl541.dll!Perl_hv_common_key_len(interpreter * my_perl, hv * hv, const char * key, long klen_i32, const int action, sv * val, const unsigned long hash) Line 482	C
 	shared.dll!sharedsv_elem_mg_FETCH(interpreter * my_perl, sv * sv, magic * mg) Line 978	C
 	perl541.dll!Perl_mg_get(interpreter * my_perl, sv * sv) Line 202	C
 	perl541.dll!Perl_pp_multideref(interpreter * my_perl) Line 4840	C
 	perl541.dll!Perl_runops_debug(interpreter * my_perl) Line 3000	C
 	perl541.dll!S_run_body(interpreter * my_perl, long oldscope) Line 2883	C
 	perl541.dll!perl_run(interpreter * my_perl) Line 2798	C
 	perl541.dll!RunPerl(int argc, char * * argv, char * * env) Line 202	C++
 	perl.exe!main(int argc, char * * argv, char * * env) Line 40	C

I'll look into it further.

[tonycoz]

PL_envgv is only initialized in perl_parse() , the interpreter used for shared storage by threads::shared is only perl_alloc()ed and perl_construct()ed, it is not perl_parse()ed, so PL_envgv is NULL.

This normally only matters on Windows and VMS, the calls to hv_is_env() are all guarded by ENV_IS_CASELESS (Win32 only) or by DYNAMIC_ENV_FETCH (Win32, VMS, riscos(!))

If I build on linux with -DDYNAMIC_ENV_FETCH I get similar crashes:

tony@venus:.../dist/threads$ gdb --args ../../t/perl -I../.. -MTestInit=U2T t/free.t
...
Reading symbols from ../../t/perl...
(gdb) r
Starting program: /home/tony/dev/perl/git/perl6/t/perl -I../.. -MTestInit=U2T t/free.t
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
1..29
[New Thread 0x7fffea3ff6c0 (LWP 1721802)]

Thread 1 "perl" received signal SIGSEGV, Segmentation fault.
0x00005555555ea182 in Perl_hv_common (my_perl=0x555555ba4cf0, 
    hv=0x555555bec6b0, keysv=keysv@entry=0x0, key=<optimized out>, 
    key@entry=0x555555d60270 "ENDED", klen=<optimized out>, flags=0, 
    action=<optimized out>, val=0x0, hash=3775954083) at hv.c:950
950             && hv_is_env(hv)) {
(gdb) bt
#0  0x00005555555ea182 in Perl_hv_common (my_perl=0x555555ba4cf0, 
    hv=0x555555bec6b0, keysv=keysv@entry=0x0, key=<optimized out>, 
    key@entry=0x555555d60270 "ENDED", klen=<optimized out>, flags=0, 
    action=<optimized out>, val=0x0, hash=3775954083) at hv.c:950
#1  0x00005555555eaf48 in Perl_hv_common_key_len (
    my_perl=my_perl@entry=0x555555ba4cf0, hv=hv@entry=0x555555bec6b0, 
    key=key@entry=0x555555d60270 "ENDED", klen_i32=klen_i32@entry=5, 
    action=action@entry=32, val=val@entry=0x0, hash=0) at hv.c:481
#2  0x00007ffff7c9b035 in sharedsv_elem_mg_FETCH (my_perl=0x555555ba4cf0, 
    sv=0x555555916530, mg=0x555555cea7e0)
    at /home/tony/dev/perl/git/perl6/dist/threads-shared/shared.xs:978
#3  0x00005555555f8ef0 in Perl_mg_get (my_perl=my_perl@entry=0x5555559132a0, 
    sv=sv@entry=0x555555916530) at mg.c:198
#4  0x0000555555655802 in Perl_pp_multideref (my_perl=0x5555559132a0)
    at pp_hot.c:4840
#5  0x00005555556d99a6 in Perl_runops_standard (my_perl=0x5555559132a0)
    at run.c:41
#6  0x00005555555c41a5 in S_run_body (oldscope=<optimized out>, 
    my_perl=<optimized out>) at perl.c:2883
#7  perl_run (my_perl=0x5555559132a0) at perl.c:2798
#8  0x0000555555598592 in main (argc=<optimized out>, argv=<optimized out>, 
    env=<optimized out>) at perlmain.c:127
(gdb) 

Changing:

diff --git a/hv.c b/hv.c
index d59bdf7429..00c1b03592 100644
--- a/hv.c
+++ b/hv.c
@@ -481,7 +481,7 @@ Perl_hv_common_key_len(pTHX_ HV *hv, const char *key, I32 klen_i32,
     return hv_common(hv, NULL, key, klen, flags, action, val, hash);
 }

-#define hv_is_env(hv)  ((hv) == GvHV(PL_envgv))
+#define hv_is_env(hv)  (PL_envgv && (hv) == GvHV(PL_envgv))

 void *
 Perl_hv_common(pTHX_ HV *hv, SV *keysv, const char *key, STRLEN klen,

allowed the threads and Thread-Queue tests to pass locally for me.

[leonerd]

Aha - thanks for the hint @tonycoz - the PL_envgv && ... test helps.

[leonerd]

@hvds That is an interesting point about local *ENV = .... The way that localization works means that the newly-created HV will be the GvHV(PL_envgv) anyway, and will additionally have the PERL_MAGIC_env annotation, so in those terms both the old and the new code will detect the current %ENV hash.

However, an interesting point is that the old hash (which don't forget, still exists as an HV and could be visible) does still have env magic, despite not currently being the HV of the *ENV GV. This might have some visible consequences in corner-case situations. I have just posted a mail to p5p@ about that case. It may be that we care, or we may not. We'll have to see what folks think there.

[haarg]

To me, the current behavior is what I would expect and I would find the new behavior from this PR confusing.

[tonycoz]

I agree with @haarg.

The code being changed is already guarded by SvMAGICAL(), so it only executes for magical HVs anyway, so the performance improvement is going to be tiny.

If hooks ends up getting in with aggregate support I can see the ENV and tie special casing being moved to hooks.

[ap]

I agree with @haarg as well. The current behavior is correct, not incidental.

[leonerd]

Recreated as a simple macro that just moves out the existing code into a convenient function-like macro, so at least there's only one place to change in future rather than multiple.

[Leont]

I agree with the others that the proposed new behavior had too many new sharp edges, but hv_is_env does look like a sensible idea to me.

[book]

After discussing this in the PSC meeting, we all agreed that the current behavior is correct.

[leonerd]

OK - the proposed code doesn't change the behaviour now, just moves the code around that implements it. Can this be merged?

[bulk88]

Recreated as a simple macro that just moves out the existing code into a convenient function-like macro, so at least there's only one place to change in future rather than multiple.

Previous code would invoke an entire mg_find() call just to check if the given HV happens to have PERL_MAGIC_env. This hardcodes knowledge of the PERL_MAGIC_env type in a lot of places that don't really care about it.

I'd rather not stick the mg_find() in yet another static inline. Macro maybe, Static inline no. mg_find() on -O1 MSVC 2022 x64 has a magic property that inside libperl/perl54x.dll all instances of mg_find() are inlined away and MSVC hates to inline ("cost" ranking logic) so when it does inline that is rare to see.

Statement mg_find((const SV *)hv, PERL_MAGIC_env) is approx these bytes after inlining

4D 85 C0                test    r8, r8
75 F1                   jnz     short loc_1800CCF65
41 80 78 12 50          cmp     byte ptr [r8+12h], 50h
74 0B                   jz      short loc_1800CCF77
4C 8B C7                mov     r8, rdi
4D 8B 00                mov     r8, [r8]
4D 85 C0                test    r8, r8
74 52                   jz      short loc_1800CCFCE

A S_hv_is_env(aTHX_ a) wrapper is more likely to cause an inline cost/score fail, on any and all CCs, since every layer at .i stage, is a penalty in the scoring algorithm that all CCs use whether to actually inline something for -O1/-O2 purposes, or "static inline" functions exactly as the developer requested, which means infinite recursion or finding out if ELF supports a 1TB big executable binary.

I'd rather not touch mg_find()/mg_findext() since they are legally extern C functions in perl54x.dll's export table, but by chance they are being MSVC LTOed very perfectly right now, and breathing on them, can cause them to go back to acting like extern dynamic linked symbols they legally are. All the CCs will fundamentally act the same regarding the inline cost analyzer algo since its box packing algorithm. The different CCs vendors just have slightly different time/cost cutoff numbers set in their code between each other.

Also IDK if this PR is touching this part of core but right here

perl5/taint.c

Line 120 in e5ef137

if (!GvHV(PL_envgv) || !(SvRMAGICAL(GvHV(PL_envgv))

GvHV() is just alot of memory derefs, but GvHVn() is even worse since its a vivifying mallocing getter. And writing GvSVn()/GvCVn() 5 times in 1 statement or 1 block/branch is a common bad habit in Perl XS.

GvHVn(PL_envgv) in a for loop

perl5/taint.c

Line 202 in e5ef137

SV * const * const svp = hv_fetch(GvHVn(PL_envgv), *e, strlen(*e), FALSE);