Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.98k stars 560 forks source link

cop_features not copied into optree #20438

Closed demerphq closed 1 year ago

demerphq commented 2 years ago

Description Unlike hints we do not copy cop_features out of PL_compiling into the constructed nextstate ops. This means feature bit checks do not work at run time. Currently it seems we have no feature that perform run time decision making, but we have at least one queued up that does need to do so. https://github.com/Perl/perl5/pull/20365

It seems to be an oversight as the hints data is properly copied. I already have a patch for this, but not prepped for a PR yet. See https://github.com/Perl/perl5/commit/6f97620ca1caf27246acb5faccdaa0ed3986e61f for example. Also @nwc10 identified the same issue and created https://github.com/nwc10/perl5/commit/d5f88b34531704a752a34ccf475e0ad62775f6c4 which i am testing as a cherry-pick to blead right now.

The following p5p irc is relevant:

(07:43:23) dmq: so i got to the bottom of the module_load problem.
(07:44:09) dmq: for some reason `use feature 'module_load';` does not set the features flag correctly but `use v5.37;` does.
(07:44:38) dmq: does that happen to ring a bell for anybody?
(08:19:34) dmq: is it possible that none of the existing features until now required *run* time decision making instead of compile time?
(08:19:52) dmq: i cant find a single COP that has a set cop_features field.
(08:20:38) dmq: except PL_compiling itself.
(08:32:06) Nicholas: Belive this is the case. It's a bug that hadn't been fixed IIRC. TonyC might remember more.
(08:34:19) Nicholas: There was this https://github.com/nwc10/perl5/commit/d5f88b34531704a752a34ccf475e0ad62775f6c4
(08:34:27) Nicholas: on that https://github.com/nwc10/perl5/commits/two-personalities
(08:34:42) Nicholas: but I can't remmeber if that commit is the same thing as what you seem to be asking
(08:35:16) Nicholas: and I really don't have time to dig further
(08:37:43) Nicholas: (you are not going mad. it is quite possibly what you think)
(08:45:02) dmq: I just found it!
(08:45:12) dmq: thanks for the confirmation nicholas.
(08:46:04) dmq: see 6f97620ca1caf27246acb5faccdaa0ed3986e61f
(08:46:21) dmq: https://github.com/Perl/perl5/commit/6f97620ca1caf27246acb5faccdaa0ed3986e61f
(08:47:03) dmq: i found one special case where cop_features was copied over for subs only.
(08:47:11) dmq: and only in a special edge case.
(08:47:24) dmq: i made it happen for all nextstate ops.
(08:47:42) dmq: now ill read those links you posted and chekc if ive done something dumb. well after more coffee
(09:01:22) dmq: Nicholas: i see your patch includes mine
(09:01:55) dmq: Was there a reason your patch didnt get merged? Should i try to wrangle it into perl?
(09:02:04) dmq: maybe not the whole branch, but that part?
(09:02:38) Nicholas: the branch was a proof of concept for something that PSC didn't want. But the patch fixed what I thought was a real bug
(09:02:54) Nicholas: However IIRC TonyC hit the same or a similar problem, and had a different (or smaller) solution
(09:03:14) Nicholas: and I can't remember if I thought that he only solved part of the whole problem. Or if he was right and I overdid it
(09:03:21) dmq: was his merged? (i guess if so it didnt include the cop_features bit)
(09:03:28) Nicholas: but I got busy and forgot all about this
(09:03:31) Nicholas: and I can't remember

@tonycoz might have some insight here as well.

Steps to Reproduce This isn't visible without a patch to the internals. There is only one place where the cop_features is copied out of PL_compiling into a nextstate op and it is a special case.

Expected behavior PL_curcop->cop_features to be non-zero in the correct next state ops when under the influence of a feature.

Perl configuration Configuration not relevant. This is the case at Perl 5.37.5 at db2b17365aa663074617cea8c7795ff608c41295

demerphq commented 2 years ago

I pushed @nwc10's patch for this as https://github.com/Perl/perl5/pull/20440

tonycoz commented 1 year ago

This was fixed by #20440