BBC: List-UtilsBy-XS-0.05 may fail since perl 5.31.2

[eserte]

Description
The test suite of List-UtilsBy-XS-0.05 fails with bleadperl if t/99_leaktrace.t is not skipped (i.e. the optional dependency Test::LeakTrace is installed).

Overview of test reports: http://fast-matrix.cpantesters.org/?dist=List-UtilsBy-XS%200.05
A sample fail report: http://www.cpantesters.org/cpan/report/e69a3e28-f6ea-11e9-a5cf-39e6fbedce4e

(Note that for some reason t/99_leaktrace.t is silent if it is skipped, so make sure that Test::LeakTrace is really installed)

[jkeenan]

Specifics:

# start without having Test::LeakTrace installed

[p5-List-Utils-By] $ bleadperl -v | head -2 | tail -1
This is perl 5, version 31, subversion 6 (v5.31.6 (v5.31.5-163-ga6d5b8290f)) built for x86_64-linux
[p5-List-Utils-By] $ bleadperl Build.PL
Created MYMETA.yml and MYMETA.json
Creating new 'Build' script for 'List-UtilsBy-XS' version '0.05'
[p5-List-Utils-By] $ ./Build
Building List-UtilsBy-XS
cc -Ixs-src -I/home/jkeenan/testing/blead/lib/perl5/5.31.6/x86_64-linux/CORE -DVERSION="0.05" -DXS_VERSION="0.05" -fPIC -DPERL_EXT -Wall -Wextra -Wc++-compat -c -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -O2 -o lib/List/UtilsBy/XS.o lib/List/UtilsBy/XS.c
ExtUtils::Mkbootstrap::Mkbootstrap('blib/arch/auto/List/UtilsBy/XS/XS.bs')
cc -shared -O2 -L/usr/local/lib -fstack-protector-strong -o blib/arch/auto/List/UtilsBy/XS/XS.so lib/List/UtilsBy/XS.o
[p5-List-Utils-By] $ ./Build test
t/00_compile.t .............. ok   
t/01_sort_by.t .............. ok   
t/02_nsort_by.t ............. ok    
t/03_max_by.t ............... ok    
t/04_min_by.t ............... ok   
t/05_uniq_by.t .............. ok   
t/06_partition_by.t ......... ok   
t/07_count_by.t ............. ok   
t/08_zip_by.t ............... ok   
t/09_extract_by.t ........... ok    
t/10_weighted_shuffle_by.t .. ok   
t/11_bundle_by.t ............ ok   
t/12_unzip_by.t ............. ok   
t/99_leaktrace.t ............ skipped: Test::LeakTrace required for memory leak testing
xt/02_perlcritic.t .......... skipped: Test::Perl::Critic 1.02+ is not installed.
All tests successful.
Files=15, Tests=92,  1 wallclock secs ( 0.05 usr  0.02 sys +  0.71 cusr  0.08 csys =  0.86 CPU)
Result: PASS

# now, install Test-LeakTrace against bleadperl

[p5-List-Utils-By] $ bleadperl -MTest::LeakTrace -E 'say q|hello world|'
hello world
[p5-List-Utils-By] $ bleadprove -vb t/99_leaktrace.t 
t/99_leaktrace.t .. 
ok 1 - sort_by (leaks 0 <= 0)
ok 2 - nsort_by (leaks 0 <= 0)
ok 3 - max_by (leaks 0 <= 0)
ok 4 - uniq_by (leaks 0 <= 0)
ok 5 - partition_by (leaks 0 <= 0)
ok 6 - count_by (leaks 0 <= 0)
ok 7 - zip_by (leaks 0 <= 0)
ok 8 - unzip_by (leaks 0 <= 0)
ok 9 - extract_by (leaks 0 <= 0)
ok 10 - extract_by with scalar context (leaks 0 <= 0)
not ok 11 - weighted_shuffle_by (leaks 2 <= 0)

#   Failed test 'weighted_shuffle_by (leaks 2 <= 0)'
#   at t/99_leaktrace.t line 58.
#     '2'
#         <=
#     '0'
# leaked SCALAR(0x563de9c0f090) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de9c0f080) at 0x563de9c0f090
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 2
# leaked SCALAR(0x563de9c9bdd8) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de9c9bdc8) at 0x563de9c9bdd8
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 9
# leaked SCALAR(0x563de9c9bbb0) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de9c9bba0) at 0x563de9c9bbb0
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 2
# leaked SCALAR(0x563de95f0bc0) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de95f0bb0) at 0x563de95f0bc0
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 9
# leaked SCALAR(0x563de95f03b0) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de95f03a0) at 0x563de95f03b0
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 10
# leaked SCALAR(0x563de95f0050) from t/99_leaktrace.t line 57.
#   56:no_leaks_ok {
#   57:    my @a = weighted_shuffle_by { $_ } 1..10;
#   58:} 'weighted_shuffle_by';
# SV = IV(0x563de95f0040) at 0x563de95f0050
#   REFCNT = 1
#   FLAGS = (IOK,pIOK)
#   IV = 10
ok 12 - bundle_by (leaks 0 <= 0)
1..12
# Looks like you failed 1 test of 12.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests 

