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.

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

Bleadperl v5.31.1-58-g73cdf3a836 breaks YVES/Sereal-Decoder-4.007.tar.gz on -DDEBUGGING builds #17114

Closed
p5pRT opened this issue Jul 28, 2019 · 7 comments
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) distro-All
Milestone

Comments

@p5pRT
Copy link

p5pRT commented Jul 28, 2019

Migrated from rt.perl.org#134320 (status was 'open')

Searchable as RT134320$

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2019

From @andk

commit 73cdf3a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Apr 8 14​:17​:59 2019 +0100

  Make op_free() non-recursive

Sample fail report​: http​://www.cpantesters.org/cpan/report/65866002-ac5b-11e9-bbd9-42315acb998f

Enjoy,
--
andreas
PS​: perl

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2019

From @iabyn

On Sun, Jul 28, 2019 at 09​:58​:58AM -0700, (Andreas J. Koenig) (via RT) wrote​:

# New Ticket Created by (Andreas J. Koenig)
# Please include the string​: [perl #134320]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134320 >

commit 73cdf3a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Apr 8 14​:17​:59 2019 +0100

Make op\_free\(\) non\-recursive

Sample fail report​: http​://www.cpantesters.org/cpan/report/65866002-ac5b-11e9-bbd9-42315acb998f

THX_ck_entersub_args_sereal_decoder() in Decoder.xs wasn't setting the
parent link correctly when munging an op tree.
The following diff makes all tests pass on blead. I haven't tested against
older perls.

Inline Patch
--- Decoder.xs-	2019-08-05 15:33:13.414931329 +0100
+++ Decoder.xs	2019-08-05 17:05:14.761365063 +0100
@@ -223,13 +223,17 @@
     if (arity > min_arity)
         opopt |= OPOPT_OUTARG_HEADER;
 
-    OpMORESIB_set(pushop, cvop);
-    OpLASTSIB_set(lastargop, op_parent(lastargop));
+    /* cut out all ops between the pushmark and the RV2CV */
+    op_sibling_splice(NULL, pushop, arity, NULL);
+    /* then throw everything else out */
     op_free(entersubop);
-    newop = newUNOP(OP_NULL, 0, firstargop);
+    newop = newUNOP(OP_NULL, 0, NULL);
     newop->op_type    = OP_CUSTOM;
     newop->op_private = opopt;
     newop->op_ppaddr = opopt & OPOPT_LOOKS_LIKE ? THX_pp_looks_like_sereal : THX_pp_sereal_decode;
+    /* attach the spliced-out args as children of the custom op, while
+     * deleting the stub op created by newUNOP() */
+    op_sibling_splice(newop, NULL, 1, firstargop);
     return newop;
 }
 


-- 

You never really learn to swear until you learn to drive.

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2019

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Aug 5, 2019

From @jkeenan

On Mon, 05 Aug 2019 16​:09​:50 GMT, davem wrote​:

On Sun, Jul 28, 2019 at 09​:58​:58AM -0700, (Andreas J. Koenig) (via RT)
wrote​:

# New Ticket Created by (Andreas J. Koenig)
# Please include the string​: [perl #134320]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134320 >

commit 73cdf3a
Author​: David Mitchell <davem@​iabyn.com>
Date​: Mon Apr 8 14​:17​:59 2019 +0100

Make op_free() non-recursive

Sample fail report​: http​://www.cpantesters.org/cpan/report/65866002-
ac5b-11e9-bbd9-42315acb998f

THX_ck_entersub_args_sereal_decoder() in Decoder.xs wasn't setting the
parent link correctly when munging an op tree.
The following diff makes all tests pass on blead. I haven't tested
against
older perls.

--- Decoder.xs- 2019-08-05 15​:33​:13.414931329 +0100
+++ Decoder.xs 2019-08-05 17​:05​:14.761365063 +0100
@​@​ -223,13 +223,17 @​@​
if (arity > min_arity)
opopt |= OPOPT_OUTARG_HEADER;

- OpMORESIB_set(pushop, cvop);
- OpLASTSIB_set(lastargop, op_parent(lastargop));
+ /* cut out all ops between the pushmark and the RV2CV */
+ op_sibling_splice(NULL, pushop, arity, NULL);
+ /* then throw everything else out */
op_free(entersubop);
- newop = newUNOP(OP_NULL, 0, firstargop);
+ newop = newUNOP(OP_NULL, 0, NULL);
newop->op_type = OP_CUSTOM;
newop->op_private = opopt;
newop->op_ppaddr = opopt & OPOPT_LOOKS_LIKE ?
THX_pp_looks_like_sereal : THX_pp_sereal_decode;
+ /* attach the spliced-out args as children of the custom op,
while
+ * deleting the stub op created by newUNOP() */
+ op_sibling_splice(newop, NULL, 1, firstargop);
return newop;
}

Entered comment in Sereal/Sereal#207 citing this patch.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT p5pRT added BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) Severity Low distro-All labels Oct 19, 2019
@p5pRT p5pRT added the 5.32.0 label Oct 25, 2019
@toddr toddr added this to the 5.32.0 milestone Oct 25, 2019
@toddr toddr removed the 5.32.0 label Oct 25, 2019
jkeenan pushed a commit to jkeenan/Sereal that referenced this issue Nov 3, 2019
This patch was originally provided by Dave Mitchell inline as a response
to a "Blead Breaks CPAN" report in the Perl 5 bug queue (originally
rt.perl.org; now github.com/Perl/perl5/issues).  Dave's comment in
original ticket.

    THX_ck_entersub_args_sereal_decoder() in Decoder.xs
    wasn't setting the parent link correctly when munging an op
    tree.  The following diff makes all tests pass on blead. I
    haven't tested against older perls.

References:

Perl/perl5#17114
(originally https://rt-archive.perl.org/perl5/Ticket/Display.html?id=134320)

Sereal#207
@jkeenan
Copy link
Contributor

jkeenan commented Nov 3, 2019

I have made the patch originally submitted inline by @iabyn into a pull request to the Sereal repository: Sereal/Sereal#210

@toddr
Copy link
Member

toddr commented Feb 17, 2020

Which was merged. I assume we can close this?

@toddr toddr added the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 17, 2020
@jkeenan
Copy link
Contributor

jkeenan commented Feb 17, 2020

Which was merged. I assume we can close this?

Yes, I believe all the distros downstream from Sereal are now PASSing again.

Thank you very much.
Jim Keenan

@jkeenan jkeenan closed this as completed Feb 17, 2020
@jkeenan jkeenan removed the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BBC Blead Breaks CPAN - changes in blead broke a cpan module(s) distro-All
Projects
None yet
Development

No branches or pull requests

3 participants