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

[Martin Sebor <msebor@gmail.com>]

On 10/7/20 9:07 AM, Jason Merrill wrote:
> On 10/7/20 10:42 AM, Martin Sebor wrote:
>> On 10/7/20 8:26 AM, Jason Merrill wrote:
>>> On 9/28/20 6:01 PM, Martin Sebor 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.
>>>
>>> It looks to me like the offset is offset_int, which is both signed 
>>> and big enough to hold all values of sizetype without turning large 
>>> positive values into negative values.  Where are these sign-switching 
>>> conversions happening?
>>
>> In get_offset_range in builtins.c.
> 
> Since we're converting to offset_int there, why not give the offset_int 
> the real value rather than a bogus negative value?

I don't understand the question: the real offset (in the program)
is negative when its sizetype representation is greater than
PTRDIFF_MAX.  It's worked this way for years.

Martin

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