Fix PR78154

Message ID CAAgBjMmheAGNCkGjBp-dzi-Rub8yezFCcSB7=8KXh6EwVXg9xA@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Nov. 16, 2016, 6:49 p.m.
Hi Richard,
Following your suggestion in PR78154, the patch checks if stmt
contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p
and returns true in that case.

Bootstrapped+tested on x86_64-unknown-linux-gnu.
Cross-testing on arm*-*-*, aarch64*-*-* in progress.
Would it be OK to commit this patch in stage-3 ?

Thanks,
Prathamesh
2016-11-17  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>

	* tree-vrp.c (gimple_str_nonzero_warnv_p): New function.
	(gimple_stmt_nonzero_warnv_p): Call gimple_str_nonzero_warnv_p.

testsuite/
	* gcc.dg/tree-ssa/pr78154.c: New test-case.

Comments

Jakub Jelinek Nov. 16, 2016, 6:55 p.m. | #1
On Thu, Nov 17, 2016 at 12:19:37AM +0530, Prathamesh Kulkarni wrote:
> --- a/gcc/tree-vrp.c

> +++ b/gcc/tree-vrp.c

> @@ -1069,6 +1069,34 @@ gimple_assign_nonzero_warnv_p (gimple *stmt, bool *strict_overflow_p)

>      }

>  }

>  

> +/* Return true if STMT is known to contain call to a string-builtin function

> +   that is known to return nonnull.  */

> +

> +static bool

> +gimple_str_nonzero_warnv_p (gimple *stmt)

> +{

> +  if (!is_gimple_call (stmt))

> +    return false;


Shouldn't this be:
  if (!gimple_call_builtin_p (stmt, BUILT_IN_NORMAL))
    return false;

> +

> +  tree fndecl = gimple_call_fndecl (stmt);


> +  if (!fndecl || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)

> +    return false;


And drop the above 2 lines?

That we you also verify the arguments for sanity.

Otherwise I'll defer to Richard.

	Jakub
Martin Sebor Nov. 16, 2016, 8:27 p.m. | #2
On 11/16/2016 11:49 AM, Prathamesh Kulkarni wrote:
> Hi Richard,

> Following your suggestion in PR78154, the patch checks if stmt

> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

> and returns true in that case.


Nice.  I think the list should also include mempcpy, stpcpy, and
stpncpy, and probably also the corresponding checking built-ins
such as __builtin___memcpy_chk.

FWIW, a more general solution to consider (possibly for GCC 8)
might be to extend attribute nonnull to apply to a functions return
value as well (e.g., use zero as the index for that), to indicate
that a pointer returned from it is not null.  That would let library
implementers annotate other functions (such as strerror)

Martin
Marc Glisse Nov. 16, 2016, 9:21 p.m. | #3
On Wed, 16 Nov 2016, Martin Sebor wrote:

> On 11/16/2016 11:49 AM, Prathamesh Kulkarni wrote:

>> Hi Richard,

>> Following your suggestion in PR78154, the patch checks if stmt

>> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

>> and returns true in that case.

>

> Nice.  I think the list should also include mempcpy, stpcpy, and

> stpncpy, and probably also the corresponding checking built-ins

> such as __builtin___memcpy_chk.

>

> FWIW, a more general solution to consider (possibly for GCC 8)

> might be to extend attribute nonnull to apply to a functions return

> value as well (e.g., use zero as the index for that), to indicate

> that a pointer returned from it is not null.  That would let library

> implementers annotate other functions (such as strerror)


We already have that, under the name returns_nonnull. IIRC, people found a 
new name clearer than using position 0, when I posted the patch. Note also 
that memcpy already has both an attribute that says that it returns its 
first argument, and an attribute that says that said first argument is 
nonnull.

(I've heard some noise in C++-land about making memcpy(0,0,0) valid, but 
that may have just been noise)

-- 
Marc Glisse
Martin Sebor Nov. 17, 2016, 12:17 a.m. | #4
On 11/16/2016 02:21 PM, Marc Glisse wrote:
> On Wed, 16 Nov 2016, Martin Sebor wrote:

>

>> On 11/16/2016 11:49 AM, Prathamesh Kulkarni wrote:

>>> Hi Richard,

>>> Following your suggestion in PR78154, the patch checks if stmt

>>> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

>>> and returns true in that case.

>>

>> Nice.  I think the list should also include mempcpy, stpcpy, and

>> stpncpy, and probably also the corresponding checking built-ins

>> such as __builtin___memcpy_chk.

>>

>> FWIW, a more general solution to consider (possibly for GCC 8)

>> might be to extend attribute nonnull to apply to a functions return

>> value as well (e.g., use zero as the index for that), to indicate

>> that a pointer returned from it is not null.  That would let library

>> implementers annotate other functions (such as strerror)

>

> We already have that, under the name returns_nonnull. IIRC, people found

> a new name clearer than using position 0, when I posted the patch. Note

> also that memcpy already has both an attribute that says that it returns

> its first argument, and an attribute that says that said first argument

> is nonnull.


Ah, right!  Thanks for the reminder!

__builtin_memcpy and __builtin_strcpy are both declared with
the same attribute list ATTR_RET1_NOTHROW_NONNULL_LEAF (as are
their checking versions) and that's defined like so:

   DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_NONNULL_LEAF, ATTR_FNSPEC,
     ATTR_LIST_STR1, ATTR_NOTHROW_NONNULL_LEAF)

