diff mbox

missing COND_EXPR case in walk_stmt_load_store_addr_ops

Message ID AANLkTinXNMYG8125PWMFSwec-3n7Umg0xBpggQPaDqjK@mail.gmail.com
State New
Headers show

Commit Message

Zbigniew Chamski Aug. 16, 2010, 2:21 p.m. UTC 
Hi Richard and all,

I don't have a general (pure trunk-based and portable) test case yet.
Given the current compilation flow, such a test case would require a
way to trigger 'execute_update_address_taken' in late GIMPLE passes.
The only way to do it is to have a portable builtin which is folded
late and triggers the creation of new statements (cf.
execute_fold_all_builtins() in tree-ssa-ccp.c and
update_call_from_tree() in tree-ssa-propagate.c).  Any other
suggestions?

BTW, there was a glitch in my original patch: the second arg of
'visit_addr()' calls in 'stmt_visit_select_addr_ops()' should have
been wrapped in TREE_OPERAND(..., 0)   Proof that this thing needs a
proper regression test...

Here's the corrected patch (retested on my experimental build and on
the r163224 trunk).  I changed the leading comment to emphasize that
it's the operand of ADDR_EXPR that is supplied to 'visit_addr()'.

   Zbigniew




2010/8/16 Richard Guenther <richard.guenther@gmail.com>:
> On Mon, Aug 16, 2010 at 1:57 PM, Zbigniew Chamski
> <zbigniew.chamski@gmail.com> wrote:
>> All,
>>
>> walk_stmt_load_store_ops does not handle the case of COND_EXPR on the
>> RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of
>> the form
>>
>>  v = [cond_expr] a RELOP b : x ? y;
>>
>> This may result in an ICE at SSA verification time if/when the
>> ADDR_EXPRs inside the selection expression are not taken into account.
>>  There is a corner case in libiberty (plus-demangle-template() in
>> cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable
>> 'pattern' ends up in a selection expression.  A call to
>> 'execute_update_addresses_taken()' will miss it, resulting in an
>> "address taken, but ADDRESSABLE bit not set" error at SSA verification
>> time.  This normally goes unnoticed because there are no calls to
>> 'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear
>> on the RHS much later, during LIM.  However, the bug will be triggered
>> if/when execute_fold_all_builtins  (or a custom pass) alters data
>> addressing and sets TODO_update_address_taken.  In my case it was
>> uncovered by a custom pass.
>>
>> The root cause is in the logic of 'walk_stmt_load_store_ops' which
>> lacks the COND_EXPR case for simple assigns.  A proposed patch is
>> below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu.
>>
>> Comments welcome!
>
> The patch is ok for trunk and the 4.5 branch (do you have a testcase?).
>
> Thanks,
> Richard.
>
>>    Zbigniew
>>
>> Index: gcc/gimple.c
>> ===================================================================
>> --- gcc/gimple.c        (revision 163224)
>> +++ gcc/gimple.c        (working copy)
>> @@ -4746,6 +4746,45 @@
>>   return NULL_TREE;
>>  }
>>
>> +
>> +/* For an assignment STMT which contains a selection expression on the RHS,
>> +   i.e.,
>> +
>> +     lhs = [cond_expr] a RELOP b ? x : y;
>> +
>> +   search the entire selection expression for occurrences of ADDR_EXPR, and
>> +   if found, apply VISIT_ADDR callback on STMT, the corresponding ADDR_EXPR
>> +   operand and DATA.  Return TRUE if any of the callback invocations
>> +   returned TRUE.  */
>> +
>> +static inline bool
>> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
>> +                           bool (*visit_addr)(gimple, tree, void *))
>> +{
>> +  bool ret = false;
>> +  tree condop1, condop2;
>> +  tree trueval, falseval;
>> +
>> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
>> +  if (TREE_CODE (condop1) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, condop1, data);
>> +
>> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 1), 0);
>> +  if (TREE_CODE (condop2) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, condop2, data);
>> +
>> +  trueval = TREE_OPERAND (select_expr, 1);
>> +  if (TREE_CODE (trueval) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, trueval, data);
>> +
>> +  falseval = TREE_OPERAND (select_expr, 2);
>> +  if (TREE_CODE (falseval) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, falseval, data);
>> +
>> +  return ret;
>> +}
>> +
>> +
>>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>>    VISIT_ADDR if non-NULL on loads, store and address-taken operands
>>    passing the STMT, the base of the operand and DATA to it.  The base
>> @@ -4761,6 +4800,7 @@
>>  {
>>   bool ret = false;
>>   unsigned i;
>> +
>>   if (gimple_assign_single_p (stmt))
>>     {
>>       tree lhs, rhs;
>> @@ -4785,6 +4825,9 @@
>>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>>                                                   0), data);
>> +         else if (TREE_CODE (rhs) == COND_EXPR)
>> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
>> +
>>           lhs = gimple_assign_lhs (stmt);
>>          if (TREE_CODE (lhs) == TARGET_MEM_REF
>>               && TMR_BASE (lhs) != NULL_TREE
>>
>

Comments

Richard Biener Aug. 16, 2010, 3:24 p.m. UTC | #1 
2010/8/16 Zbigniew Chamski <zbigniew.chamski@gmail.com>:
> Hi Richard and all,
>
> I don't have a general (pure trunk-based and portable) test case yet.
> Given the current compilation flow, such a test case would require a
> way to trigger 'execute_update_address_taken' in late GIMPLE passes.
> The only way to do it is to have a portable builtin which is folded
> late and triggers the creation of new statements (cf.
> execute_fold_all_builtins() in tree-ssa-ccp.c and
> update_call_from_tree() in tree-ssa-propagate.c).  Any other
> suggestions?
>
> BTW, there was a glitch in my original patch: the second arg of
> 'visit_addr()' calls in 'stmt_visit_select_addr_ops()' should have
> been wrapped in TREE_OPERAND(..., 0)   Proof that this thing needs a
> proper regression test...
>
> Here's the corrected patch (retested on my experimental build and on
> the r163224 trunk).  I changed the leading comment to emphasize that
> it's the operand of ADDR_EXPR that is supplied to 'visit_addr()'.

