From abc5c5a3743245ea49e26ed468112ea1215bc081 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Wed, 9 Sep 2015 19:13:27 -0400 Subject: [PATCH 1/5] new Thread::Config --- src/misc/thread.h | 185 ++++++++++++++++++++++++++++++++++++ testApp/misc/testThread.cpp | 113 +++++++++++++++++++--- 2 files changed, 287 insertions(+), 11 deletions(-) diff --git a/src/misc/thread.h b/src/misc/thread.h index 2c8e492..0f2dc94 100644 --- a/src/misc/thread.h +++ b/src/misc/thread.h @@ -11,6 +11,11 @@ #define THREAD_H #include +#include + +#if __cplusplus>=201103L +#include +#endif #ifdef epicsExportSharedSymbols #define threadepicsExportSharedSymbols @@ -47,12 +52,175 @@ typedef std::tr1::shared_ptr EpicsThreadPtr; typedef epicsThreadRunable Runnable; +//! Helper for those cases where a class should have more than one runnable +template +class epicsShareClass RunnableMethod : public Runnable, private NoDefaultMethods +{ + typedef void (C::*meth_t)(); + C *inst; + meth_t meth; + + virtual void run() + { + (inst->*meth)(); + } +public: + RunnableMethod(C* inst, void (C::*meth)()) + :inst(inst), meth(meth) + {} +}; + +namespace detail { +struct FuncRunner : public epicsThreadRunable +{ + typedef void (*fn_t)(void*); + fn_t fn; + void *arg; + FuncRunner(fn_t f, void *a) :fn(f), arg(a) {} + virtual ~FuncRunner(){} + virtual void run() + { + (*fn)(arg); + } +}; +template +struct MethRunner : public epicsThreadRunable +{ + typedef void(C::*fn_t)(); + fn_t fn; + C* inst; + MethRunner(C* i, fn_t f) :fn(f), inst(i) {} + virtual ~MethRunner() {} + virtual void run() + { + (inst->*fn)(); + } +}; +#if __cplusplus>=201103L +struct BindRunner : public epicsThreadRunable +{ + typedef std::function fn_t; + fn_t fn; + BindRunner(const fn_t f) : fn(f) {} + virtual ~BindRunner() {} + virtual void run() + { + fn(); + } +}; +#endif +} // namespace detail + /** * @brief C++ wrapper for epicsThread from EPICS base. * */ class epicsShareClass Thread : public epicsThread, private NoDefaultMethods { public: + /** @brief Holds all the configuration necessary to launch a @class Thread + * + * The defaults may be used except for the runnable, which must be given + * either in the constructor, or the @method run() method. + * + * @note Instances of @class Config may not be reused. + * + * Defaults: + * name: "" + * priority: epicsThreadPriorityLow (aka epics::pvData::lowestPriority) + * stack size: epicsThreadStackSmall + * auto start: true + * runner: nil (must be set explictly) + * + @code + stuct bar { void meth(); ... } X; + // with a static thread name + Thread foo(Thread::Config(&X, &bar::meth) + .name("example") + .prio(epicsThreadPriorityHigh)); + + // with a constructed thread name + Thread foo(Thread::Config(&X, &bar::meth) + .prio(epicsThreadPriorityHigh) + <<"example"<<1); + @endcode + */ + class epicsShareClass Config + { + unsigned int p_prio, p_stack; + std::ostringstream p_strm; + bool p_autostart; + Runnable *p_runner; +#if __cplusplus>=201103L + typedef std::unique_ptr p_owned_runner_t; +#else + typedef std::auto_ptr p_owned_runner_t; +#endif + p_owned_runner_t p_owned_runner; + friend class Thread; + Runnable& x_getrunner() + { + if(!this->p_runner) + throw std::logic_error("Thread::Config missing run()"); + return *this->p_runner; + } + void x_setdefault() + { + this->p_prio = epicsThreadPriorityLow; + this->p_autostart = true; + this->p_runner = NULL; + (*this).stack(epicsThreadStackSmall); + } + + public: + Config() {this->x_setdefault();} + Config(Runnable *r) {this->x_setdefault();this->run(r);} + Config(void(*fn)(void*), void *ptr) {this->x_setdefault();this->run(fn, ptr);} + template + Config(C* inst, void(C::*meth)()) {this->x_setdefault();this->run(inst, meth);} +#if __cplusplus>=201103L + Config(const std::function& fn) {this->x_setdefault();this->run(fn);} +#endif + + inline Config& name(const std::string& n) + { this->p_strm.str(n); return *this; } + inline Config& prio(unsigned int p) + { this->p_prio = p; return *this; } + inline Config& stack(epicsThreadStackSizeClass s) + { this->p_stack = epicsThreadGetStackSize(s); return *this; } + inline Config& autostart(bool a) + { this->p_autostart = a; return *this; } + + //! Thread will execute Runnable::run() + Config& run(Runnable* r) + { this->p_runner = r; return *this; } + //! Thread will execute (*fn)(ptr) + Config& run(void(*fn)(void*), void *ptr) + { + this->p_owned_runner.reset(new detail::FuncRunner(fn, ptr)); + this->p_runner = this->p_owned_runner.get(); + return *this; + } + //! Thread will execute (inst->*meth)() + template + Config& run(C* inst, void(C::*meth)()) + { + this->p_owned_runner.reset(new detail::MethRunner(inst, meth)); + this->p_runner = this->p_owned_runner.get(); + return *this; + } +#if __cplusplus>=201103L + Config& run(const std::function& fn) + { + this->p_owned_runner.reset(new detail::BindRunner(fn)); + this->p_runner = this->p_owned_runner.get(); + return *this; + } +#endif + + //! Append to thread name string. Argument must be understood by std::ostream::operator<< + template + Config& operator<<(T x) { this->p_strm<start(); } + //! @brief Create a new thread using the given @class Config + //! @throws std::logic_error for improper @class Config (ie. missing runner) + Thread(Config& c) + :epicsThread(c.x_getrunner(), c.p_strm.str().c_str(), + c.p_stack, c.p_prio) + { +#if __cplusplus>=201103L + p_owned = std::move(c.p_owned_runner); +#else + p_owned = c.p_owned_runner; +#endif + if(c.p_autostart) + this->start(); + } + /** * Destructor */ @@ -115,6 +298,8 @@ public: { this->exitWait(); } + + Config::p_owned_runner_t p_owned; }; diff --git a/testApp/misc/testThread.cpp b/testApp/misc/testThread.cpp index 189149f..aee569e 100644 --- a/testApp/misc/testThread.cpp +++ b/testApp/misc/testThread.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -42,11 +43,11 @@ public: Event begin, end; Action(): actuallyRan(false) {} virtual void run() { - printf("Action waiting\n"); + testDiag("Action waiting"); begin.signal(); bool waited=end.wait(); actuallyRan=true; - printf("Action %s\n", waited?"true":"false"); + testDiag("Action %s", waited?"true":"false"); } }; @@ -56,13 +57,13 @@ static void testThreadRun() { { ThreadPtr tr(new Thread(actionName,lowPriority,ax.get())); bool w=ax->begin.wait(); - printf( "main %s\n", w?"true":"false"); - printf( "Action is %s\n", ax->actuallyRan?"true":"false"); + testDiag( "main %s", w?"true":"false"); + testDiag( "Action is %s", ax->actuallyRan?"true":"false"); ax->end.signal(); } testOk1(ax->actuallyRan==true); - printf( "Action is %s\n", ax->actuallyRan?"true":"false"); - printf("testThreadRun PASSED\n"); + testDiag( "Action is %s", ax->actuallyRan?"true":"false"); + testDiag("testThreadRun PASSED"); } class Basic; @@ -84,7 +85,7 @@ public: executor->execute(getPtrSelf()); bool result = wait.wait(); testOk1(result==true); - if(result==false) printf("basic::run wait returned false\n"); + if(result==false) testDiag("basic::run wait returned false"); } virtual void command() { @@ -105,7 +106,96 @@ static void testBasic() { ExecutorPtr executor(new Executor(string("basic"),middlePriority)); BasicPtr basic( new Basic(executor)); basic->run(); - printf("testBasic PASSED\n"); + testDiag("testBasic PASSED"); +} + +namespace { +struct fninfo { + int cnt; + epicsEvent evnt; +}; + +static void threadFN(void *raw) +{ + fninfo *arg = (fninfo*)raw; + arg->evnt.signal(); + arg->cnt++; +} + +struct classMeth { + int cnt; + epicsEvent evnt; + classMeth() :cnt(0) {} + void inc() { + const char *tname = epicsThreadGetNameSelf(); + testOk(strcmp(tname, "test2")==0, "thread name '%s' == 'test2' ", tname); + evnt.signal(); + cnt++; + } +}; +} + +static void testBinders() +{ + testDiag("Testing thread bindables"); + + testDiag("C style function"); + { + fninfo info; + info.cnt = 0; + Thread foo(Thread::Config(&threadFN, (void*)&info) + .name("test1") + .prio(epicsThreadPriorityMedium) + .autostart(true) + ); + + info.evnt.wait(); + + foo.exitWait(); + + testOk(info.cnt==1, "cnt (%d) == 1", info.cnt); + } + + testDiag("class method"); + { + classMeth inst; + + Thread foo(Thread::Config(&inst, &classMeth::inc) + .prio(epicsThreadPriorityMedium) + .autostart(false) + <<"test"<<2 + ); + + epicsThreadSleep(0.1); + + testOk(inst.cnt==0, "inst.cnt (%d) == 0", inst.cnt); + + foo.start(); + inst.evnt.wait(); + foo.exitWait(); + + testOk(inst.cnt==1, "inst.cnt (%d) == 1", inst.cnt); + } + + testDiag("C++11 style lambda"); +#if __cplusplus>=201103L + { + int cnt = 0; + epicsEvent evnt; + auto fn = [&cnt,&evnt]() mutable {evnt.signal(); cnt++;}; + Thread foo(Thread::Config(fn) + .name("test3") + .prio(epicsThreadPriorityMedium) + .autostart(true) + ); + evnt.wait(); + foo.exitWait(); + + testOk(cnt==1, "cnt (%d) == 1", cnt); + } +#else + testSkip(1, "Not built as C++11"); +#endif } class MyFunc : public TimeFunctionRequester { @@ -137,17 +227,18 @@ static void testThreadContext() { TimeFunctionPtr timeFunction(new TimeFunction(myFunc)); double perCall = timeFunction->timeCall(); perCall *= 1e6; - printf("time per call %f microseconds\n",perCall); - printf("testThreadContext PASSED\n"); + testDiag("time per call %f microseconds",perCall); + testDiag("testThreadContext PASSED"); } #endif MAIN(testThread) { - testPlan(2); + testPlan(7); testDiag("Tests thread"); testThreadRun(); testBasic(); + testBinders(); #ifdef TESTTHREADCONTEXT testThreadContext(); #endif From 393d711e5f1ae9d3a5cc41d380a4186ad90ed818 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 19 Nov 2015 10:45:30 -0600 Subject: [PATCH 2/5] byteBuffer check for alloc failure and const Ensure that bad_alloc is thrown if allocations fail, presently unchecked. Also add const qualifier where possible. --- src/misc/byteBuffer.h | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/misc/byteBuffer.h b/src/misc/byteBuffer.h index 2f1013b..db3e4df 100644 --- a/src/misc/byteBuffer.h +++ b/src/misc/byteBuffer.h @@ -224,19 +224,20 @@ public: * Must be one of EPICS_BYTE_ORDER,EPICS_ENDIAN_LITTLE,EPICS_ENDIAN_BIG. */ ByteBuffer(std::size_t size, int byteOrder = EPICS_BYTE_ORDER) : - _buffer(0), _size(size), + _buffer((char*)malloc(size)), _size(size), _reverseEndianess(byteOrder != EPICS_BYTE_ORDER), _reverseFloatEndianess(byteOrder != EPICS_FLOAT_WORD_ORDER), _wrapped(false) { - _buffer = (char*)malloc(size); + if(!_buffer) + throw std::bad_alloc(); clear(); } /** * Constructor for wrapping existing buffers. * Given buffer will not be released by the ByteBuffer instance. - * @param buffer Existing buffer. + * @param buffer Existing buffer. (will be free'd with free()) * @param size The number of bytes. * @param byteOrder The byte order. * Must be one of EPICS_BYTE_ORDER,EPICS_ENDIAN_LITTLE,EPICS_ENDIAN_BIG. @@ -247,6 +248,8 @@ public: _reverseFloatEndianess(byteOrder != EPICS_FLOAT_WORD_ORDER), _wrapped(true) { + if(!_buffer) + throw std::bad_alloc(); clear(); } /** @@ -271,7 +274,7 @@ public: * Get the raw buffer data. * @return the raw buffer data. */ - inline const char* getBuffer() + inline const char* getBuffer() const { return _buffer; } @@ -303,7 +306,7 @@ public: * Returns the current position. * @return The current position in the raw data. */ - inline std::size_t getPosition() + inline std::size_t getPosition() const { return (std::size_t)(((std::ptrdiff_t)(const void *)_position) - ((std::ptrdiff_t)(const void *)_buffer)); } @@ -323,7 +326,7 @@ public: * * @return The offset into the raw buffer. */ - inline std::size_t getLimit() + inline std::size_t getLimit() const { return (std::size_t)(((std::ptrdiff_t)(const void *)_limit) - ((std::ptrdiff_t)(const void *)_buffer)); } @@ -344,7 +347,7 @@ public: * * @return The number of elements remaining in this buffer. */ - inline std::size_t getRemaining() + inline std::size_t getRemaining() const { return (std::size_t)(((std::ptrdiff_t)(const void *)_limit) - ((std::ptrdiff_t)(const void *)_position)); } @@ -353,7 +356,7 @@ public: * * @return The size of the raw data buffer. */ - inline std::size_t getSize() + inline std::size_t getSize() const { return _size; } @@ -444,7 +447,7 @@ public: * @return (false,true) if (is, is not) the EPICS_BYTE_ORDER */ template - inline bool reverse() + inline bool reverse() const { return _reverseEndianess; } @@ -643,48 +646,48 @@ public: inline double getDouble (std::size_t index) { return get(index); } // TODO remove - inline const char* getArray() + inline const char* getArray() const { return _buffer; } private: - char* _buffer; + char* const _buffer; char* _position; char* _limit; - std::size_t _size; + const std::size_t _size; bool _reverseEndianess; bool _reverseFloatEndianess; - bool _wrapped; + const bool _wrapped; }; template<> - inline bool ByteBuffer::reverse() + inline bool ByteBuffer::reverse() const { return false; } template<> - inline bool ByteBuffer::reverse() + inline bool ByteBuffer::reverse() const { return false; } template<> - inline bool ByteBuffer::reverse() + inline bool ByteBuffer::reverse() const { return false; } template<> - inline bool ByteBuffer::reverse() + inline bool ByteBuffer::reverse() const { return _reverseFloatEndianess; } template<> - inline bool ByteBuffer::reverse() + inline bool ByteBuffer::reverse() const { return _reverseFloatEndianess; } From f0c88234a0547f9f334a7b301d2b6fcc553ffded Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Tue, 15 Dec 2015 10:26:31 -0500 Subject: [PATCH 3/5] revert incorrect doc string --- src/misc/byteBuffer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/misc/byteBuffer.h b/src/misc/byteBuffer.h index db3e4df..266eee5 100644 --- a/src/misc/byteBuffer.h +++ b/src/misc/byteBuffer.h @@ -235,9 +235,9 @@ public: } /** - * Constructor for wrapping existing buffers. + * Constructor for wrapping an existing buffer. * Given buffer will not be released by the ByteBuffer instance. - * @param buffer Existing buffer. (will be free'd with free()) + * @param buffer Existing buffer. * @param size The number of bytes. * @param byteOrder The byte order. * Must be one of EPICS_BYTE_ORDER,EPICS_ENDIAN_LITTLE,EPICS_ENDIAN_BIG. From 433676226ced888f9ae8eb3f08aacf54dcc7209d Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Thu, 17 Dec 2015 16:45:56 -0500 Subject: [PATCH 4/5] byteBuffer throw invalid_argument when ctor w/ NULL --- src/misc/byteBuffer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/misc/byteBuffer.h b/src/misc/byteBuffer.h index 266eee5..945c80b 100644 --- a/src/misc/byteBuffer.h +++ b/src/misc/byteBuffer.h @@ -237,7 +237,7 @@ public: /** * Constructor for wrapping an existing buffer. * Given buffer will not be released by the ByteBuffer instance. - * @param buffer Existing buffer. + * @param buffer Existing buffer. May not be NULL. * @param size The number of bytes. * @param byteOrder The byte order. * Must be one of EPICS_BYTE_ORDER,EPICS_ENDIAN_LITTLE,EPICS_ENDIAN_BIG. @@ -249,7 +249,7 @@ public: _wrapped(true) { if(!_buffer) - throw std::bad_alloc(); + throw std::invalid_argument("ByteBuffer can't be constructed with NULL"); clear(); } /** From 14b0e409f21a7a0cda417734dd2ceab3d6c0e4a2 Mon Sep 17 00:00:00 2001 From: Michael Davidsaver Date: Mon, 28 Dec 2015 18:16:01 -0500 Subject: [PATCH 5/5] missing buffer capacity check in PVUnion::serialize Allows a buffer overflow in PVUnionArray::serialize(). --- src/factory/PVUnion.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/factory/PVUnion.cpp b/src/factory/PVUnion.cpp index b7102dc..4737bcb 100644 --- a/src/factory/PVUnion.cpp +++ b/src/factory/PVUnion.cpp @@ -145,10 +145,10 @@ void PVUnion::serialize(ByteBuffer *pbuffer, SerializableControl *pflusher) cons if (variant) { // write introspection data - if (value.get() == 0) + if (value.get() == 0) { + pflusher->ensureBuffer(1); pbuffer->put((int8)-1); - else - { + }else { pflusher->cachedSerialize(value->getField(), pbuffer); value->serialize(pbuffer, pflusher); }