| Message ID | CAAgBjMkiA6FwzYYVDBA7vymvRDhd_a08vamPf_42JQ5DV5SQ8g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: > Hi, > The attached patch tries to fix PR80806 by warning when a variable is > set using memset (and friends) but not used. I chose to warn in dse > pass since dse would detect if the variable passed as 1st argument is > a dead store. Does this approach look OK ? > > There were following fallouts observed during bootstrap build: > > * double-int.c (div_and_round_double): > Warning emitted 'den' set but not used for following call to memset: > memset (den, 0, sizeof den); > > I assume the warning is correct since there's immediately call to: > encode (den, lden, hden); > > and encode overwrites all the contents of den. > Should the above call to memset be removed from the source ? Unsure. Yes, it's dead, but from a readability standpoint should it stay? I don't know. This one isn't very clear. > > * tree-streamer.c (streamer_check_handled_ts_structures) > The function defines a local array bool handled_p[LAST_TS_ENUM]; > and the warning is emitted for: > memset (&handled_p, 0, sizeof (handled_p)); > > That's because the function then initializes each element of the array > handled_p to true > making the memset call redundant. > I am not sure if warning for the above case is a good idea ? The call > to memset() seems deliberate, to initialize all elements to 0, and > later assert checks if all the elements were explicitly set to true. This one looks like it should stay to me. It's there for defensive purposes to catch cases if programming errors. > > * value-prof.c (free_hist): > Warns for the call to memset: > > static int > free_hist (void **slot, void *data ATTRIBUTE_UNUSED) > { > histogram_value hist = *(histogram_value *) slot; > free (hist->hvalue.counters); > if (flag_checking) > memset (hist, 0xab, sizeof (*hist)); > free (hist); > return 1; > } > > Assuming flag_checking was true, the call to memset would be dead > anyway since it would be immediately freed ? Um, I don't understand > the purpose of memset in the above function. It's been like that from the day the code was introduced (initially surrounded with an ENABLE_CHECKING. Given the call to free, the memset is going to get removed. This one probably falls into the "should just be removed" bucket. > > * testsuite fallout > I verified regressing test-cases were not false positives and added > -Wno-unused-but-set-variable. I think the real question is do we want to warn here. I can certainly see both sides of the issue. jeff
On 22 May 2017 at 10:03, Jeff Law <law@redhat.com> wrote: > On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: >> Hi, >> The attached patch tries to fix PR80806 by warning when a variable is >> set using memset (and friends) but not used. I chose to warn in dse >> pass since dse would detect if the variable passed as 1st argument is >> a dead store. Does this approach look OK ? >> >> There were following fallouts observed during bootstrap build: >> >> * double-int.c (div_and_round_double): >> Warning emitted 'den' set but not used for following call to memset: >> memset (den, 0, sizeof den); >> >> I assume the warning is correct since there's immediately call to: >> encode (den, lden, hden); >> >> and encode overwrites all the contents of den. >> Should the above call to memset be removed from the source ? > Unsure. Yes, it's dead, but from a readability standpoint should it > stay? I don't know. This one isn't very clear. > > >> >> * tree-streamer.c (streamer_check_handled_ts_structures) >> The function defines a local array bool handled_p[LAST_TS_ENUM]; >> and the warning is emitted for: >> memset (&handled_p, 0, sizeof (handled_p)); >> >> That's because the function then initializes each element of the array >> handled_p to true >> making the memset call redundant. >> I am not sure if warning for the above case is a good idea ? The call >> to memset() seems deliberate, to initialize all elements to 0, and >> later assert checks if all the elements were explicitly set to true. > This one looks like it should stay to me. It's there for defensive > purposes to catch cases if programming errors. > > >> >> * value-prof.c (free_hist): >> Warns for the call to memset: >> >> static int >> free_hist (void **slot, void *data ATTRIBUTE_UNUSED) >> { >> histogram_value hist = *(histogram_value *) slot; >> free (hist->hvalue.counters); >> if (flag_checking) >> memset (hist, 0xab, sizeof (*hist)); >> free (hist); >> return 1; >> } >> >> Assuming flag_checking was true, the call to memset would be dead >> anyway since it would be immediately freed ? Um, I don't understand >> the purpose of memset in the above function. > It's been like that from the day the code was introduced (initially > surrounded with an ENABLE_CHECKING. Given the call to free, the memset > is going to get removed. This one probably falls into the "should > just be removed" bucket. > > >> >> * testsuite fallout >> I verified regressing test-cases were not false positives and added >> -Wno-unused-but-set-variable. > I think the real question is do we want to warn here. I can certainly > see both sides of the issue. Hi Jeff, Thanks for the suggestions. I agree that warning for the above cases in tree-streamer.c and double-int.c may not be ideal. Should we perhaps only warn if there's a single use of the pointer in the call to memset (and friends) like in the test-case reported in PR ? But then I fear the warning may end up being a bit artificial. Thanks, Prathamesh > > jeff >
On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: > Hi, > The attached patch tries to fix PR80806 by warning when a variable is > set using memset (and friends) but not used. I chose to warn in dse > pass since dse would detect if the variable passed as 1st argument is > a dead store. Does this approach look OK ? Detecting -Wunused-but-set-variable in the optimizer means that the warning will not be issued without optimization. It also means that the warning will trigger in cases where the variable is used conditionally and the condition is subject to constant propagation. For instance: void sink (void*); void test (int i) { char buf[10]; // -Wunused-but-set-variable memset (buf, 0, sizeof(buf)); if (i) sink (buf); } void f (void) { test (0); } I suspect this would be considered a false positive by most users. In my view, it would be more in line with the design of the warning to enhance the front end to detect this case, and it would avoid these issues. I have a patch that does that. Rather than checking the finite set of known built-in functions like memset that are known not to read the referenced object, I took the approach of adding a new function attribute (I call it write-only) and avoiding setting the DECL_READ_P flag for DECLs that are passed to function arguments decorated with the attribute. That makes it possible to issue the warning even if the variable is passed to ordinary (non-built-in) functions like getline(), and should open up optimization opportunities beyond built-ins. The only wrinkle is that the front end sets DECL_READ_P even for uses that aren't reads such as a sizeof expression, so while an otherwise unused buf is diagnosed given a call to memset(buf, 0, 10), it isn't diagnosed if a call is made to memset(buf, 0, sizeof buf). I am yet to see what impact not setting DECL_READ_P would have when the decl is used without being evaluated. (In any event, setting DECL_READ_P on a use that doesn't involve reading the DECL doesn't seem right.) I attach what I have so far in case you would like to check it out. I think you have more experience with DSE than me so I'd be interested in your thoughts on making use of the attribute for optimization. (Another couple attributes I'm considering to complement write-only is read-only and read-write, also with the hope of improving both warnings and code generation. Ideas on those would be welcome as well.) > > There were following fallouts observed during bootstrap build: > > * double-int.c (div_and_round_double): > Warning emitted 'den' set but not used for following call to memset: > memset (den, 0, sizeof den); > > I assume the warning is correct since there's immediately call to: > encode (den, lden, hden); > > and encode overwrites all the contents of den. > Should the above call to memset be removed from the source ? IIUC, this seems to be a more involved example of the simple one above. AFAICS, the buffer is subsequently read in the function, but only conditionally. That said, since encode() overwrites the whole buffer right after it has been cleared by memset, I would think that a warning pointing that out could be helpful (although I'm not sure -Wunused is the right warning to issue). > > * tree-streamer.c (streamer_check_handled_ts_structures) > The function defines a local array bool handled_p[LAST_TS_ENUM]; > and the warning is emitted for: > memset (&handled_p, 0, sizeof (handled_p)); > > That's because the function then initializes each element of the array > handled_p to true > making the memset call redundant. > I am not sure if warning for the above case is a good idea ? The call > to memset() seems deliberate, to initialize all elements to 0, and > later assert checks if all the elements were explicitly set to true. Right. Warning on this function doesn't seem right since the variable is subsequently used in the test loop. > > * value-prof.c (free_hist): > Warns for the call to memset: > > static int > free_hist (void **slot, void *data ATTRIBUTE_UNUSED) > { > histogram_value hist = *(histogram_value *) slot; > free (hist->hvalue.counters); > if (flag_checking) > memset (hist, 0xab, sizeof (*hist)); > free (hist); > return 1; > } > > Assuming flag_checking was true, the call to memset would be dead > anyway since it would be immediately freed ? Um, I don't understand > the purpose of memset in the above function. I'm guessing it's an effort to invalidate the memory to detect subsequent accesses to its contents. Warning on such code would be valuable because it's a common misconception that a memset can be used to wipe sensitive data (e.g., passwords) from memory before freeing it, to prevent them from leaking into compromised stack frames (or core files). But it should be a warning that's distinct from -Wunused. Martindiff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def index 38fb1bb8..24e6f6a 100644 --- a/gcc/builtin-attrs.def +++ b/gcc/builtin-attrs.def @@ -113,6 +113,7 @@ DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm") DEF_ATTR_IDENT (ATTR_TM_TMPURE, "transaction_pure") DEF_ATTR_IDENT (ATTR_RETURNS_TWICE, "returns_twice") DEF_ATTR_IDENT (ATTR_RETURNS_NONNULL, "returns_nonnull") +DEF_ATTR_IDENT (ATTR_WRITE_ONLY, "write_only") DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL) @@ -196,6 +197,7 @@ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL, ATTR_NONNULL, ATTR_NULL, \ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_LEAF, ATTR_NONNULL, ATTR_NULL, \ ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_LEAF_LIST, ATTR_LEAF, ATTR_NULL, ATTR_NOTHROW_NONNULL_LEAF) + /* Nothrow functions whose first parameter is a nonnull pointer. */ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_1, ATTR_NONNULL, ATTR_LIST_1, \ ATTR_NOTHROW_LIST) @@ -255,10 +257,21 @@ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_TYPEGENERIC_LEAF, /* Nothrow const functions whose pointer parameter(s) are all nonnull. */ DEF_ATTR_TREE_LIST (ATTR_CONST_NOTHROW_NONNULL, ATTR_CONST, ATTR_NULL, \ ATTR_NOTHROW_NONNULL) + /* Nothrow leaf functions whose pointer parameter(s) are all nonnull, and which return their first argument. */ DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_NONNULL_LEAF, ATTR_FNSPEC, ATTR_LIST_STR1, \ ATTR_NOTHROW_NONNULL_LEAF) + +DEF_ATTR_TREE_LIST (ATTR_NOTHROW_WRONLY1_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_1, ATTR_NOTHROW_LEAF_LIST) + +DEF_ATTR_TREE_LIST (ATTR_NOTHROW_WRONLY2_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_2, ATTR_NOTHROW_LEAF_LIST) + +DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_WRONLY1_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_1, ATTR_RET1_NOTHROW_NONNULL_LEAF) + /* Nothrow leaf functions whose pointer parameter(s) are all nonnull, and return value is also nonnull. */ DEF_ATTR_TREE_LIST (ATTR_RETNONNULL_NOTHROW_LEAF, ATTR_RETURNS_NONNULL, ATTR_NULL, \ diff --git a/gcc/builtins.def b/gcc/builtins.def index 58d78db..59d1453 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -652,15 +652,15 @@ DEF_C99_COMPL_BUILTIN (BUILT_IN_CTANL, "ctanl", BT_FN_COMPLEX_LONGDOUBLE_ /* bcmp, bcopy and bzero have traditionally accepted NULL pointers when the length parameter is zero, so don't apply attribute "nonnull". */ DEF_EXT_LIB_BUILTIN (BUILT_IN_BCMP, "bcmp", BT_FN_INT_CONST_PTR_CONST_PTR_SIZE, ATTR_PURE_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN (BUILT_IN_BCOPY, "bcopy", BT_FN_VOID_CONST_PTR_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN (BUILT_IN_BZERO, "bzero", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) +DEF_EXT_LIB_BUILTIN (BUILT_IN_BCOPY, "bcopy", BT_FN_VOID_CONST_PTR_PTR_SIZE, ATTR_NOTHROW_WRONLY2_LEAF) +DEF_EXT_LIB_BUILTIN (BUILT_IN_BZERO, "bzero", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_INDEX, "index", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_MEMCHR, "memchr", BT_FN_PTR_CONST_PTR_INT_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_MEMCMP, "memcmp", BT_FN_INT_CONST_PTR_CONST_PTR_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY, "memcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE, "memmove", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY, "memcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE, "memmove", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMPCPY, "mempcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET, "memset", BT_FN_PTR_PTR_INT_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET, "memset", BT_FN_PTR_PTR_INT_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_RINDEX, "rindex", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STPCPY, "stpcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STPNCPY, "stpncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) @@ -668,7 +668,7 @@ DEF_EXT_LIB_BUILTIN (BUILT_IN_STRCASECMP, "strcasecmp", BT_FN_INT_CONST_STRIN DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCAT, "strcat", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCHR, "strchr", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRCMP, "strcmp", BT_FN_INT_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY, "strcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY, "strcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRCSPN, "strcspn", BT_FN_SIZE_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRDUP, "strdup", BT_FN_STRING_CONST_STRING, ATTR_MALLOC_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNDUP, "strndup", BT_FN_STRING_CONST_STRING_SIZE, ATTR_MALLOC_NOTHROW_NONNULL_LEAF) @@ -676,7 +676,7 @@ DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRLEN, "strlen", BT_FN_SIZE_CONST_STRING, ATTR DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRPBRK, "strpbrk", BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRRCHR, "strrchr", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRSPN, "strspn", BT_FN_SIZE_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) @@ -825,7 +825,7 @@ DEF_EXT_LIB_BUILTIN (BUILT_IN_FFSLL, "ffsll", BT_FN_INT_LONGLONG, ATTR_CONST_ DEF_EXT_LIB_BUILTIN (BUILT_IN_FORK, "fork", BT_FN_PID, ATTR_NOTHROW_LIST) DEF_GCC_BUILTIN (BUILT_IN_FRAME_ADDRESS, "frame_address", BT_FN_PTR_UINT, ATTR_NULL) /* [trans-mem]: Adjust BUILT_IN_TM_FREE if BUILT_IN_FREE is changed. */ -DEF_LIB_BUILTIN (BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) +DEF_LIB_BUILTIN (BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_WRONLY1_LEAF) DEF_GCC_BUILTIN (BUILT_IN_FROB_RETURN_ADDR, "frob_return_addr", BT_FN_PTR_PTR, ATTR_NULL) DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) DEF_C99_BUILTIN (BUILT_IN_IMAXABS, "imaxabs", BT_FN_INTMAX_INTMAX, ATTR_CONST_NOTHROW_LEAF_LIST) @@ -916,16 +916,16 @@ DEF_BUILTIN_STUB (BUILT_IN_MEMCMP_EQ, "__builtin_memcmp_eq") /* Object size checking builtins. */ DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_PURE_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY_CHK, "__memcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE_CHK, "__memmove_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY_CHK, "__memcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE_CHK, "__memmove_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMPCPY_CHK, "__mempcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET_CHK, "__memset_chk", BT_FN_PTR_PTR_INT_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET_CHK, "__memset_chk", BT_FN_PTR_PTR_INT_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STPCPY_CHK, "__stpcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STPNCPY_CHK, "__stpncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCAT_CHK, "__strcat_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY_CHK, "__strcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY_CHK, "__strcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCAT_CHK, "__strncat_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCPY_CHK, "__strncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCPY_CHK, "__strncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_SNPRINTF_CHK, "__snprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_5_6) DEF_EXT_LIB_BUILTIN (BUILT_IN_SPRINTF_CHK, "__sprintf_chk", BT_FN_INT_STRING_INT_SIZE_CONST_STRING_VAR, ATTR_NOTHROW_NONNULL_1_FORMAT_PRINTF_4_5) DEF_EXT_LIB_BUILTIN (BUILT_IN_VSNPRINTF_CHK, "__vsnprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_5_0) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 695c58c..46ab8f0 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-iterator.h" #include "opts.h" #include "gimplify.h" +#include "bitmap.h" static tree handle_packed_attribute (tree *, tree, tree, int, bool *); static tree handle_nocommon_attribute (tree *, tree, tree, int, bool *); @@ -116,6 +117,7 @@ static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *); static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *); static tree handle_warn_unused_result_attribute (tree *, tree, tree, int, bool *); +static tree handle_write_only_attribute (tree *, tree, tree, int, bool *); static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *); static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *); static tree handle_alloc_size_attribute (tree *, tree, tree, int, bool *); @@ -345,6 +347,8 @@ const struct attribute_spec c_common_attribute_table[] = handle_bnd_instrument, false }, { "fallthrough", 0, 0, false, false, false, handle_fallthrough_attribute, false }, + { "write_only", 0, -1, false, true, true, + handle_write_only_attribute, false }, { NULL, 0, 0, false, false, false, NULL, false } }; @@ -2815,7 +2819,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name), && TREE_CODE (arg) != FUNCTION_DECL) TREE_VALUE (args) = arg = default_conversion (arg); - if (!get_nonnull_operand (arg, &arg_num)) + if (!get_attribute_operand (arg, &arg_num)) { error ("nonnull argument has invalid operand number (argument %lu)", (unsigned long) attr_arg_num); @@ -2860,6 +2864,121 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name), return NULL_TREE; } + +/* Handle the "write_only" attribute. */ + +static tree +handle_write_only_attribute (tree *node, tree name, + tree operands, int ARG_UNUSED (flags), + bool *no_add_attrs) +{ + tree type = *node; + + /* If no operands are specified, all pointer arguments should be + write-only. Verify a full prototype is given so that the arguments + will have the correct types when we actually check them later. + Avoid diagnosing type-generic built-ins since those have no + prototype. */ + if (!operands + && !prototype_p (type) + && (!TYPE_ATTRIBUTES (type) + || !lookup_attribute ("type generic", TYPE_ATTRIBUTES (type)))) + { + error ("attribute %qE without arguments on a non-prototype", name); + *no_add_attrs = true; + return NULL_TREE; + } + + /* True if operands were specified. */ + bool has_ops = operands; + + /* A bitmap of operand values already handled. Used to warn about + duplicates. */ + bitmap_head operset; + bitmap_initialize (&operset, 0); + + /* Attribute operands have been specified. Verify that each operand + value, OPERVAL, references a non-const pointer argument to the + function. */ + for (unsigned operno = 1; ; operno++, + operands = operands ? TREE_CHAIN (operands) : NULL_TREE) + { + /* The argument number the attribute operand corresponds to. */ + unsigned HOST_WIDE_INT operval; + + if (operands) + { + tree oper = TREE_VALUE (operands); + if (oper && TREE_CODE (oper) != IDENTIFIER_NODE + && TREE_CODE (oper) != FUNCTION_DECL) + TREE_VALUE (operands) = oper = default_conversion (oper); + + if (!get_attribute_operand (oper, &operval)) + { + error ("attribute %<%E(%E)%> invalid operand", name, oper); + *no_add_attrs = true; + break; + } + } + else if (!has_ops) + operval = operno; + else + break; + + if (bitmap_bit_p (&operset, operval)) + warning (OPT_Wattributes, "duplicate attribute %<%E(%u)%>", + name, operval); + + bitmap_set_bit (&operset, operval); + + if (!prototype_p (type)) + continue; + + tree argtype; + + function_args_iterator iter; + function_args_iter_init (&iter, type); + + for (unsigned idx = 1; ; ++idx, function_args_iter_next (&iter)) + { + argtype = function_args_iter_cond (&iter); + if (!argtype || VOID_TYPE_P (argtype)) + { + if (has_ops) + { + error ("attribute %<%E(%u)%> exceeds number of arguments %u", + name, operval, idx - 1); + *no_add_attrs = true; + } + bitmap_clear (&operset); + return NULL_TREE; + } + + if (idx == operval) + break; + } + + if (TREE_CODE (argtype) != POINTER_TYPE) + { + error ("attribute %<%E(%u)%> references non-pointer argument " + "type %qT", name, operval, argtype); + *no_add_attrs = true; + break; + } + + if (TYPE_READONLY (TREE_TYPE (argtype))) + { + error ("attribute %<%E(%u)%> references %qs-qualified argument " + "type %qT", name, operval, "const", argtype); + *no_add_attrs = true; + break; + } + } + + bitmap_clear (&operset); + return NULL_TREE; +} + /* Handle a "nothrow" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 5f4488a..8f95bae 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5277,7 +5277,7 @@ nonnull_check_p (tree args, unsigned HOST_WIDE_INT param_num) for (; args; args = TREE_CHAIN (args)) { - bool found = get_nonnull_operand (TREE_VALUE (args), &arg_num); + bool found = get_attribute_operand (TREE_VALUE (args), &arg_num); gcc_assert (found); @@ -5314,11 +5314,11 @@ check_nonnull_arg (void *ctx, tree param, unsigned HOST_WIDE_INT param_num) } } -/* Helper for nonnull attribute handling; fetch the operand number - from the attribute argument list. */ +/* Helper for attribute handling; fetch the operand number from + the attribute argument list. */ bool -get_nonnull_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp) +get_attribute_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp) { /* Verify the arg number is a small constant. */ if (tree_fits_uhwi_p (arg_num_expr)) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 502dc2f..266313c 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -864,7 +864,7 @@ extern bool pointer_to_zero_sized_aggr_p (tree); extern bool bool_promoted_to_int_p (tree); extern tree fold_for_warn (tree); extern tree c_common_get_narrower (tree, int *); -extern bool get_nonnull_operand (tree, unsigned HOST_WIDE_INT *); +extern bool get_attribute_operand (tree, unsigned HOST_WIDE_INT *); #define c_sizeof(LOC, T) c_sizeof_or_alignof_type (LOC, T, true, false, 1) #define c_alignof(LOC, T) c_sizeof_or_alignof_type (LOC, T, false, false, 1) diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 2e01316..95baad6 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -178,6 +178,9 @@ struct GTY(()) c_parser { BOOL_BITFIELD in_if_block : 1; /* True if we want to lex an untranslated string. */ BOOL_BITFIELD lex_untranslated_string : 1; + /* False if taking the address of a DECL should not set its DECL_READ_P + flag. */ + BOOL_BITFIELD no_set_read : 1; /* Objective-C specific parser/lexer information. */ @@ -1253,7 +1256,8 @@ static tree c_parser_transaction_cancel (c_parser *); static struct c_expr c_parser_expression (c_parser *); static struct c_expr c_parser_expression_conv (c_parser *); static vec<tree, va_gc> *c_parser_expr_list (c_parser *, bool, bool, - vec<tree, va_gc> **, location_t *, + vec<tree, va_gc> **, tree, + location_t *, tree *, vec<location_t> *, unsigned int * = NULL); static void c_parser_oacc_declare (c_parser *); @@ -4215,8 +4219,8 @@ c_parser_attributes (c_parser *parser) { tree tree_list; c_parser_consume_token (parser); - expr_list = c_parser_expr_list (parser, false, true, - NULL, NULL, NULL, NULL); + expr_list = c_parser_expr_list (parser, false, true, NULL, + NULL_TREE, NULL, NULL, NULL); tree_list = build_tree_list_vec (expr_list); attr_args = tree_cons (NULL_TREE, arg1, tree_list); release_tree_vector (expr_list); @@ -4228,8 +4232,8 @@ c_parser_attributes (c_parser *parser) attr_args = NULL_TREE; else { - expr_list = c_parser_expr_list (parser, false, true, - NULL, NULL, NULL, NULL); + expr_list = c_parser_expr_list (parser, false, true, NULL, + NULL_TREE, NULL, NULL, NULL); attr_args = build_tree_list_vec (expr_list); release_tree_vector (expr_list); } @@ -6973,7 +6977,8 @@ c_parser_unary_expression (c_parser *parser) case CPP_AND: c_parser_consume_token (parser); op = c_parser_cast_expression (parser, NULL); - mark_exp_read (op.value); + if (!parser->no_set_read) + mark_exp_read (op.value); return parser_build_unary_op (op_loc, ADDR_EXPR, op); case CPP_MULT: { @@ -8415,8 +8420,10 @@ c_parser_postfix_expression_after_primary (c_parser *parser, exprlist = NULL; else exprlist = c_parser_expr_list (parser, true, false, &origtypes, + expr.value, sizeof_arg_loc, sizeof_arg, &arg_loc, &literal_zero_mask); + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, "expected %<)%>"); orig_expr = expr; @@ -8658,6 +8665,32 @@ c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask, } } +/* Return true if expression number IDX and type ARGTYPE eclared with + the write-only attribute in ATTRS is itself declared write-only. + ATTRS may be null. */ + +static bool +is_write_only_p (tree attrs, tree argtype, unsigned idx) +{ + if (!attrs) + return false; + + if (!POINTER_TYPE_P (argtype)) + return false; + + if (TREE_READONLY (TREE_TYPE (argtype))) + return false; + + for (tree a = attrs; a; a = TREE_CHAIN (a)) + { + unsigned HOST_WIDE_INT wronlyidx; + if (!get_attribute_operand (a, &wronlyidx) || wronlyidx == idx) + return true; + } + return false; +} + + /* Parse a non-empty list of expressions. If CONVERT_P, convert functions and arrays to pointers and lvalues to rvalues. If FOLD_P, fold the expressions. If LOCATIONS is non-NULL, save the @@ -8670,7 +8703,7 @@ c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask, static vec<tree, va_gc> * c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, - vec<tree, va_gc> **p_orig_types, + vec<tree, va_gc> **p_orig_types, tree func, location_t *sizeof_arg_loc, tree *sizeof_arg, vec<location_t> *locations, unsigned int *literal_zero_mask) @@ -8680,6 +8713,8 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; + + /* Zero-based expression index within the list. */ unsigned int idx = 0; ret = make_tree_vector (); @@ -8693,9 +8728,30 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); - expr = c_parser_expr_no_commas (parser, NULL); + + tree functype = func ? TREE_TYPE (func) : NULL_TREE; + tree attrs = (functype + ? lookup_attribute ("write_only", TYPE_ATTRIBUTES (functype)) + : NULL_TREE); + + tree argtypes = func ? TYPE_ARG_TYPES (TREE_TYPE (func)) : NULL_TREE; + + /* Determine if the expression in the list is declared write-only + and only mark it DECL_READ_P() if it isn't. */ + bool wronly + = argtypes && is_write_only_p (attrs, TREE_VALUE (argtypes), idx + 1); + + { + /* Avoid setting the read bit for write-only decls. */ + bool save_set_read = parser->no_set_read; + parser->no_set_read = wronly; + expr = c_parser_expr_no_commas (parser, NULL); + parser->no_set_read = save_set_read; + } + if (convert_p) - expr = convert_lvalue_to_rvalue (loc, expr, true, true); + expr = convert_lvalue_to_rvalue (loc, expr, true, !wronly); + if (fold_p) expr.value = c_fully_fold (expr.value, false, NULL); ret->quick_push (expr.value); @@ -8721,9 +8777,28 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); - expr = c_parser_expr_no_commas (parser, NULL); + + /* Determine if the next expression in the list is declared + write-only and only mark it DECL_READ_P() if it isn't. */ + if (argtypes && attrs) + { + argtypes = TREE_CHAIN (argtypes); + wronly = is_write_only_p (attrs, TREE_VALUE (argtypes), idx + 2); + } + else + wronly = false; + + { + /* Avoid setting the read bit for write-only decls. */ + bool save_set_read = parser->no_set_read; + parser->no_set_read = wronly; + expr = c_parser_expr_no_commas (parser, NULL); + parser->no_set_read = save_set_read; + } + if (convert_p) - expr = convert_lvalue_to_rvalue (loc, expr, true, true); + expr = convert_lvalue_to_rvalue (loc, expr, true, !wronly); + if (fold_p) expr.value = c_fully_fold (expr.value, false, NULL); vec_safe_push (ret, expr.value); @@ -8742,6 +8817,7 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, } if (orig_types) *p_orig_types = orig_types; + return ret; } @@ -9831,8 +9907,8 @@ static tree c_parser_objc_keywordexpr (c_parser *parser) { tree ret; - vec<tree, va_gc> *expr_list = c_parser_expr_list (parser, true, true, - NULL, NULL, NULL, NULL); + vec<tree, va_gc> *expr_list = c_parser_expr_list (parser, true, true, NULL, + NULL_TREE, NULL, NULL, NULL); if (vec_safe_length (expr_list) == 1) { /* Just return the expression, remove a level of @@ -10682,7 +10758,8 @@ c_parser_oacc_wait_list (c_parser *parser, location_t clause_loc, tree list) if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) return list; - args = c_parser_expr_list (parser, false, true, NULL, NULL, NULL, NULL); + args = c_parser_expr_list (parser, false, true, NULL, NULL_TREE, NULL, + NULL, NULL); if (args->length () == 0) { diff --git a/gcc/testsuite/gcc.dg/attr-write-only-2.c b/gcc/testsuite/gcc.dg/attr-write-only-2.c new file mode 100644 index 0000000..a15bd55 --- /dev/null +++ b/gcc/testsuite/gcc.dg/attr-write-only-2.c @@ -0,0 +1,109 @@ +/* Test to verify that attribute write_only is recognized by + -Wunused-but-set-variable. + { dg-do compile } + { dg-options "-Wunused-but-set-variable -Wunused-but-set-parameter" } */ + +typedef __SIZE_TYPE__ size_t; + +void bcopy (const void*, void*, size_t); +void bzero (void*, size_t); +void* memcpy (void*, const void*, size_t); +void* memset (void*, int, size_t); + +char* strcat (char*, const char*); +char* strcpy (char*, const char*); +char* strncat (char*, const char*, size_t); +char* strncpy (char*, const char*, size_t); + +#define WRONLY(...) __attribute__ ((write_only (__VA_ARGS__))) + +void wronly_1 (void*) WRONLY (1); + +enum { N = sizeof (int) }; + +void test_bcopy (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + const void *p) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + bcopy (p, &a, N); + bcopy (p, &i, N); + bcopy (p, &ar, N); +} + +void test_bzero (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + bzero (&a, N); + bzero (&i, N); + bzero (&ar, N); +} + +void test_memset (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + memset (&a, 0, N); + memset (&i, 0, N); + memset (&ar, 0, N); +} + +void test_memcpy (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + const void *p) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + memcpy (&a, p, N); + memcpy (&i, p, N); + memcpy (&ar, p, N); +} + +void test_strcat (const char *s) +{ + char c = '\0'; + char ar[8] = ""; + + strcat (&c, ""); + strcat (ar, s); +} + +void test_strcpy (const char *s) +{ + char c; /* { dg-warning "-Wunused-but-set-variable" } */ + char d = '\0'; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar2[8] = ""; /* { dg-warning "-Wunused-but-set-variable" } */ + + strcpy (&c, ""); + strcpy (&d, ""); + strcpy (ar, s); + strcpy (ar2, s); +} + +void test_strncpy (const char *s, unsigned n) +{ + char c; /* { dg-warning "-Wunused-but-set-variable" } */ + char d = '\0'; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar2[8] = ""; /* { dg-warning "-Wunused-but-set-variable" } */ + + strncpy (&c, "", 1); + strncpy (&d, "", n); + strncpy (ar, s, n); + strncpy (ar2, s, n); +} + +void test_usrdef (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + wronly_1 (&a); + wronly_1 (&i); + wronly_1 (&ar); +} diff --git a/gcc/testsuite/gcc.dg/attr-write-only.c b/gcc/testsuite/gcc.dg/attr-write-only.c new file mode 100644 index 0000000..e238c81 --- /dev/null +++ b/gcc/testsuite/gcc.dg/attr-write-only.c @@ -0,0 +1,34 @@ +/* Test to verify the handling of attribute write_only syntax. + { dg-do compile } + { dg-options "-Wall -ftrack-macro-expansion=0" } */ + +#define WRONLY(...) __attribute__ ((write_only (__VA_ARGS__))) + +void wronly_v_all (void) __attribute__ ((write_only)); /* { dg-error "attribute .write_only\\(1\\). exceeds number of arguments 0" } */ + +void wronly_i_all (int) __attribute__ ((write_only)); /* { dg-error "attribute .write_only\\(1\\). references non-pointer argument type .int." } */ + +void wronly_v_1 (void*) WRONLY (1); +void wronly_iv_2 (int, void*) WRONLY (2); +void wronly_iiv_3 (int, int, void*) WRONLY (3); + +void wronly_v_0p1 (void*) WRONLY (0 + 1); +void wronly_v_2m1 (void*) WRONLY (2 - 1); + +void wronly_v_1_1 (void*) WRONLY (1, 1); /* { dg-warning "duplicate attribute .write_only\\(1\\)." } */ +void wronly_vv_1_2 (void*, void*) WRONLY (1, 2); +void wronly_vv_2_1 (void*, void*) WRONLY (2, 1); +void wronly_vv_all (void*, void*) __attribute__ ((write_only)); + +void wronly_vv_1_1 (void*, void*) WRONLY (1, 1); /* { dg-warning "duplicate attribute .write_only\\(1\\)." } */ +void wronly_vv_1_1 (void*, void*) WRONLY (2, 2); /* { dg-warning "duplicate attribute .write_only\\(2\\)." } */ + +void wronly_4_exceed (int, int, int) WRONLY (4); /* { dg-error "attribute .write_only\\(4\\). exceeds number of arguments 3" } */ + +void wronly_1_i (int) WRONLY (1); /* { dg-error "attribute .write_only\\(1\\). references non-pointer argument type .int." } */ + +void wronly_2_cst (int, const char*) WRONLY (2); /* { dg-error "attribute .write_only\\(2\\). references .const.-qualified argument type .const char *." } */ + +void wronly_m1_i (void*) WRONLY (-1); /* { dg-error "attribute .write_only\\(-1\\). invalid operand" } */ + +void wronly_str_i (void*) WRONLY ("blah"); /* { dg-error "attribute .write_only\\(\"blah\"\\). invalid operand" } */
I attach an updated patch that actually bootstraps and (with one minor exception) passes regression tests. It's a C front end-only proof of concept for now. The complete patch will include attributes read-only and read-write and support C++ of course. I think the optimization bits can be done in a subsequent pass. Martin On 05/23/2017 09:58 AM, Martin Sebor wrote: > On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: >> Hi, >> The attached patch tries to fix PR80806 by warning when a variable is >> set using memset (and friends) but not used. I chose to warn in dse >> pass since dse would detect if the variable passed as 1st argument is >> a dead store. Does this approach look OK ? > > Detecting -Wunused-but-set-variable in the optimizer means that > the warning will not be issued without optimization. It also > means that the warning will trigger in cases where the variable > is used conditionally and the condition is subject to constant > propagation. For instance: > > void sink (void*); > > void test (int i) > { > char buf[10]; // -Wunused-but-set-variable > memset (buf, 0, sizeof(buf)); > > if (i) > sink (buf); > } > > void f (void) > { > test (0); > } > > I suspect this would be considered a false positive by most users. > In my view, it would be more in line with the design of the warning > to enhance the front end to detect this case, and it would avoid > these issues. > > I have a patch that does that. Rather than checking the finite > set of known built-in functions like memset that are known not > to read the referenced object, I took the approach of adding > a new function attribute (I call it write-only) and avoiding > setting the DECL_READ_P flag for DECLs that are passed to > function arguments decorated with the attribute. That makes > it possible to issue the warning even if the variable is passed > to ordinary (non-built-in) functions like getline(), and should > open up optimization opportunities beyond built-ins. The only > wrinkle is that the front end sets DECL_READ_P even for uses that > aren't reads such as a sizeof expression, so while an otherwise > unused buf is diagnosed given a call to memset(buf, 0, 10), it > isn't diagnosed if a call is made to memset(buf, 0, sizeof buf). > I am yet to see what impact not setting DECL_READ_P would have > when the decl is used without being evaluated. (In any event, > setting DECL_READ_P on a use that doesn't involve reading the > DECL doesn't seem right.) > > I attach what I have so far in case you would like to check it > out. I think you have more experience with DSE than me so I'd > be interested in your thoughts on making use of the attribute > for optimization. (Another couple attributes I'm considering > to complement write-only is read-only and read-write, also > with the hope of improving both warnings and code generation. > Ideas on those would be welcome as well.) > >> >> There were following fallouts observed during bootstrap build: >> >> * double-int.c (div_and_round_double): >> Warning emitted 'den' set but not used for following call to memset: >> memset (den, 0, sizeof den); >> >> I assume the warning is correct since there's immediately call to: >> encode (den, lden, hden); >> >> and encode overwrites all the contents of den. >> Should the above call to memset be removed from the source ? > > IIUC, this seems to be a more involved example of the simple one > above. AFAICS, the buffer is subsequently read in the function, > but only conditionally. > > That said, since encode() overwrites the whole buffer right after > it has been cleared by memset, I would think that a warning pointing > that out could be helpful (although I'm not sure -Wunused is the > right warning to issue). > >> >> * tree-streamer.c (streamer_check_handled_ts_structures) >> The function defines a local array bool handled_p[LAST_TS_ENUM]; >> and the warning is emitted for: >> memset (&handled_p, 0, sizeof (handled_p)); >> >> That's because the function then initializes each element of the array >> handled_p to true >> making the memset call redundant. >> I am not sure if warning for the above case is a good idea ? The call >> to memset() seems deliberate, to initialize all elements to 0, and >> later assert checks if all the elements were explicitly set to true. > > Right. Warning on this function doesn't seem right since > the variable is subsequently used in the test loop. > >> >> * value-prof.c (free_hist): >> Warns for the call to memset: >> >> static int >> free_hist (void **slot, void *data ATTRIBUTE_UNUSED) >> { >> histogram_value hist = *(histogram_value *) slot; >> free (hist->hvalue.counters); >> if (flag_checking) >> memset (hist, 0xab, sizeof (*hist)); >> free (hist); >> return 1; >> } >> >> Assuming flag_checking was true, the call to memset would be dead >> anyway since it would be immediately freed ? Um, I don't understand >> the purpose of memset in the above function. > > I'm guessing it's an effort to invalidate the memory to detect > subsequent accesses to its contents. Warning on such code would > be valuable because it's a common misconception that a memset > can be used to wipe sensitive data (e.g., passwords) from memory > before freeing it, to prevent them from leaking into compromised > stack frames (or core files). But it should be a warning that's > distinct from -Wunused. > > MartinPR c/80806 - gcc does not warn if local array is memset only gcc/ChangeLog: PR c/80806 * builtin-attrs.def (DEF_ATTR_WRITE_ONLY): New. (ATTR_NOTHROW_WRONLY1_LEAF, ATTR_NOTHROW_WRONLY2_LEAF): New. (ATTR_RET1_NOTHROW_WRONLY1_LEAF): New. * builtins.def (bcopy, bzero, memcpy, memmove, memset): Update. (free, strcpy, strncpy, __memcpy_chk, __memmove_chk): Update. (__strcpy_chk, __strncpy_chk): Update. gcc/c/ChangeLog: PR c/80806 * c-parser.c (c_parser_attributes): Adjust. (c_parser_binary_expression): Same. (c_parser_unary_expression): Handle parser->no_set_read. (c_parser_sizeof_expression): Same. (c_parser_postfix_expression_after_primary): Adjust. (is_write_only_p): New function. (c_parser_expr_list): Add argument. Avoid setting DECL_READ_P for decls passed to write-only function arguments. (c_parser_oacc_wait_list): Adjust. * c-tree.h (parser_build_binary_op): Add argument. * c-typeck.c (default_conversion): Same. (parser_build_binary_op): Same. (build_binary_op): Same. gcc/c-family/ChangeLog: PR c/80806 * c-attribs.c (handle_write_only_attribute): New function. (c_common_attribute_table): Add attribute write_only. * c-common.c (nonnull_check_p): Update. (get_nonnull_operand): Rename... (get_attribute_operand): ...to this. * c-common.h (get_nonnull_operand): Rename... (get_attribute_operand): ...to this. (build_binary_op, default_conversion): Add argument. gcc/cp/ChangeLog: PR c/80806 * typeck.c (cp_default_conversion): Add argument. (build_binary_op): Same. gcc/testsuite/ChangeLog: PR c/80806 * gcc.dg/attr-write-only-2.c: New test. * gcc.dg/attr-write-only.c: New test. * c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust. * gcc.dg/Wstrict-aliasing-bogus-vla-1.c: Same. * gcc.dg/attr-alloc_size.c: Same. * gcc.dg/pr40340-2.c: Same. * gcc.dg/pr78768.c: Same. * gcc.dg/pr79715.c: Same. diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def index 38fb1bb8..24e6f6a 100644 --- a/gcc/builtin-attrs.def +++ b/gcc/builtin-attrs.def @@ -113,6 +113,7 @@ DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm") DEF_ATTR_IDENT (ATTR_TM_TMPURE, "transaction_pure") DEF_ATTR_IDENT (ATTR_RETURNS_TWICE, "returns_twice") DEF_ATTR_IDENT (ATTR_RETURNS_NONNULL, "returns_nonnull") +DEF_ATTR_IDENT (ATTR_WRITE_ONLY, "write_only") DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL) @@ -196,6 +197,7 @@ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL, ATTR_NONNULL, ATTR_NULL, \ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_LEAF, ATTR_NONNULL, ATTR_NULL, \ ATTR_NOTHROW_LEAF_LIST) DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_LEAF_LIST, ATTR_LEAF, ATTR_NULL, ATTR_NOTHROW_NONNULL_LEAF) + /* Nothrow functions whose first parameter is a nonnull pointer. */ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_1, ATTR_NONNULL, ATTR_LIST_1, \ ATTR_NOTHROW_LIST) @@ -255,10 +257,21 @@ DEF_ATTR_TREE_LIST (ATTR_NOTHROW_NONNULL_TYPEGENERIC_LEAF, /* Nothrow const functions whose pointer parameter(s) are all nonnull. */ DEF_ATTR_TREE_LIST (ATTR_CONST_NOTHROW_NONNULL, ATTR_CONST, ATTR_NULL, \ ATTR_NOTHROW_NONNULL) + /* Nothrow leaf functions whose pointer parameter(s) are all nonnull, and which return their first argument. */ DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_NONNULL_LEAF, ATTR_FNSPEC, ATTR_LIST_STR1, \ ATTR_NOTHROW_NONNULL_LEAF) + +DEF_ATTR_TREE_LIST (ATTR_NOTHROW_WRONLY1_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_1, ATTR_NOTHROW_LEAF_LIST) + +DEF_ATTR_TREE_LIST (ATTR_NOTHROW_WRONLY2_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_2, ATTR_NOTHROW_LEAF_LIST) + +DEF_ATTR_TREE_LIST (ATTR_RET1_NOTHROW_WRONLY1_LEAF, ATTR_WRITE_ONLY, \ + ATTR_LIST_1, ATTR_RET1_NOTHROW_NONNULL_LEAF) + /* Nothrow leaf functions whose pointer parameter(s) are all nonnull, and return value is also nonnull. */ DEF_ATTR_TREE_LIST (ATTR_RETNONNULL_NOTHROW_LEAF, ATTR_RETURNS_NONNULL, ATTR_NULL, \ diff --git a/gcc/builtins.def b/gcc/builtins.def index 58d78db..59d1453 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -652,15 +652,15 @@ DEF_C99_COMPL_BUILTIN (BUILT_IN_CTANL, "ctanl", BT_FN_COMPLEX_LONGDOUBLE_ /* bcmp, bcopy and bzero have traditionally accepted NULL pointers when the length parameter is zero, so don't apply attribute "nonnull". */ DEF_EXT_LIB_BUILTIN (BUILT_IN_BCMP, "bcmp", BT_FN_INT_CONST_PTR_CONST_PTR_SIZE, ATTR_PURE_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN (BUILT_IN_BCOPY, "bcopy", BT_FN_VOID_CONST_PTR_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN (BUILT_IN_BZERO, "bzero", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_LEAF_LIST) +DEF_EXT_LIB_BUILTIN (BUILT_IN_BCOPY, "bcopy", BT_FN_VOID_CONST_PTR_PTR_SIZE, ATTR_NOTHROW_WRONLY2_LEAF) +DEF_EXT_LIB_BUILTIN (BUILT_IN_BZERO, "bzero", BT_FN_VOID_PTR_SIZE, ATTR_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_INDEX, "index", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_MEMCHR, "memchr", BT_FN_PTR_CONST_PTR_INT_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_MEMCMP, "memcmp", BT_FN_INT_CONST_PTR_CONST_PTR_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY, "memcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE, "memmove", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY, "memcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE, "memmove", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMPCPY, "mempcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET, "memset", BT_FN_PTR_PTR_INT_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET, "memset", BT_FN_PTR_PTR_INT_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_RINDEX, "rindex", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STPCPY, "stpcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STPNCPY, "stpncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) @@ -668,7 +668,7 @@ DEF_EXT_LIB_BUILTIN (BUILT_IN_STRCASECMP, "strcasecmp", BT_FN_INT_CONST_STRIN DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCAT, "strcat", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCHR, "strchr", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRCMP, "strcmp", BT_FN_INT_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY, "strcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY, "strcpy", BT_FN_STRING_STRING_CONST_STRING, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRCSPN, "strcspn", BT_FN_SIZE_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRDUP, "strdup", BT_FN_STRING_CONST_STRING, ATTR_MALLOC_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNDUP, "strndup", BT_FN_STRING_CONST_STRING_SIZE, ATTR_MALLOC_NOTHROW_NONNULL_LEAF) @@ -676,7 +676,7 @@ DEF_LIB_BUILTIN_CHKP (BUILT_IN_STRLEN, "strlen", BT_FN_SIZE_CONST_STRING, ATTR DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCASECMP, "strncasecmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRNCAT, "strncat", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRNCMP, "strncmp", BT_FN_INT_CONST_STRING_CONST_STRING_SIZE, ATTR_PURE_NOTHROW_NONNULL_LEAF) -DEF_LIB_BUILTIN (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_LIB_BUILTIN (BUILT_IN_STRNCPY, "strncpy", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRPBRK, "strpbrk", BT_FN_STRING_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRRCHR, "strrchr", BT_FN_STRING_CONST_STRING_INT, ATTR_PURE_NOTHROW_NONNULL_LEAF) DEF_LIB_BUILTIN (BUILT_IN_STRSPN, "strspn", BT_FN_SIZE_CONST_STRING_CONST_STRING, ATTR_PURE_NOTHROW_NONNULL_LEAF) @@ -825,7 +825,7 @@ DEF_EXT_LIB_BUILTIN (BUILT_IN_FFSLL, "ffsll", BT_FN_INT_LONGLONG, ATTR_CONST_ DEF_EXT_LIB_BUILTIN (BUILT_IN_FORK, "fork", BT_FN_PID, ATTR_NOTHROW_LIST) DEF_GCC_BUILTIN (BUILT_IN_FRAME_ADDRESS, "frame_address", BT_FN_PTR_UINT, ATTR_NULL) /* [trans-mem]: Adjust BUILT_IN_TM_FREE if BUILT_IN_FREE is changed. */ -DEF_LIB_BUILTIN (BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST) +DEF_LIB_BUILTIN (BUILT_IN_FREE, "free", BT_FN_VOID_PTR, ATTR_NOTHROW_WRONLY1_LEAF) DEF_GCC_BUILTIN (BUILT_IN_FROB_RETURN_ADDR, "frob_return_addr", BT_FN_PTR_PTR, ATTR_NULL) DEF_EXT_LIB_BUILTIN (BUILT_IN_GETTEXT, "gettext", BT_FN_STRING_CONST_STRING, ATTR_FORMAT_ARG_1) DEF_C99_BUILTIN (BUILT_IN_IMAXABS, "imaxabs", BT_FN_INTMAX_INTMAX, ATTR_CONST_NOTHROW_LEAF_LIST) @@ -916,16 +916,16 @@ DEF_BUILTIN_STUB (BUILT_IN_MEMCMP_EQ, "__builtin_memcmp_eq") /* Object size checking builtins. */ DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_PURE_NOTHROW_LEAF_LIST) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY_CHK, "__memcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE_CHK, "__memmove_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMCPY_CHK, "__memcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMMOVE_CHK, "__memmove_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMPCPY_CHK, "__mempcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET_CHK, "__memset_chk", BT_FN_PTR_PTR_INT_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_MEMSET_CHK, "__memset_chk", BT_FN_PTR_PTR_INT_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STPCPY_CHK, "__stpcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STPNCPY_CHK, "__stpncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCAT_CHK, "__strcat_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY_CHK, "__strcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN_CHKP (BUILT_IN_STRCPY_CHK, "__strcpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCAT_CHK, "__strncat_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) -DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCPY_CHK, "__strncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) +DEF_EXT_LIB_BUILTIN (BUILT_IN_STRNCPY_CHK, "__strncpy_chk", BT_FN_STRING_STRING_CONST_STRING_SIZE_SIZE, ATTR_RET1_NOTHROW_WRONLY1_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_SNPRINTF_CHK, "__snprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VAR, ATTR_FORMAT_PRINTF_NOTHROW_5_6) DEF_EXT_LIB_BUILTIN (BUILT_IN_SPRINTF_CHK, "__sprintf_chk", BT_FN_INT_STRING_INT_SIZE_CONST_STRING_VAR, ATTR_NOTHROW_NONNULL_1_FORMAT_PRINTF_4_5) DEF_EXT_LIB_BUILTIN (BUILT_IN_VSNPRINTF_CHK, "__vsnprintf_chk", BT_FN_INT_STRING_SIZE_INT_SIZE_CONST_STRING_VALIST_ARG, ATTR_FORMAT_PRINTF_NOTHROW_5_0) diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 695c58c..ad3d18f 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-iterator.h" #include "opts.h" #include "gimplify.h" +#include "bitmap.h" static tree handle_packed_attribute (tree *, tree, tree, int, bool *); static tree handle_nocommon_attribute (tree *, tree, tree, int, bool *); @@ -116,6 +117,7 @@ static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *); static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *); static tree handle_warn_unused_result_attribute (tree *, tree, tree, int, bool *); +static tree handle_write_only_attribute (tree *, tree, tree, int, bool *); static tree handle_sentinel_attribute (tree *, tree, tree, int, bool *); static tree handle_type_generic_attribute (tree *, tree, tree, int, bool *); static tree handle_alloc_size_attribute (tree *, tree, tree, int, bool *); @@ -345,6 +347,8 @@ const struct attribute_spec c_common_attribute_table[] = handle_bnd_instrument, false }, { "fallthrough", 0, 0, false, false, false, handle_fallthrough_attribute, false }, + { "write_only", 0, -1, false, true, true, + handle_write_only_attribute, false }, { NULL, 0, 0, false, false, false, NULL, false } }; @@ -2815,7 +2819,7 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name), && TREE_CODE (arg) != FUNCTION_DECL) TREE_VALUE (args) = arg = default_conversion (arg); - if (!get_nonnull_operand (arg, &arg_num)) + if (!get_attribute_operand (arg, &arg_num)) { error ("nonnull argument has invalid operand number (argument %lu)", (unsigned long) attr_arg_num); @@ -2860,6 +2864,121 @@ handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name), return NULL_TREE; } + +/* Handle the "write_only" attribute. */ + +static tree +handle_write_only_attribute (tree *node, tree name, + tree operands, int ARG_UNUSED (flags), + bool *no_add_attrs) +{ + tree type = *node; + + /* If no operands are specified, all pointer arguments should be + write-only. Verify a full prototype is given so that the arguments + will have the correct types when we actually check them later. + Avoid diagnosing type-generic built-ins since those have no + prototype. */ + if (!operands + && !prototype_p (type) + && (!TYPE_ATTRIBUTES (type) + || !lookup_attribute ("type generic", TYPE_ATTRIBUTES (type)))) + { + error ("attribute %qE without arguments on a non-prototype", name); + *no_add_attrs = true; + return NULL_TREE; + } + + /* True if operands were specified. */ + bool has_ops = operands; + + /* A bitmap of operand values already handled. Used to warn about + duplicates. */ + bitmap_head operset; + bitmap_initialize (&operset, 0); + + /* Attribute operands have been specified. Verify that each operand + value, OPERVAL, references a non-const pointer argument to the + function. */ + for (unsigned operno = 1; ; operno++, + operands = operands ? TREE_CHAIN (operands) : NULL_TREE) + { + /* The argument number the attribute operand corresponds to. */ + unsigned HOST_WIDE_INT operval; + + if (operands) + { + tree oper = TREE_VALUE (operands); + if (oper && TREE_CODE (oper) != IDENTIFIER_NODE + && TREE_CODE (oper) != FUNCTION_DECL) + TREE_VALUE (operands) = oper = default_conversion (oper); + + if (!get_attribute_operand (oper, &operval)) + { + error ("attribute %<%E(%E)%> invalid operand", name, oper); + *no_add_attrs = true; + break; + } + } + else if (!has_ops) + operval = operno; + else + break; + + if (bitmap_bit_p (&operset, operval)) + warning (OPT_Wattributes, "duplicate attribute %<%E(%wu)%>", + name, operval); + + bitmap_set_bit (&operset, operval); + + if (!prototype_p (type)) + continue; + + tree argtype; + + function_args_iterator iter; + function_args_iter_init (&iter, type); + + for (unsigned idx = 1; ; ++idx, function_args_iter_next (&iter)) + { + argtype = function_args_iter_cond (&iter); + if (!argtype || VOID_TYPE_P (argtype)) + { + if (has_ops) + { + error ("attribute %<%E(%wu)%> exceeds number of arguments %u", + name, operval, idx - 1); + *no_add_attrs = true; + } + bitmap_clear (&operset); + return NULL_TREE; + } + + if (idx == operval) + break; + } + + if (TREE_CODE (argtype) != POINTER_TYPE) + { + error ("attribute %<%E(%wu)%> references non-pointer argument " + "type %qT", name, operval, argtype); + *no_add_attrs = true; + break; + } + + if (TYPE_READONLY (TREE_TYPE (argtype))) + { + error ("attribute %<%E(%wu)%> references %qs-qualified argument " + "type %qT", name, operval, "const", argtype); + *no_add_attrs = true; + break; + } + } + + bitmap_clear (&operset); + return NULL_TREE; +} + /* Handle a "nothrow" attribute; arguments as in struct attribute_spec.handler. */ diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 5f4488a..8f95bae 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5277,7 +5277,7 @@ nonnull_check_p (tree args, unsigned HOST_WIDE_INT param_num) for (; args; args = TREE_CHAIN (args)) { - bool found = get_nonnull_operand (TREE_VALUE (args), &arg_num); + bool found = get_attribute_operand (TREE_VALUE (args), &arg_num); gcc_assert (found); @@ -5314,11 +5314,11 @@ check_nonnull_arg (void *ctx, tree param, unsigned HOST_WIDE_INT param_num) } } -/* Helper for nonnull attribute handling; fetch the operand number - from the attribute argument list. */ +/* Helper for attribute handling; fetch the operand number from + the attribute argument list. */ bool -get_nonnull_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp) +get_attribute_operand (tree arg_num_expr, unsigned HOST_WIDE_INT *valp) { /* Verify the arg number is a small constant. */ if (tree_fits_uhwi_p (arg_num_expr)) diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 502dc2f..383604f 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -864,7 +864,7 @@ extern bool pointer_to_zero_sized_aggr_p (tree); extern bool bool_promoted_to_int_p (tree); extern tree fold_for_warn (tree); extern tree c_common_get_narrower (tree, int *); -extern bool get_nonnull_operand (tree, unsigned HOST_WIDE_INT *); +extern bool get_attribute_operand (tree, unsigned HOST_WIDE_INT *); #define c_sizeof(LOC, T) c_sizeof_or_alignof_type (LOC, T, true, false, 1) #define c_alignof(LOC, T) c_sizeof_or_alignof_type (LOC, T, false, false, 1) @@ -960,13 +960,14 @@ extern tree build_real_imag_expr (location_t, enum tree_code, tree); a variant of the C language. They are used in c-common.c. */ extern tree build_unary_op (location_t, enum tree_code, tree, bool); -extern tree build_binary_op (location_t, enum tree_code, tree, tree, int); +extern tree build_binary_op (location_t, enum tree_code, tree, tree, + bool, bool = true); extern tree perform_integral_promotions (tree); /* These functions must be defined by each front-end which implements a variant of the C language. They are used by port files. */ -extern tree default_conversion (tree); +extern tree default_conversion (tree, bool = true); /* Given two integer or real types, return the type for their sum. Given two compatible ANSI C types, returns the merged type. */ diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 2e01316..bb5be5f 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -178,6 +178,9 @@ struct GTY(()) c_parser { BOOL_BITFIELD in_if_block : 1; /* True if we want to lex an untranslated string. */ BOOL_BITFIELD lex_untranslated_string : 1; + /* False if taking the address of a DECL should not set its DECL_READ_P + flag. */ + BOOL_BITFIELD no_set_read : 1; /* Objective-C specific parser/lexer information. */ @@ -1253,7 +1256,8 @@ static tree c_parser_transaction_cancel (c_parser *); static struct c_expr c_parser_expression (c_parser *); static struct c_expr c_parser_expression_conv (c_parser *); static vec<tree, va_gc> *c_parser_expr_list (c_parser *, bool, bool, - vec<tree, va_gc> **, location_t *, + vec<tree, va_gc> **, tree, + location_t *, tree *, vec<location_t> *, unsigned int * = NULL); static void c_parser_oacc_declare (c_parser *); @@ -4215,8 +4219,8 @@ c_parser_attributes (c_parser *parser) { tree tree_list; c_parser_consume_token (parser); - expr_list = c_parser_expr_list (parser, false, true, - NULL, NULL, NULL, NULL); + expr_list = c_parser_expr_list (parser, false, true, NULL, + NULL_TREE, NULL, NULL, NULL); tree_list = build_tree_list_vec (expr_list); attr_args = tree_cons (NULL_TREE, arg1, tree_list); release_tree_vector (expr_list); @@ -4228,8 +4232,8 @@ c_parser_attributes (c_parser *parser) attr_args = NULL_TREE; else { - expr_list = c_parser_expr_list (parser, false, true, - NULL, NULL, NULL, NULL); + expr_list = c_parser_expr_list (parser, false, true, NULL, + NULL_TREE, NULL, NULL, NULL); attr_args = build_tree_list_vec (expr_list); release_tree_vector (expr_list); } @@ -6659,6 +6663,9 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, location_t loc; } stack[NUM_PRECS]; int sp; + + bool read_p = !parser->no_set_read; + /* Location of the binary operator. */ location_t binary_loc = UNKNOWN_LOCATION; /* Quiet warning. */ #define POP \ @@ -6678,10 +6685,10 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, } \ stack[sp - 1].expr \ = convert_lvalue_to_rvalue (stack[sp - 1].loc, \ - stack[sp - 1].expr, true, true); \ + stack[sp - 1].expr, true, read_p); \ stack[sp].expr \ = convert_lvalue_to_rvalue (stack[sp].loc, \ - stack[sp].expr, true, true); \ + stack[sp].expr, true, read_p); \ if (__builtin_expect (omp_atomic_lhs != NULL_TREE, 0) && sp == 1 \ && c_parser_peek_token (parser)->type == CPP_SEMICOLON \ && ((1 << stack[sp].prec) \ @@ -6699,7 +6706,8 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, stack[sp - 1].expr = parser_build_binary_op (stack[sp].loc, \ stack[sp].op, \ stack[sp - 1].expr, \ - stack[sp].expr); \ + stack[sp].expr, \ + read_p); \ sp--; \ } while (0) gcc_assert (!after || c_dialect_objc ()); @@ -6803,7 +6811,7 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, src_range = stack[sp].expr.src_range; stack[sp].expr = convert_lvalue_to_rvalue (stack[sp].loc, - stack[sp].expr, true, true); + stack[sp].expr, true, read_p); stack[sp].expr.value = c_objc_common_truthvalue_conversion (stack[sp].loc, default_conversion (stack[sp].expr.value)); c_inhibit_evaluation_warnings += (stack[sp].expr.value @@ -6814,7 +6822,7 @@ c_parser_binary_expression (c_parser *parser, struct c_expr *after, src_range = stack[sp].expr.src_range; stack[sp].expr = convert_lvalue_to_rvalue (stack[sp].loc, - stack[sp].expr, true, true); + stack[sp].expr, true, read_p); stack[sp].expr.value = c_objc_common_truthvalue_conversion (stack[sp].loc, default_conversion (stack[sp].expr.value)); c_inhibit_evaluation_warnings += (stack[sp].expr.value @@ -6973,7 +6981,8 @@ c_parser_unary_expression (c_parser *parser) case CPP_AND: c_parser_consume_token (parser); op = c_parser_cast_expression (parser, NULL); - mark_exp_read (op.value); + if (!parser->no_set_read) + mark_exp_read (op.value); return parser_build_unary_op (op_loc, ADDR_EXPR, op); case CPP_MULT: { @@ -7130,7 +7139,7 @@ c_parser_sizeof_expression (c_parser *parser) sizeof_expr: c_inhibit_evaluation_warnings--; in_sizeof--; - mark_exp_read (expr.value); + // mark_exp_read (expr.value); if (TREE_CODE (expr.value) == COMPONENT_REF && DECL_C_BIT_FIELD (TREE_OPERAND (expr.value, 1))) error_at (expr_loc, "%<sizeof%> applied to a bit-field"); @@ -8415,8 +8424,10 @@ c_parser_postfix_expression_after_primary (c_parser *parser, exprlist = NULL; else exprlist = c_parser_expr_list (parser, true, false, &origtypes, + expr.value, sizeof_arg_loc, sizeof_arg, &arg_loc, &literal_zero_mask); + c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, "expected %<)%>"); orig_expr = expr; @@ -8658,6 +8669,32 @@ c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask, } } +/* Return true if expression number IDX and type ARGTYPE eclared with + the write-only attribute in ATTRS is itself declared write-only. + ATTRS may be null. */ + +static bool +is_write_only_p (tree attrs, tree argtype, unsigned idx) +{ + if (!attrs) + return false; + + if (!POINTER_TYPE_P (argtype)) + return false; + + if (TYPE_READONLY (TREE_TYPE (argtype))) + return false; + + for (tree a = attrs; a; a = TREE_CHAIN (a)) + { + unsigned HOST_WIDE_INT wronlyidx; + if (!get_attribute_operand (a, &wronlyidx) || wronlyidx == idx) + return true; + } + return false; +} + + /* Parse a non-empty list of expressions. If CONVERT_P, convert functions and arrays to pointers and lvalues to rvalues. If FOLD_P, fold the expressions. If LOCATIONS is non-NULL, save the @@ -8670,7 +8707,7 @@ c_parser_check_literal_zero (c_parser *parser, unsigned *literal_zero_mask, static vec<tree, va_gc> * c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, - vec<tree, va_gc> **p_orig_types, + vec<tree, va_gc> **p_orig_types, tree func, location_t *sizeof_arg_loc, tree *sizeof_arg, vec<location_t> *locations, unsigned int *literal_zero_mask) @@ -8680,6 +8717,8 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, struct c_expr expr; location_t loc = c_parser_peek_token (parser)->location; location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION; + + /* Zero-based expression index within the list. */ unsigned int idx = 0; ret = make_tree_vector (); @@ -8693,9 +8732,40 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, 0); - expr = c_parser_expr_no_commas (parser, NULL); + + tree functype = func ? TREE_TYPE (func) : NULL_TREE; + if (functype) + { + while (POINTER_TYPE_P (functype)) + functype = TREE_TYPE (functype); + } + + tree attrs = (functype + ? lookup_attribute ("write_only", TYPE_ATTRIBUTES (functype)) + : NULL_TREE); + + tree argtypes = attrs ? TYPE_ARG_TYPES (functype) : NULL_TREE; + + /* Determine if the expression in the list is declared write-only + and only mark it DECL_READ_P() if it isn't. */ + bool wronly + = argtypes && is_write_only_p (attrs, TREE_VALUE (argtypes), idx + 1); + + { + /* Avoid setting the read bit for write-only decls. */ + bool save_set_read = parser->no_set_read; + parser->no_set_read = wronly; + expr = c_parser_expr_no_commas (parser, NULL); + parser->no_set_read = save_set_read; + } + if (convert_p) - expr = convert_lvalue_to_rvalue (loc, expr, true, true); + { + /* Allow DECL_READ_P to be set if the expression is a pointer. */ + wronly &= !DECL_P (expr.value) || !POINTER_TYPE_P (TREE_TYPE (expr.value)); + expr = convert_lvalue_to_rvalue (loc, expr, true, !wronly); + } + if (fold_p) expr.value = c_fully_fold (expr.value, false, NULL); ret->quick_push (expr.value); @@ -8721,9 +8791,32 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, cur_sizeof_arg_loc = UNKNOWN_LOCATION; if (literal_zero_mask) c_parser_check_literal_zero (parser, literal_zero_mask, idx + 1); - expr = c_parser_expr_no_commas (parser, NULL); + + /* Determine if the next expression in the list is declared + write-only and only mark it DECL_READ_P() if it isn't. */ + if (argtypes && attrs) + { + argtypes = TREE_CHAIN (argtypes); + wronly = is_write_only_p (attrs, TREE_VALUE (argtypes), idx + 2); + } + else + wronly = false; + + { + /* Avoid setting the read bit for write-only decls. */ + bool save_set_read = parser->no_set_read; + parser->no_set_read = wronly; + expr = c_parser_expr_no_commas (parser, NULL); + parser->no_set_read = save_set_read; + } + if (convert_p) - expr = convert_lvalue_to_rvalue (loc, expr, true, true); + { + wronly &= (!DECL_P (expr.value) + || !POINTER_TYPE_P (TREE_TYPE (expr.value))); + expr = convert_lvalue_to_rvalue (loc, expr, true, !wronly); + } + if (fold_p) expr.value = c_fully_fold (expr.value, false, NULL); vec_safe_push (ret, expr.value); @@ -8742,6 +8835,7 @@ c_parser_expr_list (c_parser *parser, bool convert_p, bool fold_p, } if (orig_types) *p_orig_types = orig_types; + return ret; } @@ -9831,8 +9925,8 @@ static tree c_parser_objc_keywordexpr (c_parser *parser) { tree ret; - vec<tree, va_gc> *expr_list = c_parser_expr_list (parser, true, true, - NULL, NULL, NULL, NULL); + vec<tree, va_gc> *expr_list = c_parser_expr_list (parser, true, true, NULL, + NULL_TREE, NULL, NULL, NULL); if (vec_safe_length (expr_list) == 1) { /* Just return the expression, remove a level of @@ -10682,7 +10776,8 @@ c_parser_oacc_wait_list (c_parser *parser, location_t clause_loc, tree list) if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) return list; - args = c_parser_expr_list (parser, false, true, NULL, NULL, NULL, NULL); + args = c_parser_expr_list (parser, false, true, NULL, NULL_TREE, NULL, + NULL, NULL); if (args->length () == 0) { diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index 17a8897..88f1449 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -641,7 +641,7 @@ extern struct c_expr parser_build_unary_op (location_t, enum tree_code, struct c_expr); extern struct c_expr parser_build_binary_op (location_t, enum tree_code, struct c_expr, - struct c_expr); + struct c_expr, bool = true); extern tree build_conditional_expr (location_t, tree, bool, tree, tree, tree, tree); extern tree build_compound_expr (location_t, tree, tree); diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 25b7dd6..c129cad 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -2131,14 +2131,15 @@ perform_integral_promotions (tree exp) In addition, manifest constants symbols are replaced by their values. */ tree -default_conversion (tree exp) +default_conversion (tree exp, bool read_p /* = true */) { tree orig_exp; tree type = TREE_TYPE (exp); enum tree_code code = TREE_CODE (type); tree promoted_type; - mark_exp_read (exp); + if (read_p) + mark_exp_read (exp); /* Functions and arrays have been converted during parsing. */ gcc_assert (code != FUNCTION_TYPE); @@ -3622,7 +3623,8 @@ char_type_p (tree type) struct c_expr parser_build_binary_op (location_t location, enum tree_code code, - struct c_expr arg1, struct c_expr arg2) + struct c_expr arg1, struct c_expr arg2, + bool read_p /* = true */) { struct c_expr result; @@ -3636,7 +3638,7 @@ parser_build_binary_op (location_t location, enum tree_code code, : TREE_TYPE (arg2.value)); result.value = build_binary_op (location, code, - arg1.value, arg2.value, 1); + arg1.value, arg2.value, true, read_p); result.original_code = code; result.original_type = NULL; @@ -10730,7 +10732,7 @@ build_vec_cmp (tree_code code, tree type, tree build_binary_op (location_t location, enum tree_code code, - tree orig_op0, tree orig_op1, int convert_p) + tree orig_op0, tree orig_op1, bool convert_p, bool read_p) { tree type0, type1, orig_type0, orig_type1; tree eptype; @@ -10835,8 +10837,8 @@ build_binary_op (location_t location, enum tree_code code, if (convert_p && VECTOR_TYPE_P (TREE_TYPE (op0)) == VECTOR_TYPE_P (TREE_TYPE (op1))) { - op0 = default_conversion (op0); - op1 = default_conversion (op1); + op0 = default_conversion (op0, read_p); + op1 = default_conversion (op1, read_p); } /* When Cilk Plus is enabled and there are array notations inside op0, then diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c index 13d90a6..5241b8d 100644 --- a/gcc/cp/typeck.c +++ b/gcc/cp/typeck.c @@ -2095,7 +2095,7 @@ cp_default_conversion (tree exp, tsubst_flags_t complain) /* C version. */ tree -default_conversion (tree exp) +default_conversion (tree exp, bool /*read_p*/) { return cp_default_conversion (exp, tf_warning_or_error); } @@ -4056,7 +4056,7 @@ enum_cast_to_int (tree op) /* For the c-common bits. */ tree build_binary_op (location_t location, enum tree_code code, tree op0, tree op1, - int /*convert_p*/) + bool /*convert_p*/, bool /*read_p*/) { return cp_build_binary_op (location, code, op0, op1, tf_warning_or_error); } diff --git a/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c b/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c index 895a50e..1a15b00 100644 --- a/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c +++ b/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c @@ -481,5 +481,6 @@ f4 (char *x, char **y, int z, char w[64]) stpncpy (x, s3, sizeof (s3)); } +/* { dg-prune-output "-Wunused-but-set-variable" } */ /* { dg-prune-output "\[\n\r\]*writing\[\n\r\]*" } */ /* { dg-prune-output "\[\n\r\]*reading\[\n\r\]*" } */ diff --git a/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c b/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c index 87f5ef9..f7f7071 100644 --- a/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c +++ b/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c @@ -9,3 +9,5 @@ int main(int argc, char *argv[]) float y[argc]; return 0 == __builtin_memcpy(y, x, argc * sizeof(*x)); } + +/* { dg-prune-output "-Wunused-but-set-variable" } */ diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size.c b/gcc/testsuite/gcc.dg/attr-alloc_size.c index f50ba7c..2422acf 100644 --- a/gcc/testsuite/gcc.dg/attr-alloc_size.c +++ b/gcc/testsuite/gcc.dg/attr-alloc_size.c @@ -32,5 +32,6 @@ test (void) p = calloc2 (2, __INT_MAX__ >= 1700000 ? 424242 : __INT_MAX__ / 4, 5); strcpy (p, "World"); strcpy (p, "Hello World"); /* { dg-warning "writing" "strcpy" } */ -} + (void)&p; /* suppress -Wunused-but-set-variable */ +} diff --git a/gcc/testsuite/gcc.dg/attr-write-only-2.c b/gcc/testsuite/gcc.dg/attr-write-only-2.c new file mode 100644 index 0000000..8176ac3 --- /dev/null +++ b/gcc/testsuite/gcc.dg/attr-write-only-2.c @@ -0,0 +1,192 @@ +/* Test to verify that attribute write_only is recognized by + -Wunused-but-set-variable. + { dg-do compile } + { dg-options "-Wunused-but-set-variable -Wunused-but-set-parameter" } */ + +typedef __SIZE_TYPE__ size_t; + +void bcopy (const void*, void*, size_t); +void bzero (void*, size_t); +void* memcpy (void*, const void*, size_t); +void* memset (void*, int, size_t); + +char* strcat (char*, const char*); +char* strcpy (char*, const char*); +char* strncat (char*, const char*, size_t); +char* strncpy (char*, const char*, size_t); + +void* malloc (size_t); +void free (void*); + +#define WRONLY(...) __attribute__ ((write_only (__VA_ARGS__))) + +void wronly_1 (void*) WRONLY (1); + +enum { N = sizeof (int) }; + +void test_bcopy (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + const void *p) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + bcopy (p, &a, N); + bcopy (p, &i, N); + bcopy (p, &ar, N); + + bcopy (p, ar, N); + bcopy (p, &ar, N); + bcopy (p, &ar + 1, N); + bcopy (p, &ar[0], N); + bcopy (p, &ar[1], N); +} + +void test_bzero (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + bzero (&a, N); + bzero (&a + 0, N); + bzero (&i, N); + bzero (&i + 0, N); + + bzero (ar, N); + bzero (ar + 1, N); + bzero (&ar, N); + bzero (&ar + 1, N); + bzero (&ar[0], N); + bzero (&ar[1], N); +} + +void test_memset (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + memset (&a, 0, N); + memset (&i, 0, N); + memset (&ar, 0, N); +} + +void test_memcpy (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + const void *p) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + memcpy (&a, p, N); + memcpy (&i, p, N); + memcpy (&ar, p, N); +} + +void test_strcat (const char *s) +{ + char c = '\0'; + char ar[8] = ""; + + strcat (&c, ""); + strcat (ar, s); +} + +void test_strcpy (const char *s) +{ + char c; /* { dg-warning "-Wunused-but-set-variable" } */ + char d = '\0'; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar2[8] = ""; /* { dg-warning "-Wunused-but-set-variable" } */ + + strcpy (&c, ""); + strcpy (&d, ""); + strcpy (ar, s); + strcpy (ar2, s); +} + +void test_strncpy (const char *s, unsigned n) +{ + char c; /* { dg-warning "-Wunused-but-set-variable" } */ + char d = '\0'; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar2[8] = ""; /* { dg-warning "-Wunused-but-set-variable" } */ + + strncpy (&c, "", 1); + strncpy (&d, "", n); + strncpy (ar, s, n); + strncpy (ar2, s, n); +} + +/* Verify that passing a local pointer initialized to the result of + malloc() (anything else should work as well) to free() on doesn't + trigger a warning because the pointer value is obviously read. */ + +void test_malloc_and_free (size_t n) +{ + void *q = malloc (n); + free (q); +} + +/* Verify that setting a local variable to a copy of an argument and + calling free() on the copy doesn't trigger a warning. */ + +void test_free_argcpy (void *p) +{ + void *q = p; + free (q); +} + +/* Similar to the free test above, verify that setting a local variable + to a copy of an argument and calling free() on the copy doesn't trigger + a warning. */ + +void test_memset_argcpy (void *p, size_t n) +{ + void *q = p; + memset (q, 0, n); +} + +void test_usrdef (int a) /* { dg-warning "-Wunused-but-set-parameter" } */ +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + wronly_1 (&a); + wronly_1 (&i); + wronly_1 (&ar); + + wronly_1 (ar); + wronly_1 (ar + 1); + wronly_1 (&ar[1]); +} + +/* Verify that using a pointer to a function works. */ +typedef struct S +{ + void (*pf)(void*) WRONLY (1); + struct { + struct { + void (*pf)(void*) WRONLY (1); + } s2; + } s1; +} S; + +void test_funcptr (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + S *ps) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + ps->pf (&a); + ps->pf (&i); + ps->pf (&ar); +} + +void test_funcptr2 (int a, /* { dg-warning "-Wunused-but-set-parameter" } */ + S *ps) +{ + int i; /* { dg-warning "-Wunused-but-set-variable" } */ + char ar[8]; /* { dg-warning "-Wunused-but-set-variable" } */ + + ps->s1.s2.pf (&a); + ps->s1.s2.pf (&i); + ps->s1.s2.pf (&ar); +} diff --git a/gcc/testsuite/gcc.dg/attr-write-only.c b/gcc/testsuite/gcc.dg/attr-write-only.c new file mode 100644 index 0000000..e238c81 --- /dev/null +++ b/gcc/testsuite/gcc.dg/attr-write-only.c @@ -0,0 +1,34 @@ +/* Test to verify the handling of attribute write_only syntax. + { dg-do compile } + { dg-options "-Wall -ftrack-macro-expansion=0" } */ + +#define WRONLY(...) __attribute__ ((write_only (__VA_ARGS__))) + +void wronly_v_all (void) __attribute__ ((write_only)); /* { dg-error "attribute .write_only\\(1\\). exceeds number of arguments 0" } */ + +void wronly_i_all (int) __attribute__ ((write_only)); /* { dg-error "attribute .write_only\\(1\\). references non-pointer argument type .int." } */ + +void wronly_v_1 (void*) WRONLY (1); +void wronly_iv_2 (int, void*) WRONLY (2); +void wronly_iiv_3 (int, int, void*) WRONLY (3); + +void wronly_v_0p1 (void*) WRONLY (0 + 1); +void wronly_v_2m1 (void*) WRONLY (2 - 1); + +void wronly_v_1_1 (void*) WRONLY (1, 1); /* { dg-warning "duplicate attribute .write_only\\(1\\)." } */ +void wronly_vv_1_2 (void*, void*) WRONLY (1, 2); +void wronly_vv_2_1 (void*, void*) WRONLY (2, 1); +void wronly_vv_all (void*, void*) __attribute__ ((write_only)); + +void wronly_vv_1_1 (void*, void*) WRONLY (1, 1); /* { dg-warning "duplicate attribute .write_only\\(1\\)." } */ +void wronly_vv_1_1 (void*, void*) WRONLY (2, 2); /* { dg-warning "duplicate attribute .write_only\\(2\\)." } */ + +void wronly_4_exceed (int, int, int) WRONLY (4); /* { dg-error "attribute .write_only\\(4\\). exceeds number of arguments 3" } */ + +void wronly_1_i (int) WRONLY (1); /* { dg-error "attribute .write_only\\(1\\). references non-pointer argument type .int." } */ + +void wronly_2_cst (int, const char*) WRONLY (2); /* { dg-error "attribute .write_only\\(2\\). references .const.-qualified argument type .const char *." } */ + +void wronly_m1_i (void*) WRONLY (-1); /* { dg-error "attribute .write_only\\(-1\\). invalid operand" } */ + +void wronly_str_i (void*) WRONLY ("blah"); /* { dg-error "attribute .write_only\\(\"blah\"\\). invalid operand" } */ diff --git a/gcc/testsuite/gcc.dg/pr40340-2.c b/gcc/testsuite/gcc.dg/pr40340-2.c index 1dc21d1..eb933c1 100644 --- a/gcc/testsuite/gcc.dg/pr40340-2.c +++ b/gcc/testsuite/gcc.dg/pr40340-2.c @@ -12,5 +12,6 @@ main (void) return 0; } +/* { dg-prune-output "-Wunused-but-set-variable" } */ /* { dg-warning "writing" "" { target *-*-* } 10 } */ /* { dg-message "file included" "In file included" { target *-*-* } 0 } */ diff --git a/gcc/testsuite/gcc.dg/pr78768.c b/gcc/testsuite/gcc.dg/pr78768.c index b6cda47..5e747c9 100644 --- a/gcc/testsuite/gcc.dg/pr78768.c +++ b/gcc/testsuite/gcc.dg/pr78768.c @@ -11,5 +11,7 @@ int main (void) __builtin_sprintf (d, "%32s", "x"); /* { dg-warning "directive writing 32 bytes into a region of size 12" "-Wformat-overflow" { xfail *-*-* } } */ + (void)&d; /* Suppress -Wunused-but-set-variable. */ + return 0; } diff --git a/gcc/testsuite/gcc.dg/pr79715.c b/gcc/testsuite/gcc.dg/pr79715.c index 0f0f90f..3f7b38d 100644 --- a/gcc/testsuite/gcc.dg/pr79715.c +++ b/gcc/testsuite/gcc.dg/pr79715.c @@ -9,6 +9,8 @@ void f (const char *s) char *p = __builtin_malloc (n); __builtin_memcpy (p, s, n); __builtin_free (p); + + (void)&p; /* Suppress -Wunused-but-set-variable. */ } void g (const char *s) @@ -17,6 +19,8 @@ void g (const char *s) char *p = __builtin_malloc (n); __builtin_strcpy (p, s); __builtin_free (p); + + (void)&p; /* Suppress -Wunused-but-set-variable. */ } /* { dg-final { scan-tree-dump-not "free" "optimized" } }
On 05/23/2017 09:58 AM, Martin Sebor wrote: > On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: >> Hi, >> The attached patch tries to fix PR80806 by warning when a variable is >> set using memset (and friends) but not used. I chose to warn in dse >> pass since dse would detect if the variable passed as 1st argument is >> a dead store. Does this approach look OK ? > > Detecting -Wunused-but-set-variable in the optimizer means that > the warning will not be issued without optimization. It also > means that the warning will trigger in cases where the variable > is used conditionally and the condition is subject to constant > propagation. For instance: Yea. There's definitely tradeoffs for implementing warnings early vs late. There's little doubt we could construct testcases where an early warning would miss cases that could be caught by a late warning. > > void sink (void*); > > void test (int i) > { > char buf[10]; // -Wunused-but-set-variable > memset (buf, 0, sizeof(buf)); > > if (i) > sink (buf); > } > > void f (void) > { > test (0); > } > > I suspect this would be considered a false positive by most users. > In my view, it would be more in line with the design of the warning > to enhance the front end to detect this case, and it would avoid > these issues. Given no knowledge of sink() here, don't we have to assume that buf is used? So, yea, I'd probably consider that a false positive. > > I have a patch that does that. Rather than checking the finite > set of known built-in functions like memset that are known not > to read the referenced object, I took the approach of adding > a new function attribute (I call it write-only) and avoiding > setting the DECL_READ_P flag for DECLs that are passed to > function arguments decorated with the attribute. That makes > it possible to issue the warning even if the variable is passed > to ordinary (non-built-in) functions like getline(), and should > open up optimization opportunities beyond built-ins. ISTM like this would be generally useful. The only > wrinkle is that the front end sets DECL_READ_P even for uses that > aren't reads such as a sizeof expression, so while an otherwise > unused buf is diagnosed given a call to memset(buf, 0, 10), it > isn't diagnosed if a call is made to memset(buf, 0, sizeof buf). > I am yet to see what impact not setting DECL_READ_P would have > when the decl is used without being evaluated. (In any event, > setting DECL_READ_P on a use that doesn't involve reading the > DECL doesn't seem right.) Agreed. > > I attach what I have so far in case you would like to check it > out. I think you have more experience with DSE than me so I'd > be interested in your thoughts on making use of the attribute > for optimization. (Another couple attributes I'm considering > to complement write-only is read-only and read-write, also > with the hope of improving both warnings and code generation. > Ideas on those would be welcome as well.) Ideally we'd integrate this into the memory web, but I don't think we have that capability these days with the sparser representation. Essentially a definition is always considered a read and a write of the underlying memory object. This (write-only) likely wouldn't be used directly in DSE, but instead would live in the alias oracle support routines. In particular ref_maybe_used_by_call seems natural as that code already knows about similar situations with various builtins. DSE would use it implicitly as would other optimizers. Jeff
On 06/29/2017 11:57 AM, Jeff Law wrote: > On 05/23/2017 09:58 AM, Martin Sebor wrote: >> On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: >>> Hi, >>> The attached patch tries to fix PR80806 by warning when a variable is >>> set using memset (and friends) but not used. I chose to warn in dse >>> pass since dse would detect if the variable passed as 1st argument is >>> a dead store. Does this approach look OK ? >> >> Detecting -Wunused-but-set-variable in the optimizer means that >> the warning will not be issued without optimization. It also >> means that the warning will trigger in cases where the variable >> is used conditionally and the condition is subject to constant >> propagation. For instance: > Yea. There's definitely tradeoffs for implementing warnings early vs > late. There's little doubt we could construct testcases where an early > warning would miss cases that could be caught by a late warning. > > >> >> void sink (void*); >> >> void test (int i) >> { >> char buf[10]; // -Wunused-but-set-variable >> memset (buf, 0, sizeof(buf)); >> >> if (i) >> sink (buf); >> } >> >> void f (void) >> { >> test (0); >> } >> >> I suspect this would be considered a false positive by most users. >> In my view, it would be more in line with the design of the warning >> to enhance the front end to detect this case, and it would avoid >> these issues. > Given no knowledge of sink() here, don't we have to assume that buf is > used? So, yea, I'd probably consider that a false positive. Oh, wait, I missed the constant propagation. That makes this one less clear cut in my mind -- it means its context sensitive. I could easily argue either way on this one. Jeff
On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: > Hi, > The attached patch tries to fix PR80806 by warning when a variable is > set using memset (and friends) but not used. I chose to warn in dse > pass since dse would detect if the variable passed as 1st argument is > a dead store. Does this approach look OK ? [ ... ] So I think the biggest question is whether or not the case like Martin's deserves a warning. What we have is an object that is conditionally set but not used depending on inlining context. We've generally "allowed" inlining to expose new warnings in the sense that inlining may (for example) allow us to remove the addressibility property on an object -- which makes the object subject to the usual -Wuninitialized analysis. In fact, I think we've generally considered that a positive outcome because it's exposing bugs in subtle paths. I'm less sure that this case falls into that same category. What we're really talking about is warning for a partially dead store. Would we want a warning if rather than a memset this was a simple store? Is that the right guiding principle here? I hate to say it, but I wonder if this is another case where there likely won't be a clear consensus and we're going to end up with a two level warning system? For something like Martin's case what I really think we should do is sink the memset call into the conditional. In cases where "i" is not a constant, but actually has the value zero at runtime we win. -- So I've got no objections to the idea of using DSE to detect the dead store and potentially warn. My concern is are we in a case where that warning is going to annoy users and we end up needing a level of -Wunused-but-set. Jeff
On 06/29/2017 12:05 PM, Jeff Law wrote: > On 06/29/2017 11:57 AM, Jeff Law wrote: >> On 05/23/2017 09:58 AM, Martin Sebor wrote: >>> On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: >>>> Hi, >>>> The attached patch tries to fix PR80806 by warning when a variable is >>>> set using memset (and friends) but not used. I chose to warn in dse >>>> pass since dse would detect if the variable passed as 1st argument is >>>> a dead store. Does this approach look OK ? >>> >>> Detecting -Wunused-but-set-variable in the optimizer means that >>> the warning will not be issued without optimization. It also >>> means that the warning will trigger in cases where the variable >>> is used conditionally and the condition is subject to constant >>> propagation. For instance: >> Yea. There's definitely tradeoffs for implementing warnings early vs >> late. There's little doubt we could construct testcases where an early >> warning would miss cases that could be caught by a late warning. >> >> >>> >>> void sink (void*); >>> >>> void test (int i) >>> { >>> char buf[10]; // -Wunused-but-set-variable >>> memset (buf, 0, sizeof(buf)); >>> >>> if (i) >>> sink (buf); >>> } >>> >>> void f (void) >>> { >>> test (0); >>> } >>> >>> I suspect this would be considered a false positive by most users. >>> In my view, it would be more in line with the design of the warning >>> to enhance the front end to detect this case, and it would avoid >>> these issues. >> Given no knowledge of sink() here, don't we have to assume that buf is >> used? So, yea, I'd probably consider that a false positive. > Oh, wait, I missed the constant propagation. That makes this one less > clear cut in my mind -- it means its context sensitive. I could easily > argue either way on this one. Suppose buf were the small buffer in std::string and sink() some conditional use of the class, like in this more complicated code: void foo (std::string&); void bar (char*); void test (int i) { std::string s; // calls memset (s.buf, 0, sizeof s.buf) char array[100] = ""; if (i > 100) foo (s); // needs a large buffer else bar (array); // works with a small buffer } Variations on this idiom aren't uncommon. Martin
On Thu, 29 Jun 2017, Jeff Law wrote: > On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: > > Hi, > > The attached patch tries to fix PR80806 by warning when a variable is > > set using memset (and friends) but not used. I chose to warn in dse > > pass since dse would detect if the variable passed as 1st argument is > > a dead store. Does this approach look OK ? > [ ... ] > So I think the biggest question is whether or not the case like Martin's > deserves a warning. > > What we have is an object that is conditionally set but not used > depending on inlining context. We've generally "allowed" inlining to > expose new warnings in the sense that inlining may (for example) allow > us to remove the addressibility property on an object -- which makes the > object subject to the usual -Wuninitialized analysis. In fact, I think > we've generally considered that a positive outcome because it's exposing > bugs in subtle paths. But set-but-not-used isn't a warning like that, it's a warning like 'unused variable' which directs the user to simply delete the affected stmts (and variable). So the warning should only trigger if that would not make the program fail to compile. > I'm less sure that this case falls into that same category. What we're > really talking about is warning for a partially dead store. Would we > want a warning if rather than a memset this was a simple store? Is > that the right guiding principle here? We had a -Wunreachable-code that was quite useless because it basically triggered at DCE so was full of useless false positives. It was merely an optimization report. I fear this one will be similar (warning: we applied DSE!). > I hate to say it, but I wonder if this is another case where there > likely won't be a clear consensus and we're going to end up with a two > level warning system? > > For something like Martin's case what I really think we should do is > sink the memset call into the conditional. In cases where "i" is not a > constant, but actually has the value zero at runtime we win. > > -- > > So I've got no objections to the idea of using DSE to detect the dead > store and potentially warn. My concern is are we in a case where that > warning is going to annoy users and we end up needing a level of > -Wunused-but-set. It's not a warning. It's an hint for an optimization the user could apply (sink the thing!) or a report for an optimization we do. Do not go down the route of -Wunreachable-code again please. Richard. > Jeff > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
On Fri, Jun 30, 2017 at 10:25:51AM +0200, Richard Biener wrote:
> Do not go down the route of -Wunreachable-code again please.
Yeah, I don't think we want -Wunused-but-set* as a late warning,
it is intentionally in the FE where is the only place where the false
positive ratio of the warning can stay under control, and even then many
projects like the Linux kernel turn the warning off.
If we want to special case memset/memcpy dest arg for DECL_READ_P, let's do
it, but in the FE only.
Jakub
diff --git a/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c b/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c index 895a50e2677..6cbcc419976 100644 --- a/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c +++ b/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c @@ -1,7 +1,7 @@ /* Test -Wsizeof-pointer-memaccess warnings. */ /* { dg-do compile } */ -/* { dg-options "-Wall -O2 -Wno-sizeof-array-argument -ftrack-macro-expansion=0" } */ -/* { dg-options "-Wall -O2 -Wno-sizeof-array-argument -Wno-c++-compat -ftrack-macro-expansion=0" {target c} } */ +/* { dg-options "-Wall -O2 -Wno-sizeof-array-argument -Wno-unused-but-set-variable -ftrack-macro-expansion=0" } */ +/* { dg-options "-Wall -O2 -Wno-sizeof-array-argument -Wno-c++-compat -Wno-unused-but-set-variable -ftrack-macro-expansion=0" {target c} } */ /* { dg-require-effective-target alloca } */ #define bos(ptr) __builtin_object_size (ptr, 1) diff --git a/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c b/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c index 87f5ef9d171..c42e3270db9 100644 --- a/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c +++ b/gcc/testsuite/gcc.dg/Wstrict-aliasing-bogus-vla-1.c @@ -1,6 +1,6 @@ /* PR 41673: bogus -Wstrict-aliasing warning from VLA dereference. */ /* { dg-do compile } */ -/* { dg-options "-std=gnu99 -O2 -Wall" } */ +/* { dg-options "-std=gnu99 -O2 -Wall -Wno-unused-but-set-variable" } */ /* { dg-require-effective-target alloca } */ int main(int argc, char *argv[]) diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size.c b/gcc/testsuite/gcc.dg/attr-alloc_size.c index f50ba7c53db..8d71b3a4e6d 100644 --- a/gcc/testsuite/gcc.dg/attr-alloc_size.c +++ b/gcc/testsuite/gcc.dg/attr-alloc_size.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */ +/* { dg-options "-O2 -Wall -Wno-unused-but-set-variable -ftrack-macro-expansion=0" } */ extern void abort (void); diff --git a/gcc/testsuite/gcc.dg/pr79715.c b/gcc/testsuite/gcc.dg/pr79715.c index 0f0f90f7122..cd59f6dbf14 100644 --- a/gcc/testsuite/gcc.dg/pr79715.c +++ b/gcc/testsuite/gcc.dg/pr79715.c @@ -1,7 +1,7 @@ /* PR tree-optimization/79715 - hand-rolled strdup with unused result not eliminated { dg-do compile } - { dg-options "-O2 -Wall -fdump-tree-optimized" } */ + { dg-options "-O2 -Wall -Wno-unused-but-set-variable -fdump-tree-optimized" } */ void f (const char *s) { diff --git a/gcc/testsuite/gcc.dg/pr80806.c b/gcc/testsuite/gcc.dg/pr80806.c new file mode 100644 index 00000000000..551555c8fd1 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr80806.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wall" } */ + +void f1(void) +{ + char *p = __builtin_malloc (10); + __builtin_memset (p, 0, 10); /* { dg-warning "'p' set but not used" } */ +} + +void f2(void) +{ + char buf[10]; + __builtin_memset (buf, 0, 10); /* { dg-warning "'buf' set but not used" } */ +} diff --git a/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c b/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c index a73e45fb809..9ca9b287d25 100644 --- a/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c +++ b/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c @@ -1,6 +1,6 @@ /* Test -Wsizeof-pointer-memaccess warnings. */ /* { dg-do compile } */ -/* { dg-options "-Wall -Wno-sizeof-array-argument -Wno-stringop-overflow" } */ +/* { dg-options "-Wall -Wno-sizeof-array-argument -Wno-stringop-overflow -Wno-unused-but-set-variable" } */ /* Test just twice, once with -O0 non-fortified, once with -O2 fortified. */ /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" "-O2" } } */ /* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */ diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c index 90230abe822..f6d583f8034 100644 --- a/gcc/tree-ssa-dse.c +++ b/gcc/tree-ssa-dse.c @@ -35,6 +35,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-cfgcleanup.h" #include "params.h" #include "alias.h" +#include "diagnostic.h" /* This file implements dead store elimination. @@ -742,7 +743,22 @@ dse_dom_walker::dse_optimize_stmt (gimple_stmt_iterator *gsi) } if (store_status == DSE_STORE_DEAD) - delete_dead_call (gsi); + { + tree ptr = gimple_call_arg (stmt, 0); + if (TREE_CODE (ptr) == SSA_NAME + || TREE_CODE (ptr) == ADDR_EXPR) + { + tree base = (TREE_CODE (ptr) == SSA_NAME) + ? SSA_NAME_VAR (ptr) + : TREE_OPERAND (ptr, 0); + + if (base && VAR_P (base)) + warning_at (gimple_location (stmt), + OPT_Wunused_but_set_variable, + "%qD set but not used", base); + } + delete_dead_call (gsi); + } return; }
Hi, The attached patch tries to fix PR80806 by warning when a variable is set using memset (and friends) but not used. I chose to warn in dse pass since dse would detect if the variable passed as 1st argument is a dead store. Does this approach look OK ? There were following fallouts observed during bootstrap build: * double-int.c (div_and_round_double): Warning emitted 'den' set but not used for following call to memset: memset (den, 0, sizeof den); I assume the warning is correct since there's immediately call to: encode (den, lden, hden); and encode overwrites all the contents of den. Should the above call to memset be removed from the source ? * tree-streamer.c (streamer_check_handled_ts_structures) The function defines a local array bool handled_p[LAST_TS_ENUM]; and the warning is emitted for: memset (&handled_p, 0, sizeof (handled_p)); That's because the function then initializes each element of the array handled_p to true making the memset call redundant. I am not sure if warning for the above case is a good idea ? The call to memset() seems deliberate, to initialize all elements to 0, and later assert checks if all the elements were explicitly set to true. * value-prof.c (free_hist): Warns for the call to memset: static int free_hist (void **slot, void *data ATTRIBUTE_UNUSED) { histogram_value hist = *(histogram_value *) slot; free (hist->hvalue.counters); if (flag_checking) memset (hist, 0xab, sizeof (*hist)); free (hist); return 1; } Assuming flag_checking was true, the call to memset would be dead anyway since it would be immediately freed ? Um, I don't understand the purpose of memset in the above function. * testsuite fallout I verified regressing test-cases were not false positives and added -Wno-unused-but-set-variable. Thanks, Prathamesh