ATTR_NOTHROW_NONNULL_LEAF doesn't include returns_nonull and neither
does ATTR_FNSPEC ATTR_LIST_STR1 (unless it's meant to imply that).

In any event, lookup_attribute ("returns_nonnull") fails for both
of these functions so I think the fix might be as "simple" as adding
the attribute.  Alternatively, if attribute fn spec str1 should imply
returns_nonnull when nonnull is set because it says (IIUC) that
the function returns the first (non-null) argument, the changes will
be a bit more involved and require adjusting other places besides
VRP (I see it used in fold-const.c as well similarly as in VRP).

(FWIW, I quoted "simple" above because it recently took me the better
part of an afternoon to figure out how to add attribute alloc_size to
malloc.)

>

> (I've heard some noise in C++-land about making memcpy(0,0,0) valid, but

> that may have just been noise)


We may have read the same discussion.  It would make some things
a little easier in C++ (and remove what most people view as yet
another unnecessary gotcha in the language).

Martin
Martin Sebor Nov. 17, 2016, 12:38 a.m. | #5
On 11/16/2016 05:17 PM, Martin Sebor wrote:
> On 11/16/2016 02:21 PM, Marc Glisse wrote:

>> On Wed, 16 Nov 2016, Martin Sebor wrote:

>>

>>> On 11/16/2016 11:49 AM, Prathamesh Kulkarni wrote:

>>>> Hi Richard,

>>>> Following your suggestion in PR78154, the patch checks if stmt

>>>> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

>>>> and returns true in that case.

>>>

>>> Nice.  I think the list should also include mempcpy, stpcpy, and

>>> stpncpy, and probably also the corresponding checking built-ins

>>> such as __builtin___memcpy_chk.

>>>

>>> FWIW, a more general solution to consider (possibly for GCC 8)

>>> might be to extend attribute nonnull to apply to a functions return

>>> value as well (e.g., use zero as the index for that), to indicate

>>> that a pointer returned from it is not null.  That would let library

>>> implementers annotate other functions (such as strerror)

>>

>> We already have that, under the name returns_nonnull. IIRC, people found

>> a new name clearer than using position 0, when I posted the patch. Note

>> also that memcpy already has both an attribute that says that it returns

>> its first argument, and an attribute that says that said first argument

>> is nonnull.

>

> Ah, right!  Thanks for the reminder!

>

> __builtin_memcpy and __builtin_strcpy are both declared with

> the same attribute list ATTR_RET1_NOTHROW_NONNULL_LEAF (as are

> their checking versions) and that's defined like so:

>

>   DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_NONNULL_LEAF, ATTR_FNSPEC,

>     ATTR_LIST_STR1, ATTR_NOTHROW_NONNULL_LEAF)

>

> ATTR_NOTHROW_NONNULL_LEAF doesn't include returns_nonull and neither

