Fix race condition exposed by compilers with more agressive optimization.

Add test procedure for epicsRingBytes.

documentation/RELEASE_NOTES.html

@@ -12,6 +12,10 @@
 <h2 align="center">Changes between 3.14.9 and 3.14.10</h2>
 <!-- Insert new items below here ... -->
 
+<h4>epicsRingPointer, epicsRingBytes</h4>
+
+<p>Fixed race condition exposed by compilers with more agressive optimization.</p>
+
 <h4>camonitor timestamp support</h4>
 
 <p>The <tt>camonitor</tt> program now supports the ability to display both

src/libCom/ring/epicsRingBytes.c

@@ -22,20 +22,28 @@
 #include "cantProceed.h"
 #include "epicsRingBytes.h"
 
+/*
+ * Need at least one extra byte to be able to distinguish a completely
+ * full buffer from a completely empty one.  Allow for a little extra
+ * space to try and keep good alignment and avoid multiple calls to
+ * memcpy for a single put/get operation.
+ */
+#define SLOP    16
+
 typedef struct ringPvt {
-    int nextPut;
-    int nextGet;
-    int          size;
-    char         *buffer;
+    volatile int   nextPut;
+    volatile int   nextGet;
+    int            size;
+    volatile char *buffer;
 }ringPvt;
 
-
 epicsShareFunc epicsRingBytesId  epicsShareAPI epicsRingBytesCreate(int size)
 {
     ringPvt *pring = mallocMustSucceed(sizeof(ringPvt),"epicsRingBytesCreate");
-    pring->size = size + 1;
+    pring->size = size + SLOP;
     pring->buffer = mallocMustSucceed(pring->size,"ringCreate");
-    pring->nextGet = pring->nextPut = 0;
+    pring->nextGet = 0;
+    pring->nextPut = 0;
     return((void *)pring);
 }
 
@@ -45,7 +53,7 @@ epicsShareFunc void epicsShareAPI epicsRingBytesDelete(epicsRingBytesId id)
     free((void *)pring->buffer);
     free((void *)pring);
 }
