Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "out of buffers" crash #3783

Merged
merged 9 commits into from
Sep 26, 2017
+60 −116
Merged

Fix "out of buffers" crash #3783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b6421d4
Reimplement BufferManager
lukas-w Aug 29, 2017
6b557d2
Find Boost in CMake, fix Travis
lukas-w Aug 29, 2017
c11d12f
Remove BufferManager implementation
lukas-w Aug 30, 2017
9c91079
Revert "Find Boost in CMake, fix Travis"
lukas-w Aug 30, 2017
73728d7
Fix C++03 compatibility
lukas-w Aug 30, 2017
aebbe22
Fix code formatting inconsistencies
lukas-w Sep 17, 2017
70ac7db
Fix code formatting inconsistencies
lukas-w Sep 17, 2017
d1aa39b
Fix C++03 compatibility again
lukas-w Sep 17, 2017
93e4bd7
Fix conventions
PhysSong Sep 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/MemoryHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
class MemoryHelper {
public:

static void* alignedMalloc(size_t );
static void* alignedMalloc( size_t );

static void alignedFree( void* );

Expand Down
10 changes: 5 additions & 5 deletions include/MemoryManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ struct MemoryPool
MemoryPool( size_t chunks ) :
m_chunks( chunks )
{
m_free = reinterpret_cast<char*>(MemoryHelper::alignedMalloc( chunks ));
m_free = reinterpret_cast<char*>( MemoryHelper::alignedMalloc( chunks ) );
memset( m_free, 1, chunks );
}

Expand Down Expand Up @@ -109,17 +109,17 @@ struct MmAllocator
typedef T value_type;
template< class U > struct rebind { typedef MmAllocator<U> other; };

T* allocate(std::size_t n)
T* allocate( std::size_t n )
{
return reinterpret_cast<T*>( MemoryManager::alloc( sizeof(T) * n ) );
}

void deallocate(T* p, std::size_t)
void deallocate( T* p, std::size_t )
{
MemoryManager::free( p );
}

typedef std::vector<T, MmAllocator<T> > vector;
typedef std::vector<T, MmAllocator<T>> vector;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the space here was removed again, have an error while compiling.

error: ‘>>’ should be ‘> >’ within a nested template argument list

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, should have remembered from 73728d7

};

Copy link
Member

@PhysSong PhysSong Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parts aren't used. Are there some reasons you leave them in our codebase?
Some more inconsistencies: you mixed < foo > and <foo>, it makes the code look inconsistent.
One more thing(minor): if you are going to use this later, I think vector is not very good name. If you want to keep the name, of course you may do.

Copy link
Member Author

@lukas-w lukas-w Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there some reasons you leave them in our codebase?

I plan on using them at a later point in time. Using an Allocator class is the standard C++ way of having custom allocators, and should be preferred over using the macros MM_ALLOC and MM_FREE.

you mixed < foo > and , it makes the code look inconsistent.

This has already been discussed by @Umcaruje and me in the comments here. C++03 doesn't allow >> for nested template arguments (it always interprets it as the stream right shift operator >>). We can't use >> here, because the headers is included by plugins that are compiled without C++11 support. See d1aa39b.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are going to use this later, I think vector is not very good name

Well it's a vector, so why not call it vector? What would you call it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but you used both template<typename T> and template< class U > above. Is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, no that's not intentional.


Expand All @@ -143,7 +143,7 @@ static void operator delete[] ( void * ptr ) \
}

// for use in cases where overriding new/delete isn't a possibility
#define MM_ALLOC( type, count ) reinterpret_cast<type*>(MemoryManager::alloc( sizeof( type ) * count ))
#define MM_ALLOC( type, count ) reinterpret_cast<type*>( MemoryManager::alloc( sizeof( type ) * count ) )
// and just for symmetry...
#define MM_FREE( ptr ) MemoryManager::free( ptr )

Expand Down
4 changes: 2 additions & 2 deletions src/core/BufferManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ void BufferManager::init( fpp_t framesPerPeriod )

sampleFrame * BufferManager::acquire()
{
return MM_ALLOC(sampleFrame, ::framesPerPeriod);
return MM_ALLOC( sampleFrame, ::framesPerPeriod );
}

void BufferManager::clear(sampleFrame *ab, const f_cnt_t frames, const f_cnt_t offset)
void BufferManager::clear( sampleFrame *ab, const f_cnt_t frames, const f_cnt_t offset )
{
memset( ab + offset, 0, sizeof( *ab ) * frames );
}
Expand Down