Change in osmo-trx[master]: SigProcLib: Improve Vector buffer allocation mess

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Harald Welte gerrit-no-reply at lists.osmocom.org
Wed Dec 5 19:41:34 UTC 2018


Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/12085 )

Change subject: SigProcLib: Improve Vector buffer allocation mess
......................................................................

SigProcLib: Improve Vector buffer allocation mess

Original issue: In order to use SSE instructions, 16-byte aligned memory
chunks are needed, and C++ version < C++11 doesn't provide for a native
new/delete store. For that reason, memalign() must be used in the
implementation of convolve_h_alloc() for some buffers.
On the other side, The C++ code relies on C++ "new T[]" operator to
allocate a chunk of memory containing an array of class instances. As
classes are complex types, they cannot be allocated through C structures
(calling malloc). Experimentally can be seen too that it's unreliable
and the process will crash during startup if malloc() is used and then a
Complex<> deferred from it.

Previous implementation allowed for use of convolve_h_alloc or new[]
based on how the (signal)Vector is called, because then the buffer is
not going to be managed internally. But that's unreliable since resize()
calling resize() on it could use "delete" operator on a malloc'ed
buffer, and end up having a new new[] allocated buffer. It was also
found that some of the callers were actually leaking memory through ASan (because the
buffer is not managed by the Vector instance).

IMHO best option would be to rewrite all this code using C structures
and malloc/free exclusively, since it would make all this cod eeasier to
maintain.

But for now, let's extend the Vector class to allow specifying an
external alloc/free function and let the Vector instance take care of
the ownership of the buffer in all scenarios.

Change-Id: Ie484a4762a7f77fe1b105188ea03a6f025730b82
---
M CommonLibs/Vector.h
M Transceiver52M/arch/common/convolve.h
M Transceiver52M/arch/common/convolve_base.c
M Transceiver52M/sigProcLib.cpp
M Transceiver52M/signalVector.cpp
M Transceiver52M/signalVector.h
6 files changed, 52 insertions(+), 47 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/CommonLibs/Vector.h b/CommonLibs/Vector.h
index 9119683..4c96b78 100644
--- a/CommonLibs/Vector.h
+++ b/CommonLibs/Vector.h
@@ -32,11 +32,14 @@
 #include <string.h>
 #include <iostream>
 #include <assert.h>
+#include <stdlib.h>
+
 // We cant use Logger.h in this file...
 extern int gVectorDebug;
 #define BVDEBUG(msg) if (gVectorDebug) {std::cout << msg;}
 
-
+typedef void (*vector_free_func)(void* wData);
+typedef void *(*vector_alloc_func)(size_t newSize);
 
 /**
 	A simplified Vector template with aliases.
@@ -60,6 +63,8 @@
 	T* mData;		///< allocated data block, if any
 	T* mStart;		///< start of useful data
 	T* mEnd;		///< end of useful data + 1
+	vector_alloc_func mAllocFunc; ///< function used to alloc new mData during resize.
+	vector_free_func mFreeFunc; ///< function used to free mData.
 
 	public:
 
@@ -85,9 +90,19 @@
 	/** Change the size of the Vector, discarding content. */
 	void resize(size_t newSize)
 	{
-		if (mData!=NULL) delete[] mData;
+		if (mData!=NULL) {
+			if (mFreeFunc)
+				mFreeFunc(mData);
+			else
+				delete[] mData;
+		}
 		if (newSize==0) mData=NULL;
-		else mData = new T[newSize];
+		else {
+			if (mAllocFunc)
+				mData = (T*) mAllocFunc(newSize);
+			else
+				mData = new T[newSize];
+		}
 		mStart = mData;
 		mEnd = mStart + newSize;
 	}
@@ -116,29 +131,31 @@
 	//@{
 
 	/** Build an empty Vector of a given size. */
-	Vector(size_t wSize=0):mData(NULL) { resize(wSize); }
+	Vector(size_t wSize=0, vector_alloc_func wAllocFunc=NULL, vector_free_func wFreeFunc=NULL)
+		:mData(NULL), mAllocFunc(wAllocFunc), mFreeFunc(wFreeFunc)
+	{ resize(wSize); }
 
 	/** Build a Vector by moving another. */
 	Vector(Vector<T>&& other)