> does ATTR_FNSPEC ATTR_LIST_STR1 (unless it's meant to imply that).

>

> In any event, lookup_attribute ("returns_nonnull") fails for both

> of these functions so I think the fix might be as "simple" as adding

> the attribute.  Alternatively, if attribute fn spec str1 should imply

> returns_nonnull when nonnull is set because it says (IIUC) that

> the function returns the first (non-null) argument, the changes will

> be a bit more involved and require adjusting other places besides

> VRP (I see it used in fold-const.c as well similarly as in VRP).


I should have mentioned: when fully implemented, the test case
will pass even without VRP or without optimization.  A test for
the VRP bits will need to save the return value in a variable
and then use it (otherwise the check for memcpy(...) == 0 will
have already been optimized away by fold-const.c.

>

> (FWIW, I quoted "simple" above because it recently took me the better

> part of an afternoon to figure out how to add attribute alloc_size to

> malloc.)

>

>>

>> (I've heard some noise in C++-land about making memcpy(0,0,0) valid, but

>> that may have just been noise)

>

> We may have read the same discussion.  It would make some things

> a little easier in C++ (and remove what most people view as yet

> another unnecessary gotcha in the language).

>

> Martin
Jeff Law Nov. 17, 2016, 4:57 a.m. | #6
On 11/16/2016 05:17 PM, Martin Sebor wrote:

>>

>> (I've heard some noise in C++-land about making memcpy(0,0,0) valid, but

>> that may have just been noise)

>

> We may have read the same discussion.  It would make some things

> a little easier in C++ (and remove what most people view as yet

> another unnecessary gotcha in the language).

And that may be a reasonable thing to do.

While GCC does take advantage of the non-null attribute when trying to 
prove certain pointers must be non-null, it only does so when the magic 
flag is turned on.  There was a sense that it was too aggressive and 
that time may be necessary for folks to come to terms with what GCC was 
doing, particularly in the the memcpy (*, *, 0) case -- but I've never 
gotten the sense that happened and we've never turned that flag on by 
default.

jeff
Richard Biener Nov. 17, 2016, 8:48 a.m. | #7
On Wed, 16 Nov 2016, Jeff Law wrote:

> On 11/16/2016 05:17 PM, Martin Sebor wrote:

> 

> > > 

> > > (I've heard some noise in C++-land about making memcpy(0,0,0) valid, but

> > > that may have just been noise)

> > 

> > We may have read the same discussion.  It would make some things

> > a little easier in C++ (and remove what most people view as yet

> > another unnecessary gotcha in the language).

> And that may be a reasonable thing to do.

> 

> While GCC does take advantage of the non-null attribute when trying to prove

> certain pointers must be non-null, it only does so when the magic flag is

> turned on.  There was a sense that it was too aggressive and that time may be

> necessary for folks to come to terms with what GCC was doing, particularly in

> the the memcpy (*, *, 0) case -- but I've never gotten the sense that happened

> and we've never turned that flag on by default.


We only have -f[no-]delete-null-pointer-checks and that's on by default.

So we _do_ take advantage of those.

Richard.
Richard Biener Nov. 17, 2016, 8:51 a.m. | #8
On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:

> Hi Richard,

> Following your suggestion in PR78154, the patch checks if stmt

> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

> and returns true in that case.

> 

> Bootstrapped+tested on x86_64-unknown-linux-gnu.

> Cross-testing on arm*-*-*, aarch64*-*-* in progress.

> Would it be OK to commit this patch in stage-3 ?


As people noted we have returns_nonnull for this and that is already
checked.  So please make sure the builtins get this attribute instead.

Thanks,
Richard.
Prathamesh Kulkarni Nov. 17, 2016, 9:34 a.m. | #9
On 17 November 2016 at 14:21, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:

>

>> Hi Richard,

>> Following your suggestion in PR78154, the patch checks if stmt

>> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

>> and returns true in that case.

>>

>> Bootstrapped+tested on x86_64-unknown-linux-gnu.

>> Cross-testing on arm*-*-*, aarch64*-*-* in progress.

>> Would it be OK to commit this patch in stage-3 ?

>

> As people noted we have returns_nonnull for this and that is already

> checked.  So please make sure the builtins get this attribute instead.

OK thanks, I will add the returns_nonnull attribute to the required
string builtins.
I noticed some of the string builtins don't have RET1 in builtins.def:
strcat, strncpy, strncat have ATTR_NOTHROW_NONNULL_LEAF.
Should they instead be having ATTR_RET1_NOTHROW_NONNULL_LEAF similar
to entries for memmove, strcpy ?

Thanks,
Prathamesh
>

> Thanks,

> Richard.
Richard Biener Nov. 17, 2016, 9:54 a.m. | #10
On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:

> On 17 November 2016 at 14:21, Richard Biener <rguenther@suse.de> wrote:

> > On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:

> >

> >> Hi Richard,

> >> Following your suggestion in PR78154, the patch checks if stmt

> >> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p

> >> and returns true in that case.

> >>

> >> Bootstrapped+tested on x86_64-unknown-linux-gnu.

> >> Cross-testing on arm*-*-*, aarch64*-*-* in progress.

> >> Would it be OK to commit this patch in stage-3 ?

> >

> > As people noted we have returns_nonnull for this and that is already

> > checked.  So please make sure the builtins get this attribute instead.

> OK thanks, I will add the returns_nonnull attribute to the required

> string builtins.

> I noticed some of the string builtins don't have RET1 in builtins.def:

> strcat, strncpy, strncat have ATTR_NOTHROW_NONNULL_LEAF.

> Should they instead be having ATTR_RET1_NOTHROW_NONNULL_LEAF similar

> to entries for memmove, strcpy ?


Yes, I think so.

Richard.
Jeff Law Nov. 17, 2016, 3:19 p.m. | #11
On 11/17/2016 01:48 AM, Richard Biener wrote:
> On Wed, 16 Nov 2016, Jeff Law wrote:

>

>> On 11/16/2016 05:17 PM, Martin Sebor wrote:

>>

>>>>

>>>> (I've heard some noise in C++-land about making memcpy(0,0,0) valid, but

>>>> that may have just been noise)

>>>

>>> We may have read the same discussion.  It would make some things

>>> a little easier in C++ (and remove what most people view as yet

>>> another unnecessary gotcha in the language).

>> And that may be a reasonable thing to do.

>>

>> While GCC does take advantage of the non-null attribute when trying to prove

>> certain pointers must be non-null, it only does so when the magic flag is

>> turned on.  There was a sense that it was too aggressive and that time may be

>> necessary for folks to come to terms with what GCC was doing, particularly in

>> the the memcpy (*, *, 0) case -- but I've never gotten the sense that happened

>> and we've never turned that flag on by default.

>

> We only have -f[no-]delete-null-pointer-checks and that's on by default.

>

> So we _do_ take advantage of those.

Hmm, maybe it's the path isolation I'm thinking about where we have the 
additional flag to control whether or not we use the attributes to help 
identify erroneous paths.

jeff

Patch hide | download patch | download mbox

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr78154.c b/gcc/testsuite/gcc.dg/tree-ssa/pr78154.c
new file mode 100644
index 0000000..d3463f4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr78154.c
@@ -0,0 +1,28 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-evrp-slim" } */
+
+void f (void *d, const void *s, __SIZE_TYPE__ n)
+{
+  if (__builtin_memcpy (d, s, n) == 0)
+    __builtin_abort ();
+
+  if (__builtin_memmove (d, s, n) == 0)
+    __builtin_abort ();
+
+  if (__builtin_memset (d, 0, n) == 0)
+    __builtin_abort ();
+
+  if (__builtin_strcpy (d, s) == 0)
+    __builtin_abort ();
+
+  if (__builtin_strcat (d, s) == 0)
+    __builtin_abort ();
+
+  if (__builtin_strncpy (d, s, n) == 0)
+    __builtin_abort ();
+
+  if (__builtin_strncat (d, s, n) == 0)
+    __builtin_abort ();
+}
+
+/* { dg-final { scan-tree-dump-not "__builtin_abort" "evrp" } } */
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index c2a4133..b563a7f 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1069,6 +1069,34 @@  gimple_assign_nonzero_warnv_p (gimple *stmt, bool *strict_overflow_p)
     }
 }
 
