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
