Re: PR80806

[Jeff Law <law@redhat.com>]

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