Add SAVEt_PADSV; fixes memory leak in pp_methstart#24254
Conversation
leonerd
force-pushed
the
fix-memoryleak-in-pr24211
branch
from
5928d7b to
726ed73
Compare
| require Config; | ||
|
|
||
| eval { require XS::APItest; XS::APItest->import('sv_count'); 1 } | ||
| or skip_all("XS::APItest not available"); |
There was a problem hiding this comment.
The downside to this is that this entire test file won't run now if XS::APItest can't be imported. Is that okay? (When is that even possible??)
There was a problem hiding this comment.
There's already quite a few tests in t/op/ that do similar things; e.g. grep for "XS::APItest not available". I think we would be in good company this way.
There was a problem hiding this comment.
Actually, that said some of them just make one test block conditional. I'll have a look at doing that.
Edit: Done.
There was a problem hiding this comment.
(When is that even possible??)
Primarily when doing a minitest
leonerd
force-pushed
the
fix-memoryleak-in-pr24211
branch
2 times, most recently
from
32a1652 to
a4ce088
Compare
|
Otherwise looks fine. |
The previous commit here (6b45044) intended to fix `pp_methstart` to allow refaliasing operations on fields. The way it did this had the unintended side-effect of no longer undoing the `SvREFCNT_inc` applied to each field, and thus caused every method call to leak SV references to every field it captured. This change fixes this, by introducing a new savestack operation, `SAVEt_PADSV`. This operation saves the current SV in a pad slot, then on scope exit restores it back by first decrementing the reference count of *whatever SV* is now found in that pad slot. This was the original bug in `pp_methstart` that refaliasing into fields highlighted and the previous commit intended to fix. In addition to the bugfix, this new savestack operation makes it more efficient than the previous code, as it combines into a single operation the two behaviours which previously required two separate ones (SAVEt_SPTR + SAVEt_CLEARSV).
leonerd
force-pushed
the
fix-memoryleak-in-pr24211
branch
from
a4ce088 to
0140fdf
Compare
|
I believe that push fixes the issues Tony raises. I'll leave this a day or so for any other comments. |
|
Do you know of any CPAN distributions which use the |
5 participants
The previous commit here (6b45044) intended to fix
pp_methstartto allow refaliasing operations on fields. The way it did this had the unintended side-effect of no longer undoing theSvREFCNT_incapplied to each field, and thus caused every method call to leak SV references to every field it captured.This change fixes this, by introducing a new savestack operation,
SAVEt_PADSV. This operation saves the current SV in a pad slot, then on scope exit restores it back by first decrementing the reference count of whatever SV is now found in that pad slot. This was the original bug inpp_methstartthat refaliasing into fields highlighted and the previous commit intended to fix.In addition to the bugfix, this new savestack operation makes it more efficient than the previous code, as it combines into a single operation the two behaviours which previously required two separate ones (SAVEt_SPTR + SAVEt_CLEARSV).