Rules for array cookies are outdated

[jwakely]

https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies says:

cxx-abi/abi.html

Lines 2123 to 2127 in 7f36ec6

	<li> No cookie is required if the array element type T has a trivial
	destructor (12.4 [class.dtor])
	and the usual (array) deallocation function
	(3.7.3.2 [basic.stc.dynamic.deallocation])
	function does not take two arguments.

This is wrong since C++17, because T::operator new[](std::size_t, std::align_val_t) has two parameters, but still needs a cookie unless T has a trivial destructor. It might also be wrong for a C++20 destroying operator delete, but I haven't spent enough time thinking about that case.

The note is also outdated:

cxx-abi/abi.html

Lines 2130 to 2134 in 7f36ec6

	(Note: if the usual array deallocation function takes two arguments,
	then it is a member function whose second argument is of type size_t.
	The standard guarantees (12.5 [class.free])
	that this function will be passed the
	number of bytes allocated with the previous array new expression.)

Since C++14 added sized deallocation there is a usual deallocation functions with two parameters that isn't a member function, operator delete[](void*, std::size_t).

[rjmccall]

This is wrong since C++17, because T::operator new[](std::size_t, std::align_val_t) has two parameters, but still needs a cookie unless T has a trivial destructor.

I don't think that array allocations using that operator need an array cookie. Did you mean to say the reverse, that it doesn't need a cookie unless T has a non-trivial destructor? Because if you're just pointing out the outdatedness of this using "two argument" to refer exclusively to sized operator delete[], yes, I agree we should be explicit.

Would you like to make a patch for this?

[jwakely]

Ugh, sorry for the garbage report. I apparently wasn't paying attention to what I was writing.

What I meant to say is that T::operator delete[](void*, size_t, align_val_t) should be treated the same as
T::operator delete[](void*, size_t) when deciding whether a cookie is needed, but the current rule treats them differently because it's specified in terms of the number of arguments. Specifying the rule in terms of the number of arguments to the deallocation function seems wrong. Maybe I've misunderstood what the rule is saying.

I don't think that array allocations using that operator need an array cookie.

Where does the size_t argument to operator delete[] come from then?

In the following code we have a new[] expression where the array element type has a trivial destructor, and the usual deallocation function does not take two arguments. The ABI says no cookie is required in this case.

#include <new>
#include <stdio.h>
#include <stdlib.h>

struct alignas(32) Foo
{
    char x;

    void * operator new[] (size_t s, std::align_val_t a)
    {
        void* p =  aligned_alloc(static_cast<size_t>(a), s);
        printf ("p: %p s: %zu, a: %zu\n", p, s, (size_t)a);
        return p;
    }

    void operator delete[] (void *p, size_t s, std::align_val_t a)
    {
        printf ("p: %p s: %zu, a: %zu\n", p, s, (size_t)a);
        free(p);
    }
};

int main()
{
    auto p = new Foo[3];
    printf("p: %p\n", p);
    delete[] p;
}

Clang and EDG do use a cookie here though, as I would expect. Without a cookie, the compiler can't provide the size_t argument. GCC gets confused and doesn't use a cookie, but then adjusts the pointer as though it had used a cookie and reads garbage before *p, but that's clearly a bug.

If the rule is trying to ensure that a cookie is used so the size can be passed to T::operator delete[](void*, size_t) then surely it needs to also ensure a cookie is used for the version taking align_val_t?

Which is why I think that "does not take two arguments" is the wrong condition.

Would you like to make a patch for this?

I don't think I understand the intended meaning well enough to try and rephrase it. I also don't understand what "usual deallocation function" means in C++98 or C++20, or why the term is defined at all.

Would the following be correct?

No cookie is required if the array element type T has a trivial destructor (12.4 [class.dtor]) and the usual (array) deallocation function (3.7.3.2 [basic.stc.dynamic.deallocation]) function does not take two arguments does not have a parameter of type size_t.

(Note: if the usual array deallocation function takes two arguments, then it is a member function whose second argument is of type size_t. The has a parameter of type size_t, then the standard guarantees (12.5 [class.free]) that this function will be passed the number of bytes allocated with the previous array new expression.)

This takes advantage of the standard's definition of "usual array deallocation function" so that we know a size_t parameter corresponds to the size. A deallocation function like operator delete[](void*, int, int, size_t) is not a usual deallocation function, so we don't need to consider what its size_t parameter means.

