BBC: Blead Breaks Devel::Size

[toddr]

Devel::Size has been broken since 5.41.7

This is reported to @nwc10 at https://rt.cpan.org/Ticket/Display.html?id=158226

https://fast2-matrix.cpantesters.org/?dist=Devel-Size+0.84

t/recurse.t ..... 1/84 
#   Failed test 'PVNV + [] > [] alone'
#   at t/recurse.t line 117.
#     '88'
#         >
#     '88'

#   Failed test '[0..size] vs PVNV'
#   at t/recurse.t line 140.
#          got: '184'
#     expected: anything else

#   Failed test '[0..size] vs. PVNV'
#   at t/recurse.t line 142.
#     '184'
#         <
#     '184'

#   Failed test 'PVNV + [] > [] alone'
#   at t/recurse.t line 117.
#     '88'
#         >
#     '88'

#   Failed test '[0..size] vs PVNV'
#   at t/recurse.t line 140.
#          got: '216'
#     expected: anything else

#   Failed test '[0..size] vs. PVNV'
#   at t/recurse.t line 142.
#     '216'
#         <
#     '216'

#   Failed test 'PVNV + [] > [] alone'
#   at t/recurse.t line 117.
#     '88'
#         >
#     '88'

#   Failed test '[0..size] vs PVNV'
#   at t/recurse.t line 140.
#          got: '344'
#     expected: anything else

#   Failed test '[0..size] vs. PVNV'
#   at t/recurse.t line 142.
#     '344'
#         <
#     '344'

#   Failed test 'PVNV + [] > [] alone'
#   at t/recurse.t line 117.
#     '88'
#         >
#     '88'

#   Failed test '[0..size] vs PVNV'
#   at t/recurse.t line 140.
#          got: '3320'
#     expected: anything else

#   Failed test '[0..size] vs. PVNV'
#   at t/recurse.t line 142.
#     '3320'
#         <
#     '3320'

#   Failed test 'Type NV containing a reference, size() does not recurse to the referent'
#   at t/recurse.t line 295.
#          got: '24'
#     expected: '56'
# Looks like you failed 13 tests of 84.
t/recurse.t ..... Dubious, test returned 13 (wstat 3328, 0xd00)

[richardleach]

Reverting fbebf96 shouldn't break anything. I'll try to raise a PR to do this sometime tomorrow.

I'll look at the breakage in detail later/at the start of the next dev cycle. It's presumably a mismatch between this commit trying to avoid doing a body allocation when we can seemingly get away without one, but Devel::Size not expecting that behaviour.

[jkeenan]

Devel-Size is one of those CPAN distributions destined to break once every couple of development cycles. Compare:

#18320

#21522

[richardleach]

Note: 5a659e4 will have to be rolled back at the same time.

[jkeenan]

Bisection with the following invocation:

perl Porting/bisect.pl \
--module=Devel::Size \
--start=9e152dfc446d8b42320fa0475cdfbe172c3ef8bd \
--end=0877c09313129ad9ab50aeabf870cf5fce9724a0

... points to this as the breaking commit:

fbebf96ce083dc15b63c04d5695997d6e2657b03 is the first bad commit
commit fbebf96ce083dc15b63c04d5695997d6e2657b03
Author: Richard Leach <richardleach@users.noreply.github.com>
Date:   Wed Oct 23 23:32:04 2024 +0000
Commit:     Richard Leach <rich+perl@hyphen-dash-hyphen.info>
CommitDate: Mon Dec 16 21:33:09 2024 +0000

@nwc10 @richardleach

[nwc10]

This is reported to @nwc10 at https://rt.cpan.org/Ticket/Display.html?id=158226

This is the first I know about it. RT didn't notify me, which seems to be because

This RT account (NWCLARK) currently has no email address set. If you are a CPAN author and logged in with your PAUSE account, your email may have been disabled because of bounces in the past. Send email to rt-cpan-admin@bestpractical.com to re-activate your email.

(in a pastel red at the op of that page) which is also the first I knew about that

