[asan] Patch - fix an ICE in asan.c

[asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

The attached test case ICEs (segfault) both on the asan branch and on 
the trunk with Dodji's patches:

fail31.ii: In static member function 'static std::size_t 
std::char_traits<char>::length(const char_type*)':
fail31.ii:13:19: internal compiler error: Segmentation fault
      static size_t length (const char_type * __s)
                    ^
0xae02ef crash_signal
         /projects/tob/gcc-git/gcc/gcc/toplev.c:334
0xaf031d gsi_next
         /projects/tob/gcc-git/gcc/gcc/gimple.h:5072
0xaf031d transform_statements
         /projects/tob/gcc-git/gcc/gcc/asan.c:1357
0xaf031d asan_instrument
         /projects/tob/gcc-git/gcc/gcc/asan.c:1556



The problem is in asa.c's transform_statements:

   FOR_EACH_BB (bb)
     {
       if (bb->index >= saved_last_basic_block) continue;
       for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
         {
           gimple s = gsi_stmt (i);

           if (gimple_assign_single_p (s))
             instrument_assignment (&i);
           else if (is_gimple_call (s))
             maybe_instrument_call (&i);
     }


Here, "gsi_end_p(i)" is the check "i->ptr == NULL" and gsi_next(&i) is 
"i->ptr = i->ptr->gsbase.next;"

Thus, it looks fine at a glance. However, the problem is that the 
gsi_end_p check is done before the loop body while "gsi_next" is called 
after the loop body. That's fine unless "i" is modified in between, 
which happens in

instrument_strlen_call (gimple_stmt_iterator *iter)
...
   gimple_stmt_iterator gsi = *iter;
...
   *iter = gsi;
}

After the call, iter->ptr == NULL.


Is the patch okay for the ASAN branch?*

Tobias

* I still have to do an all-language bootstrap and regtesting, though 
the latter is probably pointless as there is currently not a single 
-fasan test case.

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 315 bytes --]

--- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
+++ gcc/asan.c	2012-11-09 21:26:00.000000000 +0100
@@ -1362,6 +1362,8 @@ transform_statements (void)
 	    instrument_assignment (&i);
 	  else if (is_gimple_call (s))
 	    maybe_instrument_call (&i);
+	  if (gsi_end_p (i))
+	    break;
         }
     }
 }

