newCONSTSUB_flags() fiddles with PL_curcop

[Nicholas Clark]

newCONSTSUB_flags() starts with this code:

if (IN_PERL_RUNTIME) {
/* at runtime, it's not safe to manipulate PL_curcop: it may be
* an op shared between threads. Use a non-shared COP for our
* dirty work */
SAVEVPTR(PL_curcop);
SAVECOMPILEWARNINGS();
PL_compiling.cop_warnings = DUP_WARNINGS(PL_curcop->cop_warnings);
PL_curcop = &PL_compiling;
}
SAVECOPLINE(PL_curcop);
CopLINE_set(PL_curcop, PL_parser ? PL_parser->copline : NOLINE);


This all feels hacky. Why does it need to be set...


So, I think that it contributes to this. Sorry it's not clear, but note that
some of the line numbers in the redefined warnings *differ* depending on
whether it's in a BEGIN block:

$ ./perl -Ilib -we 'eval qq{ {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}; eval qq{{\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}'
Subroutine DynaLoader::dl_load_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_unload_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_find_symbol redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_undef_symbols redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_install_xsub redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_error redefined at (eval 2) line 4.
$ ./perl -Ilib -we 'eval qq{ {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}; eval qq{BEGIN {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}'
Subroutine DynaLoader::dl_load_file redefined at (eval 2) line 1.
Subroutine DynaLoader::dl_unload_file redefined at (eval 2) line 1.
Subroutine DynaLoader::dl_find_symbol redefined at (eval 2) line 1.
Subroutine DynaLoader::dl_undef_symbols redefined at (eval 2) line 1.
Subroutine DynaLoader::dl_install_xsub redefined at (eval 2) line 1.
Subroutine DynaLoader::dl_error redefined at (eval 2) line 1.


"line 4" vs "line 1" despite the fact that the only difference between
the two overlong 1-liners is the six character string "BEGIN "


So, I'd like to take the above code out. If I remove it, the build fails:

GLOB_CSH is not a valid File::Glob macro at ../lib/File/Glob.pm line 66

The problem comes down to this bit of Perl_gv_fetchpvn_flags():

if (!stash) {
no_stash:
if (len && isIDFIRST_lazy(name)) {

...

if (global)
stash = PL_defstash;
else if (IN_PERL_COMPILETIME) {
stash = PL_curstash;
...
}
else
stash = CopSTASH(PL_curcop);


$expletive

The behaviour of the function Perl_gv_fetchpvn_flags() differs between
"Compile Time" and "Run Time". That's really, um, crap.

Specifically, the way that newCONSTSUB_flags() actually controls how
gv_fetchpvn_flags() gets a stash to default from is by

* setting PL_curstash to the stash to use
* assigning &PL_compiling to PL_curcop to make gv_fetchpvn_flags() notice this.


This is not sane.

I think that the right way to fix this is to have a way to pass the default
stash into Perl_gv_fetchpvn_flags(). Or even split it into two - one half
that locates the stash to use, and the other half that takes a stash, and
does the initialisation. So something like this:

@@ -1521,7 +1529,9 @@ Perl_gv_fetchpvn_flags(pTHX_ const char *nambeg, STRLEN fu

if (!stash) {
no_stash:
- if (len && isIDFIRST_lazy(name)) {
+ if (def_stash) {
+ stash = def_stash;
+ } else if (len && isIDFIRST_lazy(name)) {
bool global = FALSE;

switch (len) {


However, it's not at all clear to me how to cleanly get that def_stash into
there. A very ugly proof of concept is attached. With that, all tests pass,
and my convoluted example becomes consistent:

$ ./perl -Ilib -we 'eval qq{ {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}; eval qq{BEGIN {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}'
Subroutine DynaLoader::dl_load_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_unload_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_find_symbol redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_undef_symbols redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_install_xsub redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_error redefined at (eval 2) line 4.
Subroutine DynaLoader::CLONE redefined at (eval 2) line 4.
$ ./perl -Ilib -we 'eval qq{ {\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}; eval qq{{\n\n\nDynaLoader::boot_DynaLoader("DynaLoader")}}'
Subroutine DynaLoader::dl_load_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_unload_file redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_find_symbol redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_undef_symbols redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_install_xsub redefined at (eval 2) line 4.
Subroutine DynaLoader::dl_error redefined at (eval 2) line 4.
Subroutine DynaLoader::CLONE redefined at (eval 2) line 4.


But it's clearly a hack. I'm not sure of the right way to move forwards.

Nicholas Clark

[Leon Timmermans]

On Fri, Aug 31, 2012 at 11:11 PM, Nicholas Clark <ni...@ccl4.org> wrote:
> The behaviour of the function Perl_gv_fetchpvn_flags() differs between
> "Compile Time" and "Run Time". That's really, um, crap.

That function is one of those gems that keeps surprising us.

> Specifically, the way that newCONSTSUB_flags() actually controls how
> gv_fetchpvn_flags() gets a stash to default from is by
>
> * setting PL_curstash to the stash to use
> * assigning &PL_compiling to PL_curcop to make gv_fetchpvn_flags() notice this.
>
>
> This is not sane.

Indeed, it is not.

> I think that the right way to fix this is to have a way to pass the default
> stash into Perl_gv_fetchpvn_flags(). Or even split it into two - one half
> that locates the stash to use, and the other half that takes a stash, and
> does the initialisation. So something like this:

I think that spliting up gv_fetchpvn_flags is generally a very good
idea, even without this issue.

Leon

[Jesse Luehrs]

Very much so. I've started this at least two or three times at this
point, but gotten bogged down in all of the suprising edge cases every
time. gv_fetchpvn_flags really needs to be at least three different
functions - one for finding the appropriate stash to use for the
variable being looked up, one for creating the actual gv (this is
basically already split out into gv_init), and one for applying the
appropriate magic to a glob, given its name and stash.

Actually doing this split, however, is annoyingly tricky, due to the
amount of interweaved logic strewn throughout various parts of the
function (short-circuit "return NULL"s in the "find a stash" portion and
the many different meanings of addmg being two of the more irritating
bits). Maybe I'll give it another shot one of these days, but I very
much wouldn't mind somebody else doing it either.

This function is one of the main blockers to anonymous stashes being
usable throughout the core - they're already mostly usable everywhere,
because everything else that uses stashes uses the HV* directly, but
there's no real way to add things to them without manually constructing
globs, since the only interface we have requires the fully qualified
variable name. If this was split up such that I could just call gv_init,
gv_magicalize (or whatever some new function is called), and hv_store,
and have things just work properly, that would be pretty great.

-doy