Bug 91146 - [11/12/13/14 Regression] -Werror=array-bounds if compile with -fsanitize=address
Summary: [11/12/13/14 Regression] -Werror=array-bounds if compile with -fsanitize=address
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 9.1.0
: P2 normal
Target Milestone: 11.5
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Warray-bounds asan
  Show dependency treegraph
 
Reported: 2019-07-11 19:02 UTC by Grigorii Lipenko
Modified: 2023-07-07 10:35 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work: 8.3.0
Known to fail: 10.0, 9.2.0
Last reconfirmed: 2020-01-23 00:00:00


Attachments
gtest llvm small_vector (135.90 KB, application/zip)
2019-07-11 19:02 UTC, Grigorii Lipenko
Details
llvm small_vector (7.45 KB, application/zip)
2019-07-12 08:25 UTC, Grigorii Lipenko
Details
pr91146.ii.xz (110.40 KB, application/octet-stream)
2020-03-17 15:43 UTC, Jakub Jelinek
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Grigorii Lipenko 2019-07-11 19:02:11 UTC
Created attachment 46592 [details]
gtest llvm small_vector

It's google test for llvm small_vector.
Compile: g++ -Wall -Wextra -O2 -Werror -fsanitize=address -c main.cpp
And get:

small_vector.h:505:7: ошибка: array subscript 1 is outside array bounds of «int [1]» [-Werror=array-bounds]
  505 |       ++EltPtr;
      |       ^~
main.cpp:7:26: замечание: while referencing «<anonymous>»
    7 |   v.insert (v.begin (), 1);
      | 

If compile "g++ -Wall -Wextra -O2 -Werror -c main.cpp" there is no warning.

I don't have this problem with gcc 8.3.0 .
I compiled my project with gcc 9.1.0 without optimization and sanitizer don't detect problems in runtime.
Comment 1 Martin Sebor 2019-07-11 21:42:12 UTC
I can reproduce the warning but I have a few comments:

1) A smaller test case would make the issue easier to debug.
2) The instrumentation inserted by the sanitizers is known to cause false positive warnings.  This is unfortunate but until we come up with a solution we advise to either avoid combining sanitizers and warnings or be prepared for false positives.
3) The code in small_vector_impl::insert copied below doesn't look quite right to me:

  iterator insert(iterator I, const T &Elt) {
    ...
    // If we just moved the element we're inserting, be sure to update
    // the reference.
    const T *EltPtr = &Elt;
    if (I <= EltPtr && EltPtr < this->EndX)
      ++EltPtr;
    *I = ::std::move(*EltPtr);

Calling it as is done in the test but on a non-empty vector:

  v.insert (v.begin (), 1);

sets EltPtr to point to the temporary 1 while I ans this->EndX point to an object unrelated to the temporary.  The result of an inequality expression between pointers to unrelated objects is unspecified in C++ and so the increment could in theory be evaluated regardless of whether Elt is a reference to an element of the vector or some other object (such as the temporary).

A false positive warning can likely be reproduced with a valid test case but I don't think this one qualifies.
Comment 2 Grigorii Lipenko 2019-07-12 08:24:10 UTC
I remove gtest.h.

#include "small_vector.h"

class A
{
public:
  virtual void f () = 0;
};

class B : public A
{
public:
  void f () override
  {
    small_vector<int, 20> v;
    v.insert (v.begin (), 1); 
  }
};

int main ()
{
  B b;
  (void)b;
  return 0;
}

I get the warning in this case.
If comment inheritance then the warning disappears.
Comment 3 Grigorii Lipenko 2019-07-12 08:25:21 UTC
Created attachment 46593 [details]
llvm small_vector
Comment 4 Martin Liška 2020-01-23 13:10:17 UTC
Just for the record, it started with r9-1948-gd893b683f40884cd00b5beb392566ecc7b67f721.
Comment 5 Jakub Jelinek 2020-03-12 11:58:59 UTC
GCC 9.3.0 has been released, adjusting target milestone.
Comment 6 Jakub Jelinek 2020-03-17 15:29:59 UTC
The testcase isn't very good, because it is optimized to nothing without the sanitization.
But using
  __attribute__((used)) void f () override
  {
    small_vector<int, 20> v;
    v.insert (v.begin (), 1);
    __asm volatile ("" : : "r" (&v) : "memory");
  }
prevents that.
With that, the dumps look comparable until fre3.
Initially, I thought the issue is that ASAN_MARK ifn isn't said to affect just the pointed memory (and directly only).  In reality it affects something different (the shadow memory corresponding to that memory), but perhaps it would work.  Unfortunately:
--- gcc/internal-fn.def.jj	2020-01-18 13:47:09.517357706 +0100
+++ gcc/internal-fn.def	2020-03-17 16:04:08.092861394 +0100
@@ -309,7 +309,7 @@ DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_
 DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
 DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..")
-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL)
+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".W.")
 DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