[-- Attachment #3: fail31.ii --]
[-- Type: text/plain, Size: 1462 bytes --]

namespace std
{
  template < typename _Alloc > class allocator;
  template < class _CharT > struct char_traits;
    template < typename _CharT, typename _Traits =
    char_traits < _CharT >, typename _Alloc =
    allocator < _CharT > >class basic_string;
  typedef basic_string < char >string;
  typedef long unsigned int size_t;
    template <> struct char_traits <char >
  {
    typedef char char_type;
    static size_t length (const char_type * __s)
    {
      return __builtin_strlen (__s);
    }
  };
  namespace __gnu_cxx
  {
    template < typename _Tp > class new_allocator
    {
    public:
      typedef size_t size_type;
        template < typename _Tp1 > struct rebind
      {
	typedef new_allocator < _Tp1 > other;
      };
    };
  }
template < typename _Tp > class allocator:public __gnu_cxx::new_allocator <
    _Tp >
  {
  };
  template < typename _CharT, typename _Traits, typename _Alloc >
    class basic_string
  {
    typedef typename _Alloc::template rebind <
      _CharT >::other _CharT_alloc_type;
    typedef _Traits traits_type;
    typedef typename _CharT_alloc_type::size_type size_type;
  public:
    basic_string & operator= (const _CharT * __s)
    {
      return this->assign (__s, traits_type::length (__s));
    }
    basic_string & assign (const _CharT * __s, size_type __n);
  };

  class Regex
  {
    std::string sub (std::string * Error);
  };

  std::string Regex::sub (std::string * Error)
  {
    *Error = "";
  }
}

Re: [asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

Tobias Burnus wrote:
> The attached test case ICEs (segfault) both on the asan branch and on
> the trunk with Dodji's patches:

I found another ICE - this time without a patch.

[That's with the patch, which I posted in this thread. Without, one 
seems to run into the problem I tried to fix with the patch.]

[As ASAN is not yet in the trunk, it is not yet suitable for a PR - but 
on the other hand, I am afraid that I loose it. Thus, I dump it here, 
which is also not the best place (sorry).]


The attached code generates (before ASAN):

StringSwitch<T, R>& StringSwitch ...
...
   <bb 2>:
   _2 = &this_1(D)->Str;
   _3 = StringRef::data (_2);
   memcmp (S_4(D), _3, 7);
   return;
}

And within this basic block, between "_3" and "memcpy", the generated 
ASAN code is added, which leads to an ICE and 10 times the message.


error: control flow in the middle of basic block 7


If one looks at the asan0 dump (after disabling this part of checking), 
one finds:

   _52 = _48 & _51;
   if (_52 != 0)
   _53 = (unsigned long) _22;

The "if" line looks odd as one would expect code of this form:

   if (_62 != 0)
     goto <bb 12>;
   else
     goto <bb 11>;

See attachments.

Tobias

[-- Attachment #2: fail10.ii --]
[-- Type: text/plain, Size: 558 bytes --]

extern "C"
{
  extern int memcmp (const void *__s1, const void *__s2,
		     long unsigned int __n) throw ();
}
struct StringRef
{
  const char *data () const { }
};
template < typename T, typename R = T > class StringSwitch
{
  StringRef Str;
  public:
     explicit StringSwitch (StringRef Str):Str (Str) { }
  template < unsigned N > StringSwitch & Case (const char (&S)[N])
  {
    memcmp (S, Str.data (), N - 1);
  }
  R Default () const { }
};

int getIt (StringRef Name)
{
  return StringSwitch < int > (Name)
    .Case ("unknown")
    .Default ();
}

[-- Attachment #3: fail10.ii.156t.asan0 --]
[-- Type: text/plain, Size: 4445 bytes --]


;; Function const char* StringRef::data() const (_ZNK9StringRef4dataEv, funcdef_no=0, decl_uid=2200, cgraph_uid=0)

const char* StringRef::data() const (const struct StringRef * const this)
{
  <bb 2>:
  GIMPLE_NOP
  return;

}



;; Function int getIt(StringRef) (_Z5getIt9StringRef, funcdef_no=4, decl_uid=2230, cgraph_uid=1)

int getIt(StringRef) (struct StringRef Name)
{
  struct StringSwitch & D.2293;
  struct StringRef D.2292;
  struct StringRef D.2277;
  struct StringSwitch D.2278;
  int D.2291;
  struct StringSwitch & _1;
  int _2;

  <bb 2>:
  StringSwitch<int>::StringSwitch (&D.2278, D.2292);
  _1 = StringSwitch<int>::Case<8u> (&D.2278, "unknown");
  _2 = StringSwitch<int>::Default (_1);
  D.2278 ={v} {CLOBBER};

<L1>:
  return _2;

}



;; Function StringSwitch<T, R>::StringSwitch(StringRef) [with T = int; R = int] (_ZN12StringSwitchIiiEC2E9StringRef, funcdef_no=6, decl_uid=2249, cgraph_uid=3)

StringSwitch<T, R>::StringSwitch(StringRef) [with T = int; R = int] (struct StringSwitch * const this, struct StringRef Str)
{
  <bb 2>:
  return;

}



;; Function StringSwitch<T, R>& StringSwitch<T, R>::Case(const char (&)[N]) [with unsigned int N = 8u; T = int; R = int] (_ZN12StringSwitchIiiE4CaseILj8EEERS0_RAT__Kc, funcdef_no=8, decl_uid=2279, cgraph_uid=5)

StringSwitch<T, R>& StringSwitch<T, R>::Case(const char (&)[N]) [with unsigned int N = 8u; T = int; R = int] (struct StringSwitch * const this, const char[8] & S)
{
  const char * D.2297;
  struct StringRef * D.2296;
  struct StringRef * _2;
  const char * _3;
  unsigned long _5;
  unsigned long _6;
  unsigned long _7;
  signed char * _8;
  signed char _9;
  bool _10;
  unsigned long _11;
  signed char _12;
  bool _13;
  bool _14;
  long unsigned int _15;
  long unsigned int _16;
  const char[8] & _17;
  const char[8] & _18;
  unsigned long _19;
  unsigned long _20;
  unsigned long _21;
  signed char * _22;
  signed char _23;
  bool _24;
  unsigned long _25;
  signed char _26;
  bool _27;
  bool _28;
  unsigned long _29;
  unsigned long _30;
  unsigned long _31;
  signed char * _32;
  signed char _33;
  bool _34;
  unsigned long _35;
  signed char _36;
  bool _37;
  bool _38;
  long unsigned int _39;
  long unsigned int _40;
  const char * _41;
  const char * _42;
  unsigned long _43;
  unsigned long _44;
  unsigned long _45;
  signed char * _46;
  signed char _47;
  bool _48;
  unsigned long _49;
  signed char _50;
  bool _51;
  bool _52;
  unsigned long _53;
  unsigned long _54;
  unsigned long _55;
  signed char * _56;
  signed char _57;
  bool _58;
  unsigned long _59;
  signed char _60;
  bool _61;
  bool _62;

  <bb 2>:
  _2 = &this_1(D)->Str;
  _3 = StringRef::data (_2);
  _5 = (unsigned long) S_4(D);
  _6 = _5 >> 3;
  _7 = _6 + 17592186044416;
  _8 = (signed char *) _7;
  _9 = *_8;
  _10 = _9 != 0;
  _11 = _5 & 7;
  _12 = (signed char) _11;
  _13 = _12 >= _9;
  _14 = _10 & _13;
  if (_14 != 0)
    goto <bb 4>;
  else
    goto <bb 3>;

  <bb 4>:
  __asan_report_load1 (_5);

  <bb 3>:
  _29 = (unsigned long) _3;
  _30 = _29 >> 3;
  _31 = _30 + 17592186044416;
  _32 = (signed char *) _31;
  _33 = *_32;
  _34 = _33 != 0;
  _35 = _29 & 7;
  _36 = (signed char) _35;
  _37 = _36 >= _33;
  _38 = _34 & _37;
  if (_38 != 0)
    goto <bb 8>;
  else
    goto <bb 7>;

  <bb 8>:
  __asan_report_load1 (_29);

  <bb 7>:
  _39 = 7;
  _40 = _39 - 1;
  _41 = _3;
  _42 = _41 + _40;
  _43 = (unsigned long) _42;
  _44 = _43 >> 3;
  _45 = _44 + 17592186044416;
  _46 = (signed char *) _45;
  _47 = *_46;
  _48 = _47 != 0;
  _49 = _43 & 7;
  _50 = (signed char) _49;
  _51 = _50 >= _47;
  _52 = _48 & _51;
  if (_52 != 0)
  _53 = (unsigned long) _22;
  _54 = _53 >> 3;
  _55 = _54 + 17592186044416;
  _56 = (signed char *) _55;
  _57 = *_56;
  _58 = _57 != 0;
  _59 = _53 & 7;
  _60 = (signed char) _59;
  _61 = _60 >= _57;
  _62 = _58 & _61;
  if (_62 != 0)
    goto <bb 12>;
  else
    goto <bb 11>;

  <bb 12>:
  __asan_report_load1 (_53);

  <bb 11>:

  <bb 10>:
  __asan_report_load1 (_43);

  <bb 9>:
  _15 = 7;
  _16 = _15 - 1;
  _17 = S_4(D);
  _18 = _17 + _16;
  _19 = (unsigned long) _18;
  _20 = _19 >> 3;
  _21 = _20 + 17592186044416;
  _22 = (signed char *) _21;
  _23 = *_22;
  _24 = _23 != 0;
  _25 = _19 & 7;
  _26 = (signed char) _25;
  _27 = _26 >= _23;
  _28 = _24 & _27;
  if (_28 != 0)
    goto <bb 6>;
  else
    goto <bb 5>;

  <bb 6>:
  __asan_report_load1 (_19);

  <bb 5>:
  memcmp (S_4(D), _3, 7);
  return;

}

Re: [asan] Patch - fix an ICE in asan.c

[Jakub Jelinek]

On Fri, Nov 09, 2012 at 09:36:53PM +0100, Tobias Burnus wrote:
> * I still have to do an all-language bootstrap and regtesting,
> though the latter is probably pointless as there is currently not a
> single -fasan test case.

> --- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
> +++ gcc/asan.c	2012-11-09 21:26:00.000000000 +0100
> @@ -1362,6 +1362,8 @@ transform_statements (void)
>  	    instrument_assignment (&i);
>  	  else if (is_gimple_call (s))
>  	    maybe_instrument_call (&i);
> +	  if (gsi_end_p (i))
> +	    break;
>          }
>      }
>  }

