User account creation filtered due to spam.

Bug 36902 - Array bound warning with dead code after optimization
Summary: Array bound warning with dead code after optimization
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: diagnostic, patch
Depends on:
Blocks:
 
Reported: 2008-07-22 18:43 UTC by H.J. Lu
Modified: 2009-04-18 09:29 UTC (History)
12 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.1.3 4.2.5
Known to fail: 4.3.1 4.4.0
Last reconfirmed: 2008-11-18 15:21:32


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2008-07-22 18:43:44 UTC
On Linux/x86-64, gcc 4.3/4.4 give:

[hjl@gnu-6 gcc]$ cat /tmp/y.c
/*
 * compile w/:
 * gcc -Wall -Werror -fno-strict-aliasing -O2 -c foo.c
 */

typedef unsigned char __u8;
typedef unsigned short __u16;

static inline void * foo(
    void * to, const void * from, int n)
{
    switch ( n )
    {
    case 3:
        *(__u16 *)to = *(const __u16 *)from;
        break;
    case 5:
        *((__u8 *)to + 4) = *((const __u8 *)from + 4);
        break;
    }
    return to;
}

int main(int argc, char *argv[])
{
    static char buf[64];
struct {
    __u16    size_of_select;
    __u8     pcr_select[4];
} sel;

    sel.size_of_select = 3;
    foo(buf, sel.pcr_select, sel.size_of_select);

    return 1;
}
[hjl@gnu-6 gcc]$ ./xgcc -B./ -Wall -Werror -fno-strict-aliasing -O2 -c /tmp/y.c
cc1: warnings being treated as errors
/tmp/y.c: In function âmainâ:
/tmp/y.c:18: error: array subscript is above array bounds
[hjl@gnu-6 gcc]$

Gcc 4.1 is OK.
Comment 1 Andrew Pinski 2008-07-22 20:50:21 UTC
This happens because the warning happens very early in the compiler so it does not know that the case5 is not going to be used.  I think the warning is correct and not really bogus if you take that into account.
Comment 2 H.J. Lu 2008-07-22 21:13:11 UTC
This regression is introduced by revision 120898:

http://gcc.gnu.org/ml/gcc-cvs/2007-01/msg00603.html

The simplified testcase:

---
typedef unsigned char __u8;
typedef unsigned short __u16;

static inline unsigned char *
foo(unsigned char * to, const unsigned char * from, int n)
{
    switch ( n )
    {
    case 3:
      *to = *from;
        break;
    case 5:
        to[4] = from [4];
        break;
    }
    return to;
}

struct {
    int    size_of_select;
    unsigned char pcr_select[4];
} sel;

int main(int argc, char *argv[])
{
    static unsigned char buf[64];

    sel.size_of_select = 3;
    foo(buf, sel.pcr_select, sel.size_of_select);

    return 1;
}
---

The warning is very fragile:  if the buffer in main() is not static then
there is no failure; is the size is passed as a constant there is no error.
Comment 3 Andrew Pinski 2008-07-22 21:18:28 UTC
> The warning is very fragile:  if the buffer in main() is not static then
> there is no failure; is the size is passed as a constant there is no error.

