On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor <mse...@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.
@@ -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.
The ao_ref_init_from_ptr_and_size change misses a changelog entry.
+detect_overlap (location_t loc, gimple *stmt, tree dst, tree src, tree size,
+ bool adjust /* = false */)
+{
+ ao_ref dstref, srcref;
+ unsigned HOST_WIDE_INT range[2];
+
+ /* Initialize and store the lower bound of a constant offset (in
+ bits), disregarding the offset for the destination. */
+ ao_ref_init_from_ptr_and_size (&dstref, dst, size, range);
+ ao_ref_init_from_ptr_and_size (&srcref, src, size, range);
just pass NULL range to the first call?
- ref->ref = NULL_TREE;
+
+ if (offrng)
+ offrng[0] = offrng[1] = 0;
+
+ ref->ref = NULL_TREE;
bogus indent
+ else if (offrng && TREE_CODE (offset) == SSA_NAME)
+ {
+ wide_int min, max;
+ value_range_type rng = get_range_info (offset, &min, &max);
+ if (rng == VR_RANGE && wi::fits_uhwi_p (min))
+ {
+ ptr = gimple_assign_rhs1 (stmt);
+ offrng[0] = BITS_PER_UNIT * min.to_uhwi ();
+ offrng[1] = BITS_PER_UNIT * max.to_uhwi ();
+
+ extra_offset = offrng[0];
you didnt' check whether max fits uhwi. The effect of passing offrng
is not documented.
+ else
+ /* Offset range is indeterminate. */
+ offrng[0] = offrng[1] = HOST_WIDE_INT_M1U;
I believe a cleaner interface would be to do
void
ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size, tree
*var_byte_offset)
and set *var_byte_offset to your 'offset' above, leaving 'ref'
unchanged. The caller
can then get at the range info of var_byte_offset and adjust
ref->offset if desired.
The indeterminate state is then much cleaner - NULL_TREE.
+unsigned HOST_WIDE_INT
+refs_overlap (ao_ref *ref1, ao_ref *ref2, unsigned HOST_WIDE_INT *aloff)
+{
bool
refs_must_overlap_p (ao_ref *, ao_ref *, unsigned HOST_WIDE_INT *off,
unsinged HOST_WIDE_INT *size)
your return values are in bytes thus
+
+ // Compare pointers.
+ offset1 += mem_ref_offset (base1) << LOG2_BITS_PER_UNIT;
+ offset2 += mem_ref_offset (base2) << LOG2_BITS_PER_UNIT;
+ base1 = TREE_OPERAND (base1, 0);
just do the intermediate computations in bytes as well.
It looks like it is unspecified relative to which ref the offset is given,
how's that useful information then -- the diagnostic doesn't seem
too helpful? Why not keep it relative to the first ref and thus make
aloff signed?
detect_overlap doesn't belong to tree-ssa-alias.[ch].
(didn't look at the diagnostic parts)
Thanks,
Richard.
> Martin
