-
Notifications
You must be signed in to change notification settings - Fork 577
feature 'class' appears to leak opslabs #20812
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
Comments
I see a leak from LSAN even without an initializer. In the following I've rebuilt perl to add class to the default feature set to reduce noise:
So the e78 op is freed, then allocated again, and it's the first OP realloced. But it isn't freed after that
So it looks like the OP_PAD* op isn't being released. The fieldvar grammar rules fetch the PADNAME for the pad variable but don't seem to do anything with the OP, should they be releasing it? |
Woo. Much thanks @tonycoz - that was indeed the problem. I've now popped the fix in 14f46f2. If that comes up OK I'll squash it into the main #20809 and we can call it done. |
With #20809 merged, most of the leaks are fixed. There's still one more memory leak left, but that's only apparent if a compiletime error causes a field expression inside a class block to fail its out-of-block controlflow test. Compare:
neither of which leak, with
which does. It only affects programs that are syntactically invalid - so it shouldn't be too high impact. In normal circumstances the entire process is about to terminate anyway, so a memory leak here wouldn't be a problem. It would affect any code that doesn't exit on a compilation failure - such as uses of |
When I tried these valgrind invocations as written above, I got a
When I substituted |
I was able to confirm your results on FreeBSD-12.
NOT leaking
Still leaking
|
The op not being freed appears to be the OP_LAST (found using roughly the same method as above.) I expect forbid_outofblock_ops() needs to release the op tree if it's going to croak. I added code to op_free() the OP before the croak in walk_ops_forbid() and that prevented the leak, but that's not a real solution -- the |
Adding the op_free() breaks other things, SAVEFREEOP() is safer, but the similar errors for defer and finally don't appear to need the free, the last OP in |
I think I know roughly what's going on, but don't understand the
recently-added Class code well enough to feel confident fixing it
(especially this new suspend_compcv() malarkey).
First, some background background. When compiling a "normal" sub, perl
does:
start_subparse(); // alloc new CV and set PL_compcv to point to it
SAVEFREESV(PL_compcv);
When ops are allocated while compiling that CV, the ops are allocated from
a chain of slabs. This chain is pointed to by CvSTART(), and CvSLABBED()
is set to indicate that that CvSTART() is temporarily special.
If the CV compiles successfully , CvSLABBED() is turned off and CvSTART()
takes on its normal meaning.
On the other hand if perl croaks while compiling the CV, then eventually
PL_compcv will get freed (due to the SAVEFREESV(PL_compcv)). When freeing
a CV, cv_undef_flags() checks CvSLABBED(sv), and if true, loops through
the chain of ops looking for ops not marked as freed, and frees them. In
this fashion, ops are never lost, even if perl croaks after an op has been
allocated but not yet embedded somewhere. The slabs themselves are then
freed.
The Class stuff goes wrong on two counts. First it doesn't do the
equivalent of SAVEFREESV(PL_compcv) in Perl_class_setup_stash(), so the CV
doesn't get freed on error (or only freed in global cleanup). The fix
isn't as simple as adding in the SAVEFREESV directly after the call to
start_subparse(), because Perl_class_setup_stash() is doing something
weird and does a leave scope a few lines after calling start_subparse()
and suspend_compcv(), which I don't yet understand.
Second, when perl is LEAVEing after croaking from a syntax error or
whatever in the class definition, it does a a SAVEt_DESTRUCTOR_X, which
calls class_seal_stash(), which blithely invokes newATTRSUB(), which
finishes off the CV even though it's only half compiled due to the earlier
syntax error.
Amongst other things, newATTRSUB() turns off CvSLABBED(), ensuring that
any orphaned ops will leak.
So in some fashion ,invoke_class_seal() should avoid sealing the class if
an error has occurred, perhaps by checking if PL_parser->error_count > 0
??
…--
Spock (or Data) is fired from his high-ranking position for not being able
to understand the most basic nuances of about one in three sentences that
anyone says to him.
-- Things That Never Happen in "Star Trek" #19
|
On Sat, Feb 25, 2023 at 02:35:29PM -0800, iabyn wrote:
I think I know roughly what's going on, but don't understand the
recently-added Class code well enough to feel confident fixing it
(especially this new suspend_compcv() malarkey).
Not having seen any update in 9 days, I just want to check that my
implicit assumption was correct: that, having helped diagnose the issue, I
can now sit back and let someone else (i.e. Paul) do any actual fixing?
(Preferably before 5.38.0 given that croak.t currently fails on builds
under ASAN.)
…--
In economics, the exam questions are the same every year.
They just change the answers.
|
Oh indeed. I was on vacation and then post-vacation covid illness, but I'm back now. I'll write a longer reply soon but in summary I think I have this yes. |
This appears to be the cause of the smoke failures on Fedora such as https://www.nntp.perl.org/group/perl.daily-build.reports/2023/05/msg293351.html:
|
This test is known to leak: see GH #20812. Skip it for now so that ASAN smokes don't fail. Start failing again on the next development branch so that the issue isn't forgotten
PR #21104 which I've just created, skips that leaking test for now. I intend to merge it today unless anyone objects. |
This test is known to leak: see GH #20812. Skip it for now so that ASAN smokes don't fail. Start failing again on the next development branch so that the issue isn't forgotten
This test is known to leak: see GH Perl#20812. Skip it for now so that ASAN smokes don't fail. Start failing again on the next development branch so that the issue isn't forgotten
This test is known to leak: see GH Perl#20812. Skip it for now so that ASAN smokes don't fail. Start failing again on the next development branch so that the issue isn't forgotten
If there was anyone else wondering, these specific OPs aren't released because at this point they're not on the parser stack (which is why the similar defer code doesn't leak), nor are they part of the CV's op tree. Since the CV is no longer marked slabbed only the OPs hold "references" to the slabs, but since ops aren't part of the tree, op_free() doesn't find all of the ops to release, hence not all of the references to the slabs are released and so the slabs leak. |
This test is known to leak: see GH Perl#20812. Skip it for now so that ASAN smokes don't fail. Start failing again on the next development branch so that the issue isn't forgotten
Previously if forbid_outofblock_ops() here threw an error the ops from defop would leak, including leaking the slab(s) containing those ops. The other callers to forbid_outofblock_ops() left the ops being checked on the parser stack when performing this check, so the parse stack clean up would release the ops, but the field initaliser code removes the OP from the parse stack before class_set_field_defop() so the OP and its children leaked. To prevent that, populate the defop for the field with the supplied ops before calling forbid_outofblock_ops(), then as the stack rewinds class_seal_stash() will check the error count and free the ops. Fixes Perl#20812
Previously if forbid_outofblock_ops() here threw an error the ops from defop would leak, including leaking the slab(s) containing those ops. The other callers to forbid_outofblock_ops() left the ops being checked on the parser stack when performing this check, so the parse stack clean up would release the ops, but the field initaliser code removes the OP from the parse stack before class_set_field_defop() so the OP and its children leaked. To prevent that, populate the defop for the field with the supplied ops before calling forbid_outofblock_ops(), then as the stack rewinds class_seal_stash() will check the error count and free the ops. Fixes Perl#20812
Previously if forbid_outofblock_ops() here threw an error the ops from defop would leak, including leaking the slab(s) containing those ops. To prevent that, populate the defop for the field with the supplied ops before calling forbid_outofblock_ops(), then as the save stack rewinds class_seal_stash() will check the error count and free the ops. Fixes Perl#20812
Previously if forbid_outofblock_ops() here threw an error the ops from defop would leak, including leaking the slab(s) containing those ops. To prevent that, populate the defop for the field with the supplied ops before calling forbid_outofblock_ops(), then as the save stack rewinds class_seal_stash() will check the error count and free the ops. Fixes #20812
The first merge of feature 'class' turned out to have many memory leaks. Most of them are fixed by #20809.
With that in place most of the more basic tests no longer leak, but any field definition that includes a
:param
attribute or a= EXPR
defaulting expression still does. In both cases the leak is visible invalgrind
as:which suggests it's because an op gets allocated somewhere and isn't released again. But so far I haven't been able to find it.
The text was updated successfully, but these errors were encountered: