Mailing List Archive

> BBC: Blead Breaks Data::Alias — Slaven notes that until recently there were some pass reports, but I think the general consensus here is that it's been totally broken for a year and probably won't ever be repaired.  I think we should call this a non-blocker.  (That said, the kind of failure Slaven shows is really weird.  Has anybody given it a glance?)

Data::Alias has some interesting downstream distributions, including TOBYINK’s Kavorka (signatures) which in turn is used by Moops and some other things. Not big numbers of deps on CPAN, but makes it worth another look. I’ll look into how these deps are using Data::Alias tomorrow.

Data::Alias is an XS module, so maybe an XS-head could take a look?

The pattern of CPAN Testers fails[1] for Data-Alias is weird too: almost entirely green until 5.31.2, but since then almost entirely red, but green for 5.33.4 and 5.33.5. And Solaris all green until 5.33.6

Neil

[1] http://matrix.cpantesters.org/?dist=Data-Alias

On Sun, 18 Apr 2021 23:09:57 +0100
Neil Bowers <neilb@neilb.org> wrote:

> > BBC: Blead Breaks Data::Alias ? Slaven notes that until recently there were some pass reports, but I think the general consensus here is that it's been totally broken for a year and probably won't ever be repaired.??I think we should call this a non-blocker.??(That said, the kind of failure Slaven shows is really weird.??Has anybody given it a glance?)
>
> Data::Alias has some interesting downstream distributions, including TOBYINK?s Kavorka (signatures) which in turn is used by Moops and some other things. Not big numbers of deps on CPAN, but makes it worth another look. I?ll look into how these deps are using Data::Alias tomorrow.

Metacpan's "reverse dependencies" are slightly misleading. It seems that
Kavorka requires Data::Alias only for Perls < 5.22[1], otherwise it's
just a "suggest" dependency. In case of Method::Signatures, it's listed
as "recommends"[2]. Devel::CallParser OTOH doesn't depend on Data::Alias
at all and has it listed under "conflicts"[3].

[1] - https://metacpan.org/source/TOBYINK/Kavorka-0.039/Makefile.PL#L125
[2] - https://metacpan.org/source/BAREFOOT/Method-Signatures-20170211/Build.PL#L62
[3] - https://metacpan.org/source/ZEFRAM/Devel-CallParser-0.002/Build.PL#L149

On Sun, 18 Apr 2021 at 21:05, Ricardo Signes <perl.p5p@rjbs.manxome.org>
wrote:

> Replacement of PL_hash_seed with PL_hash_seed_w breaks mod_perl2
> <https://github.com/Perl/perl5/issues/18617> — It looks like we (in the
> form of Yves) have said that the bug is downstream in mod_perl2. Do we
> know whether mod_perl2 is still broken? Is anyone capable of doing a test
> build and, possibly, providing a patch?
>
>
Yes, mod_perl is currently broken, but I have a "fix" for it which involves
simply removing the code in question for perl >= 5.33.7. I've spoken to
various mod_perl devs (current and old) and we're confident that the result
will only be a limitation in the test framework.

The framework has a mode for running tests in a random order, and uses the
hash seed trick to be able to re-run a previous sequence if it was found to
fail -- which it then tries to reduce to a minimal case by re-running it
over and over with fewer and fewer tests included.

We could probably live without that feature if we had to, especially since
little new development is done these days, but on the other hand, I see
that Leon has proposed an alternative fix on the perl side which would
leave mod_perl unaffected. I will test that out tomorrow.

On Mon, 19 Apr 2021 at 07:10, Neil Bowers <neilb@neilb.org> wrote:

> Data::Alias is an XS module, so maybe an XS-head could take a look?
>

The error originates from a memory problem caused by a macro called
RenewOpc around line 60 of Alias.xs which uses some features called NewOp,
Copy, and FreeOp. These don't seem to be part of the Perl API so presumably
they got changed somehow and the module no longer works. Anyway I could not
find the API documentation for these things.

I'll post all the details of the memory error on the rt.cpan.org bug
tracker:

https://rt.cpan.org/Ticket/Display.html?id=130156


> The pattern of CPAN Testers fails[1] for Data-Alias is weird too: almost
> entirely green until 5.31.2, but since then almost entirely red, but green
> for 5.33.4 and 5.33.5. And Solaris all green until 5.33.6
>

