This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)
On Tue, Aug 29, 2017 at 2:20 AM, Martin Sebor <msebor@gmail.com> wrote:
> 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.
>
>
> I changed it to only warn for calls to the library function and
> not to the __builtin_xxx. Though I haven't been able to reproduce
> the problem you referring to (bug 32667) which makes me wonder if
> it's been fixed.
@@ -699,6 +706,15 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
DEST{,+LEN,+LEN-1}. */
if (operand_equal_p (src, dest, 0))
{
+ /* Avoid diagnosing exact overlap in calls to __builtin_memcpy.
+ It's safe and may even be emitted by GCC itself (see bug
+ 32667). However, diagnose it in explicit calls to the memcpy
+ function. */
+ if (check_overlap && *IDENTIFIER_POINTER (DECL_NAME (func)) != '_')
+ warning_at (loc, OPT_Wrestrict,
You can have explicit calls to __builtin_memcpy so that check looks bogus to me.
I think there's no way to distinguish the cases and I'd simply remove the check
and set TREE_NO_WARNING on the implicit calls generated by frontends.
>>
>> @@ -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.
>
>
> My goal was to follow the approach reflected in the comments
> elsewhere in the file:
>
> /* Out of bound array access. Value is undefined,
> but don't fold. */
This is done to keep -Warray-bounds triggering IIRC. Also there's no
good value to fold to (well, we used zero). I also vetoed emitting
warnings from folding code given that's inherently fragile ... (I think
I already said I don't like warning from gimple_fold_builtin_memory_op
too much either).
> While gimple_fold_builtin_memory_op may be able to provide well-
> defined behavior for the otherwise undefined semantics in a small
> subset of cases, it doesn't attempt to fold many more that it
> otherwise could (it only folds calls with sizes that are powers
> of 2). So it seems of dubious value to make an effort in this
> relatively small subset of cases.
>
> In my experience, users also don't appreciate optimizations that
> "rely on" undefined behavior one way or the other. What they would
> like to see instead is that when their compiler detects undefined
> behavior it diagnoses it but either doesn't use it to make
> optimization decisions, or uses it to disable them. For calls to
> library functions, that in my view means making the call and not
> folding it. (Btw., do we have some sort of a policy or guideline
> for how to handle such cases in general?)
>
> With all that said, I don't see a big problem with proceeding with
> the folding as you suggest either, so I did and added a comment
> documenting it and a test to verify this guarantee.
>
> I should also acknowledge that in my approach I forgot that once
> the overlap has been diagnosed and the no-warning bit set, the
> next call to gimple_fold_builtin_memory_op() with the same
> statement would just go ahead and fold it anyway. So the tests
> were ineffective regardless.
Yeah.
@@ -748,6 +764,27 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
unsigned ilen = tree_to_uhwi (len);
if (pow2p_hwi (ilen))
{
+ if (check_overlap)
+ {
+ /* Detect overlapping copies and issue -Wrestrict. */
+ if (detect_overlap (loc, stmt, dest, src, len, endp == 2))
+ gimple_set_no_warning (stmt, true);
+ else if (TREE_CODE (dest) != SSA_NAME
+ || TREE_CODE (src) != SSA_NAME)
+ {
+ /* If no overlap is detected and at least one of
+ the arguments is not in an SSA form defer folding
+ until they both are so that aliasing can be
+ determined with greater accuracy. */
+ ao_ref dstref;
+ ao_ref srcref;
+ ao_ref_init_from_ptr_and_size (&dstref, dest, len);
+ ao_ref_init_from_ptr_and_size (&srcref, src, len);
+ if (refs_may_alias_p_1 (&dstref, &srcref, true))
+ return false;
please not -- folding these early is very important for inlining decisions
and early simplifications. IMHO diagnostics should _never_ disable
optimizations. Even more so code generation should not change
with -W options (similar to -g).
if (operand_equal_p (src, dest, 0))
{
+ tree func = gimple_call_fndecl (stmt);
+
+ warning_at (loc, OPT_Wrestrict,
+ "%qD source argument is the same as destination",
+ func);
+
syle issue - you use func once so please remove it and put
the gimple_call_fndecl call in the warning_at argument. No reason
for so much extra vertical space either.
@@ -7226,3 +7288,4 @@ gimple_stmt_integer_valued_real_p (gimple *stmt,
int depth)
return false;
}
}
+
spurious blank line added.
+ tree offset = gimple_assign_rhs2 (stmt);
+ if (TREE_CODE (offset) == INTEGER_CST)
+ {
+ ptr = gimple_assign_rhs1 (stmt);
+ extra_offset = BITS_PER_UNIT * int_cst_value (offset);
+
+ if (TREE_CODE (ptr) == SSA_NAME)
IMHO spurious vertical space.
>> 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?
>
>
> The argument needs to be non-null for the base to be determined
> the same way between the two references. (It's not obvious so
> I've added a comment to the description of the function.)
>
>>
>> - 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.
>
>
> Done.
+ 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 ();
why store those as bits? The fits_uhwi_p check is bogus that way
given the multiplication may still overflow.
+ if (wi::fits_uhwi_p (max))
+ offrng[1] = BITS_PER_UNIT * max.to_uhwi ();
+ else
+ offrng[1] = HOST_WIDE_INT_M1U;
+
+ extra_offset = offrng[0];
+ }
+ else
+ /* Offset range is indeterminate. */
+ offrng[0] = offrng[1] = HOST_WIDE_INT_M1U;
note the ao_ref in principle already has enough info to capture such
variable range
via offset, size and max_size. Indeterminate in that case would be
offset = 0 and max_size = -1. Given you adjust size by offrng[0] a good value
for "indeterminate" is zero, no? You initialize it to that at the start of the
function anyway. And you do the adjustment unconditionally in detect_overlap.
So what I'm saying is that, why not just add a bool to
ao_ref_init_from_ptr_and_size
to say "use ranges" and then appropriately adjust offset and max_size instead of
adding the offrange[] output parameter? Eventually it's even worth doing this
unconditionally.
_note_ that offsets of POINTER_PLUS_EXPR can be negative, sth that
the unsignedness of offrng doesn't seem to consider? That is,
the offset argument of POINTER_PLUS_EXPR has to be interpreted
as a signed entity (yeah, stupid IL enforcement of _un_signed sizetype
here...) which means literally using get_range_info on the offset argument
is broken. You have to convert the range/anti-range to the appropriate
signed variant.
>>
>> + 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.
>
>
> It would make the interface cleaner and the function simpler at
> the expense of all the callers having to extract the range and
> deal with the additional complexity. There are six call sites
> so that would not be an overall improvement. I'd need to
> introduce a wrapper function and do the work there. I don't
> see the advantage of that approach but I also don't have
> a problem with introducing the overload if you consider
> the change above necessary.
Please investigate the possibility to go away with offrange[] and instead
use a flag.
>> +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)
>
>
> I avoided the words "must" and "alias" on account of your concern
> that the function could be misunderstood to be intended to gate
> optimizations. It also wasn't meant to be used as a predicate
> (even though it could be) but I have changed its signature as you
> suggest.
Hmm, ok. So the function should be called refs_known_overlap_p
then to document that when it returns false we don't know whether
the refs overlap. If it returns true the computed overlap should be
correct so it can be used for optimization purposes (otherwise
it doesn't belong in tree-ssa-alias.c).
I think the function isn't conservative given it uses ->size rather
than ->max_size or doesn't bail out if size == -1 || size != max_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.
>
>
> That is (slightly) simpler, thank you.
>
>>
>> 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?
>
>
> If I understand your suggestion, an offset relative to the first
> reference (which I think is the destination pointer at all call
> sites) would be zero whenever the destination pointer were greater
> than the source, and positive(?) otherwise. E.g., like so:
No, it would be negative.
>
> memcpy (d, d + 1, 2); // offset is +1
>
> memcpy (d + 1, d, 2); // offset is 0.
offset is -1 (and aloff should be HOST_WIDE_INT *)
> The second case doesn't seem helpful.
>
> As it is, the offset is the absolute value of the distance between
> the source and the destination pointers:
>
> memcpy (d, d + 1, 2); // offset is 1
>
> memcpy (d + 1, d, 2); // offset is also 1
>
> This seems more meaningful to me but I welcome suggestions for
> improvements.
>
>> detect_overlap doesn't belong to tree-ssa-alias.[ch].
>
>
> I moved it to builtins.c. If you find that unsuitable there as
> well then please suggest an appropriate home for the function.
builtins.c works for me.
> Attached is an updated patch retested on x86_64-linux.
>
> Martin