PR80806

[Martin Sebor]

I attach an updated patch that actually bootstraps and (with
one minor exception) passes regression tests.  It's a C front
end-only proof of concept for now.  The complete patch will
include attributes read-only and read-write and support C++
of course.  I think the optimization bits can be done in
a subsequent pass.

Martin

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:
>
>   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.
>
> 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.  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.)
>
> 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.)
>
>>
>> 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 ?
>
> IIUC, this seems to be a more involved example of the simple one
> above.  AFAICS, the buffer is subsequently read in the function,
> but only conditionally.
>
> That said, since encode() overwrites the whole buffer right after
> it has been cleared by memset, I would think that a warning pointing
> that out could be helpful (although I'm not sure -Wunused is the
> right warning to issue).
>
>>
>> * 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.
>
> Right.  Warning on this function doesn't seem right since
> the variable is subsequently used in the test loop.
>
>>
>> * 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.
>
> I'm guessing it's an effort to invalidate the memory to detect
> subsequent accesses to its contents.  Warning on such code would
> be valuable because it's a common misconception that a memset
> can be used to wipe sensitive data (e.g., passwords) from memory
> before freeing it, to prevent them from leaking into compromised
> stack frames (or core files).  But it should be a warning that's
> distinct from -Wunused.
>
> Martin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-80806.diff
Type: text/x-patch
Size: 44158 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20170524/74c91693/attachment.bin>