-		:mData(other.mData),mStart(other.mStart),mEnd(other.mEnd)
+		:mData(other.mData),mStart(other.mStart),mEnd(other.mEnd), mAllocFunc(other.mAllocFunc),  mFreeFunc(other.mFreeFunc)
 	{ other.mData=NULL; }
 
 	/** Build a Vector by copying another. */
-	Vector(const Vector<T>& other):mData(NULL) { clone(other); }
+	Vector(const Vector<T>& other):mData(NULL), mAllocFunc(other.mAllocFunc), mFreeFunc(other.mFreeFunc) { clone(other); }
 
 	/** Build a Vector with explicit values. */
-	Vector(T* wData, T* wStart, T* wEnd)
-		:mData(wData),mStart(wStart),mEnd(wEnd)
+	Vector(T* wData, T* wStart, T* wEnd, vector_alloc_func wAllocFunc=NULL, vector_free_func wFreeFunc=NULL)
+		:mData(wData),mStart(wStart),mEnd(wEnd), mAllocFunc(wAllocFunc), mFreeFunc(wFreeFunc)
 	{ }
 
 	/** Build a vector from an existing block, NOT to be deleted upon destruction. */
-	Vector(T* wStart, size_t span)
-		:mData(NULL),mStart(wStart),mEnd(wStart+span)
+	Vector(T* wStart, size_t span, vector_alloc_func wAllocFunc=NULL, vector_free_func wFreeFunc=NULL)
+		:mData(NULL),mStart(wStart),mEnd(wStart+span),mAllocFunc(wAllocFunc), mFreeFunc(wFreeFunc)
 	{ }
 
 	/** Build a Vector by concatenation. */
-	Vector(const Vector<T>& other1, const Vector<T>& other2)
-		:mData(NULL)
+	Vector(const Vector<T>& other1, const Vector<T>& other2, vector_alloc_func wAllocFunc=NULL, vector_free_func wFreeFunc=NULL)
+		:mData(NULL), mAllocFunc(wAllocFunc), mFreeFunc(wFreeFunc)
 	{
 		resize(other1.size()+other2.size());
 		memcpy(mStart, other1.mStart, other1.bytes());
@@ -162,6 +179,8 @@
 		mData=other.mData;
 		mStart=other.mStart;
 		mEnd=other.mEnd;
+		mAllocFunc=other.mAllocFunc;
+		mFreeFunc=other.mFreeFunc;
 		other.mData=NULL;
 	}
 
diff --git a/Transceiver52M/arch/common/convolve.h b/Transceiver52M/arch/common/convolve.h
index 43db577..095b04c 100644
--- a/Transceiver52M/arch/common/convolve.h
+++ b/Transceiver52M/arch/common/convolve.h
@@ -1,7 +1,7 @@
 #ifndef _CONVOLVE_H_
 #define _CONVOLVE_H_
 
-void *convolve_h_alloc(int num);
+void *convolve_h_alloc(size_t num);
 
 int convolve_real(const float *x, int x_len,
 		  const float *h, int h_len,
diff --git a/Transceiver52M/arch/common/convolve_base.c b/Transceiver52M/arch/common/convolve_base.c
index 71453a1..2eb7124 100644
--- a/Transceiver52M/arch/common/convolve_base.c
+++ b/Transceiver52M/arch/common/convolve_base.c
@@ -146,7 +146,7 @@
 }
 
 /* Aligned filter tap allocation */
