Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BBC: Scalar-List-Utils-1.65 breaks Tie::RefHash::Weak #22528

Open
jkeenan opened this issue Aug 21, 2024 · 11 comments
Open

BBC: Scalar-List-Utils-1.65 breaks Tie::RefHash::Weak #22528

jkeenan opened this issue Aug 21, 2024 · 11 comments
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Closable? We might be able to close this ticket, but we need to check with the reporter

Comments

@jkeenan
Copy link
Contributor

jkeenan commented Aug 21, 2024 •
edited
Loading

CPAN distribution Tie-RefHash-Weak has begun to experience test failures on CPANtesters. Sample failure report: http://www.cpantesters.org/cpan/report/c2837ae0-5ce3-11ef-bb02-ec823e4a88bb

Excerpt from failure output:

PERL_DL_NONLAZY=1 "/usr/home/jkeenan/testing/blead/bin/perl" "-MExtUtils::Command::MM" "-MTest::Harness" "-e" "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" t/*.t
Can't coerce ARRAY to integer in goto at /usr/home/jkeenan/.cpan/build/Tie-RefHash-Weak-0.09-0/blib/lib/Tie/RefHash/Weak.pm line 48.
# Looks like your test exited with 255 just after 6.
t/01basic.t ....... 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 20/26 subtests 
Can't coerce ARRAY to integer in goto at /usr/home/jkeenan/.cpan/build/Tie-RefHash-Weak-0.09-0/blib/lib/Tie/RefHash/Weak.pm line 48.
# Looks like your test exited with 255 just after 1.
t/02gc.t .......... 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 9/10 subtests 
t/fieldhash.t ..... ok
Can't coerce ARRAY to integer in goto at /usr/home/jkeenan/.cpan/build/Tie-RefHash-Weak-0.09-0/blib/lib/Tie/RefHash/Weak.pm line 48.
# Looks like your test exited with 255 just after 1.
t/overload.t ...... 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 5/6 subtests

Bisection with the following invocation:

perl Porting/bisect.pl \
--module=Tie::RefHash::Weak \
--start=bcb5884b9b0ae1345ac74420bacb865ba11baab3 \
--end=b52ac0882220a4eb961f5028163fb75494704d7a

... points to :

commit d174c09669a057e017279ced5d33589ab3b9d10e (HEAD, refs/bisect/bad)
Author:     Paul Evans <[email protected]>
AuthorDate: Wed Aug 14 21:10:35 2024 -0400
Commit:     James E Keenan <[email protected]>
CommitDate: Wed Aug 14 21:10:41 2024 -0400

    cpan/Scalar-List-Utils - Update to version 1.65

That is, synching the most recent version of Scalar-List-Utils into blead triggered these failures. @leonerd, can you take a look? Thanks.

Cited upstream: https://rt.cpan.org/Ticket/Display.html?id=155006
@jkeenan jkeenan added BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Needs Triage labels Aug 21, 2024
@mauke
Copy link
Contributor

mauke commented Aug 21, 2024

Probably the same root cause:

$ ./perl -Ilib -e 'use Tie::RefHash (); Tie::RefHash::refaddr(0)'
Segmentation fault

@mauke
Copy link
Contributor

mauke commented Aug 21, 2024

... which in turn reduces to:

$ perl -e 'sub { goto &builtin::refaddr; }->(0)'
Segmentation fault

@leonerd
Copy link
Contributor

leonerd commented Aug 21, 2024

Huh. Exciting times:

$ bleadperl -E 'sub { goto &builtin::refaddr }->(0)'
panic: pad_sv po at -e line 1.

Seems to be specifically related to the tailcall though, because similar invocations don't fail:

$ bleadperl -E 'my $f = \&builtin::refaddr; $f->(0)'

$ bleadperl -E 'builtin::refaddr 0'

(both run OK)

mauke added a commit to mauke/Tie-RefHash that referenced this issue Aug 25, 2024
@mauke
work around Perl/perl5#22528
73fb65b 
... and simplify the code a bit.

Tie::RefHash used to either import `refaddr` from Scalar::Util, if
available, or define its own fallback solution based on
overload::StrVal, also under the name `refaddr`. This is not a
documented public API, but Tie::RefHash::Weak calls it anyway.

In commit 2cfa4bd, the code was changed to blindly call
Scalar::Util::refaddr directly, making the whole Scalar::Util
availability detection logic pointless. This commit also introduced a
weird indirection: Instead of importing `refaddr` from Scalar::Util as
before, or manually doing the equivalent (`*refaddr =
\&Scalar::Util::refaddr;`), it defined `*refaddr = sub { goto
&Scalar::Util::refaddr };`.

This `goto` construct happens to tickle Perl/perl5#22528 (in combination
with Scalar::Util 1.64+, which delegates to builtin::refaddr). Normally
this would cause crashes whenever `refaddr` is used. However,
Tie::RefHash itself is unaffected because it never calls the `refaddr`
sub that it defines, leaving Tie::RefHash::Weak as the only affected
module.

Work around the issue by getting rid of the custom detection/fallback
logic and just importing `refaddr` from Scalar::Util, (mostly) as
before.
@mauke mauke mentioned this issue Aug 25, 2024
Merged
karenetheridge added a commit to karenetheridge/Tie-RefHash that referenced this issue Aug 25, 2024
@karenetheridge
Merge pull request #3 from mauke/work-around-perl-22528
5b6861b 
work around Perl/perl5#22528
@karenetheridge
Copy link
Member

Tie-RefHash-1.41 has been released which will fix this (thanks @mauke).

@karenetheridge
Copy link
Member

(reopened.. Scalar::Util still needs to be fixed; just the collateral damage has been fixed so far)

@jkeenan
Copy link
Contributor Author

jkeenan commented Aug 26, 2024

Tie-RefHash-1.41 has been released which will fix this (thanks @mauke).

Was Tie-RefHash experiencing test failures? The failures I reported in this ticket were in Tie-RefHash-Weak.

@karenetheridge
Copy link
Member

The errors in Tie-RefHash-Weak were caused by faulty code in Tie::RefHash.

@mauke mauke mentioned this issue Aug 26, 2024
Closed
@mauke
Copy link
Contributor

mauke commented Aug 26, 2024

I've sync'd Tie::RefHash into core. Hopefully the workaround there will fix the Tie::RefHash::Weak errors. Once that is confirmed, we can close this issue, I think.

For the underlying root cause (crash in goto &builtin::refaddr) I've opened #22542. This is (sadly) not a regression: The builtin subs have been crashing since they were introduced in v5.36.

@andk
Copy link
Contributor

andk commented Aug 27, 2024

Also affected by recent List::Util: Util-Underscore-v1.4.2; see latk/p5-Util-Underscore#10

@leonerd , can you have a look there as well?

@mauke
Copy link
Contributor

mauke commented Aug 27, 2024

Same root cause. https://metacpan.org/dist/Util-Underscore/source/lib/Util/Underscore/References.pm is full of goto &Scalar::Util::..., the Scalar::Util functions are simply aliases for builtin ones (since 1.64), and goto &builtin::... is broken, which is #22542.

@jkeenan
Copy link
Contributor Author

jkeenan commented Sep 17, 2024

With the recent merge of #22582, which fixed #22542, the CPAN distributions cited in this ticket are now PASSing on CPANtesters.

http://fast-matrix.cpantesters.org/?dist=Tie-RefHash-Weak;perl=5.41.4;reports=1#sl=7,1

http://fast-matrix.cpantesters.org/?dist=Util-Underscore;perl=5.41.4;reports=1#sl=7,1

Labelling this ticket Closable.

@jkeenan jkeenan added the Closable? We might be able to close this ticket, but we need to check with the reporter label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Closable? We might be able to close this ticket, but we need to check with the reporter
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@andk @jkeenan @mauke @karenetheridge @leonerd