Any comment/review on this patch ? On Sep 3, 2013, at 4:08 PM, Tristan Gingold <ging...@adacore.com> wrote:
> Hi, > > The field state->ehp_region wasn't updated before lowering constructs in the > eh > path of EH_ELSE. As a consequence, __builtin_eh_pointer is lowered to 0 (or > possibly to a wrong region number) in this path. > > The only user of EH_ELSE looks to be trans-mem.c:lower_transaction, and the > consequence of that is a memory leak. > > Furthermore, according to calls.c:flags_from_decl_or_type, the > "transaction_pure" > attribute must be set on the function type, not on the function declaration. > Hence the change to declare __builtin_eh_pointer. > (I don't understand the guard condition to set the attribute for it in tree.c. > Why is 'builtin_decl_explicit_p (BUILT_IN_TM_LOAD_1)' needed in addition to > flag_tm ?) > > No regressions (check-host) on x86-64 GNU/Linux. > > Ok for trunk ? > > Tristan. > > 2013-09-03 Tristan Gingold <ging...@adacore.com> > > * tree.c (set_call_expr_flags): Reject ECF_TM_PURE. > (build_common_builtin_nodes): Set "transaction_pure" > attribute on __builtin_eh_pointer function type (and not on > its declaration). > * tree-eh.c (lower_try_finally_nofallthru): Set and revert > ehp_region before callinf lower_eh_constructs_1. > (lower_try_finally_onedest): Likewise. > (lower_try_finally_copy): Likewise. > (lower_try_finally_switch): Likewise. > > testsuite/ > 2013-09-03 Tristan Gingold <ging...@adacore.com> > > * gcc.dg/tm/except.c: New testcase. > > > diff --git a/gcc/testsuite/gcc.dg/tm/except.c > b/gcc/testsuite/gcc.dg/tm/except.c > new file mode 100644 > index 0000000..ed84bb3 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tm/except.c > @@ -0,0 +1,36 @@ > +/* { dg-do compile } */ > +/* { dg-options "-fgnu-tm -O1 -fexceptions -fdump-tree-optimized" } */ > + > +typedef struct node { > + int val; > + struct node *next; > +} node_t; > + > +node_t *head; > + > +__attribute__((transaction_safe)) > +node_t *new_node(int val, node_t *next); > + > +int add(int val) > +{ > + int result; > + node_t *prev, *next; > + > + __transaction_atomic { > + prev = head; > + next = prev->next; > + while (next->val < val) { > + prev = next; > + next = prev->next; > + } > + result = (next->val != val); > + if (result) { > + prev->next = new_node(val, next); > + } > + } > + return result; > +} > + > +/* { dg-final { scan-tree-dump-not "ITM_commitTransactionEH \\(0B\\)" > "optimized" } } */ > + > +/* { dg-final { cleanup-tree-dump "optimized" } } */ > diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c > index 6ffbd26..86ee77e 100644 > --- a/gcc/tree-eh.c > +++ b/gcc/tree-eh.c > @@ -1093,8 +1093,12 @@ lower_try_finally_nofallthru (struct leh_state *state, > > if (tf->may_throw) > { > + eh_region prev_ehp_region = state->ehp_region; > + > finally = gimple_eh_else_e_body (eh_else); > + state->ehp_region = tf->region; > lower_eh_constructs_1 (state, &finally); > + state->ehp_region = prev_ehp_region; > > emit_post_landing_pad (&eh_seq, tf->region); > gimple_seq_add_seq (&eh_seq, finally); > @@ -1129,6 +1133,7 @@ lower_try_finally_onedest (struct leh_state *state, > struct leh_tf_state *tf) > gimple_stmt_iterator gsi; > tree finally_label; > location_t loc = gimple_location (tf->try_finally_expr); > + eh_region prev_ehp_region = state->ehp_region; > > finally = gimple_try_cleanup (tf->top_p); > tf->top_p_seq = gimple_try_eval (tf->top_p); > @@ -1140,12 +1145,16 @@ lower_try_finally_onedest (struct leh_state *state, > struct leh_tf_state *tf) > if (x) > { > if (tf->may_throw) > - finally = gimple_eh_else_e_body (x); > + { > + state->ehp_region = tf->region; > + finally = gimple_eh_else_e_body (x); > + } > else > finally = gimple_eh_else_n_body (x); > } > > lower_eh_constructs_1 (state, &finally); > + state->ehp_region = prev_ehp_region; > > for (gsi = gsi_start (finally); !gsi_end_p (gsi); gsi_next (&gsi)) > { > @@ -1255,13 +1264,19 @@ lower_try_finally_copy (struct leh_state *state, > struct leh_tf_state *tf) > > if (tf->may_throw) > { > + eh_region prev_ehp_region = state->ehp_region; > + > /* We don't need to copy the EH path of EH_ELSE, > since it is only emitted once. */ > if (eh_else) > - seq = gimple_eh_else_e_body (eh_else); > + { > + seq = gimple_eh_else_e_body (eh_else); > + state->ehp_region = tf->region; > + } > else > seq = lower_try_finally_dup_block (finally, state, tf_loc); > lower_eh_constructs_1 (state, &seq); > + state->ehp_region = prev_ehp_region; > > emit_post_landing_pad (&eh_seq, tf->region); > gimple_seq_add_seq (&eh_seq, seq); > @@ -1432,8 +1447,12 @@ lower_try_finally_switch (struct leh_state *state, > struct leh_tf_state *tf) > { > if (tf->may_throw) > { > + eh_region prev_ehp_region = state->ehp_region; > + > + state->ehp_region = tf->region; > finally = gimple_eh_else_e_body (eh_else); > lower_eh_constructs_1 (state, &finally); > + state->ehp_region = prev_ehp_region; > > emit_post_landing_pad (&eh_seq, tf->region); > gimple_seq_add_seq (&eh_seq, finally); > diff --git a/gcc/tree.c b/gcc/tree.c > index f0ee309..e4be24d 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -9817,9 +9817,11 @@ set_call_expr_flags (tree decl, int flags) > if (flags & ECF_LEAF) > DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("leaf"), > NULL, DECL_ATTRIBUTES (decl)); > - if ((flags & ECF_TM_PURE) && flag_tm) > - DECL_ATTRIBUTES (decl) = tree_cons (get_identifier ("transaction_pure"), > - NULL, DECL_ATTRIBUTES (decl)); > + > + /* The "transaction_pure" attribute must be set on the function type, not > + on the declaration. */ > + gcc_assert (!(flags & ECF_TM_PURE)); > + > /* Looping const or pure is implied by noreturn. > There is currently no way to declare looping const or looping pure > alone. */ > gcc_assert (!(flags & ECF_LOOPING_CONST_OR_PURE) > @@ -10018,8 +10020,9 @@ build_common_builtin_nodes (void) > integer_type_node, NULL_TREE); > ecf_flags = ECF_PURE | ECF_NOTHROW | ECF_LEAF; > /* Only use TM_PURE if we we have TM language support. */ > - if (builtin_decl_explicit_p (BUILT_IN_TM_LOAD_1)) > - ecf_flags |= ECF_TM_PURE; > + if (flag_tm && builtin_decl_explicit_p (BUILT_IN_TM_LOAD_1)) > + TYPE_ATTRIBUTES (ftype) = tree_cons (get_identifier ("transaction_pure"), > + NULL, TYPE_ATTRIBUTES (ftype)); > local_define_builtin ("__builtin_eh_pointer", ftype, BUILT_IN_EH_POINTER, > "__builtin_eh_pointer", ecf_flags); > >