+/* Return true if STMT is known to contain call to a string-builtin function
+   that is known to return nonnull.  */
+
+static bool
+gimple_str_nonzero_warnv_p (gimple *stmt)
+{
+  if (!is_gimple_call (stmt))
+    return false;
+
+  tree fndecl = gimple_call_fndecl (stmt);
+  if (!fndecl || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
+    return false;
+
+  switch (DECL_FUNCTION_CODE (fndecl))
+    {
+      case BUILT_IN_MEMMOVE:
+      case BUILT_IN_MEMCPY:
+      case BUILT_IN_MEMSET:
+      case BUILT_IN_STRCPY:
+      case BUILT_IN_STRNCPY:
+      case BUILT_IN_STRCAT:
+      case BUILT_IN_STRNCAT:
+	return true;
+      default:
+	return false;
+    }
+}
+
 /* Return true if STMT is known to compute a non-zero value.
    If the return value is based on the assumption that signed overflow is
    undefined, set *STRICT_OVERFLOW_P to true; otherwise, don't change
@@ -1097,7 +1125,7 @@  gimple_stmt_nonzero_warnv_p (gimple *stmt, bool *strict_overflow_p)
 	    lookup_attribute ("returns_nonnull",
 			      TYPE_ATTRIBUTES (gimple_call_fntype (stmt))))
 	  return true;
-	return gimple_alloca_call_p (stmt);
+	return gimple_alloca_call_p (stmt) || gimple_str_nonzero_warnv_p (stmt);
       }
     default:
       gcc_unreachable ();