Re: PR80806

[Jeff Law <law@redhat.com>]

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