Resolve issue #20161: Evals that die under the debugger may or may not leak eval data#20182
Conversation
8074d9b to
f20f3b6
Compare
|
I don't know if this is a good idea.. Now the code will always use What I'm also wondering about: is this the only way to fix it? Skimming the code it appears that before commit f9bddea it first called |
|
@bram-perl I dont think you are right TBH. But I guess @iabyn will have to speak up. He was the one who suggested the CATCH_GET was superfluous after all, and removing it does fix the leaking debug data. I dont think it changes the performance signature at all, that concern relates to execution not compilation. Note that the added overhead is that of adding a call to JMPENV_PUSH()/JMPENV_POP which boils down to some bookkeeping and a call to Perl_procsetjmp(). In the grand scheme of doing an eval that is a tiny thing. Also because this is compilation we push an env and then pop it, they wont cascade except with regard to BEGIN, and that is already a problem that is being tackled in a different PR. Also it doesnt make sense to me, @iabyn added try_yyparse, which is now no longer used. It makes a lot more sense to me that it should be used than it should be there totally unused. |
Just for reference: reason for that is commit d7e3f70. DetailsThat commit added an earlier 'catchable runloop' which resulted in I suspect (but did not verify) that an alternative fix could've been to add a 'catchable runloop' further down in |
|
On Mon, 29 Aug 2022 at 21:31, Bram ***@***.***> wrote:
Also it doesnt make sense to me, @iabyn <https://github.com/iabyn> added
try_yyparse, which is now no longer used. It makes a lot more sense to me
that it should be used than it should be there totally unused.
Just for reference: reason for that is commit d7e3f70
<d7e3f70>.
That commit added an earlier 'catchable runloop' which resulted in
S_doeval_compile to always be called with a false value for CATCH_GET.
I *suspect* (but did not verify) that an alternative fix could've been to
add a 'catchable runloop' further down in S_doeval_compile namely in the
block:
if (PL_unitcheckav) {
OP *es = PL_eval_start;
call_list(PL_scopestack_ix, PL_unitcheckav);
PL_eval_start = es;
}
Alternate fix to d7e3f70 you mean?
BTW, the other thing that makes me thing this is the right thing to do is
the chain of bugs this revealed and fixes as a whole. Every step of the way
fixing this we have fallen into old reverted bugfixes. IMO that suggests
that what we are doing is essentially using a bunch of bugs to cancel each
other out, which further suggests to me we are on the right track. Also,
while admittedly this last point verges on the superstitious, it felt like
a good sign that *all* the windows and cygwin tests passed after these
patches. I have seen so many CI heisenbugs that I found that to be quite
encouraging, I almost wonder if this stuff has been at the root of other
issues and we never realized.
cheers,
Yves
…--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
|
For The test only works correct by accident (imo). |
|
A more general comment. It was suggested on some of my commit messages that it should dereference the issue's name so that the reader doesn't have to each time it is read. That effectively could be to copy/paste. |
ed5460e to
757dc7a
Compare
We can just do the count before and after.
CATCH_GET is never true in this code, so we never called try_yyparse() which in turn meant we leaked debug data from failed evals. With this in place an eval that dies during compile will always be handled by doeval_comp() properly. This includes changes to t/comp/readlines.t so it tests code that croaks during compile, which used to leak and fail test but was not actually tested. This fixes GH Issue #20161.
We test BEGIN{die}, but not UNITCHECK{die}. Lets do both.
757dc7a to
2fac0ab
Compare
Make sure we actually return via pp_evalcomp() when UNITCHECK inside of an eval dies. Thanks to Bram for the work figuring this out.
Also add SAFE_FUNCTION__ which ensures that the value is "UNKNOWN" on platforms that dont support __func__.
1997c0a to
1b569dd
Compare
We were leaking debug data, and not testing if we did. Assuming #20181 is applied, then we can apply this and fix #20161 which will enable use to merge #20168 if we want. This is rebased on top of #20181. Once that is applied only the most recent patch in this sequence will be relevant to this PR.
This patch changes the code to use try_yyparse() when compiling evals, while preserving the existing behavior for require.