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

[p5pRT]

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

Searchable as RT134320$

[p5pRT]

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]

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]

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

[p5pRT]

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)

[jkeenan]

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