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

[Martin Sebor <msebor@gmail.com>]

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