Hm, in the COND_EXPR the first operand could be a plain SSA_NAME
or an ADDR_EXPR as well, not only a comparison.  So you need to
check if TREE_CODE_CLASS is tcc_comparison before recursing
into its operands and handle ADDR_EXPR there, too.

Richard.

>   Zbigniew
>
> Index: gcc/gimple.c
> ===================================================================
> --- gcc/gimple.c        (revision 163224)
> +++ gcc/gimple.c        (working copy)
> @@ -4746,6 +4746,45 @@
>   return NULL_TREE;
>  }
>
> +
> +/* For an assignment STMT which contains a selection expression on the RHS,
> +   i.e.,
> +
> +     lhs = [cond_expr] a RELOP b ? x : y;
> +
> +   search the entire selection expression for occurrences of ADDR_EXPR, and
> +   if found, apply VISIT_ADDR callback on STMT, the corresponding operand
> +   of ADDR_EXPR and DATA.  Return TRUE if any of the callback invocations
> +   returned TRUE.  */
> +
> +static inline bool
> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
> +                           bool (*visit_addr)(gimple, tree, void *))
> +{
> +  bool ret = false;
> +  tree condop1, condop2;
> +  tree trueval, falseval;
> +
> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
> +  if (TREE_CODE (condop1) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, TREE_OPERAND (condop1, 0), data);
> +
> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 1);
> +  if (TREE_CODE (condop2) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, TREE_OPERAND (condop2, 0), data);
> +
> +  trueval = TREE_OPERAND (select_expr, 1);
> +  if (TREE_CODE (trueval) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, TREE_OPERAND (trueval, 0), data);
> +
> +  falseval = TREE_OPERAND (select_expr, 2);
> +  if (TREE_CODE (falseval) == ADDR_EXPR)
> +    ret |= visit_addr (stmt, TREE_OPERAND (falseval, 0), data);
> +
> +  return ret;
> +}
> +
> +
>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>    VISIT_ADDR if non-NULL on loads, store and address-taken operands
>    passing the STMT, the base of the operand and DATA to it.  The base
> @@ -4761,6 +4800,7 @@
>  {
>   bool ret = false;
>   unsigned i;
> +
>   if (gimple_assign_single_p (stmt))
>     {
>       tree lhs, rhs;
> @@ -4785,6 +4825,9 @@
>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>                                                   0), data);
> +         else if (TREE_CODE (rhs) == COND_EXPR)
> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
> +
>           lhs = gimple_assign_lhs (stmt);
>          if (TREE_CODE (lhs) == TARGET_MEM_REF
>               && TMR_BASE (lhs) != NULL_TREE
>
>
>
> 2010/8/16 Richard Guenther <richard.guenther@gmail.com>:
>> On Mon, Aug 16, 2010 at 1:57 PM, Zbigniew Chamski
>> <zbigniew.chamski@gmail.com> wrote:
>>> All,
>>>
>>> walk_stmt_load_store_ops does not handle the case of COND_EXPR on the
>>> RHS of a GIMPLE assign statement, i.e., in 'selection assignments' of
>>> the form
>>>
>>>  v = [cond_expr] a RELOP b : x ? y;
>>>
>>> This may result in an ICE at SSA verification time if/when the
>>> ADDR_EXPRs inside the selection expression are not taken into account.
>>>  There is a corner case in libiberty (plus-demangle-template() in
>>> cplus-dem.c) where the only occurrence of an ADDR_TAKEN on variable
>>> 'pattern' ends up in a selection expression.  A call to
>>> 'execute_update_addresses_taken()' will miss it, resulting in an
>>> "address taken, but ADDRESSABLE bit not set" error at SSA verification
>>> time.  This normally goes unnoticed because there are no calls to
>>> 'execute_update_addresses_taken()' after SRA and the COND_EXPRs appear
>>> on the RHS much later, during LIM.  However, the bug will be triggered
>>> if/when execute_fold_all_builtins  (or a custom pass) alters data
>>> addressing and sets TODO_update_address_taken.  In my case it was
>>> uncovered by a custom pass.
>>>
>>> The root cause is in the logic of 'walk_stmt_load_store_ops' which
>>> lacks the COND_EXPR case for simple assigns.  A proposed patch is
>>> below, bootstrapped and tested (C & C++) on x86_64-unknown-linux-gnu.
>>>
>>> Comments welcome!
>>
>> The patch is ok for trunk and the 4.5 branch (do you have a testcase?).
>>
>> Thanks,
>> Richard.
>>
>>>    Zbigniew
>>>
>>> Index: gcc/gimple.c
>>> ===================================================================
>>> --- gcc/gimple.c        (revision 163224)
>>> +++ gcc/gimple.c        (working copy)
>>> @@ -4746,6 +4746,45 @@
>>>   return NULL_TREE;
>>>  }
>>>
>>> +
>>> +/* For an assignment STMT which contains a selection expression on the RHS,
>>> +   i.e.,
>>> +
>>> +     lhs = [cond_expr] a RELOP b ? x : y;
>>> +
>>> +   search the entire selection expression for occurrences of ADDR_EXPR, and
>>> +   if found, apply VISIT_ADDR callback on STMT, the corresponding ADDR_EXPR
>>> +   operand and DATA.  Return TRUE if any of the callback invocations
>>> +   returned TRUE.  */
>>> +
>>> +static inline bool
>>> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
>>> +                           bool (*visit_addr)(gimple, tree, void *))
>>> +{
>>> +  bool ret = false;
>>> +  tree condop1, condop2;
>>> +  tree trueval, falseval;
>>> +
>>> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
>>> +  if (TREE_CODE (condop1) == ADDR_EXPR)
>>> +    ret |= visit_addr (stmt, condop1, data);
>>> +
>>> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 1), 0);
>>> +  if (TREE_CODE (condop2) == ADDR_EXPR)
>>> +    ret |= visit_addr (stmt, condop2, data);
>>> +
>>> +  trueval = TREE_OPERAND (select_expr, 1);
>>> +  if (TREE_CODE (trueval) == ADDR_EXPR)
>>> +    ret |= visit_addr (stmt, trueval, data);
>>> +
>>> +  falseval = TREE_OPERAND (select_expr, 2);
>>> +  if (TREE_CODE (falseval) == ADDR_EXPR)
>>> +    ret |= visit_addr (stmt, falseval, data);
>>> +
>>> +  return ret;
>>> +}
>>> +
>>> +
>>>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>>>    VISIT_ADDR if non-NULL on loads, store and address-taken operands
>>>    passing the STMT, the base of the operand and DATA to it.  The base
>>> @@ -4761,6 +4800,7 @@
>>>  {
>>>   bool ret = false;
>>>   unsigned i;
>>> +
>>>   if (gimple_assign_single_p (stmt))
>>>     {
>>>       tree lhs, rhs;
>>> @@ -4785,6 +4825,9 @@
>>>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>>>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>>>                                                   0), data);
>>> +         else if (TREE_CODE (rhs) == COND_EXPR)
>>> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
>>> +
>>>           lhs = gimple_assign_lhs (stmt);
>>>          if (TREE_CODE (lhs) == TARGET_MEM_REF
>>>               && TMR_BASE (lhs) != NULL_TREE
>>>
>>
>
>
>
> --
> dr Zbigniew Chamski - Infrasoft IT Solutions
> ul. Oskara Kolberga 33, 09-407 PŁOCK, Poland
> mobile: +48 668 773 916, phone: +48 24 262 0166
> NIP 526-286-69-21, REGON: 141082454
>
Zbigniew Chamski Aug. 16, 2010, 3:28 p.m. UTC | #2 
> Hm, in the COND_EXPR the first operand could be a plain SSA_NAME
> or an ADDR_EXPR as well, not only a comparison.  So you need to
> check if TREE_CODE_CLASS is tcc_comparison before recursing
> into its operands and handle ADDR_EXPR there, too.

Good point.  Will fix that - and work on the test case as well.

   Zbigniew

>> Index: gcc/gimple.c
>> ===================================================================
>> --- gcc/gimple.c        (revision 163224)
>> +++ gcc/gimple.c        (working copy)
>> @@ -4746,6 +4746,45 @@
>>   return NULL_TREE;
>>  }
>>
>> +
>> +/* For an assignment STMT which contains a selection expression on the RHS,
>> +   i.e.,
>> +
>> +     lhs = [cond_expr] a RELOP b ? x : y;
>> +
>> +   search the entire selection expression for occurrences of ADDR_EXPR, and
>> +   if found, apply VISIT_ADDR callback on STMT, the corresponding operand
>> +   of ADDR_EXPR and DATA.  Return TRUE if any of the callback invocations
>> +   returned TRUE.  */
>> +
>> +static inline bool
>> +stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
>> +                           bool (*visit_addr)(gimple, tree, void *))
>> +{
>> +  bool ret = false;
>> +  tree condop1, condop2;
>> +  tree trueval, falseval;
>> +
>> +  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
>> +  if (TREE_CODE (condop1) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, TREE_OPERAND (condop1, 0), data);
>> +
>> +  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 1);
>> +  if (TREE_CODE (condop2) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, TREE_OPERAND (condop2, 0), data);
>> +
>> +  trueval = TREE_OPERAND (select_expr, 1);
>> +  if (TREE_CODE (trueval) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, TREE_OPERAND (trueval, 0), data);
>> +
>> +  falseval = TREE_OPERAND (select_expr, 2);
>> +  if (TREE_CODE (falseval) == ADDR_EXPR)
>> +    ret |= visit_addr (stmt, TREE_OPERAND (falseval, 0), data);
>> +
>> +  return ret;
>> +}
>> +
>> +
>>  /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
>>    VISIT_ADDR if non-NULL on loads, store and address-taken operands
>>    passing the STMT, the base of the operand and DATA to it.  The base
>> @@ -4761,6 +4800,7 @@
>>  {
>>   bool ret = false;
>>   unsigned i;
>> +
>>   if (gimple_assign_single_p (stmt))
>>     {
>>       tree lhs, rhs;
>> @@ -4785,6 +4825,9 @@
>>                   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
>>            ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
>>                                                   0), data);
>> +         else if (TREE_CODE (rhs) == COND_EXPR)
>> +           ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
>> +
>>           lhs = gimple_assign_lhs (stmt);
>>          if (TREE_CODE (lhs) == TARGET_MEM_REF
>>               && TMR_BASE (lhs) != NULL_TREE
>>
>>
>>
diff mbox

Patch

Index: gcc/gimple.c
===================================================================
--- gcc/gimple.c	(revision 163224)
+++ gcc/gimple.c	(working copy)
@@ -4746,6 +4746,45 @@ 
   return NULL_TREE;
 }

