| Message ID | CAAgBjMmheAGNCkGjBp-dzi-Rub8yezFCcSB7=8KXh6EwVXg9xA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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.
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.
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.
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.
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
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 ();