doesn't fix it.
So, at the start of f without asan we have:
  v ={v} {CLOBBER};
  MEM[(struct  &)&v] ={v} {CLOBBER};
  MEM[(struct small_vector_base *)&v] ={v} {CLOBBER};
  MEM[(struct small_vector_base *)&v].BeginX = &MEM[(struct small_vector_template_common *)&v].FirstEl;
  MEM[(struct small_vector_base *)&v].EndX = &MEM[(struct small_vector_template_common *)&v].FirstEl;
  MEM[(struct small_vector_base *)&v].CapacityX = &MEM <union U> [(void *)&v + 104B];
  D.48526 = 1;
  _17 = MEM[(struct small_vector_template_common *)&v].D.48095.EndX;
  if (&MEM[(struct small_vector_template_common *)&v].FirstEl == _17)
where small_vector_template_common is derived from small_vector_base and thus
MEM[(struct small_vector_template_common *)&v].D.48095 and MEM[(struct small_vector_base *)&v] is the same thing.
Now, with asan we have:
  .ASAN_MARK (UNPOISON, &v, 104);
  v ={v} {CLOBBER};
  MEM[(struct  &)&v] ={v} {CLOBBER};
  MEM[(struct small_vector_base *)&v] ={v} {CLOBBER};
  MEM[(struct small_vector_base *)&v].BeginX = &MEM[(struct small_vector_template_common *)&v].FirstEl;
  MEM[(struct small_vector_base *)&v].EndX = &MEM[(struct small_vector_template_common *)&v].FirstEl;
  MEM[(struct small_vector_base *)&v].CapacityX = &MEM <union U> [(void *)&v + 104B];
  .ASAN_MARK (UNPOISON, &D.48886, 4);
  D.48886 = 1;
  _10 = MEM[(struct small_vector_template_common *)&v].D.48455.BeginX;
  _20 = MEM[(struct small_vector_template_common *)&v].D.48455.EndX;
  if (_10 == _20)
The only difference other than .ASAN_MARK calls is that in the asan case _10 hasn't been propagated into the comparison.
Anyway, fre3 seems to be able to value number the _17 load to  &MEM[(struct small_vector_template_common *)&v].FirstEl and thus optimize the comparison into true, while with the .ASAN_MARK calls it doesn't.
Comment 7 Jakub Jelinek 2020-03-17 15:40:40 UTC
Ah, should have been:
--- gcc/internal-fn.def.jj	2020-01-18 13:47:09.517357706 +0100
+++ gcc/internal-fn.def	2020-03-17 16:04:08.092861394 +0100
@@ -309,7 +309,7 @@ DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_
 DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
 DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..")
-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL)
+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.")
 DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
the ifn has 3 arguments and the pointer is the middle one.
Anyway, even with this sccvn isn't able to see through it.
Comment 8 Jakub Jelinek 2020-03-17 15:43:46 UTC
Created attachment 48050 [details]
pr91146.ii.xz

Preprocessed source.  -O2 -fsanitize=address -Wall
Comment 9 Jakub Jelinek 2020-03-17 16:43:32 UTC
Seems even with that patch
3677		  if (stmt_may_clobber_ref_p_1 (def_stmt, ref, tbaa_p))
in tree-ssa-sccvn.c thinks the
  .ASAN_MARK (UNPOISON, &D.48886, 4);
call may clobber
MEM[(struct small_vector_template_common *)&v].D.48455.BeginX
With the following completely untested patch we optimize like withouf -fsanitize=address and don't warn.
--- gcc/internal-fn.def.jj	2020-01-18 13:47:09.517357706 +0100
+++ gcc/internal-fn.def	2020-03-17 16:32:41.210800195 +0100
@@ -309,7 +309,7 @@ DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_
 DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
 DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..")
-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL)
+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.")
 DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
--- gcc/tree-ssa-alias.c.jj	2020-03-03 11:04:46.367821907 +0100
+++ gcc/tree-ssa-alias.c	2020-03-17 17:40:22.295385869 +0100
@@ -3032,6 +3032,14 @@ call_may_clobber_ref_p_1 (gcall *call, a
 	default:
 	  /* Fallthru to general call handling.  */;
       }
+  else if (gimple_call_internal_p (call)
+	   && gimple_call_internal_fn (call) == IFN_ASAN_MARK)
+    {
+      ao_ref dref;
+      tree size = gimple_call_arg (call, 2);
+      ao_ref_init_from_ptr_and_size (&dref, gimple_call_arg (call, 1), size);
+      return refs_may_alias_p_1 (&dref, ref, false);
+    }
 
   /* Check if base is a global static variable that is not written
      by the function.  */
Comment 10 Jakub Jelinek 2020-03-18 07:41:49 UTC
The #c9 patch passed bootstrap, but regresses:
FAIL: g++.dg/asan/use-after-scope-types-1.C   -O1  execution test
FAIL: g++.dg/asan/use-after-scope-types-1.C   -O2  execution test
FAIL: g++.dg/asan/use-after-scope-types-1.C   -O3 -g  execution test
FAIL: g++.dg/asan/use-after-scope-types-1.C   -Os  execution test
FAIL: g++.dg/asan/use-after-scope-types-1.C   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: g++.dg/asan/use-after-scope-types-1.C   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -O1  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -O2  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -O3 -g  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -Os  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: g++.dg/asan/use-after-scope-types-2.C   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -O1  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -O2  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -O3 -g  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -Os  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: g++.dg/asan/use-after-scope-types-3.C   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -O1  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -O2  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -O3 -g  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -Os  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -O2 -flto -fno-use-linker-plugin -flto-partition=none  execution test
FAIL: g++.dg/asan/use-after-scope-types-5.C   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  execution test

