Bug 43270 - array-bounds false negative
Summary: array-bounds false negative
Status: VERIFIED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.5.0
: P3 enhancement
Target Milestone: 4.6.0
Assignee: Richard Biener
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-05 20:19 UTC by Matt Hargett
Modified: 2019-08-16 03:12 UTC (History)
3 users (show)

See Also:
Host: x86_64-unknown-linux-gnu
Target: x86_64-unknown-linux-gnu
Build: x86_64-unknown-linux-gnu
Known to work:
Known to fail:
Last reconfirmed: 2010-03-06 14:27:49


Attachments
compilation unit that reproduces the bug (359 bytes, text/plain)
2010-03-05 20:22 UTC, Matt Hargett
Details
updated example that doesn't rely on zero-length arrays (360 bytes, text/plain)
2010-03-05 23:25 UTC, Matt Hargett
Details
yet another example, that does not rely on zero-length arrays or on the array being the 'last' field in the struct/class (387 bytes, text/plain)
2010-03-06 00:19 UTC, Matt Hargett
Details
untested patch (577 bytes, patch)
2010-03-06 14:28 UTC, Richard Biener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Hargett 2010-03-05 20:19:58 UTC
Compiling the attached code with -O3 -Wall or -O2 -Wall doesn't elicit an array bounds warning, when it should in the constructor for FixedString.
Comment 1 Matt Hargett 2010-03-05 20:22:01 UTC
Created attachment 20031 [details]
compilation unit that reproduces the bug
Comment 2 Matt Hargett 2010-03-05 20:33:24 UTC
This occurs with both gcc 4.4.1 and 4.5.0.20100304.
Comment 3 Andrew Pinski 2010-03-05 20:34:37 UTC
Well this is semi on purpose.  Though we are should reject the zero sized arrays anyways.

The problem is here we have:
struct f
{
  char a[0];
};

Which is common in GNU C/C++ to say f::a is a flexible array member.  Also we don't warn about the last array in a struct for that reason.
Comment 4 Matt Hargett 2010-03-05 22:17:30 UTC
It's not the fact that it's zero-sized in and of itself, but rather the assignment to contents[0] in the ctor should trigger the warning. Oddly, PC-Lint warns of the zero-sized array, but not the actual overflow.

As a test, I tried changing the ctor assignment to contents[1], and the warning still isn't triggered in either GCC or PC-Lint. 

Whatever this blind spot is, it's in both tools.
Comment 5 Andrew Pinski 2010-03-05 22:22:41 UTC
Well:
struct f
{
  int t[0];
};

is invalid c/C++ :). But we accept it as an extension.  Anyways the thing when you do:
f *t = ..;
t->t[1] = 1;

We don't warn there on purpose as f::t might used as flexible array.  We do the same thing for any  array that ends the struct, it does not matter.  This is very common in C and C++ code so turning this warning on for this one false negative case is going to be hard not to get that many false positive warnings.

Sorry.
Comment 6 Matt Hargett 2010-03-05 23:24:52 UTC
I see your point about supporting existing code that uses this feature in the way you describe.

I modified the example to not rely upon zero-length array and have attached it. (The bug in the original code didn't use it that way either, I was just trying to make the reproducuble test case simpler.) GCC 4.4.1 and 4.5.0.20100304 still do not warn about the array-bounds issue. PC-Lint now does warn about it.
Comment 7 Matt Hargett 2010-03-05 23:25:24 UTC
Created attachment 20032 [details]
updated example that doesn't rely on zero-length arrays
Comment 8 Andrew Pinski 2010-03-05 23:26:20 UTC
As I mentioned, it is the array at the end of the struct which is where we don't warn.  
Comment 9 Matt Hargett 2010-03-06 00:18:42 UTC
Alright. Even though PC-Lint now correctly warns, and GCC still does not, I have updated the attached example yet again to avoid the next constraint you mention.

GCC still does not detect the array-bounds issue, even when the array whose bound is being violated it is neither the first nor the last field in the struct/class. I tried using primitive types, complex types, references/pointer, and arrays for the fields in question. GCC still never detects the issue.

Even for existing code that uses this pattern for flexible array members, a private array that has not been otherwise initialized in the ctor, which is accessed out of bounds in said ctor, seems unlikely. Perhaps I will enter that as a separate bug, but I hope that this latest code example (which still decently matches the real code I had the bug with) is worthy of the bug being detected.
Comment 10 Matt Hargett 2010-03-06 00:19:48 UTC
Created attachment 20033 [details]
yet another example, that does not rely on zero-length arrays or on the array being the 'last' field in the struct/class
Comment 11 Andrew Pinski 2010-03-06 00:23:36 UTC
Oh we ignore off by one errors in some cases too.
Comment 12 Matt Hargett 2010-03-06 01:31:36 UTC
Changing contents[size] to contents[size + 10] or to contents[size+10000] is still not triggering the array-bounds warning in any of the compilers I tested (previously mentioned). In my real code, it was an OB1 bug, so that's what I would have like to have been detected.
Comment 13 Richard Biener 2010-03-06 14:27:49 UTC
Err - it's just because the code is broken:

  tree low_bound, up_bound = array_ref_up_bound (ref);

  low_sub = up_sub = TREE_OPERAND (ref, 1);

  if (!up_bound || TREE_NO_WARNING (ref)
      || TREE_CODE (up_bound) != INTEGER_CST
      /* Can not check flexible arrays.  */
      || (TYPE_SIZE (TREE_TYPE (ref)) == NULL_TREE
          && TYPE_DOMAIN (TREE_TYPE (ref)) != NULL_TREE
          && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (ref))) == NULL_TREE)

