[Patches] [ python-Patches-1355913 ] PEP 341 - Unification of try/except and try/finally
SourceForge.net
noreply at sourceforge.net
Thu Nov 17 07:03:54 CET 2005
Patches item #1355913, was opened at 2005-11-14 00:59
Message generated for change (Comment added) made by krumms
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1355913&group_id=5470
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
>Category: Parser/Compiler
Group: Python 2.5
Status: Open
Resolution: None
Priority: 5
Submitted By: Thomas Lee (krumms)
Assigned to: Reinhold Birkenfeld (birkenfeld)
Summary: PEP 341 - Unification of try/except and try/finally
Initial Comment:
Attached is a patch implementing PEP 341 against HEAD
in subversion.
It includes the following changes:
1. Grammar/Grammar updated as per the PEP
2. Python/ast.c wraps try/except blocks inside
try/finally blocks if it detects the extended syntax.
This patch is based heavily upon suggestions by Nick
Coghlan on python-dev.
----------------------------------------------------------------------
>Comment By: Thomas Lee (krumms)
Date: 2005-11-17 16:03
Message:
Logged In: YES
user_id=315535
lol ... even if it doesn't get accepted, it was a great
learning experience. The PEP itself hasn't even been
approved as yet, I've just wanted this syntax in Python for
a while :)
Anyway ... might try my hand at a few ideas I have for that
arena/pool stuff as discussed on python-dev when I get home
tonight.
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2005-11-17 15:48
Message:
Logged In: YES
user_id=33168
Maybe I'm just tired, but I can't find any problems.
Assuming this patch gets in, it ought to be the least buggy
code in compile.c. :-)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-16 23:52
Message:
Logged In: YES
user_id=315535
You're absolutely right with those bugs Nick. Fixed now. See
how you go with v9 :)
----------------------------------------------------------------------
Comment By: Nick Coghlan (ncoghlan)
Date: 2005-11-16 23:25
Message:
Logged In: YES
user_id=1038590
Closest yet, but I don't think we're quite there yet.
The 'except_st = NULL' part should be on the line
immediately after except_st is inserted into the inner
sequence. As soon as that insertion happens, we want to
ensure that the statement is only freed once. What's
currently there will technically work, but doesn't really
have the right semantics.
More importantly, the "body = handlers = orelse = NULL" part
should only be executed if the call to TryExcept *succeeds*,
not if it fails.
Neil's right - we seriously need to find a better way to
handle this memory management or it will be a source of
significant pain (well, more pain than you've already
experienced ;)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-16 19:23
Message:
Logged In: YES
user_id=315535
Okay, here we go: v8!
*sighs* this is devestating for my ego :)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-16 18:35
Message:
Logged In: YES
user_id=315535
Arrgh! :)
Well ... I'll fix up this patch & submit it again. I might
fall back to the 'goto' style of doing things just so we
don't have the mix of "do ... break ... while" and goto
(which I imagine could be almost as confusing as the mix of
check-cleanup and goto-cleanup as described by Nick
earlier). Again, if you could check the v8 patch for me it
would be much appreciated.
After that, I might hit python-dev and read up on what
everybody's been saying about pools/arenas & maybe get
started on something we can use for the AST ... anyway, I'll
discuss that on python-dev. This isn't really the place for
it :)
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2005-11-16 15:38
Message:
Logged In: YES
user_id=33168
ISTM there are a couple of problems. Some of these were
likely in there before and I may have told you to do some.
:-( I probably made the same error too. :-( :-(
One is a new one though. That starts in the for loop. You
break in there, but that only breaks out of the for loop,
doesn't it? So you really need a goto to break out I think.
After setting except_st, it holds references to body,
handlers, and orelse, so if you free except_st, it should
free all the others. Therefore if except_st was created
(after the if (except_st) break;), you should set body =
handlers = orelse = NULL; Do you agree? I think if you
don't you will get a double free.
The same would go for after you asdl_seq_SET(inner, 0,
except_st); you should set except_st = NULL;
OTOH, you fixed a memory leak. Whenever one does return
TryExcept(); (or any other similar AST call), if TryExcept
fails, we will return NULL and all the local variables will
be leaked.
*sigh* Sorry you have to deal with this crap. You've been
a very good guinea pig being the first. I really hope we do
the arenas as discussed on python-dev. If you aren't too
burnt out, it would be a really good way to learn the rest
of the AST. :-)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-15 23:15
Message:
Logged In: YES
user_id=315535
Here's the v7 patch against revision 41451, with Neal's last
fix :)
Using Marek's suggestion for try/except emulation in C from
python-dev to avoid the "goto" crap.
Any more problems here? *looks hopeful* :)
- Tom
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-15 14:50
Message:
Logged In: YES
user_id=315535
lol thanks Neal/Nick, that's fine. :) Nick Coghlan and
yourself have been extremely helpful throughout this whole
process, and it's been a great learning experience.
I'll implement your suggested fix for 'handlers' right away.
If you like, let me know when you've committed the 700+ line
patch and I'll generate the PEP 341 patch from that.
- Tom
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2005-11-15 14:43
Message:
Logged In: YES
user_id=33168
Oh, so close, I'm sorry, I see one more problem. :-)
Thomas, I hope you will write up this experience in coding
this patch. IMO it clearly demonstrates a problem with the
new AST code that needs to be addressed. ie, Memory
management is not possible to get right. I've got a 700+
line patch to ast.c to correct many more memory issues
(hopefully that won't cause conflicts with this patch). I
would like to hear ideas of how the AST code can be improved
to make it much easier to not leak memory and be safe at the
same time.
The problem is that handlers is not an asdl_seq of stmt's,
but rather an asdl_seq of exceptionhandlers. There is no
utility function to clean out this seq, so you need to do it
manually.
for (i = 0; i < asdl_seq_LEN(handlers); i++)
free_exceptionhandler(asdl_seq_GET(handlers, i));
asdl_seq_free(handlers);
I don't see any other problems, but perhaps I will when it
gets checked in or I run valgrind on it.
The Other Nick :-)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-15 14:10
Message:
Logged In: YES
user_id=315535
Okay, v6 is up. Now using test-goto-cleanup. Also using
asdl_stmt_seq_free.
Any more suggestions/feedback? Anybody having trouble using
this patch?
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 22:14
Message:
Logged In: YES
user_id=315535
Thanks for your input Nick - I'll sort out a patch taking
your suggestions into consideration tomorrow & resubmit it.
----------------------------------------------------------------------
Comment By: Nick Coghlan (ncoghlan)
Date: 2005-11-14 18:54
Message:
Logged In: YES
user_id=1038590
>From a code inspection of v5 of the patch (I haven't
actually run the patch anywhere), I'm a little uncomfortable
with the mixture of the two error handling styles (that is,
test-cleanup-return for most of the function, but
test-goto-cleanup for the one specific case). I don't mind
either style (and both are used in the Python source), but
mixing them in one function bothers me :)
Further, there appears to be a bug in the deallocation code,
in that all sequences should be freed with
adsl_seq_stmt_free. The current direct use of adsl_seq_free
means any contained statements are not released.
I suggest putting a single 'error' label at the end of the
function that uses adsl_seq_stmt_free to clean up all the
statement sequences , as well as free_stmt to clean up the
nested except statements. As both of these functions handle
nulls gracefully, the bodies of the current error cleanup
and return branches can all then be replaced with "goto error".
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 15:18
Message:
Logged In: YES
user_id=315535
Oops - I mean Neal :)
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 15:17
Message:
Logged In: YES
user_id=315535
Fix for the problem you encountered in the v5 patch, Nick.
All tests in the suite now pass for me, no assertion
failures when using --with-pydebug.
I was stupidly generating a TryFinally in every scenario.
This breaks down in the 'trace' unit test when no 'finally'
clause is present with the extended syntax. Oops.
Also added a little more error checking code and updated the
unit test to test nested exception handling.
I'm using a few goto statements in the "if (n_except > 0)"
branch to reduce duplication deallocating memory. If there's
any great objections to this feel free to yell & I'll
resubmit a patch without the use of goto.
Can you please verify this works for you, Nick?
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 10:02
Message:
Logged In: YES
user_id=315535
Fixed a bad assumption when parsing a try-finally. Should be
fixed now.
I'm not sure if this fixes the problem you're experiencing,
Neal. I'll have a look into it.
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 09:12
Message:
Logged In: YES
user_id=315535
Further correction of memory leaks.
'finally' and 'orelse' need to be deallocated if an error
occurs in the "if (n_except > 0)" branch.
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 09:12
Message:
Logged In: YES
user_id=315535
Further correction of memory leaks.
'finally' and 'orelse' need to be deallocated if an error
occurs in the "if (n_except > 0)" branch.
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2005-11-14 09:10
Message:
Logged In: YES
user_id=33168
I'm pretty sure this patch causes this problem:
python: Objects/frameobject.c:187: frame_setlineno:
Assertion `blockstack_top > 0' failed.
I'm not sure if it's just my hacked up version or the
original. Thomas, can you test your version?
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2005-11-14 07:27
Message:
Logged In: YES
user_id=33168
Updated patch to correct memory leaks. Put everything in
one file. Works for me.
There needs to be doc changes done too.
----------------------------------------------------------------------
Comment By: Thomas Lee (krumms)
Date: 2005-11-14 01:02
Message:
Logged In: YES
user_id=315535
Attaching a unit test checking the various different types
of exception handling syntax.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1355913&group_id=5470
More information about the Patches
mailing list