Test Summary Report
-------------------
t/99_leaktrace.t (Wstat: 256 Tests: 12 Failed: 1)
  Failed test:  11
  Non-zero exit status: 1
Files=1, Tests=12,  0 wallclock secs 
( 0.02 usr  0.00 sys +  0.06 cusr  0.00 csys =  0.08 CPU)
Result: FAIL

[dur-randir]

Bisect points to

commit bc62bf8 (refs/bisect/bad)
Author: Karl Williamson khw@cpan.org
AuthorDate: Sat Jun 1 14:39:55 2019 -0600
Commit: Karl Williamson khw@cpan.org
CommitDate: Thu Jun 27 09:36:35 2019 -0600

Add some defensive coding to av_store()

Don't decrement the reference count of the element about to be stored
into.

Likely, this is an error in the caller, but doing this action blindly is
like shooting yourself in the foot.  The branch prediction also added
ensures this shouldn't slow things down.

See http://nntp.perl.org/group/perl.perl5.porters/254974

[jkeenan]

With this spelling of the bisection command:

$ perl Porting/bisect.pl --module=Test::LeakTrace,List::UtilsBy::XS \
--start=0be0ef8f515958d475928ee7f95d06eb27ed16e8 \
--end=1a7108fea1f14e60c0d69b29980183cf21288be

... I got a different result:

1a7108fea1f14e60c0d69b29980183cf21288bef is the first bad commit
commit 1a7108fea1f14e60c0d69b29980183cf21288bef
Author: Karl Williamson <khw@cpan.org>
Date:   Thu Jun 27 11:09:43 2019 -0600

    PATCH: [perl #133996] reword diagnostic

:040000 040000 49cbc2357c78ec1477e4fa7ef5b2c62bab44677f ed8350ad74fc87a24be12bf6c072ef6b328958f3 M	pod
:100644 100644 fcde30dddf6abcdd5ba169b745603c010b8a21fb 9782f19f520e30f5557529d85eb821b0696bcbbf M	regcomp.c
:040000 040000 2667a1de2fd39023dff6dbc8733d871acc28df89 9b8b0ad6733744c697c66764e9e935f3b3da1f3c M	t
bisect run success
That took 1438 seconds.

commit 1a7108f
Author: Karl Williamson khw@cpan.org
AuthorDate: Thu Jun 27 11:09:43 2019 -0600
Commit: Karl Williamson khw@cpan.org
CommitDate: Thu Jun 27 11:12:27 2019 -0600

Same date as reported by @dur-randir, but two hours later.

Thank you very much.
Jim Keenan

[jkeenan]

Description
The test suite of List-UtilsBy-XS-0.05 fails with bleadperl if t/99_leaktrace.t is not skipped (i.e. the optional dependency Test::LeakTrace is installed).

Overview of test reports: http://fast-matrix.cpantesters.org/?dist=List-UtilsBy-XS%200.05
A sample fail report: http://www.cpantesters.org/cpan/report/e69a3e28-f6ea-11e9-a5cf-39e6fbedce4e

(Note that for some reason t/99_leaktrace.t is silent if it is skipped, so make sure that Test::LeakTrace is really installed)

@khwilliamson have you seen this BBC report?

Thank you very much.
Jim Keenan

[iabyn]

On Tue, Nov 05, 2019 at 02:58:23PM -0800, Sergey Aleynikov wrote: Bisect points to commit bc62bf8 (refs/bisect/bad) Author: Karl Williamson ***@***.***> AuthorDate: Sat Jun 1 14:39:55 2019 -0600 Commit: Karl Williamson ***@***.***> CommitDate: Thu Jun 27 09:36:35 2019 -0600 Add some defensive coding to av_store() Don't decrement the reference count of the element about to be stored into. Likely, this is an error in the caller, but doing this action blindly is like shooting yourself in the foot. The branch prediction also added ensures this shouldn't slow things down. See http://nntp.perl.org/group/perl.perl5.porters/254974

I think that commit was wrong. The docs for av_store() say that the caller is responsible for first incrementing the ref count of the new SV before passing it to av_store(). In the normal case of the two elements being different, av_store() will decrement the old SV before storing the new one, and everything is good. When the two elements are the same SV, av_store() *still* needs to decrement the ref count, to undo the increment the caller just did.

…

-- Never do today what you can put off till tomorrow.

[khwilliamson]

On 1/2/20 9:05 AM, iabyn wrote: On Tue, Nov 05, 2019 at 02:58:23PM -0800, Sergey Aleynikov wrote: > Bisect points to > > commit bc62bf8 (refs/bisect/bad) > Author: Karl Williamson ***@***.***> > AuthorDate: Sat Jun 1 14:39:55 2019 -0600 > Commit: Karl Williamson ***@***.***> > CommitDate: Thu Jun 27 09:36:35 2019 -0600 > > Add some defensive coding to av_store() > > Don't decrement the reference count of the element about to be stored > into. > > Likely, this is an error in the caller, but doing this action blindly is > like shooting yourself in the foot. The branch prediction also added > ensures this shouldn't slow things down. > > See http://nntp.perl.org/group/perl.perl5.porters/254974 I think that commit was wrong. The docs for av_store() say that the caller is responsible for first incrementing the ref count of the new SV before passing it to av_store(). In the normal case of the two elements being different, av_store() will decrement the old SV before storing the new one, and everything is good. When the two elements are the same SV, av_store() *still* needs to decrement the ref count, to undo the increment the caller just did.

Reverted. I'm leaving the ticket open until I hear that it actually fixed the problem. Maybe there should be an assert that the refcount is non-zero after the decrement?

[jkeenan]

On 1/2/20 9:05 AM, iabyn wrote: On Tue, Nov 05, 2019 at 02:58:23PM -0800, Sergey Aleynikov wrote: > Bisect points to > > commit bc62bf8 (refs/bisect/bad) > Author: Karl Williamson @.> > AuthorDate: Sat Jun 1 14:39:55 2019 -0600 > Commit: Karl Williamson @.> > CommitDate: Thu Jun 27 09:36:35 2019 -0600 > > Add some defensive coding to av_store() > > Don't decrement the reference count of the element about to be stored > into. > > Likely, this is an error in the caller, but doing this action blindly is > like shooting yourself in the foot. The branch prediction also added > ensures this shouldn't slow things down. > > See http://nntp.perl.org/group/perl.perl5.porters/254974 I think that commit was wrong. The docs for av_store() say that the caller is responsible for first incrementing the ref count of the new SV before passing it to av_store(). In the normal case of the two elements being different, av_store() will decrement the old SV before storing the new one, and everything is good. When the two elements are the same SV, av_store() still needs to decrement the ref count, to undo the increment the caller just did.
Reverted. I'm leaving the ticket open until I hear that it actually fixed the problem. Maybe there should be an assert that the refcount is non-zero after the decrement?

$ ./bin/perl -v | head -2 | tail -1
This is perl 5, version 31, subversion 8 (v5.31.8 (v5.31.7-54-g811f8a246e)) built for x86_64-linux
$ /bin/cpanm List::UtilsBy::XS
...
Successfully installed List-UtilsBy-XS-0.05
13 distributions installed

$ ./bin/cpanm Test::LeakTrace
...
Successfully installed Test-LeakTrace-0.16
1 distribution installed

$ bleadprove -vb t/99_leaktrace.t 
t/99_leaktrace.t .. 
ok 1 - sort_by (leaks 0 <= 0)
ok 2 - nsort_by (leaks 0 <= 0)
ok 3 - max_by (leaks 0 <= 0)
ok 4 - uniq_by (leaks 0 <= 0)
ok 5 - partition_by (leaks 0 <= 0)
ok 6 - count_by (leaks 0 <= 0)
ok 7 - zip_by (leaks 0 <= 0)
ok 8 - unzip_by (leaks 0 <= 0)
ok 9 - extract_by (leaks 0 <= 0)
ok 10 - extract_by with scalar context (leaks 0 <= 0)
ok 11 - weighted_shuffle_by (leaks 0 <= 0)
ok 12 - bundle_by (leaks 0 <= 0)
1..12
ok
All tests successful.
Files=1, Tests=12,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.05 cusr  0.00 csys =  0.07 CPU)
Result: PASS

So the reversion in blead resolved the original complaint in this ticket.

[jkeenan]

So the reversion in blead resolved the original complaint in this ticket.

@khwilliamson, can we close this ticket?

Thank you very much.
Jim Keenan