Re: PR80806

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

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
>