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.