cplusplus / CWG

Core Working Group
23 stars 7 forks source link

[basic.life] CWG2625 broke dynamic arrays providing storage #415

Open languagelawyer opened 12 months ago

languagelawyer commented 12 months ago

Full name of submitter: JMC

Reference (section label): [basic.life]

Link to reflector thread (if any): https://stackoverflow.com/questions/77023929/c23-changes-disallow-using-dynamically-allocated-array-as-storage-provider

Issue description: CWG2625 did to [basic.life]/6.1:

… after the lifetime of an object has ended … The program has undefined behavior if: • the object will be or was of a class type with a non-trivial destructor and the pointer is used as the operand of a delete-expression,

now consider

auto buf = new unsigned char[sizeof(int)];
new (buf) int; // kills buf[0]
delete[] buf; // the pointer to buf[0] is used as the operand of delete-expression

Suggested resolution: Undo CWG2625 resolution. Or, at least, limit the restriction to single-object delete expression.

sergey-anisimov-dev commented 11 months ago

I'd say it should've been the object will be or was of a class type: this would include even the trivial destructors thus maintaining the internal consistency and allow the cases that involve no implicit non-static member function invocations to remain valid (which, again, would keep the symmetry with, as I undestand it, benign at-scope-exit implicit destructions of already explicitly destroyed (through the means of pseudo-destructors) non-class-type objects). Apart from that, I agree that the lifetimes of the elements of the arrays that provide storage end upon their storage being reused, and the pointers to the elements destroyed henceforth point to the respective storage locations instead.

There would be a way to circumvent this issue somewhat if only there would exist a way to transparently replace the array immediately prior the deletion. Unfortunately, as per my knowledge, there is none: under the current wording new (buf) unsigned char[sizeof(int)] would nest another array of the same size within the original one (I'm almost certain about seeing an issue that addressed exactly that but can't recall where exactly that was) and thus would neither recreate this original array, nor rebind the buf pointer (otherwise it would've worked); iirc, there also existed a proposal to relax the rules of transparent replaceability and allow it to work for individual subobjects - that also would've solved the issue (new (buf) unsigned char). Regardless, those are the possible hypothetical workarounds that come to mind, not the reasons of why it shouldn't work in the first place.

frederick-vs-ja commented 11 months ago

IMO the least terrible way is keeping array elements living after new (buf) int...

Or, perhaps we can just restore the "the object will be or was of a class type" part.

jensmaurer commented 11 months ago

Even for non-class types (e.g. "int"), we want this rule to apply.

The problem here really is the array-delete, where we don't pass a pointer to the entire array, but just a pointer to the first element (because that's how array-delete works).

In the initial example, the array actually provides storage, so its lifetime does not end, and array-deleting it is fine.

sergey-anisimov-dev commented 11 months ago

Even for non-class types (e.g. "int"), we want this rule to apply.

Then the intention goes that the following should be undefined?

{
  int i;
  i.~decltype(i)() /* lifetime ended once */;
} /* lifetime ended twice */

It's likely that there are no formal grounds for this to be the case either. You might want to visit this post, @jensmaurer: I've attempted to dissect the problem at length there (the second to last paragraph summarizes the relevance; the discussion there stems from #361 which might turn out to be relevant as well).

jensmaurer commented 11 months ago

Then the intention goes that the following should be undefined?

Yes, that's at least my opinion (possibly with the exception of "unsigned char"). I think this is a slightly larger can of worms, though.

sergey-anisimov-dev commented 11 months ago

I think this is a slightly larger can of worms, though.

Some revisions would be needed then, I guess. I would much prefer for such cases not to rely on vague interpretations of [basic.life#4] (personally, I would like it moved to notes altogether while fixing the probable remaining gaps with less... arcane clauses).

frederick-vs-ja commented 10 months ago

Should we consider striking the whole [basic.life] p6.1?

As discussed in #435, a delete-expression generally destroys the pointed-to object, and it's already UB to do this for out-of-lifetime class objects. Although the issue for double destruction of scalar objects is not yet resolved.

As a contrived case (not related to dynamic arrays), the delete-expression may call a destroying operator delete which neither accesses nor destroys the object. IMO the behavior can be simply well-defined.

frederick-vs-ja commented 4 months ago

I guess the preferred resolution would be...

Change [basic.life] p1.5 as:

the storage which the object occupies is released, or is reused by an object that is not nested within o, unless o is an element object of an array that provides storage for the new object ([intro.object]).

Change [intro.object] p3 as (add before the note):

Whenever the new object is within its lifetime, the value of elements of its object representation ([basic.types.general]) are always same as the value of array elements that overlap with it.

Just keeping array elements living avoids all issues with double destruction.