This may be too much information but it's not unusual for these kinds of
memory errors to cause different symptoms or no symptoms at all depending
on the operating system. This is so-called "Undefined behaviour" in C where
the specification actually doesn't say what to do, so various
implementations can do whatever they like.

On Sun, Apr 18, 2021 at 11:09:57PM +0100, Neil Bowers wrote:
> > BBC: Blead Breaks Data::Alias — Slaven notes that until recently there were some pass reports, but I think the general consensus here is that it's been totally broken for a year and probably won't ever be repaired.  I think we should call this a non-blocker.  (That said, the kind of failure Slaven shows is really weird.  Has anybody given it a glance?)
>
> Data::Alias has some interesting downstream distributions, including TOBYINK’s Kavorka (signatures) which in turn is used by Moops and some other things. Not big numbers of deps on CPAN, but makes it worth another look. I’ll look into how these deps are using Data::Alias tomorrow.
>
> Data::Alias is an XS module, so maybe an XS-head could take a look?
>
> The pattern of CPAN Testers fails[1] for Data-Alias is weird too: almost entirely green until 5.31.2, but since then almost entirely red, but green for 5.33.4 and 5.33.5. And Solaris all green until 5.33.6

In https://github.com/Perl/perl5/issues/17072 Dave suggested it was
the module (which does OP manipulation), not being updated for
OP_PARENT, since the commit mentioned in #17072 was the first that
depended on them being correct.

I'll take a look at it.

Tony

On Mon, Apr 19, 2021 at 12:40:05PM +0900, Ben Bullock wrote:
> On Mon, 19 Apr 2021 at 07:10, Neil Bowers <neilb@neilb.org> wrote:
>
> > Data::Alias is an XS module, so maybe an XS-head could take a look?
> >
>
> The error originates from a memory problem caused by a macro called
> RenewOpc around line 60 of Alias.xs which uses some features called NewOp,
> Copy, and FreeOp. These don't seem to be part of the Perl API so presumably
> they got changed somehow and the module no longer works. Anyway I could not
> find the API documentation for these things.

Copy() is API and documented in perlguts.

NewOp() isn't documented, but is useful for creating new OPs that
don't quite match the standard ones created with the various
newXXXOP() functions.

FreeOp() is correctly used here.

Tony

>>>>> On Mon, 19 Apr 2021 14:25:29 +1000, Tony Cook <tony@develop-help.com> said:

> In https://github.com/Perl/perl5/issues/17072 Dave suggested it was
> the module (which does OP manipulation), not being updated for
> OP_PARENT, since the commit mentioned in #17072 was the first that
> depended on them being correct.

In case a bisect helps, I have two of them:

- v5.31.0-72-g2324bdb9a8 for DEBUGGING builds
- v5.33.5-62-gc588171e62 for non-DEBUGGING builds

Regards,
--
andreas

On Mon, 19 Apr 2021 at 00:31, Steve Hay <steve.m.hay@googlemail.com> wrote:
>
> On Sun, 18 Apr 2021 at 21:05, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
>>
>> Replacement of PL_hash_seed with PL_hash_seed_w breaks mod_perl2 — It looks like we (in the form of Yves) have said that the bug is downstream in mod_perl2. Do we know whether mod_perl2 is still broken? Is anyone capable of doing a test build and, possibly, providing a patch?
>>
>
> Yes, mod_perl is currently broken, but I have a "fix" for it which involves simply removing the code in question for perl >= 5.33.7. I've spoken to various mod_perl devs (current and old) and we're confident that the result will only be a limitation in the test framework.
>
> The framework has a mode for running tests in a random order, and uses the hash seed trick to be able to re-run a previous sequence if it was found to fail -- which it then tries to reduce to a minimal case by re-running it over and over with fewer and fewer tests included.
>
> We could probably live without that feature if we had to, especially since little new development is done these days, but on the other hand, I see that Leon has proposed an alternative fix on the perl side which would leave mod_perl unaffected. I will test that out tomorrow.

Leon's solution (which is a change to mod_perl, not perl like I
mistakenly wrote above) has made this problem go away.

I've checked it in to mod_perl, which I hope can be released in time
for perl 5.34.0, and perl issue #18617 is now closed.

