Re: [PATCH] tree-optimization/97151 - improve PTA for C++ operator delete

[Jason Merrill <jason@redhat.com>]

On 9/28/20 3:56 AM, Richard Biener wrote:
> On Fri, 25 Sep 2020, Jason Merrill wrote:
> 
>> On 9/25/20 2:30 AM, Richard Biener wrote:
>>> On Thu, 24 Sep 2020, Jason Merrill wrote:
>>>
>>>> On 9/24/20 3:43 AM, Richard Biener wrote:
>>>>> On Wed, 23 Sep 2020, Jason Merrill wrote:
>>>>>
>>>>>> On 9/23/20 2:42 PM, Richard Biener wrote:
>>>>>>> On September 23, 2020 7:53:18 PM GMT+02:00, Jason Merrill
>>>>>>> <jason@redhat.com>
>>>>>>> wrote:
>>>>>>>> On 9/23/20 4:14 AM, Richard Biener wrote:
>>>>>>>>> C++ operator delete, when DECL_IS_REPLACEABLE_OPERATOR_DELETE_P,
>>>>>>>>> does not cause the deleted object to be escaped.  It also has no
>>>>>>>>> other interesting side-effects for PTA so skip it like we do
>>>>>>>>> for BUILT_IN_FREE.
>>>>>>>>
>>>>>>>> Hmm, this is true of the default implementation, but since the function
>>>>>>>>
>>>>>>>> is replaceable, we don't know what a user definition might do with the
>>>>>>>> pointer.
>>>>>>>
>>>>>>> But can the object still be 'used' after delete? Can delete fail /
>>>>>>> throw?
>>>>>>>
>>>>>>> What guarantee does the predicate give us?
>>>>>>
>>>>>> The deallocation function is called as part of a delete expression in
>>>>>> order
>>>>>> to
>>>>>> release the storage for an object, ending its lifetime (if it was not
>>>>>> ended
>>>>>> by
>>>>>> a destructor), so no, the object can't be used afterward.
>>>>>
>>>>> OK, but the delete operator can access the object contents if there
>>>>> wasn't a destructor ...
>>>>
>>>>>> A deallocation function that throws has undefined behavior.
>>>>>
>>>>> OK, so it seems the 'replaceable' operators are the global ones
>>>>> (for user-defined/class-specific placement variants I see arbitrary
>>>>> extra arguments that we'd possibly need to handle).
>>>>>
>>>>> I'm happy to revert but I'd like to have a testcase that FAILs
>>>>> with the patch ;)
>>>>>
>>>>> Now, the following aborts:
>>>>>
>>>>> struct X {
>>>>>      static struct X saved;
>>>>>      int *p;
>>>>>      X() { __builtin_memcpy (this, &saved, sizeof (X)); }
>>>>> };
>>>>> void operator delete (void *p)
>>>>> {
>>>>>      __builtin_memcpy (&X::saved, p, sizeof (X));
>>>>> }
>>>>> int main()
>>>>> {
>>>>>      int y = 1;
>>>>>      X *p = new X;
>>>>>      p->p = &y;
>>>>>      delete p;
>>>>>      X *q = new X;
>>>>>      *(q->p) = 2;
>>>>>      if (y != 2)
>>>>>        __builtin_abort ();
>>>>> }
>>>>>
>>>>> and I could fix this by not making *p but what *p points to escape.
>>>>> The testcase is of course maximally awkward, but hey ... ;)
>>>>>
>>>>> Now this would all be moot if operator delete may not access
>>>>> the object (or if the object contents are undefined at that point).
>>>>>
>>>>> Oh, and the testcase segfaults when compiled with GCC 10 because
>>>>> there we elide the new X / delete p pair ... which is invalid then?
>>>>> Hmm, we emit
>>>>>
>>>>>      MEM[(struct X *)_8] ={v} {CLOBBER};
>>>>>      operator delete (_8, 8);
>>>>>
>>>>> so the object contents are undefined _before_ calling delete
>>>>> even when I do not have a DTOR?  That is, the above,
>>>>> w/o -fno-lifetime-dse, makes the PTA patch OK for the testcase.
>>>>
>>>> Yes, all classes have a destructor, even if it's trivial, so the object's
>>>> lifetime definitely ends before the call to operator delete. This is less
>>>> clear for scalar objects, but treating them similarly would be consistent
>>>> with
>>>> other recent changes, so I think it's fine for us to assume that scalar
>>>> objects are also invalidated before the call to operator delete.  But of
>>>> course this doesn't apply to explicit calls to operator delete outside of a
>>>> delete expression.
>>>
>>> OK, so change the testcase main slightly to
>>>
>>> int main()
>>> {
>>>     int y = 1;
>>>     X *p = new X;
>>>     p->p = &y;
>>>     ::operator delete(p);
>>>     X *q = new X;
>>>     *(q->p) = 2;
>>>     if (y != 2)
>>>       __builtin_abort ();
>>> }
>>>
>>> in this case the lifetime of *p does not end before calling
>>> ::operator delete() and delete can stash the object contents
>>> somewhere before ending its lifetime.  For the very same reason
>>> we may not elide a new/delete pair like in
>>>
>>> int main()
>>> {
>>>     int *p = new int;
>>>     *p = 1;
>>>     ::operator delete (p);
>>> }
>>
>> Correct; the permission to elide new/delete pairs are for the expressions, not
>> the functions.
>>
>>> which we before the change did not do only because calling
>>> operator delete made p escape.  Unfortunately points-to analysis
>>> cannot really reconstruct whether delete was called as part of
>>> a delete expression or directly (and thus whether object lifetime
>>> ended already), neither can DCE.  So I guess we need to mark
>>> the operator delete call in some way to make those transforms
>>> safe.  At least currently any operator delete call makes the
>>> alias guarantee of a operator new call moot by forcing the object
>>> to be aliased with all global and escaped memory ...
>>>
>>> Looks like there are some unallocated flags for CALL_EXPR we could
>>> pick but I wonder if we can recycle protected_flag which is
>>>
>>>          CALL_FROM_THUNK_P and
>>>          CALL_ALLOCA_FOR_VAR_P in
>>>              CALL_EXPR
>>>
>>> for calls to DECL_IS_OPERATOR_{NEW,DELETE}_P, thus whether
>>> we have CALL_FROM_THUNK_P for those operators.  Guess picking
>>> a new flag is safer.
>>
>> We won't ever call those operators from a thunk, so it should be OK to reuse
>> it.
>>
>>> But, does it seem correct that we need to distinguish
>>> delete expressions from plain calls to operator delete?
>>
>> A reason for that distinction came up in the context of omitting new/delete
>> pairs: we want to consider the operator first called by the new or delete
>> expression, not a call from that first operator to another operator new/delete
>> and exposed by inlining.
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543404.html
>>
>>> In this context I also wonder about non-replaceable operator delete,
>>> specifically operator delete in classes - are there any semantic
>>> differences between those or why did we choose to only mark
>>> the replaceable ones?
>>
>> The standard says that for omitting a 'new' allocation, the operator new has
>> to be a replaceable one, but does not say the same about 'delete'; it just
>> says that if the allocation was omitted, the delete-expression does not call a
>> deallocation function.  It may not be necessary to make this distinction for
>> delete.  And this distinction could be local to the front end.
>>
>> In the front end, we currently have cxx_replaceable_global_alloc_fn that
>> already ignores the replaceability of operator delete.  And we have
>> CALL_FROM_NEW_OR_DELETE_P, that would just need to move into the middle end.
>> And perhaps get renamed to CALL_OMITTABLE_NEW_OR_DELETE_P, and not get set for
>> calls to non-replaceable operator new.
> 
> CALL_FROM_NEW_OR_DELETE_P indeed looks like the best fit - it's
> only evaluated when cxx_replaceable_global_alloc_fn matches in the C++
> FE so could be made to cover only replaceable variants.
> 
> CALL_REPLACEABLE_NEW_OR_DELETE_P () maybe, since we already use
> REPLACEABLE for the fndecl flags?  OMITTABLE is too specific
> for the PTA case where it really matters whether the object
> lifetime ends before the delete call, not whether it can be
> omitted (hmm, guess that's not 100% overlap then either...).

That seems like good overlap to me, if we agree that object lifetime 
ends before any delete call from a delete-expression, whether or not the 
operator delete is replaceable.

> Mind doing the C++ side of things recycling protected_flag as suggested?

OK.

Jason