PR80806

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

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 ?
[ ... ]
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 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
> 
>

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