[PING][PATCH] correct handling of indices into arrays with elements larger than 1 (PR c++/96511)

Martin Sebor msebor@gmail.com
Tue Oct 13 16:59:17 GMT 2020


On 10/13/20 3:46 AM, Christophe Lyon wrote:
> On Tue, 29 Sep 2020 at 00:02, Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 9/25/20 11:17 PM, Jason Merrill wrote:
>>> On 9/22/20 4:05 PM, Martin Sebor wrote:
>>>> The rebased and retested patches are attached.
>>>>
>>>> On 9/21/20 3:17 PM, Martin Sebor wrote:
>>>>> Ping:
>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553906.html
>>>>>
>>>>> (I'm working on rebasing the patch on top of the latest trunk which
>>>>> has changed some of the same code but it'd be helpful to get a go-
>>>>> ahead on substance the changes.  I don't expect the rebase to
>>>>> require any substantive modifications.)
>>>>>
>>>>> Martin
>>>>>
>>>>> On 9/14/20 4:01 PM, Martin Sebor wrote:
>>>>>> On 9/4/20 11:14 AM, Jason Merrill wrote:
>>>>>>> On 9/3/20 2:44 PM, Martin Sebor wrote:
>>>>>>>> On 9/1/20 1:22 PM, Jason Merrill wrote:
>>>>>>>>> On 8/11/20 12:19 PM, Martin Sebor via Gcc-patches wrote:
>>>>>>>>>> -Wplacement-new handles array indices and pointer offsets the same:
>>>>>>>>>> by adjusting them by the size of the element.  That's correct for
>>>>>>>>>> the latter but wrong for the former, causing false positives when
>>>>>>>>>> the element size is greater than one.
>>>>>>>>>>
>>>>>>>>>> In addition, the warning doesn't even attempt to handle arrays of
>>>>>>>>>> arrays.  I'm not sure if I forgot or if I simply didn't think of
>>>>>>>>>> it.
>>>>>>>>>>
>>>>>>>>>> The attached patch corrects these oversights by replacing most
>>>>>>>>>> of the -Wplacement-new code with a call to compute_objsize which
>>>>>>>>>> handles all this correctly (plus more), and is also better tested.
>>>>>>>>>> But even compute_objsize has bugs: it trips up while converting
>>>>>>>>>> wide_int to offset_int for some pointer offset ranges.  Since
>>>>>>>>>> handling the C++ IL required changes in this area the patch also
>>>>>>>>>> fixes that.
>>>>>>>>>>
>>>>>>>>>> For review purposes, the patch affects just the middle end.
>>>>>>>>>> The C++ diff pretty much just removes code from the front end.
>>>>>>>>>
>>>>>>>>> The C++ changes are OK.
>>>>>>>>
>>>>>>>> Thank you for looking at the rest as well.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -compute_objsize (tree ptr, int ostype, access_ref *pref,
>>>>>>>>>> -                bitmap *visited, const vr_values *rvals /* =
>>>>>>>>>> NULL */)
>>>>>>>>>> +compute_objsize (tree ptr, int ostype, access_ref *pref, bitmap
>>>>>>>>>> *visited,
>>>>>>>>>> +                const vr_values *rvals)
>>>>>>>>>
>>>>>>>>> This reformatting seems unnecessary, and I prefer to keep the
>>>>>>>>> comment about the default argument.
>>>>>>>>
>>>>>>>> This overload doesn't take a default argument.  (There was a stray
>>>>>>>> declaration of a similar function at the top of the file that had
>>>>>>>> one.  I've removed it.)
>>>>>>>
>>>>>>> Ah, true.
>>>>>>>
>>>>>>>>>> -      if (!size || TREE_CODE (size) != INTEGER_CST)
>>>>>>>>>> -       return false;
>>>>>>>>>   >...
>>>>>>>>>
>>>>>>>>> You change some failure cases in compute_objsize to return
>>>>>>>>> success with a maximum range, while others continue to return
>>>>>>>>> failure. This needs commentary about the design rationale.
>>>>>>>>
>>>>>>>> This is too much for a comment in the code but the background is
>>>>>>>> this: compute_objsize initially returned the object size as a
>>>>>>>> constant.
>>>>>>>> Recently, I have enhanced it to return a range to improve warnings
>>>>>>>> for
>>>>>>>> allocated objects.  With that, a failure can be turned into
>>>>>>>> success by
>>>>>>>> having the function set the range to that of the largest object.
>>>>>>>> That
>>>>>>>> should simplify the function's callers and could even improve
>>>>>>>> the detection of some invalid accesses.  Once this change is made
>>>>>>>> it might even be possible to change its return type to void.
>>>>>>>>
>>>>>>>> The change that caught your eye is necessary to make the function
>>>>>>>> a drop-in replacement for the C++ front end code which makes this
>>>>>>>> same assumption.  Without it, a number of test cases that exercise
>>>>>>>> VLAs fail in g++.dg/warn/Wplacement-new-size-5.C.  For example:
>>>>>>>>
>>>>>>>>     void f (int n)
>>>>>>>>     {
>>>>>>>>       char a[n];
>>>>>>>>       new (a - 1) int ();
>>>>>>>>     }
>>>>>>>>
>>>>>>>> Changing any of the other places isn't necessary for existing tests
>>>>>>>> to pass (and I didn't want to introduce too much churn).  But I do
>>>>>>>> want to change the rest of the function along the same lines at some
>>>>>>>> point.
>>>>>>>
>>>>>>> Please do change the other places to be consistent; better to have
>>>>>>> more churn than to leave the function half-updated.  That can be a
>>>>>>> separate patch if you prefer, but let's do it now rather than later.
>>>>>>
>>>>>> I've made most of these changes in the other patch (also attached).
>>>>>> I'm quite happy with the result but it turned out to be a lot more
>>>>>> work than either of us expected, mostly due to the amount of testing.
>>>>>>
>>>>>> I've left a couple of failing cases in place mainly as reminders
>>>>>> to handle them better (which means I also didn't change the caller
>>>>>> to avoid testing for failures).  I've also added TODO notes with
>>>>>> reminders to handle some of the new codes more completely.
>>>>>>
>>>>>>>
>>>>>>>>>> +  special_array_member sam{ };
>>>>>>>>>
>>>>>>>>> sam is always set by component_ref_size, so I don't think it's
>>>>>>>>> necessary to initialize it at the declaration.
>>>>>>>>
>>>>>>>> I find initializing pass-by-pointer local variables helpful but
>>>>>>>> I don't insist on it.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> @@ -187,7 +187,7 @@ decl_init_size (tree decl, bool min)
>>>>>>>>>>     tree last_type = TREE_TYPE (last);
>>>>>>>>>>     if (TREE_CODE (last_type) != ARRAY_TYPE
>>>>>>>>>>         || TYPE_SIZE (last_type))
>>>>>>>>>> -    return size;
>>>>>>>>>> +    return size ? size : TYPE_SIZE_UNIT (type);
>>>>>>>>>
>>>>>>>>> This change seems to violate the comment for the function.
>>>>>>>>
>>>>>>>> By my reading (and writing) the change is covered by the first
>>>>>>>> sentence:
>>>>>>>>
>>>>>>>>      Returns the size of the object designated by DECL considering
>>>>>>>>      its initializer if it either has one or if it would not affect
>>>>>>>>      its size, ...
>>>>>>>
>>>>>>> OK, I see it now.
>>>>>>>
>>>>>>>> It handles a number of cases in Wplacement-new-size.C fail that
>>>>>>>> construct a larger object in an extern declaration of a template,
>>>>>>>> like this:
>>>>>>>>
>>>>>>>>     template <class> struct S { char c; };
>>>>>>>>     extern S<int> s;
>>>>>>>>
>>>>>>>>     void f ()
>>>>>>>>     {
>>>>>>>>       new (&s) int ();
>>>>>>>>     }
>>>>>>>>
>>>>>>>> I don't know why DECL_SIZE isn't set here (I don't think it can
>>>>>>>> be anything but equal to TYPE_SIZE, can it?) and other than struct
>>>>>>>> objects with a flexible array member where this identity doesn't
>>>>>>>> hold I can't think of others.  Am I missing something?
>>>>>>>
>>>>>>> Good question.  The attached patch should fix that, so you
>>>>>>> shouldn't need the change to decl_init_size:
>>>>>>
>>>>>> I've integrated it into the bug fix.
>>>>>>
>>>>>> Besides the usual x86_64-linux bootstrap/regtest I tested both
>>>>>> patches by building a few packages, including Binutils/GDB, Glibc,
>>>>>> and  verifying no new warnings show up.
>>>>>>
>>>>>> Martin
>>>
>>>> +offset_int
>>>> +access_ref::size_remaining (offset_int *pmin /* = NULL */) const
>>>
>>> For the various member functions, please include the comments with the
>>> definition as well as the in-class declaration.
>>
>> Only one access_ref member function is defined out-of-line:
>> offset_bounded().  I've adjusted the comment and copied it above
>> the function definition.
>>
>>>
>>>> +      if (offrng[1] < offrng[0])
>>>
>>> What does it mean for the max offset to be less than the min offset?  I
>>> wouldn't expect that to ever happen with wide integers.
>>
>> The offset is represented in sizetype with negative values represented
>> as large positive values, but has to be converted to ptrdiff_t.  These
>> cases come up when the unsigned offset is an ordinary range that
>> corresponds to an anti-range, such as here:
>>
>>     extern char a[2];
>>
>>     void f (unsigned long i)
>>     {
>>       if (i == 0)
>>         return;
>>       a[i] = 0;   // i's range is [1, -1] (i.e., [1, SIZE_MAX]
>>     }
>>
>>>
>>>> +  /* Return true if OFFRNG is bounded to a subrange of possible offset
>>>> +     values.  */
>>>> +  bool offset_bounded () const;
>>>
>>> I don't understand how you're using this.  The implementation checks for
>>> the possible offset values falling outside those representable by
>>> ptrdiff_t, unless the range is only a single value.  And then the only
>>> use is
>>>
>>>> +  if (ref.offset_zero () || !ref.offset_bounded ())
>>>> +    inform (DECL_SOURCE_LOCATION (ref.ref),
>>>> +        "%qD declared here", ref.ref);
>>>> +  else if (ref.offrng[0] == ref.offrng[1])
>>>> +    inform (DECL_SOURCE_LOCATION (ref.ref),
>>>> +        "at offset %wi from %qD declared here",
>>>> +        ref.offrng[0].to_shwi (), ref.ref);
>>>> +  else
>>>> +    inform (DECL_SOURCE_LOCATION (ref.ref),
>>>> +        "at offset [%wi, %wi] from %qD declared here",
>>>> +        ref.offrng[0].to_shwi (), ref.offrng[1].to_shwi (), ref.ref);
>>>
>>> So if the possible offsets are all representable by ptrdiff_t, we don't
>>> print the range?  The middle case also looks unreachable, since
>>> offset_bounded will return false in that case.
>>
>> The function was originally named "offset_unbounded."  I changed
>> it to "offset_bounded" but looks like I didn't finish the job or
>> add any tests for it.
>>
>> The goal of conditionals is to avoid overwhelming the user with
>> excessive numbers that may not be meaningful or even relevant
>> to the warning.  I've corrected the function body, tweaked and
>> renamed the get_range function to get_offset_range to do a better
>> job of extracting ranges from the types of some nonconstant
>> expressions the front end passes it, and added a new test for
>> all this.  Attached is the new revision.
>>
>> Martin
> 
> Hi Martin,
> 
> One of t new tests fails on many arm configurations and on aarch64
> FAIL: gcc.dg/Wstringop-overflow-47.c  (test for warnings, line 29)
> FAIL: gcc.dg/Wstringop-overflow-47.c  (test for warnings, line 32)
> FAIL: gcc.dg/Wstringop-overflow-47.c  (test for warnings, line 37)
> 
> It looks like it is also failing on x86, s390 and ia64 according to
> gcc-testresults.
> 
> Can you check?

Yes, I'm tracking the problem in pr97027, but I didn't realize
the test was susceptible to this dependency when I wrote it.  It
could be adjusted to avoid the missing warnings but the right fix
is to improve the detection.  Let me see if I can quickly handle
the latter.  In not, I'll constrain the test to just the targets
where I know is passes.

Thanks
Martin


More information about the Gcc-patches mailing list