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

[Martin Sebor]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-96511.diff
Type: text/x-patch
Size: 49026 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200914/ff1e3ed7/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-compute_objsize.diff
Type: text/x-patch
Size: 57824 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200914/ff1e3ed7/attachment-0003.bin>