[jwakely]

P.S. I've just noticed the current wording says:

the usual (array) deallocation function (3.7.3.2 [basic.stc.dynamic.deallocation]) function

which also seems wrong.

[rjmccall]

Right, yes, if there can be a sized deallocation function that takes an align_val_t, then that requires a cookie. Your example just cited the allocation function. I think we're in agreement here that the wording should be revised to talk about whether the deallocation function requires a size argument instead of talking about it purely in terms of argument count.

[zygoloid]

It might also be wrong for a C++20 destroying operator delete, but I haven't spent enough time thinking about that case.

I don't think destroying delete introduces any new concerns here, because destroying delete is only supported for the single-object form, not for array delete (which is, in turn, largely because we didn't find a good way to handle array cookies).

[rjmccall]

I've posted a proposed rewrite of this section: #123. This seems editorial, so I'll commit in a day or two if there aren't any comments.

@zygoloid, @jicama: I was considering describing the ARM variant ABI for array cookies, but I wasn't quite sure what the alignment rules for it were. Apparently the answer, as observed in at least GCC and Clang, is that it completely ignores alignment and will happily mis-align all the array elements if alignof(T) > 2 * sizeof(size_t). This seems like something we should fix now that the standard is taking new-extended alignments seriously. I think the only question is whether to pad before or after the cookie; the consistent thing with the standard ABI would be to pad before.

[rjmccall]

Also, technically, I think cookie alignment needs to affect the std::align_val_t argument value. I'm not sure this can happen in new expressions: cookies should never have new-extended alignment, and I believe the standard says that a std::align_val_t allocation function is only ever used with types with new-extended alignment. However, it can happen in delete expressions, because a std::align_val_t deallocation function will be used even for allocations that don't have new-extended alignment if there are no deallocation functions available that don't take a std::align_val_t.

It is vanishingly unlikely that this would ever produce an observable difference, of course, since you'd need a class type with alignment less than size_t which nonetheless forces an array cookie.

[zygoloid]

It is vanishingly unlikely that this would ever produce an observable difference, of course, since you'd need a class type with alignment less than size_t which nonetheless forces an array cookie.

Even then, it's not obvious that you want the cookie to affect the align_val_t value. The call to operator delete will be the first time the alignment of the allocation has been mentioned, so it's not like you can use this to tell the allocator which size class the allocation was originally requested from. And incorporating the cookie alignment into the passed align_val_t will result in invoking a deallocation function with a size that's not a multiple of the alignment. For:

#include <new>
struct A {
    ~A();
    void operator delete[](void*, std::align_val_t);
};
void f() { delete[] new A[5]; }

... GCC, Clang, and ICC all pass 1 as the alignment in the deallocation call; maybe we should pick that rule, given that it doesn't seem to matter?

I was considering describing the ARM variant ABI for array cookies, but I wasn't quite sure what the alignment rules for it were. Apparently the answer, as observed in at least GCC and Clang, is that it completely ignores alignment and will happily mis-align all the array elements if alignof(T) > 2 * sizeof(size_t)

That doesn't match what I'm seeing here: https://godbolt.org/z/WY6W4sq7r

GCC is allocating 168 bytes and misaligning the elements, but Clang is allocating 192 bytes, putting the array cookie at the start of the buffer, and correctly-aligning the elements.

Given that GCC needs an ABI break to stop miscompiling and Clang does not, I'd suggest we settle on the Clang rule if we don't have a strong reason to want to pad on the left.

[rjmccall]

It is vanishingly unlikely that this would ever produce an observable difference, of course, since you'd need a class type with alignment less than size_t which nonetheless forces an array cookie.

Even then, it's not obvious that you want the cookie to affect the align_val_t value. The call to operator delete will be the first time the alignment of the allocation has been mentioned, so it's not like you can use this to tell the allocator which size class the allocation was originally requested from. And incorporating the cookie alignment into the passed align_val_t will result in invoking a deallocation function with a size that's not a multiple of the alignment. For:

Ah, that's an interesting consideration. And that works because even the std::align_val_t allocators are required to produce a pointer sufficiently aligned for any fundamental type — that's a floor for all allocators. Alright.

Given that GCC needs an ABI break to stop miscompiling and Clang does not, I'd suggest we settle on the Clang rule if we don't have a strong reason to want to pad on the left.

Hmm, I must've messed up my experiments. I agree about using the Clang rule here, then.

I'll update the draft.