Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Perl: porters

[BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f

 

 

Perl porters RSS feed   Index | Next | Previous | View Threaded


jdhedden at cpan

May 24, 2010, 3:49 PM

Post #1 of 5 (200 views)
Permalink
[BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f

This change breaks my module Object::InsideOut. I have determined the
cause to be that using 'weaken' in combination with threads and objects
is broken. The attached self-contained script replicates the bug.


On Thu, May 20, 2010 at 16:56, Nicholas Clark <nick [at] ccl4> wrote:
> In perl.git, the branch blead has been updated
>
> <http://perl5.git.perl.org/perl.git/commitdiff/39f3f7f442aed93239540238d19a15f6020da747?hp=e77da3b2183d0f9c5ea81dc5780d9a480fc4fa88>
>
> - Log -----------------------------------------------------------------
> commit 39f3f7f442aed93239540238d19a15f6020da747
> Author: Nicholas Clark <nick [at] ccl4>
> Date:   Tue Feb 23 20:35:29 2010 +0000
>
>    PL_endav can be NULL, so in S_ithread_create() no need to set it to newAV().
>
> M       dist/threads/threads.xs
>
> commit b1faab355535118c9fb99fcc343501012923ece2
> Author: Nicholas Clark <nick [at] ccl4>
> Date:   Sat Feb 13 09:05:18 2010 +0000
>
>    Remove redundant hv_exists() calls from ithread_create()'s spec parser.
>
>    hv_fetch(..., 0) won't create the element if it doesn't exist, returning a NULL
>    pointer, so hv_exists() and hv_fetch() is doing two hash lookups where one would
>    suffice.
>
>    On this machine, reduces the object code by 3K, about 7%. Everyone's a winner.
>
> M       dist/threads/threads.xs
>
> commit 4cf5eae5e58faebb6ae9e68e493cd85343f7a76c
> Author: Nicholas Clark <nick [at] ccl4>
> Date:   Sat Feb 13 08:31:55 2010 +0000
>
>    Change S_ithread_create() params from a single AV* to a pair of SV** pointers.
>
>    This saves creating, duplicating and freeing and AV, which is only ever used for
>    an internal calling convention.
>
> M       dist/threads/threads.xs
>
> commit 78b7eff23b1757f7fccef902e7e706080a997e2c
> Author: Nicholas Clark <nick [at] ccl4>
> Date:   Sat Feb 13 08:01:45 2010 +0000
>
>    In threads.xs, convert thread->params from RV to AV.
>
>    Pass around and store the array directly, rather than creating, holding and
>    dereferencing a reference to it.
>
> M       dist/threads/threads.xs
> -----------------------------------------------------------------------
Attachments: bug_thr_weak.pl (1.16 KB)


nick at ccl4

May 25, 2010, 2:38 PM

Post #2 of 5 (188 views)
Permalink
Re: [BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f [In reply to]

On Mon, May 24, 2010 at 06:49:15PM -0400, Jerry D. Hedden wrote:
> This change breaks my module Object::InsideOut. I have determined the
> cause to be that using 'weaken' in combination with threads and objects
> is broken. The attached self-contained script replicates the bug.


> > commit 39f3f7f442aed93239540238d19a15f6020da747

> > commit b1faab355535118c9fb99fcc343501012923ece2

> > commit 4cf5eae5e58faebb6ae9e68e493cd85343f7a76c

> > commit 78b7eff23b1757f7fccef902e7e706080a997e2c

Thanks for the short self contained test case. I bisect with this:

#!/bin/sh
git clean -dxf
# If you can use ccache, add -Dcc=ccache\ gcc -Dld=gcc to the Configure line
# if Encode is not needed for the test, you can speed up the bisect by
# excluding it from the runs with -Dnoextensions=Encode
sh Configure -des -Dusedevel -Dusethreads -Doptimize="-g" -Dcc=ccache\ gcc -Dld=gcc -Dnoextensions=Encode
test -f config.sh || exit 125
# Correct makefile for newer GNU gcc
perl -ni -we 'print unless /<(?:built-in|command)/' makefile x2p/makefile
# if you just need miniperl, replace test_prep with miniperl
make -j4 test_prep
[ -x ./perl ] || exit 125
./perl -Ilib ../Run/bug_thr_weak.pl
ret=$?
[ $ret -gt 127 ] && ret=127
git clean -dxf
exit $ret

and it was actually this commit

commit adf8f095c5881bcedf07b8e41072f8125e00b5a6
Author: Nicholas Clark <nick [at] ccl4>
Date: Fri Feb 26 09:18:44 2010 +0000

Set PADSTALE on all lexicals at the end of sub creation.

The PADSTALEness of lexicals between the 0th and 1st call to a subroutine is now
consistent with the state between the nth and (n + 1)th call.

This permits a work around in Perl_padlist_dup() to avoid leaking active pad
data into a new thread, whilst still correctly bodging the external references
needed by the current ?{} implementation. Fix that, and this can be removed.



The aim of this (and the preceding refactoring) was to reduce the amount of
stuff in pads needlessly cloned as part of thread creation (but not pseudo
forking). Specifically, the cloning code used to copy all the scalars in pads
that had been entered in the parent thread, despite the fact that a child
thread isn't going to "return" to those subroutines.

I think it had the side effect of no longer cloning the pad that held a
(non-weak) reference to the value in question, which meant that during the
CLONE call, that value had a reference count of 0, and the new code in
e42956688f2e0df9 to better handle things with a reference count of 0 doesn't
do its fixups until after the calls to CLONE.

Hence if code in CLONE takes a reference to that value in CLONE, as was, the
value's reference count went from 0 to 1 and back to 0, at which point all
sorts of cleanup activity triggered, including that SV being put onto the
free list. (And then the fixup code would fix things up, so the SV would also
be referenced from elsewhere. Bad).

Fixed in

commit 04518cc3f43b495f85caf2ec89c8b06540a60f8c
Author: Nicholas Clark <nick [at] ccl4>
Date: Tue May 25 17:23:10 2010 +0100

Fix CLONE/weakref bug revealed by adf8f095c5881bce.

The AV unreferenced in the clone_params needs to be reference counted, rather
than not referenced counted, because the fixup to ensure that all otherwise
0-reference count scalars have a reference (on the temps stack) happens after
CLONE is run, and CLONE can run Perl code that causes their reference counts
to increase from then return to zero, which prematurely triggers sv_free().


Also, it seems that this commit:

commit bb1bc619ea68d9703fbd3fe5bc65ae000f90151f
Author: Father Chrysostomos (via RT) <perlbug-followup [at] perl>
Date: Sun Jan 17 14:32:24 2010 -0800

Deref ops ignore get-magic when SvROK(sv)

This is just like bug 68192, except in this case it’s a different set
of operators that have had this problem for much longer.

causes the TODO test at the end of dist/threads-shared/t/object.t to start
passing:

ok 24 - Main: Object ID 1
ok 25 - Thread: Object ID 1
ok 26 - Thread: Changed object ID 10
ok 27 - Thread: New object ID 2
ok 28 - Main: New object ID 2 # TODO - should be 2

Nicholas Clark


jdhedden at cpan

May 26, 2010, 11:25 AM

Post #3 of 5 (193 views)
Permalink
Re: [BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f [In reply to]

On Tue, May 25, 2010 at 17:38, Nicholas Clark <nick [at] ccl4> wrote:
> Also, it seems that this commit:
>
> commit bb1bc619ea68d9703fbd3fe5bc65ae000f90151f
> Author: Father Chrysostomos (via RT) <perlbug-followup [at] perl>
> Date:   Sun Jan 17 14:32:24 2010 -0800
>
>    Deref ops ignore get-magic when SvROK(sv)
>
>    This is just like bug 68192, except in this case it’s a different set
>    of operators that have had this problem for much longer.
>
> causes the TODO test at the end of dist/threads-shared/t/object.t to start
> passing:
>
> ok 24 - Main: Object ID 1
> ok 25 - Thread: Object ID 1
> ok 26 - Thread: Changed object ID 10
> ok 27 - Thread: New object ID 2
> ok 28 - Main: New object ID 2  # TODO - should be 2

Interesting. Since the change that fixed this test did not involve
shared.xs code should this test be moved to t/op/???
Or should I just make it conditionally TODO based on perl version?


nick at ccl4

May 26, 2010, 11:41 AM

Post #4 of 5 (204 views)
Permalink
Re: [BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f [In reply to]

On Wed, May 26, 2010 at 02:25:23PM -0400, Jerry D. Hedden wrote:
> On Tue, May 25, 2010 at 17:38, Nicholas Clark <nick [at] ccl4> wrote:
> > Also, it seems that this commit:
> >
> > commit bb1bc619ea68d9703fbd3fe5bc65ae000f90151f
> > Author: Father Chrysostomos (via RT) <perlbug-followup [at] perl>
> > Date:   Sun Jan 17 14:32:24 2010 -0800
> >
> >    Deref ops ignore get-magic when SvROK(sv)
> >
> >    This is just like bug 68192, except in this case it?s a different set
> >    of operators that have had this problem for much longer.
> >
> > causes the TODO test at the end of dist/threads-shared/t/object.t to start
> > passing:
> >
> > ok 24 - Main: Object ID 1
> > ok 25 - Thread: Object ID 1
> > ok 26 - Thread: Changed object ID 10
> > ok 27 - Thread: New object ID 2
> > ok 28 - Main: New object ID 2  # TODO - should be 2
>
> Interesting. Since the change that fixed this test did not involve
> shared.xs code should this test be moved to t/op/???
> Or should I just make it conditionally TODO based on perl version?

I don't know. I had assumed make it conditionally TODO, but I don't know *why*
it starts passing. (Other than the description implies that magic will now be
called where previously it incorrectly wasn't, and shared scalars
implemented via magic)

Nicholas Clark


jdhedden at cpan

May 26, 2010, 11:48 AM

Post #5 of 5 (188 views)
Permalink
Re: [BUG] Re: [perl.git] branch blead, updated. v5.13.1-8-g39f3f7f [In reply to]

On Wed, May 26, 2010 at 14:41, Nicholas Clark <nick [at] ccl4> wrote:
> On Wed, May 26, 2010 at 02:25:23PM -0400, Jerry D. Hedden wrote:
>> On Tue, May 25, 2010 at 17:38, Nicholas Clark <nick [at] ccl4> wrote:
>> > Also, it seems that this commit:
>> >
>> > commit bb1bc619ea68d9703fbd3fe5bc65ae000f90151f
>> > Author: Father Chrysostomos (via RT) <perlbug-followup [at] perl>
>> > Date:   Sun Jan 17 14:32:24 2010 -0800
>> >
>> >    Deref ops ignore get-magic when SvROK(sv)
>> >
>> >    This is just like bug 68192, except in this case it?s a different set
>> >    of operators that have had this problem for much longer.
>> >
>> > causes the TODO test at the end of dist/threads-shared/t/object.t to start
>> > passing:
>> >
>> > ok 24 - Main: Object ID 1
>> > ok 25 - Thread: Object ID 1
>> > ok 26 - Thread: Changed object ID 10
>> > ok 27 - Thread: New object ID 2
>> > ok 28 - Main: New object ID 2  # TODO - should be 2
>>
>> Interesting.  Since the change that fixed this test did not involve
>> shared.xs code should this test be moved to t/op/???
>> Or should I just make it conditionally TODO based on perl version?
>
> I don't know. I had assumed make it conditionally TODO, but I don't know *why*
> it starts passing. (Other than the description implies that magic will now be
> called where previously it incorrectly wasn't, and shared scalars
> implemented via magic)

I'll make it conditionally TODO. Patch to blead forthcoming.

Perl porters RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.