Patchwork [C] Fix PR37985

login
register
mail settings
Submitter Richard Guenther
Date Jan. 16, 2012, 9:04 a.m.
Message ID <alpine.LNX.2.00.1201161001140.4999@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/136262/
State New
Headers show

Comments

Richard Guenther - Jan. 16, 2012, 9:04 a.m.
On Fri, 13 Jan 2012, Joseph S. Myers wrote:

> On Fri, 13 Jan 2012, Richard Guenther wrote:
> 
> > This fixes PR37985 where since 
> > http://gcc.gnu.org/ml/gcc-patches/2006-08/msg01041.html we
> > mark conversions produced by convert_to_integer with TREE_NO_WARNING.
> > This may shadow "real" stmts with no effects, thus we should
> > simply strip them again before checking for TREE_SIDE_EFFECTS.
> > 
> > Bootstrap & regtest pending on x86_64-unknown-linux-gnu.
> > 
> > Ok if that passes?
> 
> OK.

It FAILs gcc.dg/20040202-1.c (we warn about a folded memcpy).  It
looks like wrapping things in TREE_NO_WARNING NOP_EXPRs is
fragile as soon as you consider multiple warning kinds (thus, of
course the patch causing this regression is at fault).

OTOH for the regression warning on folded stmts I really wonder
why c_process_expr_stmt folds at all before emitting warnings - why
intentionally divert from the source AST here?  I'm testing
the following before giving up on this regression (which solves
the gcc.dg/20040202-1.c FAIL at least).

Richard.

2012-01-13  Richard Guenther  <rguenther@suse.de>

	PR c/37985
	* c-typeck.c (emit_side_effect_warnings): Strip conversions
	with TREE_NO_WARNING.
	(c_process_expr_stmt): Fold the stmt after emitting warnings.

	* gcc.dg/Wunused-value-4.c: New testcase.

Index: gcc/testsuite/gcc.dg/Wunused-value-4.c
===================================================================
*** gcc/testsuite/gcc.dg/Wunused-value-4.c	(revision 0)
--- gcc/testsuite/gcc.dg/Wunused-value-4.c	(revision 0)
***************
*** 0 ****
--- 1,9 ----
+ /* PR c/37985 */
+ /* { dg-do compile } */
+ /* { dg-options "-Wunused-value" } */
+ 
+ unsigned char foo(unsigned char a)
+ {
+   a >> 2; /* { dg-warning "statement with no effect" } */
+   return a;
+ }
Richard Guenther - Jan. 16, 2012, 11:25 a.m.
On Mon, 16 Jan 2012, Richard Guenther wrote:

> On Fri, 13 Jan 2012, Joseph S. Myers wrote:
> 
> > On Fri, 13 Jan 2012, Richard Guenther wrote:
> > 
> > > This fixes PR37985 where since 
> > > http://gcc.gnu.org/ml/gcc-patches/2006-08/msg01041.html we
> > > mark conversions produced by convert_to_integer with TREE_NO_WARNING.
> > > This may shadow "real" stmts with no effects, thus we should
> > > simply strip them again before checking for TREE_SIDE_EFFECTS.
> > > 
> > > Bootstrap & regtest pending on x86_64-unknown-linux-gnu.
> > > 
> > > Ok if that passes?
> > 
> > OK.
> 
> It FAILs gcc.dg/20040202-1.c (we warn about a folded memcpy).  It
> looks like wrapping things in TREE_NO_WARNING NOP_EXPRs is
> fragile as soon as you consider multiple warning kinds (thus, of
> course the patch causing this regression is at fault).
> 
> OTOH for the regression warning on folded stmts I really wonder
> why c_process_expr_stmt folds at all before emitting warnings - why
> intentionally divert from the source AST here?  I'm testing
> the following before giving up on this regression (which solves
> the gcc.dg/20040202-1.c FAIL at least).

