From: Rico Tzschichholz Date: Wed, 23 Jan 2019 21:39:42 +0000 (+0100) Subject: codegen: Properly handle and catch inner-error of finally-block X-Git-Tag: 0.43.90~13 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fmerge-requests%2F47%2Fhead;p=thirdparty%2Fvala.git codegen: Properly handle and catch inner-error of finally-block If all inner-errors are caught there is no jump-out of the current finally- block therefore the control-flow analyzer is happy. To make this work the surrounding inner-error must not be used or changed here. Fixes https://gitlab.gnome.org/GNOME/vala/issues/742 --- diff --git a/codegen/valaccodebasemodule.vala b/codegen/valaccodebasemodule.vala index 32f42c992..de0bae868 100644 --- a/codegen/valaccodebasemodule.vala +++ b/codegen/valaccodebasemodule.vala @@ -36,6 +36,7 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { public ArrayList ccode_stack = new ArrayList (); public ArrayList temp_ref_values = new ArrayList (); public int next_temp_var_id; + public int current_inner_error_id; public bool current_method_inner_error; public bool current_method_return; public int next_coroutine_state = 1; @@ -81,6 +82,11 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { set { emit_context.current_catch = value; } } + public int current_inner_error_id { + get { return emit_context.current_inner_error_id; } + set { emit_context.current_inner_error_id = value; } + } + public TypeSymbol? current_type_symbol { get { var sym = current_symbol; @@ -273,6 +279,9 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { int next_block_id = 0; Map block_map = new HashMap (); + /* count of emitted inner_error variables in methods */ + Map method_inner_error_var_count = new HashMap (); + public DataType void_type = new VoidType (); public DataType bool_type; public DataType char_type; @@ -1914,7 +1923,7 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { } if (current_method_inner_error) { - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } cfile.add_function (function); @@ -1993,6 +2002,24 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { ccode.open_block (); } + if (b.parent_node is TryStatement && b == ((TryStatement) b.parent_node).finally_body) { + // finally-block with nested error handling + if (((TryStatement) b.parent_node).body.tree_can_fail) { + if (is_in_coroutine ()) { + // _inner_error0_ gets added to closure_struct in CCodeMethodModule.visit_method() + if (current_inner_error_id > 0 + && (!method_inner_error_var_count.contains (current_method) + || method_inner_error_var_count.get (current_method) < current_inner_error_id)) { + // no initialization necessary, closure struct is zeroed + closure_struct.add_field ("GError*", "_inner_error%d_".printf (current_inner_error_id)); + method_inner_error_var_count.set (current_method, current_inner_error_id); + } + } else { + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); + } + } + } + if (b.captured) { var parent_block = next_closure_block (b.parent_symbol); @@ -2369,6 +2396,10 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { return get_cexpression ("self"); } + public CCodeExpression get_inner_error_cexpression () { + return get_cexpression ("_inner_error%d_".printf (current_inner_error_id)); + } + public string get_local_cname (LocalVariable local) { var cname = get_variable_cname (local.name); if (cname[0].isdigit ()) { @@ -4926,7 +4957,7 @@ public abstract class Vala.CCodeBaseModule : CodeGenerator { // method can fail current_method_inner_error = true; // add &inner_error before the ellipsis arguments - out_arg_map.set (get_param_pos (-1), new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + out_arg_map.set (get_param_pos (-1), new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); } if (ellipsis) { diff --git a/codegen/valaccodemethodcallmodule.vala b/codegen/valaccodemethodcallmodule.vala index 3cca7771b..aa06395ed 100644 --- a/codegen/valaccodemethodcallmodule.vala +++ b/codegen/valaccodemethodcallmodule.vala @@ -631,7 +631,7 @@ public class Vala.CCodeMethodCallModule : CCodeAssignmentModule { // method can fail current_method_inner_error = true; // add &inner_error before the ellipsis arguments - out_arg_map.set (get_param_pos (get_ccode_error_pos ((Callable) m ?? deleg)), new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + out_arg_map.set (get_param_pos (get_ccode_error_pos ((Callable) m ?? deleg)), new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); } else if (m != null && m.has_error_type_parameter () && async_call != ccall) { // inferred error argument from base method out_arg_map.set (get_param_pos (get_ccode_error_pos (m)), new CCodeConstant ("NULL")); diff --git a/codegen/valaccodemethodmodule.vala b/codegen/valaccodemethodmodule.vala index fcf12def3..4adf1f4b3 100644 --- a/codegen/valaccodemethodmodule.vala +++ b/codegen/valaccodemethodmodule.vala @@ -756,11 +756,10 @@ public abstract class Vala.CCodeMethodModule : CCodeStructModule { * as error may be set to NULL but we're always interested in inner errors */ if (m.coroutine) { - closure_struct.add_field ("GError *", "_inner_error_"); - // no initialization necessary, closure struct is zeroed + closure_struct.add_field ("GError*", "_inner_error%d_".printf (current_inner_error_id)); } else { - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } } diff --git a/codegen/valagdbusclientmodule.vala b/codegen/valagdbusclientmodule.vala index 53dd6b07e..8961af672 100644 --- a/codegen/valagdbusclientmodule.vala +++ b/codegen/valagdbusclientmodule.vala @@ -321,7 +321,7 @@ public class Vala.GDBusClientModule : GDBusModule { var ccall = new CCodeFunctionCall (new CCodeIdentifier ("g_async_initable_new_finish")); ccall.add_argument (new CCodeCastExpression (source_ref, "GAsyncInitable *")); ccall.add_argument (get_cvalue (res)); - ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); var temp_var = get_temp_variable (expr.value_type, expr.value_type.value_owned); var temp_ref = get_variable_cexpression (temp_var.name); @@ -376,7 +376,7 @@ public class Vala.GDBusClientModule : GDBusModule { ccall.add_argument (get_delegate_target (callback)); } } else { - ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); } ccall.add_argument (new CCodeConstant ("\"g-flags\"")); ccall.add_argument (get_cvalue (flags)); @@ -418,7 +418,7 @@ public class Vala.GDBusClientModule : GDBusModule { ccall.add_argument (new CCodeCastExpression (new CCodeMemberAccess.pointer (new CCodeIdentifier ("_data_"), "_source_object_"), "GAsyncInitable *")); // pass GAsyncResult stored in closure to finish function ccall.add_argument (new CCodeMemberAccess.pointer (new CCodeIdentifier ("_data_"), "_res_")); - ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + ccall.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); } else { // begin ccode.add_expression (ccall); diff --git a/codegen/valagdbusservermodule.vala b/codegen/valagdbusservermodule.vala index 904e04502..0d5a323a4 100644 --- a/codegen/valagdbusservermodule.vala +++ b/codegen/valagdbusservermodule.vala @@ -1022,7 +1022,7 @@ public class Vala.GDBusServerModule : GDBusClientModule { cregister.add_argument (get_cvalue (obj_arg)); cregister.add_argument (get_cvalue (ma.inner)); cregister.add_argument (get_cvalue (path_arg)); - cregister.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + cregister.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); if (expr.parent_node is ExpressionStatement) { ccode.add_expression (cregister); diff --git a/codegen/valagerrormodule.vala b/codegen/valagerrormodule.vala index d29d09ca1..6855377c8 100644 --- a/codegen/valagerrormodule.vala +++ b/codegen/valagerrormodule.vala @@ -89,7 +89,7 @@ public class Vala.GErrorModule : CCodeDelegateModule { public override void visit_throw_statement (ThrowStatement stmt) { // method will fail current_method_inner_error = true; - ccode.add_assignment (get_variable_cexpression ("_inner_error_"), get_cvalue (stmt.error_expression)); + ccode.add_assignment (get_inner_error_cexpression (), get_cvalue (stmt.error_expression)); add_simple_check (stmt, true); } @@ -170,13 +170,11 @@ public class Vala.GErrorModule : CCodeDelegateModule { public override void add_simple_check (CodeNode node, bool always_fails = false) { current_method_inner_error = true; - var inner_error = get_variable_cexpression ("_inner_error_"); - if (always_fails) { // inner_error is always set, avoid unnecessary if statement // eliminates C warnings } else { - var ccond = new CCodeBinaryExpression (CCodeBinaryOperator.INEQUALITY, inner_error, new CCodeConstant ("NULL")); + var ccond = new CCodeBinaryExpression (CCodeBinaryOperator.INEQUALITY, get_inner_error_cexpression (), new CCodeConstant ("NULL")); var unlikely = new CCodeFunctionCall (new CCodeIdentifier ("G_UNLIKELY")); unlikely.add_argument (ccond); ccode.open_if (unlikely); @@ -222,7 +220,7 @@ public class Vala.GErrorModule : CCodeDelegateModule { if (catch_type.error_code != null) { /* catch clause specifies a specific error code */ var error_match = new CCodeFunctionCall (new CCodeIdentifier ("g_error_matches")); - error_match.add_argument (inner_error); + error_match.add_argument (get_inner_error_cexpression ()); error_match.add_argument (new CCodeIdentifier (get_ccode_upper_case_name (catch_type.data_type))); error_match.add_argument (new CCodeIdentifier (get_ccode_name (catch_type.error_code))); @@ -230,7 +228,7 @@ public class Vala.GErrorModule : CCodeDelegateModule { } else { /* catch clause specifies a full error domain */ var ccond = new CCodeBinaryExpression (CCodeBinaryOperator.EQUALITY, - new CCodeMemberAccess.pointer (inner_error, "domain"), new CCodeIdentifier + new CCodeMemberAccess.pointer (get_inner_error_cexpression (), "domain"), new CCodeIdentifier (get_ccode_upper_case_name (clause.error_type.data_type))); ccode.open_if (ccond); @@ -256,7 +254,7 @@ public class Vala.GErrorModule : CCodeDelegateModule { // as jump out of finally block is not supported } else { // should never happen with correct bindings - uncaught_error_statement (inner_error, true); + uncaught_error_statement (get_inner_error_cexpression (), true); } } else if (current_method != null && current_method.tree_can_fail) { // current method can fail, propagate error @@ -273,7 +271,7 @@ public class Vala.GErrorModule : CCodeDelegateModule { // Check the allowed error domains to propagate var domain_check = new CCodeBinaryExpression (CCodeBinaryOperator.EQUALITY, new CCodeMemberAccess.pointer - (inner_error, "domain"), new CCodeIdentifier (get_ccode_upper_case_name (error_type.data_type))); + (get_inner_error_cexpression (), "domain"), new CCodeIdentifier (get_ccode_upper_case_name (error_type.data_type))); if (ccond == null) { ccond = domain_check; } else { @@ -283,16 +281,16 @@ public class Vala.GErrorModule : CCodeDelegateModule { if (ccond != null) { ccode.open_if (ccond); - return_with_exception (inner_error); + return_with_exception (get_inner_error_cexpression ()); ccode.add_else (); - uncaught_error_statement (inner_error); + uncaught_error_statement (get_inner_error_cexpression ()); ccode.close (); } else { - return_with_exception (inner_error); + return_with_exception (get_inner_error_cexpression ()); } } else { - uncaught_error_statement (inner_error); + uncaught_error_statement (get_inner_error_cexpression ()); } if (!always_fails) { @@ -332,7 +330,11 @@ public class Vala.GErrorModule : CCodeDelegateModule { ccode.add_label ("__finally%d".printf (this_try_id)); if (stmt.finally_body != null) { + // use a dedicated inner_error variable, if there + // is some error handling happening in finally-block + current_inner_error_id++; stmt.finally_body.emit (this); + current_inner_error_id--; } // check for errors not handled by this try statement @@ -354,14 +356,14 @@ public class Vala.GErrorModule : CCodeDelegateModule { if (clause.error_variable != null) { visit_local_variable (clause.error_variable); - ccode.add_assignment (get_variable_cexpression (get_local_cname (clause.error_variable)), get_variable_cexpression ("_inner_error_")); + ccode.add_assignment (get_variable_cexpression (get_local_cname (clause.error_variable)), get_inner_error_cexpression ()); } else { // error object is not used within catch statement, clear it var cclear = new CCodeFunctionCall (new CCodeIdentifier ("g_clear_error")); - cclear.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_variable_cexpression ("_inner_error_"))); + cclear.add_argument (new CCodeUnaryExpression (CCodeUnaryOperator.ADDRESS_OF, get_inner_error_cexpression ())); ccode.add_expression (cclear); } - ccode.add_assignment (get_variable_cexpression ("_inner_error_"), new CCodeConstant ("NULL")); + ccode.add_assignment (get_inner_error_cexpression (), new CCodeConstant ("NULL")); clause.body.emit (this); diff --git a/codegen/valagobjectmodule.vala b/codegen/valagobjectmodule.vala index 05e45e35d..b2adc9a6f 100644 --- a/codegen/valagobjectmodule.vala +++ b/codegen/valagobjectmodule.vala @@ -533,7 +533,7 @@ public class Vala.GObjectModule : GTypeModule { /* always separate error parameter and inner_error local variable * as error may be set to NULL but we're always interested in inner errors */ - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } if (cl.is_singleton) { @@ -575,7 +575,7 @@ public class Vala.GObjectModule : GTypeModule { /* always separate error parameter and inner_error local variable * as error may be set to NULL but we're always interested in inner errors */ - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } pop_context (); @@ -597,7 +597,7 @@ public class Vala.GObjectModule : GTypeModule { /* always separate error parameter and inner_error local variable * as error may be set to NULL but we're always interested in inner errors */ - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } pop_context (); diff --git a/codegen/valagtypemodule.vala b/codegen/valagtypemodule.vala index 3274b904d..af53b8370 100644 --- a/codegen/valagtypemodule.vala +++ b/codegen/valagtypemodule.vala @@ -1815,7 +1815,7 @@ public class Vala.GTypeModule : GErrorModule { cl.destructor.body.emit (this); if (current_method_inner_error) { - ccode.add_declaration ("GError *", new CCodeVariableDeclarator.zero ("_inner_error_", new CCodeConstant ("NULL"))); + ccode.add_declaration ("GError*", new CCodeVariableDeclarator.zero ("_inner_error%d_".printf (current_inner_error_id), new CCodeConstant ("NULL"))); } if (current_method_return) { diff --git a/tests/Makefile.am b/tests/Makefile.am index a60f77f88..27f18f030 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -365,6 +365,7 @@ TESTS = \ objects/bug795225-4.test \ objects/bug795521.vala \ errors/catch-error-code.vala \ + errors/catch-in-finally.vala \ errors/errors.vala \ errors/errorcode.vala \ errors/errordomain.vala \ @@ -414,6 +415,7 @@ TESTS = \ asynchronous/bug792942.vala \ asynchronous/bug793158.vala \ asynchronous/catch-error-scope.vala \ + asynchronous/catch-in-finally.vala \ asynchronous/closures.vala \ asynchronous/generator.vala \ asynchronous/out-parameter-invalid.test \ diff --git a/tests/asynchronous/catch-in-finally.vala b/tests/asynchronous/catch-in-finally.vala new file mode 100644 index 000000000..9f0f95226 --- /dev/null +++ b/tests/asynchronous/catch-in-finally.vala @@ -0,0 +1,72 @@ +errordomain FooError { + FAIL +} + +async void fail () throws FooError { + throw new FooError.FAIL ("fail"); +} + +async void may_fail () throws FooError { +} + +async void foo () throws FooError { + try { + yield fail (); + } finally { + try { + yield may_fail (); + } catch (FooError e) { + assert_not_reached (); + } + } + assert_not_reached (); +} + +async void bar () throws FooError { + try { + yield may_fail (); + } finally { + try { + yield fail (); + } catch (FooError e) { + } + } + + try { + yield fail (); + } finally { + try { + yield may_fail (); + } catch (FooError e) { + assert_not_reached (); + } + } + assert_not_reached (); +} + +async void run () { + foo.begin ((o,r) => { + try { + foo.end (r); + } catch (FooError e) { + assert (e.message == "fail"); + } + }); + + bar.begin ((o,r) => { + try { + bar.end (r); + } catch (FooError e) { + assert (e.message == "fail"); + loop.quit (); + } + }); +} + +MainLoop loop; + +void main () { + loop = new MainLoop (); + run.begin (); + loop.run (); +} diff --git a/tests/errors/catch-in-finally.vala b/tests/errors/catch-in-finally.vala new file mode 100644 index 000000000..8845e4b6b --- /dev/null +++ b/tests/errors/catch-in-finally.vala @@ -0,0 +1,59 @@ +errordomain FooError { + FAIL +} + +void fail () throws FooError { + throw new FooError.FAIL ("fail"); +} + +void may_fail () throws FooError { +} + +void foo () throws FooError { + try { + fail (); + } finally { + try { + may_fail (); + } catch (FooError e) { + assert_not_reached (); + } + } + assert_not_reached (); +} + +void bar () throws FooError { + try { + may_fail (); + } finally { + try { + fail (); + } catch (FooError e) { + } + } + + try { + fail (); + } finally { + try { + may_fail (); + } catch (FooError e) { + assert_not_reached (); + } + } + assert_not_reached (); +} + +void main () { + try { + foo (); + } catch (FooError e) { + assert (e.message == "fail"); + } + + try { + bar (); + } catch (FooError e) { + assert (e.message == "fail"); + } +}