That looks a wrong place for this.  Instead, maybe_instrument_call
should ensure that *iter is set to the last stmt that shouldn't be
instrumented.  instrument_derefs does that correctly, so assignments and
__atomic/__sync builtins should be correct (*iter is set to the
assignment/call), for strlen call it seems to DTRT, but for other builtin
calls it would leave *iter elsewhere.  As we want to scan for accesses
the rest of the bb that contained the call (but that bb after splitting
already is above the highest bb number to be insturmented), we
need to keep *iter at the call we just processed, so if there are say
two consecutive calls the second one is going to be processed.

So untested:

2012-11-10  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (maybe_instrument_builtin_call): Set *iter
	to gsi for the call at the end.

--- gcc/asan.c.jj	2012-11-02 00:09:22.000000000 +0100
+++ gcc/asan.c	2012-11-10 10:00:03.717715834 +0100
@@ -1191,6 +1191,7 @@ maybe_instrument_builtin_call (gimple_st
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
       return true;
     }
   return false;


	Jakub

Re: [asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

Jakub Jelinek wrote:
>> --- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
>> +++ gcc/asan.c	2012-11-09 21:26:00.000000000 +0100
>> @@ -1362,6 +1362,8 @@ transform_statements (void)
>>   	    instrument_assignment (&i);
>>   	  else if (is_gimple_call (s))
>>   	    maybe_instrument_call (&i);
>> +	  if (gsi_end_p (i))
>> +	    break;
>>           }
>>       }
>>   }
>
> That looks a wrong place for this.

I already expected that it was not fully correct ;-)


> So untested:

Thanks for the patch! It fixed the problem half way: It fixes the second 
issue I had (fail10.ii, 
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00791.html ).

However, it didn't fix the original problem: As the call for strlen 
directly returns, it never reaches your patch. Hence, it doesn't fix 
fail31.ii of http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00786.html

If one uses the same line for strlen, it works.

Updated patches attached – one is on top of the trunk + Dodji's patches, 
the other is on top of the asan branch.

  * * *

The question is whether one also needs to do something for the atomics 
handling in maybe_instrument_builtin_call, which has:
         instrument_derefs (iter, dest, loc, is_store);
         return;

The instrument_derefs calls - in some cases - build_check_stmt, which in 
turn calls:
   *iter = gsi_start_bb (else_bb)

Tobias

