Hi Martin,
> On 07/19/2018 03:51 PM, Jeff Law wrote:
>> On 07/13/2018 05:45 PM, Martin Sebor wrote:
>>>>
>>>> + offset_int ofr[] = {
>>>> + wi::to_offset (fold_convert (ptrdiff_type_node, vr->min)),
>>>> + wi::to_offset (fold_convert (ptrdiff_type_node, vr->max))
>>>> + };
>>>>
>>>> huh. Do you maybe want to use widest_int for ofr[]? What's
>>>> wrong with wi::to_offset (vr->min)? Building another intermediate
>>>> tree node here looks just bogus.
>>>
>>> I need to convert size_type indices to signed to keep their sign
>>> if it's negative and include it in warnings. I've moved the code
>>> into a conditional where it's used to minimize the cost but other
>>> than that I don't know how else to convert it.
>>>
>>>>
>>>> What are you trying to do in this loop anyways?
>>>
>>> The loop It determines the range of the final index/offset for
>>> a series of POINTER_PLUS_EXPRs. It handles cases like this:
>>>
>>> int g (int i, int j, int k)
>>> {
>>> if (i < 1) i = 1;
>>> if (j < 1) j = 1;
>>> if (k < 1) k = 1;
>>>
>>> const char *p0 = "123";
>>> const char *p1 = p0 + i;
>>> const char *p2 = p1 + j;
>>> const char *p3 = p2 + k;
>>>
>>> // p2[3] and p3[1] are out of bounds
>>> return p0[4] + p1[3] + p2[2] + p3[1];
>>> }
>>>
>>>> I suppose
>>>> look at
>>>>
>>>> p_1 = &obj[i_2]; // already bounds-checked, but with ignore_off_by_one
>>>> ... = MEM[p_1 + CST];
>>>>
>>>> ? But then
>>>>
>>>> + if (TREE_CODE (varoff) != SSA_NAME)
>>>> + break;
>>>>
>>>> you should at least handle INTEGER_CSTs here?
>>>
>>> It's already handled (and set in CSTOFF). There should be no
>>> more constant offsets after that (I haven't come across any.)
>>>
>>>>
>>>> + if (!vr || vr->type == VR_UNDEFINED || !vr->min || !vr->max)
>>>> + break;
>>>>
>>>> please use positive tests here, VR_RANGE || VR_ANTI_RANGE. As usual
>>>> the anti-range handling looks odd. Please add comments so we can follow
>>>> what you were thinking when writing range merging code. Even better if you
>>>>
>>>> can stick to use existing code rather than always re-inventing the wheel...
>>>>
>>>
>>> The anti-range handling is to conservatively add
>>> [-MAXOBJSIZE -1, MAXOBJSIZE] to OFFRANGE. I've added comments
>>> to make it clear. I'd be more than happy to reuse existing
>>> code if I knew where to find it (if it exists). It sure would
>>> save me lots of work to have a library of utility functions to
>>> call instead of rolling my own code each time.
>> Finding stuff is never easy :( GCC's just gotten so big it's virtually
>> impossible for anyone to know all the APIs.
>>
>> The suggestion I'd have would be to (when possible) factor this stuff
>> into routines you can reuse. We (as a whole) have this tendency to
>> open-code all kinds of things rather than factoring the code into
>> reusable routines.
>>
>> In addition to increasing the probability that you'll be able to reuse
>> code later, just reading a new function's header tends to make me (as a
>> reviewer) internally ask if there's already a routine we should be using
>> instead. When it's open-coded it's significantly harder to spot those
>> cases (at least for me).
>>
>>
>>>
>>>>
>>>> I think I commented on earlier variants but this doesn't seem to resemble
>>>> any of them.
>>>
>>> I've reworked the patch (sorry) to also handle arrays. For GCC
>>> 9 it seems I might as well do both in one go.
>>>
>>> Attached is an updated patch with these changes.
>>>
>>> Martin
>>>
>>> gcc-83776.diff
>>>
>>>
>>> PR tree-optimization/84047 - missing -Warray-bounds on an out-of-bounds
>>> index into an array
>>> PR tree-optimization/83776 - missing -Warray-bounds indexing past the
>>> end of a string literal
>>>
>>> gcc/ChangeLog:
>>>
>>> PR tree-optimization/84047
>>> PR tree-optimization/83776
>>> * tree-vrp.c (vrp_prop::check_mem_ref): New function.
>>> (check_array_bounds): Call it.
>>> * /gcc/tree-sra.c (get_access_for_expr): Fail for out-of-bounds
>>> array offsets.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> PR tree-optimization/83776
>>> PR tree-optimization/84047
>>> * gcc.dg/Warray-bounds-29.c: New test.
>>> * gcc.dg/Warray-bounds-30.c: New test.
>>> * gcc.dg/Warray-bounds-31.c: New test.
>>> * gcc.dg/Warray-bounds-32.c: New test.
>>>
>>
>>> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>>> index 3e30f6b..8221a06 100644
>>> --- a/gcc/tree-sra.c
>>> +++ b/gcc/tree-sra.c
>>> @@ -3110,6 +3110,19 @@ get_access_for_expr (tree expr)
>>> || !DECL_P (base))
>>> return NULL;
>>>
>>> + /* Fail for out-of-bounds array offsets. */
>>> + tree basetype = TREE_TYPE (base);
>>> + if (TREE_CODE (basetype) == ARRAY_TYPE)
>>> + {
>>> + if (offset < 0)
>>> + return NULL;
>>> +
>>> + if (tree size = DECL_SIZE (base))
>>> + if (tree_fits_uhwi_p (size)
>>> + && tree_to_uhwi (size) < (unsigned HOST_WIDE_INT) offset)
>>> + return NULL;
>>> + }
>>> +
>>> if (!bitmap_bit_p (candidate_bitmap, DECL_UID (base)))
>>> return NULL;
>> So I'm a bit curious about this hunk. Did we end up creating an access
>> structure that walked off the end of the array? Presumably you
>> suppressing SRA at this point so that you can see the array access later
>> and give a suitable warning. Right?
>
> Yes, but I didn't make a note of the test case that triggered
> it and I'm not able to trigger the code path now, so the change
> might not be necessary. I've removed it from the patch. If it
> turns out that it is needed after all I'll commit it later.
>
>>> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>>> index a7453ab..27021760 100644
>>> --- a/gcc/tree-vrp.c
>>> +++ b/gcc/tree-vrp.c
>>> @@ -4930,21 +4931,280 @@ vrp_prop::check_array_ref (location_t location,
>>> tree ref,
>>> }
>>> }
>>>
>>> +/* Checks one MEM_REF in REF, located at LOCATION, for out-of-bounds
>>> + references to string constants. If VRP can determine that the array
>>> + subscript is a constant, check if it is outside valid range.
>>> + If the array subscript is a RANGE, warn if it is non-overlapping
>>> + with valid range.
>>> + IGNORE_OFF_BY_ONE is true if the MEM_REF is inside an ADDR_EXPR
>>> + (used to allow one-past-the-end indices for code that takes
>>> + the address of the just-past-the-end element of an array). */
>>> +
>>> +void
>>> +vrp_prop::check_mem_ref (location_t location, tree ref, bool
>>> ignore_off_by_one)
>>> +{
>>> + if (TREE_NO_WARNING (ref))
>>> + return;
>>> +
>>> + tree arg = TREE_OPERAND (ref, 0);
>>> + /* The constant and variable offset of the reference. */
>>> + tree cstoff = TREE_OPERAND (ref, 1);
>>> + tree varoff = NULL_TREE;
>>> +
>>> + const offset_int maxobjsize = tree_to_shwi (max_object_size ());
>>> +
>>> + /* The array or string constant bounds in bytes. Initially set
>>> + to [-MAXOBJSIZE - 1, MAXOBJSIZE] until a tighter bound is
>>> + determined. */
>>> + offset_int arrbounds[2];
>>> + arrbounds[1] = maxobjsize;
>>> + arrbounds[0] = -arrbounds[1] - 1;
>> I realize that arrbounds[1] == maxobjsize at this point. But is there
>> any reason not to use maxobjsize in the assignment to arrbounds[0] so
>> that its computation matches the comment. It would also seem advisable
>> to set the min first, then the max since that's the ordering in the
>> comment and in the array.
>
> Done.
>
>>
>>
>>> +
>>> + /* The minimum and maximum intermediate offset. For a reference
>>> + to be valid, not only does the final offset/subscript must be
>>> + in bounds but all intermediate offsets must be as well. */
>>> + offset_int ioff = wi::to_offset (fold_convert (ptrdiff_type_node,
>>> cstoff));
>>> + offset_int extrema[2] = { 0, wi::abs (ioff) };
>> You should probably tone back the comment here since the intermediate
>> offsets do not have to be in bounds.
>
> Done.
>
>>
>>
>> [ Big snip ]
>>
>>> + /* The type of the object being referred to. It can be an array,
>>> + string literal, or a non-array type when the MEM_REF represents
>>> + a reference/subscript via a pointer to an object that is not
>>> + an element of an array. References to members of structs and
>>> + unions are excluded because MEM_REF doesn't make it possible
>>> + to identify the member where the reference orginated. */
>> s/orginated/originated/
>>
>> OK with those changes.
>
> Committed without the SRA change as 262893.
your patch has caused quite a number of testsuite failures:
+FAIL: c-c++-common/Warray-bounds-2.c -std=gnu++11 (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c -std=gnu++14 (test for excess errors)
+FAIL: c-c++-common/Warray-bounds-2.c -std=gnu++98 (test for excess errors)
Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/Warray-bounds-2.c:200:11:
warning: array subscript -1 is outside array bounds of 'Array [2]'
[-Warray-bounds]
+FAIL: c-c++-common/Warray-bounds-2.c -Wc++-compat (test for excess errors)
on 32 and 64-bit Solaris/SPARC and x86, also Linux/x86_64
+XPASS: gcc.dg/Warray-bounds-31.c (test for warnings, line 26)
+XPASS: gcc.dg/Warray-bounds-31.c (test for warnings, line 40)
+FAIL: gcc.dg/Warray-bounds-31.c (test for excess errors)
+XPASS: gcc.dg/Warray-bounds-31.c bug 84047 (test for warnings, line 72)
+XPASS: gcc.dg/Warray-bounds-31.c bug 84047 (test for warnings, line 90)
Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/Warray-bounds-31.c:100:3:
warning: array subscript -2147483648 is outside array bounds of 'char[9]'
[-Warray-bounds]
32-bit Solaris and Linux only.
+FAIL: gcc.dg/Warray-bounds-32.c (test for warnings, line 28)
Please fix.
Rainer
--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