Not really, if you read my comment, you will understand why this is not that fragile after all.  I can make it even worse if you do a couple of things to trick one optimization pass up enough so we warn in the first VRP but don't optimize it away until the last VRP pass.  But really this is the normal issue with optimizers  warnings and is a hard problem to solve in general and I don't think we can count this as a regression really.
Comment 4 Paolo Carlini 2008-07-22 21:26:43 UTC
Out of curiosity: if this kind of code appears in a system header, is #pragma GCC system_header able to suppress the warning? Of course I'm asking because this is the most annoying feature of PR 36633 (which immediately came to my mind when I saw this one ;)
Comment 5 H.J. Lu 2008-07-22 21:29:02 UTC
It is a regression since the same correct code no longer compiles
with "-Werror -Wall" after upgrading from gcc 4.1/4.2 to 4.3.
Comment 6 H.J. Lu 2008-07-22 21:30:47 UTC
(In reply to comment #4)
> Out of curiosity: if this kind of code appears in a system header, is #pragma
> GCC system_header able to suppress the warning? Of course I'm asking because
> this is the most annoying feature of PR 36633 (which immediately came to my
> mind when I saw this one ;)
> 

It comes from an application.
Comment 7 Paolo Carlini 2008-07-22 21:32:44 UTC
(In reply to comment #6)
> It comes from an application.

This doesn't answer my question. 

Comment 8 H.J. Lu 2008-07-22 21:42:42 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > It comes from an application.
> 
> This doesn't answer my question. 
> 

I said "It comes from an application." It isn't from system header file.
Comment 9 Paolo Carlini 2008-07-22 22:53:57 UTC
> I said "It comes from an application." It isn't from system header file.

Yes, and that doesn't answer my question. I asked if the pragma is able to suppress a warning triggered by your kind of snippet, IF, when, it appears in an header file: I asked about an HYPOTHETICAL situation, not your actual situation. Is that more clear?

Comment 10 H.J. Lu 2008-07-23 01:04:24 UTC
(In reply to comment #9)
> > I said "It comes from an application." It isn't from system header file.
> 
> Yes, and that doesn't answer my question. I asked if the pragma is able to
> suppress a warning triggered by your kind of snippet, IF, when, it appears in
> an header file: I asked about an HYPOTHETICAL situation, not your actual
> situation. Is that more clear?
> 

It doesn't make a difference:

[hjl@gnu-6 tmp]$ cat y.h
#pragma GCC system_header
static inline unsigned char *
foo(unsigned char * to, const unsigned char * from, int n)
{
    switch ( n )
    {
    case 3:
      *to = *from;
        break;
    case 5:
        to[4] = from [4];
        break;
    }
    return to;
}
[hjl@gnu-6 tmp]$ cat y.c
#include <y.h>

struct {
    int    size_of_select;
    unsigned char pcr_select[4];
} sel;

int main(int argc, char *argv[])
{
    static unsigned char buf[64];

    sel.size_of_select = 3;
    foo(buf, sel.pcr_select, sel.size_of_select);

    return 1;
}
[hjl@gnu-6 tmp]$ gcc -Wall -O2 -c y.c -Werror -I.
cc1: warnings being treated as errors
y.c: In function âmainâ:
./y.h:11: error: array subscript is above array bounds
[hjl@gnu-6 tmp]$
Comment 11 Paolo Carlini 2008-07-23 01:16:05 UTC
Thanks. Actually, I think the experiment would be more meaningful if you could put also the equivalent of your main (a calling function, that is) inside the header, because in your testcase the warning is triggered inside the main, not in foo.

If you can, in your spare time, of course.
Comment 12 H.J. Lu 2008-07-23 01:23:18 UTC
(In reply to comment #11)
> Thanks. Actually, I think the experiment would be more meaningful if you could
> put also the equivalent of your main (a calling function, that is) inside the
> header, because in your testcase the warning is triggered inside the main, not
> in foo.
> 

Same:

[hjl@gnu-6 tmp]$ cat y.h
#pragma GCC system_header
static inline unsigned char *
foo(unsigned char * to, const unsigned char * from, int n)
{
    switch ( n )
    {
    case 3:
      *to = *from;
        break;
    case 5:
        to[4] = from [4];
        break;
    }
    return to;
}

struct {
    int    size_of_select;
    unsigned char pcr_select[4];
} sel;

static int
bar (void)
{
    static unsigned char buf[64];

    sel.size_of_select = 3;
    foo(buf, sel.pcr_select, sel.size_of_select);

    return 1;
}
[hjl@gnu-6 tmp]$ cat y.c
#include <y.h>

int
main ()
{
  return bar ();
}
[hjl@gnu-6 tmp]$ gcc -Wall -O2 -c y.c -Werror -I.
cc1: warnings being treated as errors
y.c: In function âmainâ:
./y.h:11: error: array subscript is above array bounds
[hjl@gnu-6 tmp]$
Comment 13 Paolo Carlini 2008-07-23 01:28:41 UTC
You see, as I feared: this class of warnings coming from the middle-end is especially nasty, because cannot be suppressed by any normal means.
Comment 14 Manuel López-Ibáñez 2008-08-15 20:11:02 UTC
(In reply to comment #13)
> You see, as I feared: this class of warnings coming from the middle-end is
> especially nasty, because cannot be suppressed by any normal means.

What location is being passed to that warning?
Comment 15 Manuel López-Ibáñez 2008-08-15 20:18:19 UTC
GCC system_headers should suppress this warning now, doesn't it?
Comment 16 Paolo Carlini 2008-08-15 20:36:37 UTC
No news, Manuel, still unsuppressed by the pragma
Comment 17 Manuel López-Ibáñez 2008-08-22 16:44:22 UTC
The location passed to check_array_ref is correct but the new way to check if we are in system headers does not work well with the %H hack. This will go away soon  hopefully when everything takes an explicit location. The fix is to use warning_at. This kind of thing may happen in the middle-end and in the front-end. This doesn't fix the bogus warning though, it just suppresses it within system headers.


Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c      (revision 139373)
+++ gcc/tree-vrp.c      (working copy)
@@ -4811,11 +4811,11 @@ insert_range_assertions (void)
    range. If the array subscript is a RANGE, warn if it is
    non-overlapping with valid range.
    IGNORE_OFF_BY_ONE is true if the ARRAY_REF is inside a ADDR_EXPR.  */

 static void
-check_array_ref (tree ref, const location_t *location, bool ignore_off_by_one)
+check_array_ref (tree ref, location_t location, bool ignore_off_by_one)
 {
   value_range_t* vr = NULL;
   tree low_sub, up_sub;
   tree low_bound, up_bound = array_ref_up_bound (ref);

@@ -4850,12 +4850,12 @@ check_array_ref (tree ref, const locatio
       if (TREE_CODE (up_sub) == INTEGER_CST
           && tree_int_cst_lt (up_bound, up_sub)
           && TREE_CODE (low_sub) == INTEGER_CST
           && tree_int_cst_lt (low_sub, low_bound))
         {
-          warning (OPT_Warray_bounds,
-                   "%Harray subscript is outside array bounds", location);
+          warning_at (location, OPT_Warray_bounds,
+                     "array subscript is outside array bounds");
           TREE_NO_WARNING (ref) = 1;
         }
     }
   else if (TREE_CODE (up_sub) == INTEGER_CST
            && tree_int_cst_lt (up_bound, up_sub)
@@ -4865,28 +4865,28 @@ check_array_ref (tree ref, const locatio
                                                         up_bound,
                                                         integer_one_node,
                                                         0),
                                        up_sub)))
     {
-      warning (OPT_Warray_bounds, "%Harray subscript is above array bounds",
-               location);
+      warning_at (location, OPT_Warray_bounds,
+                 "array subscript is above array bounds");
       TREE_NO_WARNING (ref) = 1;
     }
   else if (TREE_CODE (low_sub) == INTEGER_CST
            && tree_int_cst_lt (low_sub, low_bound))
     {
-      warning (OPT_Warray_bounds, "%Harray subscript is below array bounds",
-               location);
+      warning_at (location, OPT_Warray_bounds,
+                 "array subscript is below array bounds");
       TREE_NO_WARNING (ref) = 1;
     }
 }

 /* Searches if the expr T, located at LOCATION computes
    address of an ARRAY_REF, and call check_array_ref on it.  */

 static void
-search_for_addr_array(tree t, const location_t *location)
+search_for_addr_array(tree t, location_t location)
 {
   while (TREE_CODE (t) == SSA_NAME)
     {
       gimple g = SSA_NAME_DEF_STMT (t);

@@ -4925,11 +4925,11 @@ search_for_addr_array(tree t, const loca
 static tree
 check_array_bounds (tree *tp, int *walk_subtree, void *data)
 {
   tree t = *tp;
   struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
-  const location_t *location = (const location_t *) wi->info;
+  location_t location = *((location_t *) wi->info);

   *walk_subtree = TRUE;

   if (TREE_CODE (t) == ARRAY_REF)
     check_array_ref (t, location, false /*ignore_off_by_one*/);
@@ -4972,11 +4972,11 @@ check_all_array_refs (void)
          continue;
       }
       for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
        {
          gimple stmt = gsi_stmt (si);
-         const location_t *location = gimple_location_ptr (stmt);
+         location_t location = gimple_location (stmt);
          struct walk_stmt_info wi;
          if (!gimple_has_location (stmt))
            continue;

          if (is_gimple_call (stmt))
@@ -4990,12 +4990,12 @@ check_all_array_refs (void)
                }
            }
          else
            {
              memset (&wi, 0, sizeof (wi));
-             wi.info = CONST_CAST (void *, (const void *) location);
-
+             /*              wi.info = CONST_CAST (void *, (const void *) &location);*/
+             wi.info = (void *) &location;
              walk_gimple_op (gsi_stmt (si),
                              check_array_bounds,
                              &wi);
            }
        }
Comment 18 Manuel López-Ibáñez 2008-08-22 17:04:52 UTC
(In reply to comment #1)
> This happens because the warning happens very early in the compiler so it does
> not know that the case5 is not going to be used.  I think the warning is
> correct and not really bogus if you take that into account.

I think that if the compiler knows that the code is never executed then, we shouldn't warn. Anyway, this could be seen as a request enhancement. BTW, the offending code is removed within the first vrp pass, so perhaps we are not properly detecting that the BB is unreachable.
Comment 19 Andrew Pinski 2008-08-22 17:30:17 UTC
(In reply to comment #18)
> I think that if the compiler knows that the code is never executed then, we
> shouldn't warn. 

It does but only later on in the optimization it knows that the code is dead.
Comment 20 H.J. Lu 2008-08-22 17:37:12 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > I think that if the compiler knows that the code is never executed then, we
> > shouldn't warn. 
> 
> It does but only later on in the optimization it knows that the code is dead.
> 

Why can't we queue those warnings which may come from dead code and
suppress them if they are dead?
Comment 21 Manuel López-Ibáñez 2008-08-22 17:54:02 UTC
(In reply to comment #19)
> (In reply to comment #18)
> > I think that if the compiler knows that the code is never executed then, we
> > shouldn't warn. 
> 
> It does but only later on in the optimization it knows that the code is dead.

My point is that if I understand the vrp code correctly, we can detect that the code is dead before warning. In this case, the code is detected dead within VRP, not in a later pass. Maybe I am wrong. Let's see what Mueller and Guenther have to say. In any case, we should consider whether warning in conditional BBs is a good idea at all. I think that saying 

"array subscript *is* above array bounds"

in a BB that is executed conditionally is not a good idea in general.
Comment 22 Dirk Mueller 2008-08-25 07:59:55 UTC
there is currently no good way to detect if a block is dead during the VRP pass, as the VRP information is used for *determining* wether or not a block is dead. 

Is there a general warning-queuing implementation that I could make use of or is there some other warning that does that already? Iirc the "is used/ might be used uninitialized" type of warnings do not use something like that. 
Comment 23 Manuel López-Ibáñez 2008-08-25 10:50:13 UTC
(In reply to comment #22)
> there is currently no good way to detect if a block is dead during the VRP
> pass, as the VRP information is used for *determining* wether or not a block is
> dead. 

I think in this case it is.

simplify_switch_using_ranges is called before the warning occurs. However, it doesn't handle the case where the switch index is an integer constant! So it just doesn't do anything. I think this is a missed optimization, it should simplify the switch.

> Is there a general warning-queuing implementation that I could make use of or
> is there some other warning that does that already? Iirc the "is used/ might be
> used uninitialized" type of warnings do not use something like that. 

No, we don't have that but "is used/ may be used" does detect whether the current BB is always executed or not. That would be an improvement in my opinion. Not in this case anyway because as I said above, in this case we could do perfect.

Comment 24 H.J. Lu 2008-10-26 22:06:12 UTC
*** Bug 37921 has been marked as a duplicate of this bug. ***
Comment 25 Andrew Pinski 2008-10-27 00:17:11 UTC
*** Bug 37921 has been marked as a duplicate of this bug. ***
Comment 26 Andrew Pinski 2008-11-15 00:07:37 UTC
*** Bug 35392 has been marked as a duplicate of this bug. ***
Comment 27 Paolo Carlini 2008-11-18 10:16:42 UTC
Isn't this a regression?
Comment 28 H.J. Lu 2008-11-18 14:42:02 UTC
(In reply to comment #27)
> Isn't this a regression?
> 

The warning is new. But the same code won't compile with -Wall while
gcc 4.1 has no problems.
Comment 29 Manuel López-Ibáñez 2008-11-18 15:21:32 UTC
There is a patch here: http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01117.html
Comment 30 Paolo Carlini 2008-11-18 15:25:41 UTC
Thanks Manuel. I'm not sure that this is technically a regression, but in any case I consider it a serious problem and really hope we can have a fix for 4.4.0.
Comment 31 Manuel López-Ibáñez 2008-11-18 15:43:51 UTC
(In reply to comment #30)
> Thanks Manuel. I'm not sure that this is technically a regression, but in any
> case I consider it a serious problem and really hope we can have a fix for
> 4.4.0.

I submitted the patch long ago. We are in regressions-only mode. This is not a regression. Not sure what else you want me to do.

(It doesn't fix PR37921, though. I haven't tested PR35392.)
Comment 32 Paolo Carlini 2008-11-18 15:47:43 UTC
(In reply to comment #31)
> I submitted the patch long ago. We are in regressions-only mode. This is not a
> regression. Not sure what else you want me to do.

I'm not sure either ;) Maybe you could just work on the complete solution (per your posted scheme), fixing also the other known issues in this area, and submit it again for mainline after we branched...
Comment 33 Manuel López-Ibáñez 2008-11-18 16:05:50 UTC
(In reply to comment #32)
> (In reply to comment #31)
> > I submitted the patch long ago. We are in regressions-only mode. This is not a
> > regression. Not sure what else you want me to do.
> 
> I'm not sure either ;) Maybe you could just work on the complete solution (per
> your posted scheme), fixing also the other known issues in this area, and
> submit it again for mainline after we branched...

Or maybe I can just wait until this patch is reviewed, so I can get the official answer to my approach before wasting my free time foolishly in a futile endeavour. ;-)
Comment 34 Manuel López-Ibáñez 2009-04-18 09:25:09 UTC
Subject: Bug 36902

Author: manu
Date: Sat Apr 18 09:24:45 2009
New Revision: 146305

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=146305
Log:
2009-04-18  Manuel López-Ibáñez  <manu@gcc.gnu.org>

	PR middle-end/36902
	* tree-vrp.c (check_array_ref): Pass a location_t instead of a
	pointer. Use warning_at instead of warning.
	(search_for_addr_array): Likewise.
	(check_array_bounds): Likewise.
	(check_all_array_refs): Check that the incoming edge is not in the
	list of edges to be removed.
	(check_all_array_refs): Avoid the temporal pointer.
	(vrp_visit_cond_stmt): Fix typo.
	(simplify_switch_using_ranges): Handle the case where the switch
	index is an integer constant.
testsuite/
	* gcc.dg/pr36902.c: New.

Added:
    trunk/gcc/testsuite/gcc.dg/pr36902.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

Comment 35 Manuel López-Ibáñez 2009-04-18 09:29:41 UTC
FIXED in GCC 4.5