well - this checks TYPE_SIZE/DOMAIN on the element type ...

and the struct hack check is too strict:

      /* Accesses after the end of arrays of size 0 (gcc
         extension) and 1 are likely intentional ("struct
         hack").  */
      || compare_tree_int (up_bound, 1) <= 0)

Replacing that with a more proper (but still too strict) check like

  /* Accesses after the end of arrays at the end of structures
     are likely intentional ("struct hack").  */
  if (TREE_CODE (TREE_OPERAND (ref, 0)) == COMPONENT_REF
      && !TREE_CHAIN (TREE_OPERAND (TREE_OPERAND (ref, 0), 1)))
    return;

gets you

t.C: In function 'int main()':
t.C:27:45: warning: array subscript is above array bounds

it doesn't print that this is from an inlined constructor though.
Comment 14 Richard Biener 2010-03-06 14:28:33 UTC
Created attachment 20036 [details]
untested patch
Comment 15 Richard Biener 2010-04-07 12:31:46 UTC
Subject: Bug 43270

Author: rguenth
Date: Wed Apr  7 12:31:32 2010
New Revision: 158058

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=158058
Log:
2010-04-07  Richard Guenther  <rguenther@suse.de>

	PR tree-optimization/43270
	* tree-vrp.c (check_array_ref): Fix flexible array member
	detection.
	* tree-ssa-sccvn.h (fully_constant_vn_reference_p): Declare.
	* tree-ssa-pre.c (phi_translate_1): Adjust.
	(fully_constant_expression): Split out vn_reference handling to ...
	* tree-ssa-sccvn.c (fully_constant_vn_reference_p): ... here.
	Fold reads from constant strings.
	(vn_reference_lookup): Handle fully constant references.
	(vn_reference_lookup_pieces): Likewise.
	* Makefile.in (expmed.o-warn): Add -Wno-error.

	* g++.dg/warn/Warray-bounds-4.C: New testcase.
	* gcc.dg/Warray-bounds-7.c: Likewise.

Added:
    trunk/gcc/testsuite/g++.dg/warn/Warray-bounds-4.C
    trunk/gcc/testsuite/gcc.dg/Warray-bounds-7.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/Makefile.in
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-pre.c
    trunk/gcc/tree-ssa-sccvn.c
    trunk/gcc/tree-ssa-sccvn.h
    trunk/gcc/tree-vrp.c

Comment 16 Richard Biener 2010-04-07 12:34:12 UTC
Fixed for 4.6.
Comment 17 Andrew Pinski 2010-07-05 00:11:01 UTC
>        * Makefile.in (expmed.o-warn): Add -Wno-error.

There is no comment in Makefile.in why this is there, can you add one.  Also is this a false warning from gcc or a true one, I cannot tell.

Note I also get a warning for:
/home/apinski/src/gcc-fsf/local//gcc/libgcc/../gcc/crtstuff.c:372:19: warning: array subscript is above array bounds [-Warray-bounds]

Where the code does:
  if (__JCR_LIST__[0])

__JCR_LIST__ is defined as:
STATIC void *__JCR_LIST__[]
  __attribute__ ((used, section(JCR_SECTION_NAME), aligned(sizeof(void*))))
  = { };

Which means it is not above the array bounds after all since there is no real array bounds for that array :).
Comment 18 Richard Biener 2010-07-07 10:08:01 UTC
*** Bug 44848 has been marked as a duplicate of this bug. ***
Comment 19 AJ 2011-03-26 16:00:53 UTC
not sure if this will help.
but i found this problem with -O2 optimzed code in GCC 4.5.1  (and 4.4.1)

example to show the kind of problem (not a literal testcase)

int a[8];
a[4] = 9;

issues an   array-bounds warning,  -Warray-bounds
seems to be confusing the array elements value with the array size.
if i have

a[4] = 7;

no error.
I hope that gives someone an idea about where to look for the problem.
(i haven't access to newer GCCs)
Comment 20 AJ 2011-03-26 16:56:37 UTC
(In reply to comment #19)

ignore my comments. i can't confirm it sufficiently. i might be wrong.
Comment 21 Matt Hargett 2011-04-12 18:13:30 UTC
verified fixed in 4.6.0.