Crash on Vector Math Library "=" operator

[jiangwei]

Hi all:
I had written a console app for test SSE version Vector3,but its crash on line "m_minimum = val" in Region::setMinimum function.
The Scalar version has no problem.
Why?

Code: Select all

#include "stdafx.h"
#include <vector>
#include "Region.h"

int _tmain(int argc, _TCHAR* argv[])
{

	std::vector<Region*> tmpArray;

	for (int i = 0;i < 10;i ++)
	{
		Region* tmpRegion = new Region;

		Vectormath::Aos::Vector3 tmpMin = Vectormath::Aos::Vector3(-0.6f,-0.6f,-0.6f);

		tmpRegion->setMinimum(tmpMin);

		tmpArray.push_back(tmpRegion);
	}
	system("pause");
	return 0;
}

Code: Select all

#ifndef Region_h__
#define Region_h__

#include "vectormath_aos.h"

class Region
{
public:
	
	void setMinimum( const Vectormath::Aos::Vector3& val )
	{
		m_minimum = val;
	}
protected:
	Vectormath::Aos::Vector3 m_minimum;

};
#endif // Region_h__

[Flix]

This seems to work for me (but don't ask me why ).
There seems to be a problem with the default allocator.
(BTW: I used the VC 7.1 compiler.)

Code: Select all

// Note: bulletMath.lib (or libbulletmath.lib) must be added to the linker options of the project.

#include <iostream>
#include <cstdlib>
#include <vector>
#include <tchar.h>
#define _VECTORMATH_DEBUG	// static char* Vectormath::Aos::print(const Vectormath::Aos::Vector3& vector,const char * name=NULL)
#include "vectormath_aos.h"
#include <btAlignedObjectArray.h>
#include <new> // bad_alloc definition

using namespace std;
using namespace Vectormath::Aos;

// To monitor memory leaks (available for free when using btAlignedAlloc(...) btAlignedFree())
extern int gNumAlignedAllocs;
extern int gNumAlignedFree;
extern int gTotalBytesAlignedAllocs;
//--------------------------------------------------------------------------------------------

class Region
{
public:
  
   void setMinimum( const Vectormath::Aos::Vector3& val )
   {
   m_minimum = val;
   }
   
   #ifdef _VECTORMATH_DEBUG
   void print(const char * name )	{
   	Vectormath::Aos::print(m_minimum,name); 
   }
   #endif

// Here I OVERLOAD the class new AND delete methods to make them memory aligned (we can move them outside the class to make them global, (but they will be used by EVERYTHING in that case)):

// An alignment of 16 seems to work (Vectormath::Aos::Vector3 has a size of 128 bits = 32bits x 4 = 4bytes x 4 = 16 bytes, I think):
#define MY_ALLOCATOR_ALIGNMENT (sizeof(Vectormath::Aos::Vector3)/sizeof(unsigned char))
// I would like to know which value is correct: bulletmath usually uses 16 too.
// The above definition returns 16 on my system: so all the sources seem to point out that 16 is correct...

void* operator new(size_t sz) throw (bad_alloc) {
  void* m = btAlignedAlloc(sz, MY_ALLOCATOR_ALIGNMENT);
  if (!m) throw bad_alloc(); 
  // If we move these methods outside the class we can't probably use cout<<[...], since it allocates memory using new -> endless loop. Just in case we use printf(...) that is safer.
  printf("	Object Memory Allocated (%i bytes / %i alignment)\n",sz,MY_ALLOCATOR_ALIGNMENT);
  return m;
}
void operator delete(void* m) {
  btAlignedFree(m);	
  printf("	Object Memory Released\n");
}
// Never used:-----------------------
void* operator new[](size_t sz) throw (bad_alloc) {
	void* m = (unsigned char*) btAlignedAlloc(sz,MY_ALLOCATOR_ALIGNMENT);
	if (!m) throw bad_alloc(); 
	printf("	Array Memory Allocated (%i bytes / %i alignment)\n",sz,MY_ALLOCATOR_ALIGNMENT);
	return m;
}
void operator delete[](void* m) {
  btAlignedFree(m);	
  printf("	Array Memory Released\n");
}
//------------------------------------



// I've added a "debug" ctr and dtr, because I've tried to use directly btAlignedAlloc(...) and 
// btAlignedFree(...) without overriding new and delete: the result worked, but the ctr and dtr
// were never called. Now that the problem is solved, I leave them here, so that people understands
// I've made some effort to make it work :).
Region()	{cout<<"	Region::Region()"<<endl;}
~Region()	{cout<<"	Region::~Region()"<<endl;}

protected:
   Vectormath::Aos::Vector3 m_minimum;

};


int _tmain(int argc, _TCHAR* argv[])
{
  // Both std::vector and btAlignedObjectArray work in this case (which one is better/faster?).
  // (And now that my global new is aligned there is still some difference between the two? I guess so).
  //btAlignedObjectArray < Region* > tmpArray;
  std::vector < Region* > tmpArray;

  // OPTIONAL NOTE:	For storing instances instead of pointers, std::vector does not work for me:
  // (error C2719: '_Val': formal parameter with __declspec(align('16')) won't be aligned: see reference to class template instantiation 'std::vector<_Ty>' being compiled)
  // std::vector < Region > tmpArrayError;
  // btAlignedObjectArray < Region > tmpArrayGood;


	//--- Fill the container: ----	
   for (int i = 0;i < 10;i ++)	{
      	Region* tmpRegion=new Region;
		Vectormath::Aos::Vector3 tmpMin = Vectormath::Aos::Vector3(-0.6f,-0.6f,-0.6f);
		tmpRegion->setMinimum(tmpMin);
		tmpArray.push_back(tmpRegion);
   }

   #ifdef _VECTORMATH_DEBUG
	//--- Display the content: ----	
   for (int i = 0;i < 10;i ++)	{
   		cout<<"Region["<<i<<"]:"<<endl;
   		tmpArray[i]->print("m_minimum");
   }
   #endif
 
   	//--- Clean Up: --- 
   for (int i = 9;i >= 0;i --)	{
   		delete tmpArray[i];
   		tmpArray.pop_back();
   }   
   

   cout<<endl<<endl;
   cout<<"ALIGNED MEMORY ALLOCATION INFO:"<<endl;
   cout<<"gNumAlignedAllocs="<<gNumAlignedAllocs<<endl;
   cout<<"gNumAlignedFree="<<gNumAlignedFree<<endl;
   cout<<"gTotalBytesAlignedAllocs="<<gTotalBytesAlignedAllocs<<endl;
   cout<<endl<<endl;
   
   system("PAUSE");
   return 0;
}

[Erwin Coumans]

Code: Select all

Region* tmpRegion = new Region;

When using SIMD, the values need to be 16-byte aligned in memory, so you need to make sure to call the btAlignedAlloc is used, instead of the un-aligned allocator used by 'new'.

You can add a macro 'BT_DECLARE_ALIGNED_ALLOCATOR' in your Region class, it overloads the new/delete operator for that class to use btAlignedAlloc/btAlignedFree. Then the destructor/constructor will be called fine. If you use btAlignedAlloc/btAlignedFree manually, you need to call the constructor/destructor manually if necessary.

Use the btAlignedObjectArray to store aligned objects (it will call the constructor/destructor explicitly internally).
Hope this helps,
Erwin

[Flix]

Thank you for your clarifications. All makes more sense to me now.

The use of your macro BT_DECLARE_ALIGNED_ALLOCATOR() make things much easier.
Anyway I've noticed that the new[] and delete[] overloads are missing, so that if I
modify the program a bit:

Code: Select all

// Note: libBulletMath.lib (or bulletmath.lib) must be added to the linker options of the project.
#include <iostream>
#include <cstdlib>
#include <vector>
#include <tchar.h>

#define _VECTORMATH_DEBUG
#include "bt_vectormath_aos.h"
#include <btAlignedObjectArray.h>

using namespace std;
using namespace Vectormath::Aos;

// To monitor memory leaks (available for free when using btAlignedAlloc(...) btAlignedFree())
extern int gNumAlignedAllocs;
extern int gNumAlignedFree;
extern int gTotalBytesAlignedAllocs;
//--------------------------------------------------------------------------------------------

class Region
{
public:
  
   void setMinimum( const Vectormath::Aos::Vector3& val )
   {
   m_minimum = val;
   }
   
   #ifdef _VECTORMATH_DEBUG
   void print(const char * name )	{
   	Vectormath::Aos::print(m_minimum,name); 
   }
   #endif

BT_DECLARE_ALIGNED_ALLOCATOR()

Region()	{cout<<"	Region::Region()"<<endl;}
~Region()	{cout<<"	Region::~Region()"<<endl;}

protected:
   Vectormath::Aos::Vector3 m_minimum;

};


int _tmain(int argc, _TCHAR* argv[])
{
	
   Region* tmpRegion=new Region[10];
		
   for (int i = 0;i < 10;i ++)	{
      	Vectormath::Aos::Vector3 tmpMin = Vectormath::Aos::Vector3(-0.6f,-0.6f,-0.6f);
		tmpRegion[i].setMinimum(tmpMin);
   }

   #ifdef _VECTORMATH_DEBUG
	//--- Display the content: ----	
   for (int i = 0;i < 10;i ++)	{
   		cout<<"Region["<<i<<"]:"<<endl;
   		tmpRegion[i].print("m_minimum");
   }
   #endif
 
   	//--- Clean Up: --- 
   delete[] tmpRegion;

   cout<<endl<<endl;
   cout<<"ALIGNED MEMORY ALLOCATION INFO:"<<endl;
   cout<<"gNumAlignedAllocs="<<gNumAlignedAllocs<<endl;
   cout<<"gNumAlignedFree="<<gNumAlignedFree<<endl;
   cout<<"gTotalBytesAlignedAllocs="<<gTotalBytesAlignedAllocs<<endl;
   cout<<endl<<endl;
   
   system("PAUSE");
   return 0;
}

it still crashes.
By adding the new[] and delete[] overloads to the macro everything works again:

In <btScalar.h>:

Code: Select all

#define BT_DECLARE_ALIGNED_ALLOCATOR() \
	SIMD_FORCE_INLINE void* operator new(size_t sizeInBytes)	{ return btAlignedAlloc(sizeInBytes,16); }	\
	SIMD_FORCE_INLINE void  operator delete(void* ptr)			{ btAlignedFree(ptr); }	\
	SIMD_FORCE_INLINE void* operator new(size_t, void* ptr)	{ return ptr; }	\
	SIMD_FORCE_INLINE void  operator delete(void*, void*)		{ }	\
	SIMD_FORCE_INLINE void* operator new[](size_t sizeInBytes)	{ return btAlignedAlloc(sizeInBytes,16); }	\
	SIMD_FORCE_INLINE void  operator delete[](void* ptr)			{ btAlignedFree(ptr); }	\
	SIMD_FORCE_INLINE void* operator new[](size_t, void* ptr)	{ return ptr; }	\
	SIMD_FORCE_INLINE void  operator delete[](void*, void*)		{ }	\

BTW: I don't know if this modification is a good one for every scenario (maybe there are good reasons to avoid the new[] and delete[] versions when the macro is used at the global scope to save memory on large arrays or something like that).

Hope it may be useful.
Flix

[Erwin Coumans]

Yes, it would be good to add those methods new[] and delete[] to the macro. It will be added for next version.

Internally, Bullet uses btAlignedObjectArray, instead of array de/allocations.

Thanks,
Erwin

[russellbartley]

I also came acros a problem because of the lack of the overloaded new [] operator. in 2.69, nd 2.7

inside of btAxisSweep3::btAxisSweep3():

there are the lines:

// allocate handles buffer and put all handles on free list
void* ptr = btAlignedAlloc(sizeof(Handle)*maxHandles,16);
m_pHandles = new(ptr) Handle[maxHandles];

and in the destructor there is:

btAlignedFree(m_pHandles);

This, I believe means that:

- the memory is allocated via the btAlignedAlloc, which includes enough memory for the alignment and its real pointer.
- the placement new [] operator is called, and since the placement new [] operator has not been overloaded in the BT_DECLARE_ALIGNED_ALLOCATOR(); macro, then the os's placement new gets called.
- this new operator needs to store the size and number of array elements, so puts these at the start of the memory block, and shifts the returned pointer of the array along 16 bytes.
- this shifting along of 16 bytes may or may not be covered by the original size of the btAlignedAlloc?
- when the btAlignedFree is called in the destructor, it expects the pointer to be pointing to what the btAlignedAlloc returned, which it is not since the os's placement operator new [] was called, ending in a crash.


This I solved by adding in the placement operator new [] overload and corresponding delete in BT_DECLARE_ALIGNED_ALLOCATOR();, and changed the cod in the constructor from:

// allocate handles buffer and put all handles on free list
void* ptr = btAlignedAlloc(sizeof(Handle)*maxHandles,16);
m_pHandles = new(ptr) Handle[maxHandles];

to

m_pHandles = new Handle[maxHandles];

and the code in the desructor from:

btAlignedFree(m_pHandles);

to:

delete [] m_pHandles;


Is this the best solution, and can we get a solution for this merged into bullet?

Doing placement new [] and then freeing ith a btAlignedFree seem to be a bit dangerous to me?

Also would it be more consitent to have these operators overloaded for the entire bullet libraries, rather than just a single class? or am I missing something?

Thanks

Russ

[Erwin Coumans]

- this new operator needs to store the size and number of array elements, so puts these at the start of the memory block, and shifts the returned pointer of the array along 16 bytes.

No, this is a placement new operator, so it won't store the size/number of array elements. Current implementation should be fine with most compilers, they just return the address when using placement new, see MSVC:

Code: Select all

inline void *__CRTDECL operator new[](size_t, void *_Where) _THROW0()
	{	// construct array with placement at _Where
	return (_Where);
	}

But we shouldn't rely on the compiler, so we finally applied the fix now, and added an assert:

Code: Select all

// allocate handles buffer and put all handles on free list
m_pHandlesRawPtr = btAlignedAlloc(sizeof(Handle)*maxHandles,16);
m_pHandles = new(m_pHandlesRawPtr) Handle[maxHandles];
btAssert(m_pHandlesRawPtr==m_pHandles); //placement new[] should just return the same pointer

Also would it be more consitent to have these operators overloaded for the entire bullet libraries, rather than just a single class? or am I missing something?

We only use 16-byte alignment for structs/classes that really need it, using BT_DECLARE_ALIGNED_ALLOCATOR or ATTRIBUTE_ALIGNED16(class).
It would hurt performance and wastes memory if we aligned every single structure.

Thanks for the feedback,
Erwin

By the way, those fixes should have been reported in the issue tracker at google code. Then they wouldn't have gone lost.

[russellbartley]

I've posted a reply on the google code tracker issue. regarding the use of placement new array.

[Erwin Coumans]

Please keep discussion on this forum. Only the actual bug report and patches should go into the issue tracker.

I still think the placement new array operator can cause problems.
I'd prefer a plain new array, and corresponding delete.

The wii compiler I'm using needs an extra 16 bytes when doing a placement new array,
which is not in the btAlignedAlloc, and as far as I can tell cannot be worked out
before doing the alloc.

Ok, agreed. The code has been changed into:

Code: Select all

template <typename BP_FP_INT_TYPE>
btAxisSweep3Internal<BP_FP_INT_TYPE>::btAxisSweep3Internal(const btPoint3& worldAabbMin,const btPoint3& worldAabbMax, BP_FP_INT_TYPE handleMask, BP_FP_INT_TYPE handleSentinel,BP_FP_INT_TYPE userMaxHandles, btOverlappingPairCache* pairCache )
...

	// allocate handles buffer, using btAlignedAlloc, and put all handles on free list
	m_pHandles = new Handle[maxHandles];
...
}

template <typename BP_FP_INT_TYPE>
btAxisSweep3Internal<BP_FP_INT_TYPE>::~btAxisSweep3Internal()
{
...
	delete [] m_pHandles;
}

The main point was that btAlignedAlloc/btAlignedFree is used. As a rule, all memory allocations inside the Bullet SDK (src folder) should go through the btAlignedAlloc / btAlignedFree functions. This way, the developer can easily hook up his own custom allocator.

Are you happy with this solution?

Thanks,
Erwin

[russellbartley]

Yeah thats great thanks Erwin.