On Tue, Apr 20, 2021, at 10:02 AM, Steve Hay wrote:
> Leon's solution (which is a change to mod_perl, not perl like I
> mistakenly wrote above) has made this problem go away.

Thank you very much, Steve and Leon.

--
rjbs

Oops, I somehow lost the subject line when I tried to send this.

Andreas Koenig <andreas.koenig.7os6VVqR@franz.ak.mind.de> wrote:
:>>>>> On Mon, 19 Apr 2021 14:25:29 +1000, Tony Cook <tony@develop-help.com> said:
:
: > In https://github.com/Perl/perl5/issues/17072 Dave suggested it was
: > the module (which does OP manipulation), not being updated for
: > OP_PARENT, since the commit mentioned in #17072 was the first that
: > depended on them being correct.
:
:In case a bisect helps, I have two of them:
:
:- v5.31.0-72-g2324bdb9a8 for DEBUGGING builds
:- v5.33.5-62-gc588171e62 for non-DEBUGGING builds

Ah, I had looked at 2324bdb9a8 in trying to understand the DEBUGGING failure,
but discounted it; I now see that Data::Alias also #defines PERL_CORE in
its header, which to my mind makes it even more egregious.

Hugo

On Mon, Apr 19, 2021 at 02:25:29PM +1000, Tony Cook wrote:
> In https://github.com/Perl/perl5/issues/17072 Dave suggested it was
> the module (which does OP manipulation), not being updated for
> OP_PARENT, since the commit mentioned in #17072 was the first that
> depended on them being correct.
>
> I'll take a look at it.

(This ignores using SvCUR() as an lvalue, which D::A will need to
fix.)

The original problem in #17072 turned out not to be a problem with
parent links, but something a bit more subtle.

When post processing code like:

$foo = copy($_) for 1; # Data::Alias::copy

Data::Alias turns an op tree like:

1 entersub UNOP(0x555555c435b0) ===> [0x0]
PARENT ===> [0x0]
FLAGS = (UNKNOWN,KIDS,STACKED,SLABBED)
PRIVATE = (INARGS)
|
2 +--null (ex-list) UNOP(0x555555c42cc8) ===> [0x0]
FLAGS = (UNKNOWN,KIDS,SLABBED)
|
3 +--pushmark OP(0x555555c42d08) ===> [SELF]
| FLAGS = (SCALAR,SLABBED,MORESIB)
|
4 +--rv2sv UNOP(0x555555c42d38) ===> [0x0]
| FLAGS = (SCALAR,KIDS,SLABBED,MORESIB)
| PRIVATE = (0x1)
| |
5 | +--gv SVOP(0x555555c42da8) ===> [SELF]
| FLAGS = (SCALAR,SLABBED)
| GV = main::_ (0x555555c186e0)
|
6 +--rv2cv UNOP(0x555555c42d70) ===> [0x0]
FLAGS = (SCALAR,KIDS,SPECIAL,SLABBED)
PRIVATE = (0x1)
|
7 +--gv SVOP(0x555555c42de0) ===> [SELF]
FLAGS = (SCALAR,SLABBED)
GV = main::copy (0x555555c44ef8)

into:

8 leave LISTOP(0x555555c43570) ===> [0x0]
PARENT ===> [0x0]
FLAGS = (UNKNOWN,KIDS,STACKED,SLABBED)
PRIVATE = (0x1)
|
2 +--list LISTOP(0x555555c42cc8) ===> [0x0]
FLAGS = (UNKNOWN,KIDS,SLABBED)
|
1 +--custom OP(0x555555c435b0) ===> [SELF]
| FLAGS = (SCALAR,SLABBED,MORESIB)
|
4 +--rv2sv UNOP(0x555555c42d38) ===> [0x0]
FLAGS = (SCALAR,KIDS,SLABBED)
PRIVATE = (0x1)
|
5 +--gv SVOP(0x555555c42da8) ===> [SELF]
FLAGS = (SCALAR,SLABBED)
GV = main::_ (0x555555c186e0)

Now leave's first child is typically an enter, followed by the
contents of the code it wraps:

tony@venus:.../git/perl$ ./perl -Dx -e '$x = 1'