Blead is arguably correct (it's axiomatic - core is correct, Devel::Size tracks it and cheats).

I can probably get to a dev release in a day or two. If someone has a patch, that might shave 24 hours off that.

[richardleach]

I can probably get to a dev release in a day or two. If someone has a patch, that might shave 24 hours off that.

Ok, I'll have a look at the failing test tonight.

[toddr]

Note: 5a659e4 will have to be rolled back at the same time.

I apologize. I was in a hurry last night and didn't state it. I wasn't proposing a revert so much as making everyone aware of it. I'm sure this is fixable in Devel::Size. Thanks for all your hard work, @nwc10 and @richardleach!

[richardleach]

I'm sure this is fixable in Devel::Size.

Yeah, I took a quick look earlier and it looks (hah) straightforward. I wasn't sure if it would be at the off, which is why I mentioned reverting.

[richardleach]

Something like this, but I'll tidy it up before adding to the other ticket:

diff --git a/../Devel-Size-0.84/t/recurse.t b/t/recurse.t
index 7141245..b8dcec8 100644
--- a/../Devel-Size-0.84/t/recurse.t
+++ b/t/recurse.t
@@ -58,6 +58,12 @@ is (total_size($hash),
 #############################################################################
 # #24846 (Does not correctly recurse into references in a PVNV-type scalar)
 
+# Perl 5.41.7 modified Perl_sv_setsv_flags such that if source and destination
+# SVs are both bodyless, the SV type is just changed to the destination type
+# and no SV body is allocated.
+my $iv2nv_check = 1; $iv2nv_check = 2.2;
+my $head_only_upgrades = (total_size($iv2nv_check) == total_size (1)) ? 1 : 0;
+
 # run the following tests with different sizes
 
 for my $size (2, 3, 7, 100)
@@ -114,7 +120,11 @@ for my $size (2, 3, 7, 100)
   my $element_size = total_size(\$hash->{a});
 
   cmp_ok($element_size, '<', total_size($hash), "element < hash with one element");
-  cmp_ok($element_size, '>', total_size(\[]), "PVNV + [] > [] alone");
+  if ($head_only_upgrades) {
+    cmp_ok($element_size, '==', total_size(\[]), "RV + [] == [] alone");
+  } else {
+    cmp_ok($element_size, '>', total_size(\[]), "PVNV + [] > [] alone");
+  }
 
   # Dereferencing the PVNV (the argument to total_size) leaves us with
   # just the array, and this should be equal to a dereferenced array:
@@ -135,11 +145,16 @@ for my $size (2, 3, 7, 100)
   # the outer references stripped away, so they should be the same
   is (total_size([0..$size]), total_size( $hash->{a} ), "hash element vs. array");
 
-  # the outer references included, one is just a normal ref, while the other
-  # is a PVNV, so they shouldn't be the same:
-  isnt (total_size(\[0..$size]), total_size( \$hash->{a} ), "[0..size] vs PVNV");
-  # and the plain ref should be smaller
-  cmp_ok(total_size(\[0..$size]), '<', total_size( \$hash->{a} ), "[0..size] vs. PVNV");
+  if ($head_only_upgrades) {
+    is (total_size(\[0..$size]), total_size( \$hash->{a} ), "[0..size] vs TODO");
+    cmp_ok(total_size(\[0..$size]), '==', total_size( \$hash->{a} ), "[0..size] vs. TODO");
+  } else {
+    # the outer references included, one is just a normal ref, while the other
+    # is a PVNV, so they shouldn't be the same:
+    isnt (total_size(\[0..$size]), total_size( \$hash->{a} ), "[0..size] vs PVNV");
+    # and the plain ref should be smaller
+    cmp_ok(total_size(\[0..$size]), '<', total_size( \$hash->{a} ), "[0..size] vs. PVNV");
+  }
 
   $full_hash = total_size($hash);
   $element_size = total_size(\$hash->{a});
@@ -286,9 +301,10 @@ sub cmp_array_ro {
 
        $a->[0] = \('x' x 1024);
 
-       $expect = $sizes{RV} if $type eq 'NULL';
-       $expect = $sizes{PVNV} if $type eq 'NV';
-       $expect = $sizes{PVIV} if $type eq 'IV' && $] < 5.012;
+        $expect = ($type eq 'NULL') ? $sizes{RV} :
+                  ($type eq 'IV' && $] < 5.012) ? $sizes{PVIV} :
+                  ($type eq 'NV') ? ( ( $head_only_upgrades )
+                      ? $sizes{NV} : $sizes{PVNV} ) : $expect;
 
        # Remember, size() removes a level of referencing if present. So add
        # one, so that we get the size of our reference:

[richardleach]

A slighly nicer patch diff has been attached to https://rt.cpan.org/Ticket/Display.html?id=158226