electronicarts / EASTL

EASTL stands for Electronic Arts Standard Template Library. It is an extensive and robust implementation that has an emphasis on high performance.
BSD 3-Clause "New" or "Revised" License
7.85k stars 907 forks source link

Integration docs lack Windows support #258

Open Const-me opened 5 years ago

Const-me commented 5 years ago

EASTL_Project_Integration.md says following:

EASTL requires you to have an overload for the operator new[], here is an example that just forwards to global new[]:

void* __cdecl operator new[](size_t size, const char* name, int flags, unsigned debugFlags, const char* file, int line)

That’s true but not enough. Another one is required, too:

void* operator new[]( size_t size, size_t alignment, size_t alignmentOffset, const char* pName, int flags, unsigned debugFlags, const char* file, int line )

You have a sample code in SampleNewAndDelete.cpp with the following in it:

// Substitute your aligned malloc.
return malloc_aligned(size, alignment, alignmentOffset);

On Windows, _aligned_malloc API requires the pointers to be freed by corresponding _aligned_free API. However, EASTL only calls a single operator delete for both aligned and non-aligned blocks.

There’s a comment in allocator_malloc.h that acknowledges the problem:

// VC++ _aligned_malloc http://msdn.microsoft.com/en-us/library/8z34s9c6%28VS.80%29.aspx This is not suitable, since it has a limitation that you need to free via _aligned_free.

Am I correct in the assumption that default allocators are incompatible with Windows?

I've implemented my own ones, take a look: https://gist.github.com/Const-me/8c106cdd968587b11de4212b6e84573a Appears to work OK. Feel free to copy-paste and use to make better documentation.

rparolin commented 5 years ago

Yeah, the 'SampleNewAndDelete.cpp' file hasn't been touched in a while. I'd point to other allocators in the tests as much better examples. In addition this file doesn't get built so I will remove it in a future release.

vdemichev commented 4 weeks ago

My understanding is that the issue still applies to the current code: any user-defined implementation of new[] not compatible with simple delete[] can potentially be an UB. E.g. if using C++17 aligned new, it's also UB. So what makes sense indeed is (i) use the current code under EASTL_DLL or (ii) - better - for C++17 use _aligned_malloc/aligned_free #if _MSC_VER or std::aligned_alloc/std::free otherwise.

jhopkins-ea commented 4 weeks ago

You're correct. At a minimum we should update EASTL_Project_Integration.md to mention delete[].