Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law <law@redhat.com>]

On 08/22/2017 02:45 AM, Richard Biener wrote:
> On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor <msebor@gmail.com> wrote:
>> On 08/09/2017 10:14 AM, Jeff Law wrote:
>>>
>>> On 08/06/2017 05:08 PM, Martin Sebor wrote:
>>>
>>>>>
>>>>> Well, simply because the way as implemented isn't a must-alias query
>>>>> but maybe one that's good enough for warnings (reduces false positives
>>>>> but surely doesn't eliminate them).
>>>>
>>>>
>>>> I'm very interested in reducing the rate of false positives in
>>>> these and all other warnings.  As I mentioned in my comments,
>>>> I did my best to weed them out of the implementation by building
>>>> GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
>>>> a guarantee that there aren't any.  But the first implementation
>>>> of any non-trivial feature is never perfect, and hardly any
>>>> warning of sufficient complexity is free of false positives, no
>>>> matter here it's implemented (the middle-end, front-end, or
>>>> a standalone static analysis tool).  If you spotted some cases
>>>> I had missed I'd certainly be grateful for examples.  Otherwise,
>>>> they will undoubtedly be reported as more software is exposed
>>>> to the warning and, if possible, fixed, as happens with all
>>>> other warnings.
>>>
>>> I think Richi is saying that the must alias query you've built isn't
>>> proper/correct.  It's less about false positives for Richi and more
>>> about building a proper must alias query if I understand him correctly.
>>>
>>> I suspect he's also saying that you can't reasonably build must-alias on
>>> top of a may-alias query framework.  They're pretty different queries.
>>>
>>> If you need something that is close to, but not quite a must alias
>>> query, then you're going to have to make a argument for that and you
>>> can't call it a must alias query.
>>
>>
>> Attached is an updated and simplified patch that avoids making
>> changes to any of the may-alias functions.  It turns out that
>> all the information the logic needs to determine the overlap
>> is already in the ao_ref structures populated by
>> ao_ref_init_from_ptr_and_size.  The only changes to the pass
>> are the enhancement to ao_ref_init_from_ptr_and_size to make
>> use of range info and the addition of the two new functions
>> used by the -Wrestrict clients outside the pass.
> 
> Warning for memcpy (p, p, ...) is going to fire false positives all around
> given the C++ FE emits those in some cases and optimization can
> expose that we are dealing with self-assignments.  And *p = *p is
> valid.
Correct.  I wound my way through this mess a while back.  Essentially
Red Hat had a customer with code that had overlapped memcpy arguments.
We had them use the memstomp interposition library to start tracking
these problems down.

One of the things that popped up was structure/class copies which were
implemented via calls to memcpy.    In the case of self assignment, the
interposition library would note the overlap and (rightly IMHO) complain.


One could argue that GCC should emit memmove by default for structure
assignments, only using memcpy when it knows its not doing self
assignment (which may be hard to determine).  Furthermore, GCC should
eliminate self structure/class assignment.


> 
> @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
>             }
>         }
> 
> +      /* Avoid folding the call if overlap is detected.  */
> +      if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
> +       return false;
> +
> 
> no, please not.  You diagnosed the call (which might be a false positive)
> so why keep it undefined?  The folded stmt will either have the same
> semantics (aggregate copies expanded as memcpy) or have all reads
> performed before writes.
So can we distinguish here between overlap and the self-copy case?

A self-copy should just be folded away.  It's no different than x = x on
scalars except that we've dropped it to a memcpy in the IL.  Doing so
makes the code more efficient and removes false positives from tools
like the memstomp interposition library, making those tools more useful.




Jeff