[-- Attachment #2: asan-for-trunk.diff --]
[-- Type: text/x-patch, Size: 834 bytes --]

(This patch is for the "trunk" after the asan integration patches.)

2012-11-10  Jakub Jelinek  <jakub@redhat.com>
            Tobias Burnus  <burnus@net-b.de>

        * asan.c (maybe_instrument_builtin_call): Set *iter
        to gsi for the call at the end.

--- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
+++ gcc/asan.c	2012-11-10 13:44:51.000000000 +0100
@@ -1068,6 +1068,7 @@ instrument_builtin_call (gimple_stmt_ite
 
     case BUILT_IN_STRLEN:
       instrument_strlen_call (iter);
+      *iter = gsi_for_stmt (call);
       return;
 
     /* And now the __atomic* and __sync builtins.
@@ -1307,6 +1308,7 @@ instrument_builtin_call (gimple_stmt_ite
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
     }
 }
 

[-- Attachment #3: asan-branch-patch.diff --]
[-- Type: text/x-patch, Size: 849 bytes --]

(This patch is for the "asan" branch.)

2012-11-10  Jakub Jelinek  <jakub@redhat.com>
	    Tobias Burnus  <burnus@net-b.de>

	* asan.c (maybe_instrument_builtin_call): Set *iter
	to gsi for the call at the end.

diff --git a/gcc/asan.c b/gcc/asan.c
index 155e84b..3297b52 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -952,6 +952,7 @@ maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
 
     case BUILT_IN_STRLEN:
       instrument_strlen_call (iter);
+      *iter = gsi_for_stmt (call);
       return true;
 
     /* And now the __atomic* and __sync builtins.
@@ -1191,6 +1192,7 @@ maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
       return true;
     }
   return false;

Re: [asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

Tobias Burnus wrote:
>> So untested:
>
> Thanks for the patch! It fixed the problem half way: It fixes the 
> second issue I had (fail10.ii, 
> http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00791.html ).
>
> However, it didn't fix the original problem: As the call for strlen 
> directly returns, it never reaches your patch. Hence, it doesn't fix 
> fail31.ii of http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00786.html
>
> If one uses the same line for strlen, it works.

I spoke too early. With the updated patch, there is no ICE, but one 
crashes for the following valid program with:

==27313== ERROR: AddressSanitizer crashed on unknown address 
0x13fffe8fc737 (pc 0x0000004008e3 sp 0x7fffa3f1c6d0 bp 0x7fffa3f1c700 T0)
AddressSanitizer can not provide additional info.


The crucial part is the "strlen" call.


#include <stdio.h>
#include <string.h>

int main(int argc, char *argv[])
{
  int i;
  for (i = 0; i < argc; i++)
    {
      printf("%d: '%s':%zd\n", i, argv[i], strlen(argv[i]));
    }
  return 0;
}


Tobias

Re: [asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

Tobias Burnus wrote:
> I spoke too early. With the updated patch, there is no ICE, but one 
> crashes for the following valid program with:

But with my original patch, it works.

To recap: My "if (gsi_end_p (i)) break;" (cf. [1]) fixes my original 
issue (ICE for fail31.ii; [1]) and gives the correct diagnostic at run 
time for strlen in the code [4] (both for correct and out-of-bounds 
programs).

While Jakub's "*iter = gsi_for_stmt (call);" (cf. [3]) fixes the ICE for 
my fail10.ii program [2]; I haven't tried to construct a run-time 
version for that code.

Updated patches attached (for the "asan" branch and for the trunk on top 
of Dodji's patches; I have only tested the latter).

Hopefully, the test suite will be working soon, it should help finding 
such issues.

Tobias

[1] fail31.ii (strlen ICE): 
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00786.html
[2] fail10.ii (control flow in BB ICE): 
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00791.html
[3] Jakub's patch: http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00801.html
[4] strlen run test: http://gcc.gnu.org/ml/gcc-patches/2012-11/msg00809.html

[-- Attachment #2: asan-branch-patch.diff --]
[-- Type: text/x-patch, Size: 1261 bytes --]

(This patch is for the "asan" branch.)

2012-11-10  Tobias Burnus  <burnus@net-b.de>
	    Jakub Jelinek  <jakub@redhat.com>

        * asan.c (maybe_instrument_builtin_call): Set *iter
        to gsi for the call at the end.
	(transform_statements): Leave loop when gsi_end_p.

diff --git a/gcc/asan.c b/gcc/asan.c
index 155e84b..f5e357a 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1187,14 +1187,15 @@ maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
 				      loc, /*is_store=*/false);
       if (source1 != NULL_TREE)
 	instrument_mem_region_access (source1, len, iter,
 				      loc, /*is_store=*/false);
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
       return true;
     }
   return false;
 }
 
 /*  Instrument the assignment statement ITER if it is subject to
     instrumentation.  */
@@ -1243,14 +1244,16 @@ transform_statements (void)
         {
 	  gimple s = gsi_stmt (i);
 
 	  if (gimple_assign_single_p (s))
 	    instrument_assignment (&i);
 	  else if (is_gimple_call (s))
 	    maybe_instrument_call (&i);
+         if (gsi_end_p (i))
+           break;
         }
     }
 }
 
 /* Build
    struct __asan_global
    {

[-- Attachment #3: asan-for-trunk.diff --]
[-- Type: text/x-patch, Size: 1442 bytes --]

(This patch is for the trunk after the "asan" patch has been applied.)

2012-11-10  Tobias Burnus  <burnus@net-b.de>
	    Jakub Jelinek  <jakub@redhat.com>

        * asan.c (maybe_instrument_builtin_call): Set *iter
        to gsi for the call at the end.
	(transform_statements): Leave loop when gsi_end_p.

--- gcc/asan.c.orig	2012-11-09 21:26:26.000000000 +0100
+++ gcc/asan.c	2012-11-10 19:23:33.000000000 +0100
@@ -1302,16 +1302,17 @@ instrument_builtin_call (gimple_stmt_ite
 	instrument_mem_region_access (source0, len, iter,
 				      loc, /*is_store=*/false);
       if (source1 != NULL_TREE)
 	instrument_mem_region_access (source1, len, iter,
 				      loc, /*is_store=*/false);
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
+      *iter = gsi_for_stmt (call);
     }
 }
 
 /*  Instrument the assignment statement ITER if it is subject to
     instrumentation.  */
 
 static void
 instrument_assignment (gimple_stmt_iterator *iter)