-void *convolve_h_alloc(int len)
+void *convolve_h_alloc(size_t len)
 {
 #ifdef HAVE_SSE3
 	return memalign(16, len * 2 * sizeof(float));
diff --git a/Transceiver52M/sigProcLib.cpp b/Transceiver52M/sigProcLib.cpp
index 28c4ded..f720828 100644
--- a/Transceiver52M/sigProcLib.cpp
+++ b/Transceiver52M/sigProcLib.cpp
@@ -84,14 +84,13 @@
  * perform 16-byte memory alignment required by many SSE instructions.
  */
 struct CorrelationSequence {
-  CorrelationSequence() : sequence(NULL), buffer(NULL)
+  CorrelationSequence() : sequence(NULL)
   {
   }
 
   ~CorrelationSequence()
   {
     delete sequence;
-    free(buffer);
   }
 
   signalVector *sequence;
@@ -106,8 +105,7 @@
  * for SSE instructions.
  */
 struct PulseSequence {
-  PulseSequence() : c0(NULL), c1(NULL), c0_inv(NULL), empty(NULL),
-		    c0_buffer(NULL), c1_buffer(NULL), c0_inv_buffer(NULL)
+  PulseSequence() : c0(NULL), c1(NULL), c0_inv(NULL), empty(NULL)
   {
   }
 
@@ -117,17 +115,12 @@
     delete c1;
     delete c0_inv;
     delete empty;
-    free(c0_buffer);
-    free(c1_buffer);
   }
 
   signalVector *c0;
   signalVector *c1;
   signalVector *c0_inv;
   signalVector *empty;
-  void *c0_buffer;
-  void *c1_buffer;
-  void *c0_inv_buffer;
 };
 
 static CorrelationSequence *gMidambles[] = {NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL};
@@ -340,7 +333,7 @@
   if (y && (len > y->size()))
     return NULL;
   if (!y) {
-    y = new signalVector(len);
+    y = new signalVector(len, convolve_h_alloc, free);
     alloc = true;
   }
 
@@ -403,8 +396,7 @@
   if (!pulse)
     return false;
 
-  pulse->c0_inv_buffer = convolve_h_alloc(5);
-  pulse->c0_inv = new signalVector((complex *) pulse->c0_inv_buffer, 0, 5);
+  pulse->c0_inv = new signalVector((complex *) convolve_h_alloc(5), 0, 5, convolve_h_alloc, free);
   pulse->c0_inv->isReal(true);
   pulse->c0_inv->setAligned(false);
 
@@ -433,9 +425,7 @@
     return false;
   }
 
-  pulse->c1_buffer = convolve_h_alloc(len);
-  pulse->c1 = new signalVector((complex *)
-                                  pulse->c1_buffer, 0, len);
+  pulse->c1 = new signalVector((complex *) convolve_h_alloc(len), 0, len, convolve_h_alloc, free);
   pulse->c1->isReal(true);
 
   /* Enable alignment for SSE usage */
@@ -489,8 +479,7 @@
     len = 4;
   }
 
-  pulse->c0_buffer = convolve_h_alloc(len);
-  pulse->c0 = new signalVector((complex *) pulse->c0_buffer, 0, len);
+  pulse->c0 = new signalVector((complex *) convolve_h_alloc(len), 0, len, convolve_h_alloc, free);
   pulse->c0->isReal(true);
 
   /* Enable alingnment for SSE usage */
@@ -1019,7 +1008,7 @@
 
   for (int i = 0; i < DELAYFILTS; i++) {
     data = (complex *) convolve_h_alloc(h_len);
-    h = new signalVector(data, 0, h_len);
+    h = new signalVector(data, 0, h_len, convolve_h_alloc, free);
     h->setAligned(true);
     h->isReal(true);
 
@@ -1263,7 +1252,7 @@
 
   /* For SSE alignment, reallocate the midamble sequence on 16-byte boundary */
   data = (complex *) convolve_h_alloc(midMidamble->size());
-  _midMidamble = new signalVector(data, 0, midMidamble->size());
+  _midMidamble = new signalVector(data, 0, midMidamble->size(), convolve_h_alloc, free);
   _midMidamble->setAligned(true);
   midMidamble->copyTo(*_midMidamble);
 
@@ -1274,7 +1263,6 @@
   }
 
   gMidambles[tsc] = new CorrelationSequence;
-  gMidambles[tsc]->buffer = data;
   gMidambles[tsc]->sequence = _midMidamble;
   gMidambles[tsc]->gain = peakDetect(*autocorr, &toa, NULL);
 
@@ -1319,13 +1307,12 @@
   conjugateVector(*midamble);
 
   data = (complex *) convolve_h_alloc(midamble->size());