The thing is that those tests do those use after scope accesses but don't really have anything afterwards that would allow optimizing those accesses away.  With the previous behavior of ASAN_MARK ifns such barriers were in those ifns.
So, the big question is, what do we want.
For -O0 -fsanitize=address both the unpatched and patched versions work similarly, and for -O1 -fsanitize=address and higher,
do we want the sanitizer to detect just those after scope uses that would remain after optimization even in non-instrumented code?  Then we want the patch.  Or do we want to detect more after scope uses, even at the expense of some false positive warnings?
If I try clang++ -O{0,1} -fsanitize=address on the use-after-scope-types-1.C testcase, then it detects the violation (patched g++ with already -O1 -fsanitize=address doesn't), but clang++ -O{2,3} -fsanitize=address doesn't detect it (while unpatched g++ -O{2,3} -fsanitize=address does).
Comment 11 Jakub Jelinek 2020-03-18 09:23:18 UTC
Regarding the original testcase, small tweak results in the same warning even without sanitization:
#include "pr91146.h"

class A
{
public:
  virtual void f () = 0;
};

template <typename T>
void foo (T &);

class B : public A
{
public:
  void f () override
  {
    small_vector<int, 20> v;
    auto i = v.begin ();
    foo (i);
    v.insert (i, 1); 
    foo (v);
  }
};

int main ()
{
  B b;
  b.f ();
  return 0;
}

Here we just make sure the method isn't optimized away completely and that the optimizer doesn't know v.insert is called with v.begin () iterator.
And the problem is that the llvm small vector implementation does:
iterator insert(iterator I, T &&Elt) {
...
    T *EltPtr = &Elt;
    if (I <= EltPtr && EltPtr < this->EndX)
      ++EltPtr;
   
    *I = ::std::move(*EltPtr);
    return I;
  }

*++EltPtr on an int (not array) is obviously UB, but the condition should guard that this doesn't happen.  Except that there is UB already in the condition that guards this, because non-equality pointer comparisons are valid only if both pointers point into the same object or one past the end of it, or both are NULL.
That isn't the case here, as I points into the payload of the v variable and so does this->endX, while EltPtr points to a temporary holding 1.
So, I'd say this is LLVM bug.
Comment 12 Jakub Jelinek 2020-03-18 09:45:01 UTC
Though, admittedly fixing that:
     T *EltPtr = &Elt;
-    if (I <= EltPtr && EltPtr < this->EndX)
+    if ((std::uintptr_t) I <= (std::uintptr_t) EltPtr && (std::uintptr_t) EltPtr < (std::uintptr_t) this->EndX)
       ++EltPtr;
 
     *I = ::std::move(*EltPtr);
doesn't make the warning go away.
We have:
  int D.48534;
...
  <bb 30> [local count: 885408257]:
  # EltPtr_49 = PHI <&D.48534(27), &D.48534(28), &MEM <int> [(void *)&D.48534 + 4B](29)>
  _50 = MEM[(type &)EltPtr_49];
  *I_42 = _50;
before phiprop and phiprop changes that into:
  <bb 38> [local count: 442704129]:
  _33 = MEM[(type &)&D.48534];
  goto <bb 30>; [100.00%]
...
  <bb 39> [local count: 221352064]:
  _34 = MEM[(type &)&D.48534];
  goto <bb 30>; [100.00%]
  
  <bb 29> [local count: 221352064]:
  _48 = MEM[(type &)&D.48534 + 4];
    
  <bb 30> [local count: 885408257]:
  # EltPtr_49 = PHI <&MEM <int> [(void *)&D.48534 + 4B](29), &D.48534(38), &D.48534(39)>
  # _50 = PHI <_48(29), _33(38), _34(39)>
  *I_42 = _50;
where the _48 load is clearly UB, but with -fsanitize=address the compiler can't prove that it is unreachable, or with the adjusted testcase either.
So we are back to the dozens of other PRs of this kind, late warnings warning on UB in dead code and the question what the users actually want, whether a (false positive) warning, or no warning, or whether the compiler should replace the UB code with __builtin_unreachable and let the code be further simplified, or replace it with __builtin_trap.
Comment 13 Richard Biener 2021-06-01 08:14:48 UTC
GCC 9.4 is being released, retargeting bugs to GCC 9.5.
Comment 14 Richard Biener 2022-05-27 09:41:10 UTC
GCC 9 branch is being closed
Comment 15 Jakub Jelinek 2022-06-28 10:37:59 UTC
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
Comment 16 Richard Biener 2023-07-07 10:35:41 UTC
GCC 10 branch is being closed.