@@ -1357,16 +1358,18 @@ transform_statements (void)
       for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
         {
 	  gimple s = gsi_stmt (i);
 
 	  if (gimple_assign_single_p (s))
 	    instrument_assignment (&i);
 	  else if (is_gimple_call (s))
 	    maybe_instrument_call (&i);
+	  if (gsi_end_p (i))
+	    break;
         }
     }
 }
 
 /* Build
    struct __asan_global
    {
      const void *__beg;

Re: [asan] Patch - fix an ICE in asan.c

[Jakub Jelinek]

On Sat, Nov 10, 2012 at 07:54:34PM +0100, Tobias Burnus wrote:
> 2012-11-10  Tobias Burnus  <burnus@net-b.de>
> 	    Jakub Jelinek  <jakub@redhat.com>
> 
>         * asan.c (maybe_instrument_builtin_call): Set *iter
>         to gsi for the call at the end.
> 	(transform_statements): Leave loop when gsi_end_p.

But IMHO it is still wrong.  On e.g.
typedef __SIZE_TYPE__ size_t;

size_t
foo (size_t x, char *y)
{
  if (x)
    x = __builtin_strlen (y);
  return x;
}

size_t
bar (const char *y, const char *z)
{
  size_t a, b;
  a = __builtin_strlen (y);
  b = __builtin_strlen (z);
  return a + b;
}
you don't want to crash in foo (where there are no statements after
strlen in the same bb (with my previous patch it would still crash),
but in bar you also want to instrument both calls, not just the first one.
So, for strlen we need to make sure gsi_next isn't performed in
transform_statements.

2012-11-12  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (instrument_strlen_call): Return bool whether the call has
	been instrumented.
	(maybe_instrument_builtin_call): Change return value to mean whether
	caller should avoid gsi_next before processing next statement.  Pass
	thru return value from instrument_strlen_call.  Set *iter to gsi for
	the call at the end.
	(maybe_instrument_call): Return bool whether caller should avoid
	gsi_next.
	(transform_statements): Don't do gsi_next if maybe_instrument_call
	returned true.

--- gcc/asan.c.jj	2012-11-02 00:09:22.000000000 +0100
+++ gcc/asan.c	2012-11-12 09:34:03.226777133 +0100
@@ -824,7 +824,7 @@ instrument_mem_region_access (tree base,
    access to the last byte of the argument; it uses the result of the
    call to deduce the offset of that last byte.  */
 
-static void
+static bool
 instrument_strlen_call (gimple_stmt_iterator *iter)
 {
   gimple call = gsi_stmt (*iter);
@@ -839,7 +839,7 @@ instrument_strlen_call (gimple_stmt_iter
   if (len == NULL)
     /* Some passes might clear the return value of the strlen call;
        bail out in that case.  */
-    return;
+    return false;
   gcc_assert (INTEGRAL_TYPE_P (TREE_TYPE (len)));
 
   location_t loc = gimple_location (call);
@@ -881,13 +881,20 @@ instrument_strlen_call (gimple_stmt_iter
 		    /*before_p=*/false, /*is_store=*/false, 1);
 
   /* Ensure that iter points to the statement logically following the
-     one it was initially pointing to.  */
+     one it was initially pointing to.  Return true to inform
+     transform_statements that it shouldn't do gsi_next (&i) in that
+     case, that statement is the first statement in a block that would
+     be otherwise skipped (too high block number), by doing gsi_next (&i)
+     either that statement wouldn't be instrumented or, if there are no
+     statement, transform_statements could crash in gsi_next (&i).  */
   *iter = gsi;
+  return true;
 }
 
-/* if the statement pointed to by the iterator iter is a call to a
-   builtin memory access function, instrument it and return true.
-   otherwise, return false.  */
+/* If the statement pointed to by the iterator iter is a call to a
+   builtin memory access function, instrument it.  Return true
+   if *ITER should be processed next, false if gsi_next should
+   be done on it first.  */
 
 static bool
 maybe_instrument_builtin_call (gimple_stmt_iterator *iter)
@@ -951,8 +958,7 @@ maybe_instrument_builtin_call (gimple_st
       break;
 
     case BUILT_IN_STRLEN:
-      instrument_strlen_call (iter);
-      return true;
+      return instrument_strlen_call (iter);
 
     /* And now the __atomic* and __sync builtins.
        These are handled differently from the classical memory memory
@@ -1170,7 +1176,7 @@ maybe_instrument_builtin_call (gimple_st
 	  gcc_unreachable ();
 
 	instrument_derefs (iter, dest, loc, is_store);
-	return true;
+	return false;
       }
 
     default:
@@ -1191,7 +1197,8 @@ maybe_instrument_builtin_call (gimple_st
       else if (dest != NULL_TREE)
 	instrument_mem_region_access (dest, len, iter,
 				      loc, /*is_store=*/true);
-      return true;
+      *iter = gsi_for_stmt (call);
+      return false;
     }
   return false;
 }
@@ -1217,10 +1224,10 @@ instrument_assignment (gimple_stmt_itera
    calls that are instrumented are some built-in functions that access
    memory.  Look at maybe_instrument_builtin_call to learn more.  */
 
-static void
+static bool
 maybe_instrument_call (gimple_stmt_iterator *iter)
 {
-  maybe_instrument_builtin_call (iter);
+  return maybe_instrument_builtin_call (iter);
 }
 
 /* asan: this looks too complex. Can this be done simpler? */
@@ -1239,14 +1246,19 @@ transform_statements (void)
   FOR_EACH_BB (bb)
     {
       if (bb->index >= saved_last_basic_block) continue;
-      for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
+      for (i = gsi_start_bb (bb); !gsi_end_p (i); )
         {
 	  gimple s = gsi_stmt (i);
 
 	  if (gimple_assign_single_p (s))
 	    instrument_assignment (&i);
 	  else if (is_gimple_call (s))
-	    maybe_instrument_call (&i);
+	    {
+	      if (maybe_instrument_call (&i))
+		/* Avoid gsi_next (&i).  */
+		continue;
+	    }
+          gsi_next (&i);
         }
     }
 }


	Jakub