-  _midamble = new signalVector(data, 0, midamble->size());
+  _midamble = new signalVector(data, 0, midamble->size(), convolve_h_alloc, free);
   _midamble->setAligned(true);
   midamble->copyTo(*_midamble);
 
   /* Channel gain is an empirically measured value */
   seq = new CorrelationSequence;
-  seq->buffer = data;
   seq->sequence = _midamble;
   seq->gain = Complex<float>(-19.6432, 19.5006) / 1.18;
   seq->toa = 0;
@@ -1360,7 +1347,7 @@
 
   /* For SSE alignment, reallocate the midamble sequence on 16-byte boundary */
   data = (complex *) convolve_h_alloc(seq1->size());
-  _seq1 = new signalVector(data, 0, seq1->size());
+  _seq1 = new signalVector(data, 0, seq1->size(), convolve_h_alloc, free);
   _seq1->setAligned(true);
   seq1->copyTo(*_seq1);
 
@@ -1372,7 +1359,6 @@
 
   *seq = new CorrelationSequence;
   (*seq)->sequence = _seq1;
-  (*seq)->buffer = data;
   (*seq)->gain = peakDetect(*autocorr, &toa, NULL);
 
   /* For 1 sps only
diff --git a/Transceiver52M/signalVector.cpp b/Transceiver52M/signalVector.cpp
index fc8157e..710eda5 100644
--- a/Transceiver52M/signalVector.cpp
+++ b/Transceiver52M/signalVector.cpp
@@ -1,20 +1,20 @@
 #include "signalVector.h"
 
-signalVector::signalVector(size_t size)
-	: Vector<complex>(size),
+signalVector::signalVector(size_t size, vector_alloc_func wAllocFunc, vector_free_func wFreeFunc)
+	: Vector<complex>(size, wAllocFunc, wFreeFunc),
 	  real(false), aligned(false), symmetry(NONE)
 {
 }
 
-signalVector::signalVector(size_t size, size_t start)
-	: Vector<complex>(size + start),
+signalVector::signalVector(size_t size, size_t start, vector_alloc_func wAllocFunc, vector_free_func wFreeFunc)
+	: Vector<complex>(size + start, wAllocFunc, wFreeFunc),
 	  real(false), aligned(false), symmetry(NONE)
 {
 	mStart = mData + start;
 }
 
-signalVector::signalVector(complex *data, size_t start, size_t span)
-	: Vector<complex>(NULL, data + start, data + start + span),
+signalVector::signalVector(complex *data, size_t start, size_t span, vector_alloc_func wAllocFunc, vector_free_func wFreeFunc)
+	: Vector<complex>(data, data + start, data + start + span, wAllocFunc, wFreeFunc),
 	  real(false), aligned(false), symmetry(NONE)
 {
 }
diff --git a/Transceiver52M/signalVector.h b/Transceiver52M/signalVector.h
index 83f141e..d9486af 100644
--- a/Transceiver52M/signalVector.h
+++ b/Transceiver52M/signalVector.h
@@ -13,13 +13,13 @@
 class signalVector: public Vector<complex> {
 public:
 	/** Default constructor */
-	signalVector(size_t size = 0);
+	signalVector(size_t size = 0, vector_alloc_func wAllocFunc = NULL, vector_free_func wFreeFunc = NULL);
 
 	/** Construct with head room */
-	signalVector(size_t size, size_t start);
+	signalVector(size_t size, size_t start, vector_alloc_func wAllocFunc = NULL, vector_free_func wFreeFunc = NULL);
 
 	/** Construct from existing buffer data (buffer not managed) */
-	signalVector(complex *data, size_t start, size_t span);
+	signalVector(complex *data, size_t start, size_t span, vector_alloc_func wAllocFunc = NULL, vector_free_func wFreeFunc = NULL);
 
 	/** Construct by from existing vector */
 	signalVector(const signalVector &vector);

-- 
To view, visit https://gerrit.osmocom.org/12085
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie484a4762a7f77fe1b105188ea03a6f025730b82
Gerrit-Change-Number: 12085
Gerrit-PatchSet: 3
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181205/92c23e75/attachment.htm>


More information about the gerrit-log mailing list