Skip to content

eval_sv: add a G_USEHINTS flag #21421

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
Merged
merged 2 commits into from
Aug 31, 2023
Merged

eval_sv: add a G_USEHINTS flag #21421

merged 2 commits into from
Aug 31, 2023

Conversation

tonycoz
Copy link
Contributor

@tonycoz tonycoz commented Aug 24, 2023

Fixes #21415
@tonycoz tonycoz requested a review from leonerd August 24, 2023 01:37
leonerd
leonerd reviewed Aug 24, 2023
View reviewed changes
Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall idea (and name) look fine.

Though I'm unsure about the idea of dropping the "feature/bits" hh key in favour of this reconstruction mechanism.
op.c
@@ -12227,7 +12227,6 @@ Perl_ck_eval(pTHX_ OP *o)
/* Store a copy of %^H that pp_entereval can pick up. */
HV *hh = hv_copy_hints_hv(GvHV(PL_hintgv));
OP *hhop;
STOREFEATUREBITSHH(hh);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, took me a little while to work out why this bit, until I saw that we're now reconstructing the feature bits from the entire hinthash. Is that OK? In general the hh might contain a lot more keys (e.g. other bits of enabled syntax and options), so iterating the whole thing might take a little while, as compared just storing/fetching the UV as we used to.
@tonycoz
features: populate cop_features from a hints hash the hard way
e4a57a5 
I originally optimised populating cop_features for eval by storing
the hints mask in "feature/bits" and then fetching that when
re-populating the hints for eval.

But that has turned out to be too fragile, so iterate over the
possible feature keys and populate cop_features from that.

I could perhaps have avoided this cost by ensuring "feature/bits" was
set where else it was needed, but this code already iterates to build
the hints hash, iterating again doesn't increase the scale of the work
we're doing.
@tonycoz tonycoz force-pushed the 21415-eval_sv-hints branch from 06129df to 37f3e5a Compare August 28, 2023 04:18
@tonycoz
Copy link
Contributor Author

tonycoz commented Aug 28, 2023

I've changed this to iterate over the possible keys instead
@leonerd
Copy link
Contributor

leonerd commented Aug 28, 2023

I'm confused though. Why is all the code necessary to reconstruct the features bits, when you removed the call to the STOREFEATUREBITSHH() macro. I would have thought the original scheme of just storing the bitflags in one extra UV in the HH was the way to do this..? Is there something about it that doesn't work here?
leonerd
leonerd reviewed Aug 28, 2023
View reviewed changes
@leonerd
Copy link
Contributor

leonerd commented Aug 28, 2023

Oh, now I read the commit message in the first of the two commits, that explains the situation more.

In that case this is probably fine, though I think adding a use strict test would be useful too.
@tonycoz tonycoz force-pushed the 21415-eval_sv-hints branch from 37f3e5a to 346e58f Compare August 29, 2023 00:22
@tonycoz
Copy link
Contributor Author

tonycoz commented Aug 29, 2023

Unfortunately the XS::APItest eval_sv() wrapper returns the result count rather than the result, so I can't check for 5, but I can check whether a result is returned.

Which I've done.
leonerd
leonerd approved these changes Aug 29, 2023
View reviewed changes
Copy link
Contributor

@leonerd leonerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@leonerd
Copy link
Contributor

leonerd commented Aug 29, 2023

  • don't forget a perldelta
@tonycoz tonycoz merged commit 9537c88 into Perl:blead Aug 31, 2023
@iabyn
Copy link
Contributor

iabyn commented Sep 2, 2023

ext/XS-APItest/t/call.t is now producing noise on stderr. I'm not confident enough about what exactly is being tested to just add in a 'my $unused ='' etc.

ok 537 - call_sv('d', G_EVAL|G_LIST|G_KEEPERR) - the correct error message
Useless use of string eq in void context at (eval 85) line 1.
ok 538 - don't inherit hints by default (so the eval fails)
ok 539 - inherit hints when requested (so the eval succeeds)
ok 540 - don't inherit hints (strict) by default, so the eval succeeds
Variable "$x" is not imported at (eval 88) line 1.
ok 541 - inherit hints (strict) when requested, so the evail fails
@tonycoz
Copy link
Contributor Author

tonycoz commented Sep 3, 2023

Thanks fixed in e78b109
bulk88
bulk88 reviewed Apr 18, 2025
View reviewed changes
regen/feature.pl
SV **fbsv = hv_fetchs((hh), "feature/bits", FALSE); \\
PL_compiling.cop_features = fbsv ? SvUV(*fbsv) : 0; \\
} STMT_END
#define FETCHFEATUREBITSHH(hh) S_fetch_feature_bits_hh(aTHX_ (hh))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit turned 1 hv_fetch(), into 26 always failing hv_fetch() calls, on a HV* with only 1 key/1 HE* in it, with a key name of "CORE/prevailing_version".
bulk88
bulk88 reviewed Apr 18, 2025
View reviewed changes
regen/feature.pl
SV **svp = hv_fetch(hh, fb->name, (I32)fb->namelen, 0);
if (svp && SvTRUE(*svp))
PL_compiling.cop_features |= fb->mask;
++fb;
Copy link
Contributor

@bulk88 bulk88 Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This while loop has 26 hash key name entries to test/look for. All 26 hash keys, are never found in typical production perl code. The HV* called hh, has only 1 HE*/key in it total called "CORE/prevailing_version". Pretty bad O(n) problems here with this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants
@tonycoz @leonerd @iabyn @bulk88 @rwp0