Which works besides now FAILing gcc.dg/strncpy-fix-1.c.  We get
a "value computed is not used" warning because we are still folding
the strncpy to a memcpy because we fold all builtin calls at the very
point they are created (build_function_call_vec):

  if (name != NULL_TREE
      && !strncmp (IDENTIFIER_POINTER (name), "__builtin_", 10))
    {
      if (require_constant_value)
        result =
          fold_build_call_array_initializer_loc (loc, TREE_TYPE (fntype),
                                                 function, nargs, 
argarray);
      else
        result = fold_build_call_array_loc (loc, TREE_TYPE (fntype),
                                            function, nargs, argarray);

"Fixed" by not doing that if !require_constant_value.  We'll fold
it during the c_fully_fold call anyway (and during gimplification again).

Re-testing as follows.

Richard.

2012-01-13  Richard Guenther  <rguenther@suse.de>

	PR c/37985
	* c-typeck.c (emit_side_effect_warnings): Strip conversions
	with TREE_NO_WARNING.
	(c_process_expr_stmt): Fold the stmt after emitting warnings.
	(build_function_call_vec): Do not fold calls here if we do not
	require a constant value.

	* gcc.dg/Wunused-value-4.c: New testcase.

Index: gcc/testsuite/gcc.dg/Wunused-value-4.c
===================================================================
*** gcc/testsuite/gcc.dg/Wunused-value-4.c	(revision 0)
--- gcc/testsuite/gcc.dg/Wunused-value-4.c	(revision 0)
***************
*** 0 ****
--- 1,9 ----
+ /* PR c/37985 */
+ /* { dg-do compile } */
+ /* { dg-options "-Wunused-value" } */
+ 
+ unsigned char foo(unsigned char a)
+ {
+   a >> 2; /* { dg-warning "statement with no effect" } */
+   return a;
+ }
Index: gcc/c-typeck.c
===================================================================
*** gcc/c-typeck.c	(revision 183205)
--- gcc/c-typeck.c	(working copy)
*************** build_function_call_vec (location_t loc,
*** 2826,2841 ****
    /* Check that the arguments to the function are valid.  */
    check_function_arguments (fntype, nargs, argarray);
  
!   if (name != NULL_TREE
        && !strncmp (IDENTIFIER_POINTER (name), "__builtin_", 10))
      {
!       if (require_constant_value)
! 	result =
! 	  fold_build_call_array_initializer_loc (loc, TREE_TYPE (fntype),
! 						 function, nargs, argarray);
!       else
! 	result = fold_build_call_array_loc (loc, TREE_TYPE (fntype),
! 					    function, nargs, argarray);
        if (TREE_CODE (result) == NOP_EXPR
  	  && TREE_CODE (TREE_OPERAND (result, 0)) == INTEGER_CST)
  	STRIP_TYPE_NOPS (result);
--- 2826,2838 ----
    /* Check that the arguments to the function are valid.  */
    check_function_arguments (fntype, nargs, argarray);
  
!   if (require_constant_value
!       && name != NULL_TREE
        && !strncmp (IDENTIFIER_POINTER (name), "__builtin_", 10))
      {
!       result =
! 	fold_build_call_array_initializer_loc (loc, TREE_TYPE (fntype),
! 					       function, nargs, argarray);
        if (TREE_CODE (result) == NOP_EXPR
  	  && TREE_CODE (TREE_OPERAND (result, 0)) == INTEGER_CST)
  	STRIP_TYPE_NOPS (result);
*************** c_finish_bc_stmt (location_t loc, tree *
*** 9163,9168 ****
--- 9160,9169 ----
  static void
  emit_side_effect_warnings (location_t loc, tree expr)
  {
+   /* Strip conversions marked with TREE_NO_WARNING.  */
+   while (TREE_NO_WARNING (expr) && CONVERT_EXPR_P (expr))
+     expr = TREE_OPERAND (expr, 0);
+ 
    if (expr == error_mark_node)
      ;
    else if (!TREE_SIDE_EFFECTS (expr))
*************** c_process_expr_stmt (location_t loc, tre
*** 9186,9193 ****
    if (!expr)
      return NULL_TREE;
  
-   expr = c_fully_fold (expr, false, NULL);
- 
    if (warn_sequence_point)
      verify_sequence_points (expr);
  
--- 9187,9192 ----
*************** c_process_expr_stmt (location_t loc, tre
*** 9213,9218 ****
--- 9212,9219 ----
        || TREE_CODE (exprv) == ADDR_EXPR)
      mark_exp_read (exprv);
  
+   expr = c_fully_fold (expr, false, NULL);
+ 
    /* If the expression is not of a type to which we cannot assign a line
       number, wrap the thing in a no-op NOP_EXPR.  */
    if (DECL_P (expr) || CONSTANT_CLASS_P (expr))
Richard Guenther - Jan. 16, 2012, 12:18 p.m.
On Mon, 16 Jan 2012, Richard Guenther wrote:

> On Mon, 16 Jan 2012, Richard Guenther wrote:
> 
> > On Fri, 13 Jan 2012, Joseph S. Myers wrote:
> > 
> > > On Fri, 13 Jan 2012, Richard Guenther wrote:
> > > 
> > > > This fixes PR37985 where since 
> > > > http://gcc.gnu.org/ml/gcc-patches/2006-08/msg01041.html we
> > > > mark conversions produced by convert_to_integer with TREE_NO_WARNING.
> > > > This may shadow "real" stmts with no effects, thus we should
> > > > simply strip them again before checking for TREE_SIDE_EFFECTS.
> > > > 
> > > > Bootstrap & regtest pending on x86_64-unknown-linux-gnu.
> > > > 
> > > > Ok if that passes?
> > > 
> > > OK.
> > 
> > It FAILs gcc.dg/20040202-1.c (we warn about a folded memcpy).  It
> > looks like wrapping things in TREE_NO_WARNING NOP_EXPRs is
> > fragile as soon as you consider multiple warning kinds (thus, of
> > course the patch causing this regression is at fault).
> > 
> > OTOH for the regression warning on folded stmts I really wonder
> > why c_process_expr_stmt folds at all before emitting warnings - why
> > intentionally divert from the source AST here?  I'm testing
> > the following before giving up on this regression (which solves
> > the gcc.dg/20040202-1.c FAIL at least).
> 
> Which works besides now FAILing gcc.dg/strncpy-fix-1.c.  We get
> a "value computed is not used" warning because we are still folding
> the strncpy to a memcpy because we fold all builtin calls at the very
> point they are created (build_function_call_vec):
> 
>   if (name != NULL_TREE
>       && !strncmp (IDENTIFIER_POINTER (name), "__builtin_", 10))
>     {
>       if (require_constant_value)
>         result =
>           fold_build_call_array_initializer_loc (loc, TREE_TYPE (fntype),
>                                                  function, nargs, 
> argarray);
>       else
>         result = fold_build_call_array_loc (loc, TREE_TYPE (fntype),
>                                             function, nargs, argarray);
> 
> "Fixed" by not doing that if !require_constant_value.  We'll fold
> it during the c_fully_fold call anyway (and during gimplification again).
> 
> Re-testing as follows.

Which does not work either - we seem to rely on that early folding
at least for the type-generic math stuff (gcc.dg/c99-tgmath-*.c).

I'm dropping all these patches.  There doesn't seem to be a good
way to make everything well-behaved with the patch causing the
regression installed.

Richard.

Patch

Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 183205)
+++ gcc/c-typeck.c	(working copy)
@@ -9163,6 +9163,10 @@  c_finish_bc_stmt (location_t loc, tree *
 static void
 emit_side_effect_warnings (location_t loc, tree expr)
 {
+  /* Strip conversions marked with TREE_NO_WARNING.  */
+  while (TREE_NO_WARNING (expr) && CONVERT_EXPR_P (expr))
+    expr = TREE_OPERAND (expr, 0);
+
   if (expr == error_mark_node)
     ;
   else if (!TREE_SIDE_EFFECTS (expr))
@@ -9186,8 +9190,6 @@  c_process_expr_stmt (location_t loc, tre
   if (!expr)
     return NULL_TREE;
 
-  expr = c_fully_fold (expr, false, NULL);
-
   if (warn_sequence_point)
     verify_sequence_points (expr);
 
@@ -9213,6 +9215,8 @@  c_process_expr_stmt (location_t loc, tre
       || TREE_CODE (exprv) == ADDR_EXPR)
     mark_exp_read (exprv);
 
+  expr = c_fully_fold (expr, false, NULL);
+
   /* If the expression is not of a type to which we cannot assign a line
      number, wrap the thing in a no-op NOP_EXPR.  */
   if (DECL_P (expr) || CONSTANT_CLASS_P (expr))