Re: [asan] Patch - fix an ICE in asan.c

[Dodji Seketeli]

Tobias Burnus <burnus@net-b.de> writes:

> The attached test case ICEs (segfault) both on the asan branch and on
> the trunk with Dodji's patches:

Thank you for reporting this.

I believe the neqw series of patches I have juste posted address this
issue.

To ease the testing, you can check out an updated git tree with all
these patches cleanly stacked on top of trunk by doing:

    git clone -b asan-merge-assemble git://seketeli.net/~dodji/gcc.git

You can browse the patchset it via the web at: 

    http://seketeli.net/git/~dodji/gcc.git/log/?h=asan-merge-assemble

Cheers.

-- 
		Dodji

Re: [asan] Patch - fix an ICE in asan.c

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

Dodji Seketeli wrote:
> I believe the neqw series of patches I have juste posted address this 
> issue.

Thanks a lot for your merging work! I am really looking forward to have 
it available on the trunk!

I guess that your updated patch fixes the issue as it incorporates 
Jakub's patch. (I have not yet re-build your merge branch but have just 
applied Jakub's patch to the "asan" branch. Cloning your merge branch 
takes quite some time and due to --rebase, a simple "git update" doesn't 
work.)

  * * *

I have two minor issues which can be fixed after committal of the 
patches to the trunk.


First, I have a small hyphen fix patch, which is on top of your merge 
branch. (The "asan" branch itself is okay.)

--- invoke.texi.orig    2012-11-12 15:41:31.000000000 +0100
+++ invoke.texi 2012-11-12 15:16:33.856424039 +0100
@@ -356,5 +356,5 @@ Objective-C and Objective-C++ Dialects}.
  -falign-labels[=@var{n}] -falign-loops[=@var{n}] -faddress-sanitizer @gol
---fassociative-math fauto-inc-dec -fbranch-probabilities @gol
---fbranch-target-load-optimize fbranch-target-load-optimize2 @gol
---fbtr-bb-exclusive -fcaller-saves @gol
+-fassociative-math -fauto-inc-dec -fbranch-probabilities @gol
+-fbranch-target-load-optimize -fbranch-target-load-optimize2 @gol
+-fbtr-bb-exclusive -fcaller-saves @gol
  -fcheck-data-deps -fcombine-stack-adjustments -fconserve-stack @gol


  * * *

Secondly, the following code fails on both the asan branch and on the 
merge branch with an ICE:

void TI_ASM_Pack_Inst (const int *opnd)
{
   int bopnd[5];
   __builtin_bcopy(opnd, bopnd, sizeof (bopnd));
}


The ICE is:

fail7.i:1:6: error: type mismatch in pointer plus expression
   _40 = _39 + _38;
as the operands aren't POINTER_P.


That's fixed by the following patch; I am not sure whether it is fully 
correct, but that patched line is used for both pointers and nonpointers 
in *this* example.

Tobias

