: Fix use of __builtin_eh_pointer in EH_ELSE

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  <gingold@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  <gingold@adacore.com>

	* gcc.dg/tm/except.c: New testcase.

Any comment/review on this patch ?

On Sep 3, 2013, at 4:08 PM, Tristan Gingold <gingold@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  <gingold@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  <gingold@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);
> 
>

On 09/03/2013 07:08 AM, Tristan Gingold 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 ?)

Clearly these are totally unrelated and should not be in the same patch.

The BUILT_IN_TM_LOAD_1 thing looks like it might have something to do with
non-C front ends, which don't all create the builtins.

> 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;

This doesn't look right.

Does it work to save and restore ehp_region in the two places that
we currently set it instead?


r~