FAQ

[P5P] [perl #78742] eval '__PACKAGE__' works differently with threads than without

Father Chrysostomos via RT
Jun 4, 2012 at 6:25 am

On Sun Oct 31 17:16:43 2010, sprout wrote:
Threaded:
$ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
"__PACKAGE__"'
baz

Unthreaded:
$ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
"__PACKAGE__"'
bar

I think the latter is correct.
I have a partially-written fix (I’m still getting assertion failures)
for this on the smoke-me/copstash branch.

It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
points to an offset in the newly-added PL_stashpad array.

In order to get B to compile, I had to comment out the stashpv method.

How low-level is B supposed to be? Should I replace stashpv with
stashoff? Or should I simply provide B::COP::stash for both threaded
and non-threaded perls, and not expose the fact that it uses an offset
in PL_stashpad?

--

Father Chrysostomos
reply

Search Discussions

13 responses

  • Reini Urban at Jun 5, 2012 at 3:52 pm

    On 06/04/2012 01:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.

    In order to get B to compile, I had to comment out the stashpv method.

    How low-level is B supposed to be? Should I replace stashpv with
    stashoff? Or should I simply provide B::COP::stash for both threaded
    and non-threaded perls, and not expose the fact that it uses an offset
    in PL_stashpad?
    Ad B:
    Since I have to recreate the new PL_stashpad in B::C, Byteloader and
    friends I need B::COP::stashoff exposed.

    Having reliable public readers and writers for B::COP::stash would also
    help, but I don't trust p5p's reliability on such low-level APIs.
    Now the string API is gone and we have hashes again.
    I rather do it by myself, as p5p tends to break the low-level APIs very
    often and refuse to fix it later.

    What about CopSTASH_set, CopSTASH_len and CopSTASH_len_set? They were
    public API's from cop.h.

    Ad stashpad:
    Why a new pad anyway? Why not keep the stash names in PL_strtab
    (sharedpvn) with threads?

    stashoff is faster, indeed, but I assume also more complicate to handle.
    On comparison: The similar handled regex_padav is still broken threaded
    in the compiler, it should really be moved to the REGEXP body.
    --
    Reini
  • Father Chrysostomos via RT at Jun 5, 2012 at 4:16 pm

    On Tue Jun 05 08:52:54 2012, reini@cpanel.net wrote:
    On 06/04/2012 01:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.

    In order to get B to compile, I had to comment out the stashpv method.

    How low-level is B supposed to be? Should I replace stashpv with
    stashoff? Or should I simply provide B::COP::stash for both threaded
    and non-threaded perls, and not expose the fact that it uses an offset
    in PL_stashpad?
    Ad B:
    Since I have to recreate the new PL_stashpad in B::C, Byteloader and
    friends I need B::COP::stashoff exposed.

    Having reliable public readers and writers for B::COP::stash would also
    help, but I don't trust p5p's reliability on such low-level APIs.
    Now the string API is gone and we have hashes again.
    I rather do it by myself, as p5p tends to break the low-level APIs very
    often and refuse to fix it later.
    You are being awfully vague. I’m perfectly willing to accommodate the
    needs of B::C, but I don’t understand what you need.

    Does B::C need access to all of perl’s innards, such that we can’t even
    fix bugs?
    What about CopSTASH_set, CopSTASH_len and CopSTASH_len_set? They were
    public API's from cop.h.
    They were only ‘public’ in the sense that they had to be accessible for
    the CopSTASH macro to work. They were never documented in perlapi.
    Ad stashpad:
    Why a new pad anyway?
    Using the lexical pad results in very fragile code, because PL_curstash
    has to be set before CopSTASH can be used.
    Why not keep the stash names in PL_strtab
    (sharedpvn) with threads?
    Using strings at all causes bugs like the one quoted at the beginning of
    this message.
    stashoff is faster, indeed, but I assume also more complicate to handle.
    On comparison: The similar handled regex_padav is still broken threaded
    in the compiler, it should really be moved to the REGEXP body.
    How is it broken?

    --

    Father Chrysostomos
  • Reini Urban at Jun 5, 2012 at 4:32 pm

    On Tue, Jun 5, 2012 at 11:16 AM, Father Chrysostomos via RT wrote:
    On Tue Jun 05 08:52:54 2012, reini@cpanel.net wrote:
    On 06/04/2012 01:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.

    In order to get B to compile, I had to comment out the stashpv method.

    How low-level is B supposed to be?  Should I replace stashpv with
    stashoff?  Or should I simply provide B::COP::stash for both threaded
    and non-threaded perls, and not expose the fact that it uses an offset
    in PL_stashpad?
    Ad B:
    Since I have to recreate the new PL_stashpad in B::C, Byteloader and
    friends I need B::COP::stashoff exposed.

    Having reliable public readers and writers for B::COP::stash would also
    help, but I don't trust p5p's reliability on such low-level APIs.
    Now the string API is gone and we have hashes again.
    I rather do it by myself, as p5p tends to break the low-level APIs very
    often and refuse to fix it later.
    You are being awfully vague.  I’m perfectly willing to accommodate the
    needs of B::C, but I don’t understand what you need.

    Does B::C need access to all of perl’s innards, such that we can’t even
    fix bugs?
    I never said that. On the contrary I would like to get bug fixed,
    but they are typically warnocked for about two major releases until
    they get fixed.

    I said that I need either a reliable public API to read+write new data,
    or expose the innards so that I can do it by myself.
    There's also no need for a deprecation cycle for those innards.
    What about CopSTASH_set, CopSTASH_len and CopSTASH_len_set? They were
    public API's from cop.h.
    They were only ‘public’ in the sense that they had to be accessible for
    the CopSTASH macro to work.  They were never documented in perlapi.
    I'm fine with that since I don't need them.
    But others with the expectation of deprecation cycles might need them.
    Ad stashpad:
    Why a new pad anyway?
    Using the lexical pad results in very fragile code, because PL_curstash
    has to be set before CopSTASH can be used.
    Why not keep the stash names in PL_strtab
    (sharedpvn) with threads?
    Using strings at all causes bugs like the one quoted at the beginning of
    this message.
    I see, agreed.
    stashoff is faster, indeed, but I assume also more complicate to handle.
    On comparison: The similar handled regex_padav is still broken threaded
    in the compiler, it should really be moved to the REGEXP body.
    How is it broken?
    I don't know yet.
    my_perl->regex_padav gets overwritten in the first call to Perl_call_pv@plt
    which looks like loading a shared library section. I haven't
    disassembled it yet.
    Logically I thought it has something to do with body arena init, but
    it does not look like so.
    It is compiler independent and repro. The rest of my_perl seems to be
    not affected.
    http://code.google.com/p/perl-compiler/issues/detail?id=100

    stashpad seems very alike so I'm sceptical. We'll see.
  • Father Chrysostomos via RT at Jun 5, 2012 at 4:48 pm

    On Tue Jun 05 09:33:15 2012, rurban wrote:
    On Tue, Jun 5, 2012 at 11:16 AM, Father Chrysostomos via RT
    wrote:
    On Tue Jun 05 08:52:54 2012, reini@cpanel.net wrote:
    On 06/04/2012 01:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.

    In order to get B to compile, I had to comment out the stashpv
    method.
    How low-level is B supposed to be?  Should I replace stashpv with
    stashoff?  Or should I simply provide B::COP::stash for both threaded
    and non-threaded perls, and not expose the fact that it uses an
    offset
    in PL_stashpad?
    Ad B:
    Since I have to recreate the new PL_stashpad in B::C, Byteloader and
    friends I need B::COP::stashoff exposed.

    Having reliable public readers and writers for B::COP::stash would also
    help, but I don't trust p5p's reliability on such low-level APIs.
    Now the string API is gone and we have hashes again.
    I rather do it by myself, as p5p tends to break the low-level APIs very
    often and refuse to fix it later.
    You are being awfully vague.  I’m perfectly willing to accommodate the
    needs of B::C, but I don’t understand what you need.

    Does B::C need access to all of perl’s innards, such that we can’t even
    fix bugs?
    I never said that. On the contrary I would like to get bug fixed,
    but they are typically warnocked for about two major releases until
    they get fixed.

    I said that I need either a reliable public API to read+write new data,
    or expose the innards so that I can do it by myself.
    There's also no need for a deprecation cycle for those innards.
    If I add stashoff to B::COP, is that sufficient for your needs?

    It can simply return 0 for non-threaded perls.

    And we can backport to 5.16.1 your patch that added B::COP::stashlen, in
    which case we should add it back to bleadperl, even if it is no longer
    necessary.

    Does that solve the issues?

    --

    Father Chrysostomos
  • Reini Urban at Jun 5, 2012 at 6:07 pm

    On Tue, Jun 5, 2012 at 11:48 AM, Father Chrysostomos via RT wrote:
    On Tue Jun 05 09:33:15 2012, rurban wrote:
    On Tue, Jun 5, 2012 at 11:16 AM, Father Chrysostomos via RT
    wrote:
    On Tue Jun 05 08:52:54 2012, reini@cpanel.net wrote:
    On 06/04/2012 01:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.

    In order to get B to compile, I had to comment out the stashpv
    method.
    How low-level is B supposed to be?  Should I replace stashpv with
    stashoff?  Or should I simply provide B::COP::stash for both threaded
    and non-threaded perls, and not expose the fact that it uses an
    offset
    in PL_stashpad?
    Ad B:
    Since I have to recreate the new PL_stashpad in B::C, Byteloader and
    friends I need B::COP::stashoff exposed.

    Having reliable public readers and writers for B::COP::stash would also
    help, but I don't trust p5p's reliability on such low-level APIs.
    Now the string API is gone and we have hashes again.
    I rather do it by myself, as p5p tends to break the low-level APIs very
    often and refuse to fix it later.
    You are being awfully vague.  I’m perfectly willing to accommodate the
    needs of B::C, but I don’t understand what you need.

    Does B::C need access to all of perl’s innards, such that we can’t even
    fix bugs?
    I never said that. On the contrary I would like to get bug fixed,
    but they are typically warnocked for about two major releases until
    they get fixed.

    I said that I need either a reliable public API to read+write new data,
    or expose the innards so that I can do it by myself.
    There's also no need for a deprecation cycle for those innards.
    If I add stashoff to B::COP, is that sufficient for your needs?
    It can simply return 0 for non-threaded perls.
    Yes.
    I already check threaded, so no need.
    I closely follow cop.h. All the struct cop fields must be exposed.
    cop_stashoff is threaded only.
    BTW: the comment for
    PADOFFSET cop_stashoff; /* package line was compiled in */
    is wrong. PL_stashpad offset it should be.
    And we can backport to 5.16.1 your patch that added B::COP::stashlen, in
    which case we should add it back to bleadperl, even if it is no longer
    necessary.
    If it's not needed I don't need it either.

    I have now added the exception that CopSTASHPV_set threaded needs 3
    args on 5.16.0 only,
    all other versions need the old and new 2 args.
  • Father Chrysostomos via RT at Jun 6, 2012 at 3:36 am

    On Tue Jun 05 11:07:51 2012, rurban wrote:
    On Tue, Jun 5, 2012 at 11:48 AM, Father Chrysostomos via RT
    If I add stashoff to B::COP, is that sufficient for your needs?
    It can simply return 0 for non-threaded perls.
    Yes.
    I have just added it in commit a60c099b83.

    --

    Father Chrysostomos
  • Reini Urban at Jun 7, 2012 at 6:01 pm

    On Mon, Jun 4, 2012 at 1:25 AM, Father Chrysostomos via RT wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.
    The more the think about the stashpad the more I think it's worse then before.

    As analog we have a regex_padav, and each PMOP stores an offset into
    that for threaded perls.
    The improvement would be to get rid of this interpreter-global array
    and store the padav in the
    new REGEXP object itself.

    For stashpad's which were previously stored shared amongst threads in
    strtab, they are
    now stored thread-private in these stashpad's, which means that each
    thread has to copy around a lot of data.
    stash pv's are not likely to be seperate for each thread, the strtab
    is a much better location for that.
    Also has the strtab the hek infrastructure already to store len and
    utf8 flag efficiently, in contrary to the new stashpad.

    I propose to revert the recent stashpad changes and just use the
    cop_stashlen from the hek.
    No need to store this in the COP itself.
  • Father Chrysostomos via RT at Jun 7, 2012 at 7:41 pm

    On Thu Jun 07 11:01:38 2012, rurban wrote:
    On Mon, Jun 4, 2012 at 1:25 AM, Father Chrysostomos via RT
    wrote:
    On Sun Oct 31 17:16:43 2010, sprout wrote:
    Threaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    baz

    Unthreaded:
    $ perl -le' package bar; *foo:: = *bar::; *bar:: = *baz::; print
    eval
    "__PACKAGE__"'
    bar

    I think the latter is correct.
    I have a partially-written fix (I’m still getting assertion failures)
    for this on the smoke-me/copstash branch.

    It replaces cop->cop_stashpv and cop_stashlen with cop_stashoff, that
    points to an offset in the newly-added PL_stashpad array.
    The more the think about the stashpad the more I think it's worse then
    before.

    As analog we have a regex_padav, and each PMOP stores an offset into
    that for threaded perls.
    The improvement would be to get rid of this interpreter-global array
    and store the padav in the
    new REGEXP object itself.
    I don’t know enough about regex_padav to comment on that.
    For stashpad's which were previously stored shared amongst threads in
    strtab,
    Actually, there was a separate PV allocated for *every single* COP -- a
    huge waste of memory.
    they are
    now stored thread-private in these stashpad's, which means that each
    thread has to copy around a lot of data.
    A lot less than before. Bear in mind that multiple COPs compiled into
    the same package share the same stashpad slot. So that’s 4 octets per
    package on a 32-bit architecture.
    stash pv's are not likely to be seperate for each thread, the strtab
    is a much better location for that.
    Also has the strtab the hek infrastructure already to store len and
    utf8 flag efficiently, in contrary to the new stashpad.

    I propose to revert the recent stashpad changes and just use the
    cop_stashlen from the hek.
    No need to store this in the COP itself.
    As, I said before, storing the package name (whether as a separate PV
    owned by the COP, or a shared HEK), rather than a pointer to the stash
    itself, causes bugs.

    What I failed to mention is that PL_stashpad contains, not strings, but
    stash pointers.

    --

    Father Chrysostomos
  • Reini Urban at Jun 7, 2012 at 11:19 pm

    On Thu, Jun 7, 2012 at 2:41 PM, Father Chrysostomos via RT wrote:
    On Thu Jun 07 11:01:38 2012, rurban wrote: ...
    What I failed to mention is that PL_stashpad contains, not strings, but
    stash pointers.
    Oh, that makes a difference! Sorry. I've read over it somehow in the sources.
  • Father Chrysostomos via RT at Jun 8, 2012 at 12:54 am

    On Thu Jun 07 16:20:15 2012, rurban wrote:
    On Thu, Jun 7, 2012 at 2:41 PM, Father Chrysostomos via RT
    wrote:
    On Thu Jun 07 11:01:38 2012, rurban wrote: ...
    What I failed to mention is that PL_stashpad contains, not strings, but
    stash pointers.
    Oh, that makes a difference! Sorry. I've read over it somehow in the
    sources.
    It’s in alloccopstash in op.c:

    PL_stashpad[PL_stashpadix = off] = hv;

    --

    Father Chrysostomos
  • Dave Mitchell at Jun 8, 2012 at 7:14 pm

    On Thu, Jun 07, 2012 at 01:01:09PM -0500, Reini Urban wrote:
    As analog we have a regex_padav, and each PMOP stores an offset into
    that for threaded perls.
    The improvement would be to get rid of this interpreter-global array
    and store the padav in the
    new REGEXP object itself.
    How could that possibly work? The whole point of the regex_padav is to
    allow a (shared-across-threads) PMOP to retrieve a (per-thread) REGEXP
    object notionally attached to it.

    You would need to have the REGEXP to retrieve the regex_padav in order to
    be able to, err,..., retrieve the REGEXP.

    PS my desire, if possible, is to get rid of PL_regex_padav completely and
    just use the standard pad; in the same way that other things like GVs are
    moved from ops into the pad for threaded builds.


    --
    Gravity is just a theory; teach Intelligent Falling in our schools!
    http://www.theonion.com/content/node/39512
  • Reini Urban at Jun 8, 2012 at 11:33 pm

    On Fri, Jun 8, 2012 at 2:14 PM, Dave Mitchell wrote:
    On Thu, Jun 07, 2012 at 01:01:09PM -0500, Reini Urban wrote:
    As analog we have a regex_padav, and each PMOP stores an offset into
    that for threaded perls.
    The improvement would be to get rid of this interpreter-global array
    and store the padav in the
    new REGEXP object itself.
    How could that possibly work? The whole point of the regex_padav is to
    allow a (shared-across-threads) PMOP to retrieve a (per-thread) REGEXP
    object notionally attached to it.

    You would need to have the REGEXP to retrieve the regex_padav in order to
    be able to, err,..., retrieve the REGEXP.
    Yes, you are right. I got a thinko.
    PS my desire, if possible, is to get rid of PL_regex_padav completely and
    just use the standard pad; in the same way that other things like GVs are
    moved from ops into the pad for threaded builds.
    Since the REGEXPs are now standalone SVs, yes.

    @Father:
    I've got one more problem with CopSTASHPV_set:
    alloccopstash needs to be properly exported. I need to call CopSTASHPV_set.
    --
    Reini
  • Father Chrysostomos via RT at Jun 9, 2012 at 3:25 am

    On Fri Jun 08 16:33:49 2012, rurban wrote:
    On Fri, Jun 8, 2012 at 2:14 PM, Dave Mitchell wrote:
    On Thu, Jun 07, 2012 at 01:01:09PM -0500, Reini Urban wrote:
    As analog we have a regex_padav, and each PMOP stores an offset into
    that for threaded perls.
    The improvement would be to get rid of this interpreter-global array
    and store the padav in the
    new REGEXP object itself.
    How could that possibly work? The whole point of the regex_padav is to
    allow a (shared-across-threads) PMOP to retrieve a (per-thread) REGEXP
    object notionally attached to it.

    You would need to have the REGEXP to retrieve the regex_padav in
    order to
    be able to, err,..., retrieve the REGEXP.
    Yes, you are right. I got a thinko.
    PS my desire, if possible, is to get rid of PL_regex_padav
    completely and
    just use the standard pad; in the same way that other things like
    GVs are
    moved from ops into the pad for threaded builds.
    Since the REGEXPs are now standalone SVs, yes.

    @Father:
    I've got one more problem with CopSTASHPV_set:
    alloccopstash needs to be properly exported. I need to call
    CopSTASHPV_set.

    I’ve taken care of that in c0b8aebd32.

    --

    Father Chrysostomos

Related Discussions

Discussion Navigation
viewthread | post