+
+/* For an assignment STMT which contains a selection expression on the RHS,
+   i.e.,
+
+     lhs = [cond_expr] a RELOP b ? x : y;
+
+   search the entire selection expression for occurrences of ADDR_EXPR, and
+   if found, apply VISIT_ADDR callback on STMT, the corresponding operand
+   of ADDR_EXPR and DATA.  Return TRUE if any of the callback invocations
+   returned TRUE.  */
+
+static inline bool
+stmt_visit_select_addr_ops (gimple stmt, tree select_expr, void *data,
+			    bool (*visit_addr)(gimple, tree, void *))
+{
+  bool ret = false;
+  tree condop1, condop2;
+  tree trueval, falseval;
+
+  condop1 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 0);
+  if (TREE_CODE (condop1) == ADDR_EXPR)
+    ret |= visit_addr (stmt, TREE_OPERAND (condop1, 0), data);
+
+  condop2 = TREE_OPERAND (TREE_OPERAND (select_expr, 0), 1);
+  if (TREE_CODE (condop2) == ADDR_EXPR)
+    ret |= visit_addr (stmt, TREE_OPERAND (condop2, 0), data);
+
+  trueval = TREE_OPERAND (select_expr, 1);
+  if (TREE_CODE (trueval) == ADDR_EXPR)
+    ret |= visit_addr (stmt, TREE_OPERAND (trueval, 0), data);
+
+  falseval = TREE_OPERAND (select_expr, 2);
+  if (TREE_CODE (falseval) == ADDR_EXPR)
+    ret |= visit_addr (stmt, TREE_OPERAND (falseval, 0), data);
+
+  return ret;
+}
+
+
 /* For the statement STMT call the callbacks VISIT_LOAD, VISIT_STORE and
    VISIT_ADDR if non-NULL on loads, store and address-taken operands
    passing the STMT, the base of the operand and DATA to it.  The base
@@ -4761,6 +4800,7 @@ 
 {
   bool ret = false;
   unsigned i;
+
   if (gimple_assign_single_p (stmt))
     {
       tree lhs, rhs;
@@ -4785,6 +4825,9 @@ 
 		   && TREE_CODE (OBJ_TYPE_REF_OBJECT (rhs)) == ADDR_EXPR)
 	    ret |= visit_addr (stmt, TREE_OPERAND (OBJ_TYPE_REF_OBJECT (rhs),
 						   0), data);
+	  else if (TREE_CODE (rhs) == COND_EXPR)
+	    ret |= stmt_visit_select_addr_ops (stmt, rhs, data, visit_addr);
+
           lhs = gimple_assign_lhs (stmt);
 	  if (TREE_CODE (lhs) == TARGET_MEM_REF
               && TMR_BASE (lhs) != NULL_TREE