1 leave LISTOP(0x56195688b280) ===> [0x0]
PARENT ===> [0x0]
TARG = 1
FLAGS = (VOID,KIDS,PARENS,SLABBED)
PRIVATE = (REFC)
REFCNT = 1
|
2 +--enter OP(0x56195688b250) ===> 3 [nextstate 0x56195688b2c0]
| FLAGS = (VOID,SLABBED,MORESIB)
|
3 +--nextstate COP(0x56195688b2c0) ===> 4 [const 0x56195688b360]
| FLAGS = (VOID,SLABBED,MORESIB)
| LINE = 1
| PACKAGE = "main"
| SEQ = 4294967246
|
5 +--sassign BINOP(0x56195688b320) ===> 1 [leave 0x56195688b280]
FLAGS = (VOID,KIDS,STACKED,SLABBED)
PRIVATE = (0x2)
|
4 +--const SVOP(0x56195688b360) ===> 6 [gvsv 0x56195688b3d0]
| FLAGS = (SCALAR,SLABBED,MORESIB)
| SV = IV(1)
|
7 +--null (ex-rv2sv) UNOP(0x56195688b398) ===> 5 [sassign 0x56195688b320]
FLAGS = (SCALAR,KIDS,REF,MOD,SPECIAL,SLABBED)
PRIVATE = (0x1)
|
6 +--gvsv SVOP(0x56195688b3d0) ===> 5 [sassign 0x56195688b320]
FLAGS = (SCALAR,SLABBED)
GV = main::x (0x561956884778)

This is confusing Perl_scalar(), which expects an enter to be the
first child, followed by more ops, but the D::A modified op tree only
has a single child for the leave, which is a list op.

Now the code in Perl_scalar() that makes that assumption has been
there since 1997:

79072805bf6 (Larry Wall 1993-10-07 975) case OP_LEAVE:
79072805bf6 (Larry Wall 1993-10-07 976) case OP_LEAVETRY:
5dc0d6134eb (Malcolm Beattie 1997-05-26 977) kid = cLISTOPo->op_first;
54310121b44 (Perl 5 Porters 1997-03-26 978) scalar(kid);
54310121b44 (Perl 5 Porters 1997-03-26 979) while (kid = kid->op_sibling) {

Currently:

case OP_LEAVETRY:
kid = cLISTOPo->op_first;
scalar(kid);
kid = OpSIBLING(kid);
do_kids:
while (kid) {

so what broke?

Previously Perl_scalar() was generally recursive, each child op that
needed to be marked as being in scalar context would result in a
recursive call to Perl_scalar(). This could lead to crashes due to
stack exhaustion with very deep op trees, both here and in similar
ways in a few other places.

The PERL_OP_PARENT change made the op_sibling member of OP into
op_sibparent, and added the op_moresib flag to indicate which it was.
This allows code to perform crawl through the op tree without any
recursion.

The leave without enter + body was harmless with recursion, the while
loop would simply fall off the end and return from the recursive call.

Without recursion the loop body is a bit more complex:

while (kid) {
OP *sib = OpSIBLING(kid);
if (!sib
|| ( !OpHAS_SIBLING(sib)
&& sib->op_type == OP_NULL
&& ( sib->op_targ == OP_NEXTSTATE
|| sib->op_targ == OP_DBSTATE )
)
)
{
/* tail call optimise calling scalar() on the last kid */
next_kid = kid;
goto do_next;
}
else if (kid->op_type == OP_LEAVEWHEN)
scalar(kid);
else
scalarvoid(kid);
kid = sib;
}
NOT_REACHED; /* NOTREACHED */

to handle tracing back up to the parent op, but in the D::A case, the
loop isn't entered - kid starts out NULL, and the loop falls off the
end and breaks, rather than setting next_kid and jumping to do_next.

A similar problem occurs in Perl_list(), which was modified in
f23e1643, the bisect result mentioned in #17072.

I'll do a PR with a code workaround for this in perl.

I originally debugged this with perl 5.32.0, and the changes will fix
that, but it looks like another problem was introduced around 5.33.6,
for example:

http://www.cpantesters.org/cpan/report/811f9446-6587-11eb-99f5-c2341f24ea8f

vs the older failure:

http://www.cpantesters.org/cpan/report/83a7ebaa-0786-11eb-8bb9-2b0d2fb83bff

which I'll look at next.

Tony

On Wed, Apr 21, 2021 at 01:38:06PM +1000, Tony Cook wrote:
> I originally debugged this with perl 5.32.0, and the changes will fix
> that, but it looks like another problem was introduced around 5.33.6,
> for example:
>
> http://www.cpantesters.org/cpan/report/811f9446-6587-11eb-99f5-c2341f24ea8f
>
> vs the older failure:
>
> http://www.cpantesters.org/cpan/report/83a7ebaa-0786-11eb-8bb9-2b0d2fb83bff
>
> which I'll look at next.

This turned out to be easily fixable in Data::Alias itself, with a
patch attached at:

https://rt.cpan.org/Ticket/Display.html?id=130156#txn-1972072

(which also fixes the compilation errors)

Tony

> This turned out to be easily fixable in Data::Alias itself, with apatch attached at:
> https://rt.cpan.org/Ticket/Display.html?id=130156#txn-1972072
> (which also fixes the compilation errors)

This is good news — thank you.

Does anyone have any contact with Zefram to see whether he’s going to be able to do a release? I’ll email him as well. I see that XMATH has co-maint, but doesn’t appear to be active. Who else would be appropriate for co-maint?

Neil

The current situation with Data::Alias

• Tony fixed one of the problems and produced a patch (see the RT issue[1])
• Matthijs (XMATH) has co-maint; he's applied the patch and done a release[2] to CPAN
• Another problem remains — "the op tree problem"
• The "right" place to fix that is in D::A
• Matthijs doesn’t think he’s up to fixing it
• Zefram isn’t likely to fix it
• Tony has a draft PR[3] with a Perl workaround to avoid the need for a fix in D::A


[1] https://rt.cpan.org/Public/Bug/Display.html?id=130156
[2] https://metacpan.org/release/XMATH/Data-Alias-1.22
[3] https://github.com/Perl/perl5/pull/18738

On Thu, Apr 22, 2021 at 04:42:24PM +0100, Neil Bowers wrote:
> The current situation with Data::Alias
>
> • Tony fixed one of the problems and produced a patch (see the RT issue[1])
> • Matthijs (XMATH) has co-maint; he's applied the patch and done a release[2] to CPAN
> • Another problem remains — "the op tree problem"
> • The "right" place to fix that is in D::A
> • Matthijs doesn’t think he’s up to fixing it
> • Zefram isn’t likely to fix it
> • Tony has a draft PR[3] with a Perl workaround to avoid the need for a fix in D::A
>
>
> [1] https://rt.cpan.org/Public/Bug/Display.html?id=130156
> [2] https://metacpan.org/release/XMATH/Data-Alias-1.22
> [3] https://github.com/Perl/perl5/pull/18738

I've now supplied a patch to Data::Alias at:

https://rt.cpan.org/Ticket/Display.html?id=130156#txn-1976186

which I believe fixes the problem.

I'm not entirely comfortable with manipulating the op tree, so if
anyone else has the tuits, please have a look over it.

Thanks,

Tony

Tony

It's a Perl version-independent patch. Great!


2021?4?27?(?) 9:36 Tony Cook <tony@develop-help.com>:

> On Thu, Apr 22, 2021 at 04:42:24PM +0100, Neil Bowers wrote:
> > The current situation with Data::Alias
> >
> > • Tony fixed one of the problems and produced a patch (see the RT
> issue[1])
> > • Matthijs (XMATH) has co-maint; he's applied the patch and done a
> release[2] to CPAN
> > • Another problem remains — "the op tree problem"
> > • The "right" place to fix that is in D::A
> > • Matthijs doesn’t think he’s up to fixing it
> > • Zefram isn’t likely to fix it
> > • Tony has a draft PR[3] with a Perl workaround to avoid the need
> for a fix in D::A
> >
> >
> > [1] https://rt.cpan.org/Public/Bug/Display.html?id=130156
> > [2] https://metacpan.org/release/XMATH/Data-Alias-1.22
> > [3] https://github.com/Perl/perl5/pull/18738
>
> I've now supplied a patch to Data::Alias at:
>
> https://rt.cpan.org/Ticket/Display.html?id=130156#txn-1976186
>
> which I believe fixes the problem.
>
> I'm not entirely comfortable with manipulating the op tree, so if
> anyone else has the tuits, please have a look over it.
>
> Thanks,
>
> Tony
>