[-- Attachment #2: pointer-plus.diff --]
[-- Type: text/x-patch, Size: 504 bytes --]

diff --git a/gcc/asan.c b/gcc/asan.c
index 639dd9f..42f1abe 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -928,7 +928,8 @@ instrument_mem_region_access (tree base, tree len,
 
   /* _2 = _1 + offset;  */
   region_end =
-    gimple_build_assign_with_ops (POINTER_PLUS_EXPR,
+    gimple_build_assign_with_ops (POINTER_TYPE_P (TREE_TYPE (base))
+				  ? POINTER_PLUS_EXPR : PLUS_EXPR,
 				  make_ssa_name (TREE_TYPE (base), NULL),
 				  gimple_assign_lhs (region_end), 
 				  gimple_assign_lhs (offset));

Re: [asan] Patch - fix an ICE in asan.c

[Jakub Jelinek]

On Mon, Nov 12, 2012 at 04:45:55PM +0100, Tobias Burnus wrote:
> First, I have a small hyphen fix patch, which is on top of your
> merge branch. (The "asan" branch itself is okay.)

This patch is preapproved with appropriate ChangeLog entry.
Thanks.

> --- invoke.texi.orig    2012-11-12 15:41:31.000000000 +0100
> +++ invoke.texi 2012-11-12 15:16:33.856424039 +0100
> @@ -356,5 +356,5 @@ Objective-C and Objective-C++ Dialects}.
>  -falign-labels[=@var{n}] -falign-loops[=@var{n}] -faddress-sanitizer @gol
> ---fassociative-math fauto-inc-dec -fbranch-probabilities @gol
> ---fbranch-target-load-optimize fbranch-target-load-optimize2 @gol
> ---fbtr-bb-exclusive -fcaller-saves @gol
> +-fassociative-math -fauto-inc-dec -fbranch-probabilities @gol
> +-fbranch-target-load-optimize -fbranch-target-load-optimize2 @gol
> +-fbtr-bb-exclusive -fcaller-saves @gol
>  -fcheck-data-deps -fcombine-stack-adjustments -fconserve-stack @gol

> Secondly, the following code fails on both the asan branch and on
> the merge branch with an ICE:
> 
> void TI_ASM_Pack_Inst (const int *opnd)
> {
>   int bopnd[5];
>   __builtin_bcopy(opnd, bopnd, sizeof (bopnd));
> }

The bug is elsewhere, the following patch should fix this
(and I've reordered the assignments according to the call arg
number, so that it is more readable at the same time).
Ok for trunk?

2012-11-12  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (instrument_builtin_call) <case BUILT_IN_BCOPY>: Fix up
	dest assignment.

--- gcc/asan.c.jj	2012-11-12 17:16:16.000000000 +0100
+++ gcc/asan.c	2012-11-12 17:39:19.673022734 +0100
@@ -1044,16 +1044,16 @@ instrument_builtin_call (gimple_stmt_ite
       /* (s, s, n) style memops.  */
     case BUILT_IN_BCMP:
     case BUILT_IN_MEMCMP:
-      len = gimple_call_arg (call, 2);
       source0 = gimple_call_arg (call, 0);
       source1 = gimple_call_arg (call, 1);
+      len = gimple_call_arg (call, 2);
       break;
 
       /* (src, dest, n) style memops.  */
     case BUILT_IN_BCOPY:
-      len = gimple_call_arg (call, 2);
       source0 = gimple_call_arg (call, 0);
-      dest = gimple_call_arg (call, 2);
+      dest = gimple_call_arg (call, 1);
+      len = gimple_call_arg (call, 2);
       break;
 
       /* (dest, src, n) style memops.  */


	Jakub

Re: [asan] Patch - fix an ICE in asan.c

[Dodji Seketeli]

Jakub Jelinek <jakub@redhat.com> writes:

> The bug is elsewhere, the following patch should fix this
> (and I've reordered the assignments according to the call arg
> number, so that it is more readable at the same time).
> Ok for trunk?
>
> 2012-11-12  Jakub Jelinek  <jakub@redhat.com>
>
> 	* asan.c (instrument_builtin_call) <case BUILT_IN_BCOPY>: Fix up
> 	dest assignment.
>
> --- gcc/asan.c.jj	2012-11-12 17:16:16.000000000 +0100
> +++ gcc/asan.c	2012-11-12 17:39:19.673022734 +0100
> @@ -1044,16 +1044,16 @@ instrument_builtin_call (gimple_stmt_ite
>        /* (s, s, n) style memops.  */
>      case BUILT_IN_BCMP:
>      case BUILT_IN_MEMCMP:
> -      len = gimple_call_arg (call, 2);
>        source0 = gimple_call_arg (call, 0);
>        source1 = gimple_call_arg (call, 1);
> +      len = gimple_call_arg (call, 2);
>        break;
>  
>        /* (src, dest, n) style memops.  */
>      case BUILT_IN_BCOPY:
> -      len = gimple_call_arg (call, 2);
>        source0 = gimple_call_arg (call, 0);
> -      dest = gimple_call_arg (call, 2);
> +      dest = gimple_call_arg (call, 1);
> +      len = gimple_call_arg (call, 2);
>        break;
>  
>        /* (dest, src, n) style memops.  */

Indeed.  I was about to send a similar patch after Tobias' report.

Thanks.

-- 
		Dodji

Re: [asan] Patch - fix an ICE in asan.c

[Markus Trippelsdorf]

Another ICE:

 % cat test.ii
 int i;

 % g++ -faddress-sanitizer -c -g -O1 test.ii
test.ii:1:7: internal compiler error: Segmentation fault
  int i;
       ^
0xa5cb5f crash_signal
        /home/markus/gcc/gcc/toplev.c:334
inconsistent DWARF line number info
0x4cf588 cp_classify_record
        /home/markus/gcc/gcc/cp/cp-lang.c:131
0x7ee7e3 record_type_tag
        /home/markus/gcc/gcc/dwarf2out.c:16914
0x7ee7e3 gen_struct_or_union_type_die
        /home/markus/gcc/gcc/dwarf2out.c:19078
0x7ee7e3 gen_tagged_type_die
        /home/markus/gcc/gcc/dwarf2out.c:19303
0x7ee7e3 gen_tagged_type_die
        /home/markus/gcc/gcc/dwarf2out.c:19246
0x7f5dce gen_type_die_with_usage
        /home/markus/gcc/gcc/dwarf2out.c:19450
0x7f6b9c modified_type_die
        /home/markus/gcc/gcc/dwarf2out.c:10180
0x7f68c5 modified_type_die
        /home/markus/gcc/gcc/dwarf2out.c:10238
0x7f8cd0 add_type_attribute
        /home/markus/gcc/gcc/dwarf2out.c:16486
0x7f4bf1 gen_formal_parameter_die
        /home/markus/gcc/gcc/dwarf2out.c:17079
0x7f520b gen_formal_types_die
        /home/markus/gcc/gcc/dwarf2out.c:17175
0x7f181c gen_subprogram_die
        /home/markus/gcc/gcc/dwarf2out.c:17909
0x7f8075 force_decl_die
        /home/markus/gcc/gcc/dwarf2out.c:19733
0x7f8721 resolve_addr
        /home/markus/gcc/gcc/dwarf2out.c:22661
0x7f83f9 resolve_addr
        /home/markus/gcc/gcc/dwarf2out.c:22683
0x7f83f9 resolve_addr
        /home/markus/gcc/gcc/dwarf2out.c:22683
0x807392 dwarf2out_finish
        /home/markus/gcc/gcc/dwarf2out.c:23305
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.
-- 
Markus

Re: [asan] Patch - fix an ICE in asan.c

[Jakub Jelinek]

On Mon, Nov 12, 2012 at 06:13:08PM +0100, Markus Trippelsdorf wrote:
> Another ICE:
> 
>  % cat test.ii
>  int i;
> 
>  % g++ -faddress-sanitizer -c -g -O1 test.ii
> test.ii:1:7: internal compiler error: Segmentation fault
>   int i;
>        ^

The RECORD_TYPE doesn't have lang specific payload allocated for it.
There is no point why we need to describe that in the debug info though.

So I'd do something like:

2012-11-12  Jakub Jelinek  <jakub@redhat.com>

	* asan.c (report_error_func): Set DECL_IGNORED_P, don't touch
	DECL_ASSEMBLER_NAME.
	(asan_init_func): Likewise.
	(asan_finish_file): Use void * instead of __asan_global * as
	type of __asan_{,un}register_globals.  Set DECL_IGNORED_P on
	the decls.

--- gcc/asan.c.jj	2012-11-12 17:39:19.000000000 +0100
+++ gcc/asan.c	2012-11-12 18:54:57.164914324 +0100
@@ -493,10 +493,10 @@ report_error_func (bool is_store, int si
   fn_type = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
   def = build_fn_decl (name, fn_type);
   TREE_NOTHROW (def) = 1;
+  DECL_IGNORED_P (def) = 1;
   TREE_THIS_VOLATILE (def) = 1;  /* Attribute noreturn. Surprise!  */
   DECL_ATTRIBUTES (def) = tree_cons (get_identifier ("leaf"),
 				     NULL, DECL_ATTRIBUTES (def));
-  DECL_ASSEMBLER_NAME (def);
   return def;
 }
 
@@ -511,7 +511,7 @@ asan_init_func (void)
   fn_type = build_function_type_list (void_type_node, NULL_TREE);
   def = build_fn_decl ("__asan_init", fn_type);
   TREE_NOTHROW (def) = 1;
-  DECL_ASSEMBLER_NAME (def);
+  DECL_IGNORED_P (def) = 1;
   return def;
 }
 
@@ -1535,11 +1535,11 @@ asan_finish_file (void)
       DECL_INITIAL (var) = ctor;
       varpool_assemble_decl (varpool_node_for_decl (var));
 
-      type = build_function_type_list (void_type_node,
-				       build_pointer_type (TREE_TYPE (type)),
+      type = build_function_type_list (void_type_node, ptr_type_node,
 				       uptr, NULL_TREE);
       decl = build_fn_decl ("__asan_register_globals", type);
       TREE_NOTHROW (decl) = 1;
+      DECL_IGNORED_P (decl) = 1;
       append_to_statement_list (build_call_expr (decl, 2,
 						 build_fold_addr_expr (var),
 						 build_int_cst (uptr, gcount)),
@@ -1547,6 +1547,7 @@ asan_finish_file (void)
 
       decl = build_fn_decl ("__asan_unregister_globals", type);
       TREE_NOTHROW (decl) = 1;
+      DECL_IGNORED_P (decl) = 1;
       append_to_statement_list (build_call_expr (decl, 2,
 						 build_fold_addr_expr (var),
 						 build_int_cst (uptr, gcount)),


	Jakub

Re: [asan] Patch - fix an ICE in asan.c

[Markus Trippelsdorf]

On 2012.11.12 at 19:02 +0100, Jakub Jelinek wrote:
> On Mon, Nov 12, 2012 at 06:13:08PM +0100, Markus Trippelsdorf wrote:
> > Another ICE:
> > 
> >  % cat test.ii
> >  int i;
> > 
> >  % g++ -faddress-sanitizer -c -g -O1 test.ii
> > test.ii:1:7: internal compiler error: Segmentation fault
> >   int i;
> >        ^
> 
> The RECORD_TYPE doesn't have lang specific payload allocated for it.
> There is no point why we need to describe that in the debug info though.
> 
> So I'd do something like:
> 
> 2012-11-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* asan.c (report_error_func): Set DECL_IGNORED_P, don't touch
> 	DECL_ASSEMBLER_NAME.
> 	(asan_init_func): Likewise.
> 	(asan_finish_file): Use void * instead of __asan_global * as
> 	type of __asan_{,un}register_globals.  Set DECL_IGNORED_P on
> 	the decls.

Thanks.
With your patch applied, it is now possible to successfully (lto-)
bootstrap gcc with CC="gcc -faddress-sanitizer" 
CXX="g++ -faddress-sanitizer".

One just has to apply the following patch from H.J. (that originally
fixed --enable-checking=valgrind):


diff --git a/libcpp/charset.c b/libcpp/charset.c
index cba19a6..6d67bca 100644
--- a/libcpp/charset.c
+++ b/libcpp/charset.c
@@ -1731,7 +1731,10 @@ _cpp_convert_input (cpp_reader *pfile, const char *input_charset,
   /* Resize buffer if we allocated substantially too much, or if we
      haven't enough space for the \n-terminator.  */
   if (to.len + 4096 < to.asize || to.len >= to.asize)
-    to.text = XRESIZEVEC (uchar, to.text, to.len + 1);
+    {
+      to.text = XRESIZEVEC (uchar, to.text, to.len + 17);
+      memset (to.text + to.len + 1, 0, 16);
+    }
 
   /* If the file is using old-school Mac line endings (\r only),
      terminate with another \r, not an \n, so that we do not mistake
-- 
Markus

Re: [asan] Patch - fix an ICE in asan.c

[Dodji Seketeli]

Jakub Jelinek <jakub@redhat.com> writes:

> 2012-11-12  Jakub Jelinek  <jakub@redhat.com>
>
> 	* asan.c (report_error_func): Set DECL_IGNORED_P, don't touch
> 	DECL_ASSEMBLER_NAME.
> 	(asan_init_func): Likewise.
> 	(asan_finish_file): Use void * instead of __asan_global * as
> 	type of __asan_{,un}register_globals.  Set DECL_IGNORED_P on
> 	the decls.

This looks OK to me.

Thanks!

-- 
		Dodji