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

[Jason Merrill <jason@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4955 bytes --]

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.

>>> +  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:


[-- Attachment #2: layout.diff --]
[-- Type: text/x-patch, Size: 745 bytes --]

commit 95c284379d67efb79a30273236fd6769a12f3031
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Sep 4 12:14:19 2020 -0400

    layout

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 31d68745844..f6ca51ad8ba 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -17444,10 +17444,10 @@ complete_vars (tree type)
 	      && (TYPE_MAIN_VARIANT (strip_array_types (type))
 		  == iv->incomplete_type))
 	    {
-	      /* Complete the type of the variable.  The VAR_DECL itself
-		 will be laid out in expand_expr.  */
+	      /* Complete the type of the variable.  */
 	      complete_type (type);
 	      cp_apply_type_quals_to_decl (cp_type_quals (type), var);
+	      layout_decl (var, 0);
 	    }
 
 	  /* Remove this entry from the list.  */