-
+
 epicsShareFunc int epicsShareAPI epicsRingBytesGet(
     epicsRingBytesId id, char *value,int nbytes)
 {
@@ -59,7 +67,8 @@ epicsShareFunc int epicsShareAPI epicsRingBytesGet(
         count = nextPut - nextGet;
         if (count < nbytes)
             nbytes = count;
-        memcpy (value, &pring->buffer[nextGet], nbytes);
+        if (nbytes)
+            memcpy (value, &pring->buffer[nextGet], nbytes);
         nextGet += nbytes;
     }
     else {
@@ -67,7 +76,8 @@ epicsShareFunc int epicsShareAPI epicsRingBytesGet(
         if (count > nbytes)
             count = nbytes;
         memcpy (value, &pring->buffer[nextGet], count);
-        if ((nextGet = nextGet + count) == size) {
+        nextGet += count;
+        if (nextGet == size) {
             int nLeft = nbytes - count;
             if (nLeft > nextPut)
                 nLeft = nextPut;
@@ -90,78 +100,68 @@ epicsShareFunc int epicsShareAPI epicsRingBytesPut(
     int nextGet = pring->nextGet;
     int nextPut = pring->nextPut;
     int size = pring->size;
-    int count;
+    int freeCount, copyCount, topCount;
 
     if (nextPut < nextGet) {
-        count = nextGet - nextPut - 1;
-        if (nbytes > count)
-            nbytes = count;
-        memcpy (&pring->buffer[nextPut], value, nbytes);
-        nextPut += nbytes;
-    }
-    else if (nextGet == 0) {
-        count = size - nextPut - 1;
-        if (nbytes > count)
-            nbytes = count;
-        memcpy (&pring->buffer[nextPut], value, nbytes);
+        freeCount = nextGet - nextPut - SLOP;
+        if (nbytes > freeCount)
+            nbytes = freeCount;
+        if (nbytes)
+            memcpy (&pring->buffer[nextPut], value, nbytes);
         nextPut += nbytes;
     }
     else {
-        count = size - nextPut;
-        if (count > nbytes)
-            count = nbytes;
-        memcpy (&pring->buffer[nextPut], value, count);
-        if ((nextPut = nextPut + count) == size) {
-            int nLeft = nbytes - count;
-            if (nLeft > (nextGet - 1))
-                nLeft = nextGet - 1;
-            memcpy (&pring->buffer[0], value+count, nLeft);
+        freeCount = size - nextPut + nextGet - SLOP;
+        if (nbytes > freeCount)
+            nbytes = freeCount;
+        topCount = size - nextPut;
+        copyCount = (nbytes > topCount) ?  topCount : nbytes;
+        if (copyCount)
+            memcpy (&pring->buffer[nextPut], value, copyCount);
+        nextPut += copyCount;
+        if (nextPut == size) {
+            int nLeft = nbytes - copyCount;
+            if (nLeft)
+                memcpy (&pring->buffer[0], value+copyCount, nLeft);
             nextPut = nLeft;
-            nbytes = count + nLeft;
-        }
-        else {
-            nbytes = count;
-            nextPut = nextPut;
         }
     }
     pring->nextPut = nextPut;
     return nbytes;
 }
-
+
 epicsShareFunc void epicsShareAPI epicsRingBytesFlush(epicsRingBytesId id)
 {
     ringPvt *pring = (ringPvt *)id;
 
-    pring->nextGet = pring->nextPut = 0;
+    pring->nextGet = 0;
+    pring->nextPut = 0;
 }
-
+
 epicsShareFunc int epicsShareAPI epicsRingBytesFreeBytes(epicsRingBytesId id)
 {
     ringPvt *pring = (ringPvt *)id;
-    int n;
+    int nextGet = pring->nextGet;
+    int nextPut = pring->nextPut;
 
-    n = pring->nextGet - pring->nextPut - 1;
-    if (n < 0)
-        n += pring->size;
-    return n;
+    if (nextPut < nextGet)
+        return nextGet - nextPut - SLOP;
+    else
+        return pring->size - nextPut + nextGet - SLOP;
 }
 
 epicsShareFunc int epicsShareAPI epicsRingBytesUsedBytes(epicsRingBytesId id)
 {
     ringPvt *pring = (ringPvt *)id;
-    int n;
 
-    n = pring->nextPut - pring->nextGet;
-    if (n < 0)
-        n += pring->size;
-    return n;
+    return pring->size - epicsRingBytesFreeBytes(id) - SLOP;
 }
 
 epicsShareFunc int epicsShareAPI epicsRingBytesSize(epicsRingBytesId id)
 {
     ringPvt *pring = (ringPvt *)id;
 
-    return pring->size - 1;
+    return pring->size - SLOP;
 }
 
 epicsShareFunc int epicsShareAPI epicsRingBytesIsEmpty(epicsRingBytesId id)
@@ -173,9 +173,5 @@ epicsShareFunc int epicsShareAPI epicsRingBytesIsEmpty(epicsRingBytesId id)
 
 epicsShareFunc int epicsShareAPI epicsRingBytesIsFull(epicsRingBytesId id)
 {
-    ringPvt *pring = (ringPvt *)id;
-    int count;
-
-    count = (pring->nextPut - pring->nextGet) + 1;
-    return ((count == 0) || (count == pring->size));
+    return (epicsRingBytesFreeBytes(id) <= 0);
 }

src/libCom/ring/epicsRingPointer.h

@@ -43,10 +43,10 @@ private: /* Prevent compiler-generated member functions */
     epicsRingPointer& operator=(const epicsRingPointer &);
 
 private: /* Data */
-    int nextPush;
-    int nextPop;
+    volatile int nextPush;
+    volatile int nextPop;
     int size;
-    T **buffer;
+    T  * volatile * buffer;
 };
 
 extern "C" {
@@ -93,10 +93,11 @@ inline epicsRingPointer<T>::~epicsRingPointer()
 template <class T>
 inline bool epicsRingPointer<T>::push(T *p)
 {
-    int newNext = nextPush +1;
+    int next = nextPush;
+    int newNext = next + 1;
     if(newNext>=size) newNext=0;
     if(newNext==nextPop) return(false);
-    buffer[nextPush] = p;
+    buffer[next] = p;
     nextPush = newNext;
     return(true);
 }
@@ -104,18 +105,21 @@ inline bool epicsRingPointer<T>::push(T *p)
 template <class T>
 inline T* epicsRingPointer<T>::pop()
 {
-    if(nextPop == nextPush) return(0);
-    T*p  = buffer[nextPop];
-    int newNext= nextPop;
-    ++newNext;
-    if(newNext >=size) newNext = 0;
-    nextPop = newNext;
+    int next = nextPop;
+    if(next == nextPush) return(0);
+    T*p  = buffer[next];
+    ++next;
+    if(next >=size) next = 0;
+    nextPop = next;
     return(p);
 }
 
 template <class T>
 inline void epicsRingPointer<T>::flush()
-{   nextPop = nextPush = 0;}
+{
+    nextPop = 0;
+    nextPush = 0;
+}
 
 template <class T>
 inline int epicsRingPointer<T>::getFree() const

src/libCom/test/Makefile

@@ -77,6 +77,11 @@ ringPointerTest_SRCS += ringPointerTest.c
 testHarness_SRCS += ringPointerTest.c
 TESTS += ringPointerTest
 
+TESTPROD_HOST += ringBytesTest
+ringBytesTest_SRCS += ringBytesTest.c
+testHarness_SRCS += ringBytesTest.c
+TESTS += ringBytesTest
+
 TESTPROD_HOST += epicsEventTest
 epicsEventTest_SRCS += epicsEventTest.cpp
 testHarness_SRCS += epicsEventTest.cpp

src/libCom/test/epicsRunLibComTests.c

@@ -32,6 +32,7 @@ int epicsTimeTest(void);
 int macLibTest(void);
 int macEnvExpandTest(void);
 int ringPointerTest(void);
+int ringBytesTest(void);
 int blockingSockTest(void);
 int taskwdTest(void);
 int epicsExitTest(void);
@@ -80,6 +81,8 @@ void epicsRunLibComTests(void)
 
     runTest(ringPointerTest);
 
+    runTest(ringBytesTest);
+
     runTest(blockingSockTest);
 
     runTest(taskwdTest);

src/libCom/test/ringBytesTest.c

@@ -0,0 +1,142 @@
+/*************************************************************************\
+* Copyright (c) 2008 UChicago Argonne LLC, as Operator of Argonne
+*     National Laboratory.
+* Copyright (c) 2002 The Regents of the University of California, as
+*     Operator of Los Alamos National Laboratory.
+* EPICS BASE is distributed subject to a Software License Agreement found
+* in file LICENSE that is included with this distribution.
+\*************************************************************************/
+/* ringBytesTest.c */
+
+#include <stddef.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+#include <time.h>
+
+#include "epicsThread.h"
+#include "epicsRingBytes.h"
+#include "errlog.h"
+#include "epicsEvent.h"
+#include "epicsUnitTest.h"
+#include "testMain.h"
+
+#define RINGSIZE 10
+
+static char msg[] = "This is a very long string to be sent through the ring.  " 
+                    "It will take multiple transmissions, you know.";
+
+typedef struct info {
+    epicsEventId consumerEvent;
+    epicsRingBytesId	ring;
+}info;
+
+static void check(epicsRingBytesId ring, int expectedFree)
+{
+    int expectedUsed = RINGSIZE - expectedFree;
+    int expectedEmpty = (expectedUsed == 0);
+    int expectedFull = (expectedFree == 0);
+    int nFree = epicsRingBytesFreeBytes(ring);
+    int nUsed = epicsRingBytesUsedBytes(ring);
+    int isEmpty = epicsRingBytesIsEmpty(ring);
+    int isFull = epicsRingBytesIsFull(ring);
+    
+    testOk(nFree == expectedFree, "Free: %d == %d", nFree, expectedFree);
+    testOk(nUsed == expectedUsed, "Used: %d == %d", nUsed, expectedUsed);
+    testOk(isEmpty == expectedEmpty, "Empty: %d == %d", isEmpty, expectedEmpty);
+    testOk(isFull == expectedFull, "Full: %d == %d", isFull, expectedFull);
+}
+    
+static void consumer(void *arg)
+{
+    info *pinfo = (info *)arg;
+    char getMsg[sizeof(msg) + RINGSIZE];
+    int i = 0;
+    int n;
+
+    testDiag("Consumer starting");
+    while(1) {
+        epicsEventMustWait(pinfo->consumerEvent);
+        n = epicsRingBytesGet(pinfo->ring, getMsg+i, RINGSIZE);
+        testOk(n != 0, "Read from ring");
+        i += n;
+        if (i >= sizeof(msg))
+            break;
+    }
+    testOk(i==sizeof(msg), "Correct number of bytes received %d==%d", i, (int)sizeof(msg));
+    testOk(strcmp(msg, getMsg) == 0, "Message received");
+}
+
+MAIN(ringBytesTest)
+{
+    int i, n;
+    info *pinfo;
+    epicsEventId consumerEvent;
+    char put[RINGSIZE+1];
+    char get[RINGSIZE+1];
+    epicsRingBytesId ring;
+
+    testPlan(242);
+
+    pinfo = calloc(1,sizeof(info));
+    if (!pinfo) {
+        testAbort("calloc failed");
+    }
+    pinfo->consumerEvent = consumerEvent = epicsEventCreate(epicsEventEmpty);
+    if (!consumerEvent) {
+        testAbort("epicsEventCreate failed");
+    }
+
+    pinfo->ring = ring = epicsRingBytesCreate(RINGSIZE);
+    if (!ring) {
+        testAbort("epicsRingBytesCreate failed");
+    }
+    check(ring, RINGSIZE);
+
+    for (i = 0 ; i < sizeof(put) ; i++)
+        put[i] = i;
+    for(i = 0 ; i < RINGSIZE ; i++) {
+        n = epicsRingBytesPut(ring, put, i);
+        testOk(n==i, "ring put %d", i);
+        check(ring, RINGSIZE-i);
+        n = epicsRingBytesGet(ring, get, i);
+        testOk(n==i, "ring get %d", i);
+        check(ring, RINGSIZE);
+        testOk(memcmp(put,get,i)==0, "get matches write");
+    }
+
+    for(i = 0 ; i < RINGSIZE ; i++) {
+        n = epicsRingBytesPut(ring, put+i, 1);
+        testOk(n==1, "ring put 1, %d", i);
+        check(ring, RINGSIZE-1-i);
+    }
+    n = epicsRingBytesPut(ring, put+RINGSIZE, 1);
+    testOk(n==0, "put to full ring");
+    check(ring, 0);
+    for(i = 0 ; i < RINGSIZE ; i++) {
+        n = epicsRingBytesGet(ring, get+i, 1);
+        testOk(n==1, "ring get 1, %d", i);
+        check(ring, 1+i);
+    }
+    testOk(memcmp(put,get,RINGSIZE)==0, "get matches write");
+    n = epicsRingBytesGet(ring, get+RINGSIZE, 1);
+    testOk(n==0, "get from empty ring");
+    check(ring, RINGSIZE);
+
+    epicsThreadCreate("consumer", 50, 
+        epicsThreadGetStackSize(epicsThreadStackSmall), consumer, pinfo);
+    epicsThreadSleep(0.1);
+
+    for (i = 0; i < sizeof(msg) ; ) {
+        n = epicsRingBytesPut(ring, msg+i, sizeof(msg)-i);
+        epicsEventSignal(consumerEvent);
+        epicsThreadSleep(0.2);
+        i += n;
+    }
+    epicsThreadSleep(0.2);
+    check(ring, RINGSIZE);
+
+    return testDone();
+}

src/libCom/test/ringPointerTest.c

@@ -27,7 +27,7 @@
 
 #define ringSize 10
 
-static int testExit = 0;
+static volatile int testExit = 0;
 
 typedef struct info {
     epicsEventId consumerEvent;