Merge ~epics-core/epics-base/+git/database:putf-pact into ~epics-core/epics-base/+git/epics-base:7.0

[mdavidsaver]

Attempt to address #1809570 by removing the warning, and fixing dbNotify.

[Andrew Johnson (anj)]

Not tested, but this looks good to me.

review: Approve

[mdavidsaver (mdavidsaver)]

Added proposed fix for the second part of lp:1809570.

This adds dbCommonPvt::procThread to track the thread which is currently processing a record.
This is set, and cleared, by processTarget(). This is called during dbPutLink() for a DB_LINK.

procThread will either be NULL, or contain the epicsThreadId for the current thread. Any other value is a logic error (missing dbScanLock(), or maybe incorrect lockset calculation).

The logic in processTarget() checks both the source record and destination record. procThread for the source will be NULL when the first link in a record chain is handled. Subsequent links in a chain will find source procThread==self. For the target record, procThread will be NULL, unless a loop would be closed.

The possible states of PACT and procThread on entry to processTarget() are

!PACT && !procThread - Idle record
!PACT && procThread - Logic error?
PACT && !procThread - Waiting for driver/rset to complete
PACT & procThread - process() in progress on this thread.

[Andrew Johnson (anj)]

What does the "set" part of your new variable names stand for? They don't read very obviously to me, especially out of the context of this bug-fix. The srcset/dstset variable is true if the psrc/pdst record has no procThread ID set, but I don't get any feel for what this means (weset in the previous iteration had a similar problem). Can you find something more meaningful?

You're still putting assert() statements in code that runs frequently inside the IOC. You may be more certain than last time that these assertions should never fire, but that makes IOCs more likely to die in the event of a memory error. I would prefer that these conditions just generate a warning (and maybe set procThread to whatever it's supposed to be if that's possible) without stopping the IOC thread. Brittle IOC code doesn't contribute to 98+% beam availability.

This doesn't look like it should have any problems with the synapps busy record, but have you tried it?

[mdavidsaver (mdavidsaver)]

Added additional handling for self link case (psrc==pdst).

> ... Can you find something more meaningful?

Would a comment like the following add sufficient meaning for you? If not, then I'm going to need more input.

/* decide if this call will manage procThread for the src and/or dst records */

> You're still putting assert() statements in code that runs frequently inside the IOC. ...

Yes, and I will continue to do so. Warnings don't always get reported. Not every user is as attentive as Mark. Travis certainly isn't.

> ... I would prefer that these conditions just generate a warning ...

Let's hold off on this until the change has been verified. CI doesn't grok warnings.

[Andrew Johnson (anj)]

Should we also have a separate test case for an INP self link that is marked PP? Your latest commit isn't symmetrical between src and dst, and I'm not certain whether this handles the INP case as well — an extra test would dispel that concern.

Regarding the names, just putting the verb before the noun helps a little with comprehension, so setsrc would be one step better. Looking deeper, these variables are controlling whether the code lower down should mark the src/dst record and then unmark it again after record processing, so instead of "set" the verb could be "mark". If I've read it right though that mark is actually saying "my thread currently owns this record" so maybe the verb should be "claim" so claimsrc/claimSrc/claim_src.

[mdavidsaver (mdavidsaver)]

Pushed name change, and additional tests.

> ... I'm not certain whether this handles the INP case as well ...

It does. 'src' and 'dst' in the link API is always about the direction of control flow, not data flow.

> This doesn't look like it should have any problems with the synapps busy record, but have you tried it?

Yes, it it appears to work from my (superficial) testing with AD's sim detector.

[mdavidsaver (mdavidsaver)]

@anj, does your "Approve" stand? Should I merge?

[mdavidsaver (mdavidsaver)]

@anj, ping...

[Andrew Johnson (anj)]

With the observation that the assertions are still present and I hope they never fire, this looks good to me, thanks!

review: Approve

[mdavidsaver (mdavidsaver)]

Let it not be said that I ignore ANJ. I've replaced the assertions with errlog'd warnings. Let's hope they are promptly reported if they appear.

diff --git a/modules/database/src/ioc/db/dbAccess.c b/modules/database/src/ioc/db/dbAccess.c
index af396c7..be81d22 100644
--- a/modules/database/src/ioc/db/dbAccess.c
+++ b/modules/database/src/ioc/db/dbAccess.c
@@ -684,7 +684,7 @@ finish:
 void dbInitEntryFromAddr(struct dbAddr *paddr, DBENTRY *pdbentry)
 {
     struct dbCommon *prec = paddr->precord;
-    dbCommonPvt *ppvt = CONTAINER(prec, dbCommonPvt, common);
+    dbCommonPvt *ppvt = dbRec2Pvt(prec);
 
     memset((char *)pdbentry,'\0',sizeof(DBENTRY));
 
@@ -698,7 +698,7 @@ void dbInitEntryFromAddr(struct dbAddr *paddr, DBENTRY *pdbentry)
 
 void dbInitEntryFromRecord(struct dbCommon *prec, DBENTRY *pdbentry)
 {
-    dbCommonPvt *ppvt = CONTAINER(prec, dbCommonPvt, common);
+    dbCommonPvt *ppvt = dbRec2Pvt(prec);
 
     memset((char *)pdbentry,'\0',sizeof(DBENTRY));
 
diff --git a/modules/database/src/ioc/db/dbCommonPvt.h b/modules/database/src/ioc/db/dbCommonPvt.h
index 3dfce8b..eda8b18 100644
--- a/modules/database/src/ioc/db/dbCommonPvt.h
+++ b/modules/database/src/ioc/db/dbCommonPvt.h
@@ -1,14 +1,27 @@
 #ifndef DBCOMMONPVT_H
 #define DBCOMMONPVT_H
 
+#include <compilerDependencies.h>
+#include <dbDefs.h>
 #include "dbCommon.h"
 
+struct epicsThreadOSD;
+
 /** Base internal additional information for every record
  */
 typedef struct dbCommonPvt {
     struct dbRecordNode *recnode;
 
+    /* Thread which is currently processing this record */
+    struct epicsThreadOSD* procThread;
+
     struct dbCommon common;
 } dbCommonPvt;
 
+static EPICS_ALWAYS_INLINE
+dbCommonPvt* dbRec2Pvt(struct dbCommon *prec)
+{
+    return CONTAINER(prec, dbCommonPvt, common);
+}
+
 #endif // DBCOMMONPVT_H
diff --git a/modules/database/src/ioc/db/dbDbLink.c b/modules/database/src/ioc/db/dbDbLink.c
index ce0110a..0da9271 100644
--- a/modules/database/src/ioc/db/dbDbLink.c
+++ b/modules/database/src/ioc/db/dbDbLink.c
@@ -56,7 +56,7 @@
 #include "dbAddr.h"
 #include "dbBase.h"
 #include "dbBkpt.h"
-#include "dbCommon.h"
+#include "dbCommonPvt.h"
 #include "dbConvertFast.h"
 #include "dbConvert.h"
 #include "db_field_log.h"
@@ -386,8 +386,11 @@ static long processTarget(dbCommon *psrc, dbCommon *pdst)
 {
     char context[40] = "";
     int trace = dbAccessDebugPUTF && *dbLockSetAddrTrace(psrc);
+    int claim_src = dbRec2Pvt(psrc)->procThread==NULL;
+    int claim_dst = psrc!=pdst && dbRec2Pvt(pdst)->procThread==NULL;
     long status;
     epicsUInt8 pact = psrc->pact;
+    epicsThreadId self = epicsThreadGetIdSelf();
 
     psrc->pact = TRUE;
 
@@ -406,14 +409,11 @@ static long processTarget(dbCommon *psrc, dbCommon *pdst)
             printf("%s: '%s' -> '%s' with PUTF=%u\n",
                 context, psrc->name, pdst->name, psrc->putf);
 
-        if (pdst->putf)
-            errlogPrintf("Warning: '%s.PUTF' found true with PACT false\n",
-                pdst->name);
-
         pdst->putf = psrc->putf;
     }
-    else if (psrc->putf) {
-        /* The dst record is busy (awaiting async reprocessing) and
+    else if (psrc->putf && claim_dst) {
+        /* The dst record is busy (awaiting async reprocessing),
+         * not being processed recursively by us, and
          * we were originally triggered by a call to dbPutField(),
          * so we mark the dst record for reprocessing once the async
          * completion is over.
@@ -426,17 +426,43 @@ static long processTarget(dbCommon *psrc, dbCommon *pdst)
         pdst->rpro = TRUE;
     }
     else {
-        /* The dst record is busy, but we weren't triggered by a call
-         * to dbPutField(). Do nothing.
+        /* The dst record is busy, but either is being processed recursively,
+         * or wasn't triggered by a call to dbPutField(). Do nothing.
          */
         if (trace)
             printf("%s: '%s' -> Active '%s', done\n",
                 context, psrc->name, pdst->name);
     }
 
+    if(claim_src) {
+        dbRec2Pvt(psrc)->procThread = self;
+    }
+    if(claim_dst) {
+        dbRec2Pvt(pdst)->procThread = self;
+    }
+
+    if(dbRec2Pvt(psrc)->procThread!=self ||
+       dbRec2Pvt(pdst)->procThread!=self) {
+        errlogPrintf("Logic Error: processTarget 1 from %p, %s(%p) -> %s(%p)\n",
+                     self, psrc->name, dbRec2Pvt(psrc), pdst->name, dbRec2Pvt(pdst));
+    }
+
     status = dbProcess(pdst);
 
     psrc->pact = pact;
 
+    if(dbRec2Pvt(psrc)->procThread!=self ||
+       dbRec2Pvt(pdst)->procThread!=self) {
+        errlogPrintf("Logic Error: processTarget 2 from %p, %s(%p) -> %s(%p)\n",
+                     self, psrc->name, dbRec2Pvt(psrc), pdst->name, dbRec2Pvt(pdst));
+    }
+
+    if(claim_src) {
+        dbRec2Pvt(psrc)->procThread = NULL;
+    }
+    if(claim_dst) {
+        dbRec2Pvt(pdst)->procThread = NULL;
+    }
+
     return status;
 }
diff --git a/modules/database/src/ioc/db/dbNotify.c b/modules/database/src/ioc/db/dbNotify.c
index c2420af..794672a 100644
--- a/modules/database/src/ioc/db/dbNotify.c
+++ b/modules/database/src/ioc/db/dbNotify.c
@@ -86,12 +86,6 @@ typedef struct notifyGlobal {
 
 static notifyGlobal *pnotifyGlobal = 0;
 
-/*Local routines*/
-static void notifyInit(processNotify *ppn);
-static void notifyCleanup(processNotify *ppn);
-static void restartCheck(processNotifyRecord *ppnr);
-static void callDone(dbCommon *precord,processNotify *ppn);
-static void processNotifyCommon(processNotify *ppn,dbCommon *precord);
 static void notifyCallback(CALLBACK *pcallback);
 
 #define ellSafeAdd(list,listnode) \
@@ -210,7 +204,7 @@ static void callDone(dbCommon *precord, processNotify *ppn)
     return;
 }
 
-static void processNotifyCommon(processNotify *ppn,dbCommon *precord)
+static void processNotifyCommon(processNotify *ppn, dbCommon *precord, int first)
 {
     notifyPvt *pnotifyPvt = (notifyPvt *) ppn->pnotifyPvt;
     int didPut = 0;
@@ -256,6 +250,9 @@ static void processNotifyCommon(processNotify *ppn,dbCommon *precord)
             doProcess = 1;
 
     if (doProcess) {
+        if (first) {
+            precord->putf = TRUE;
+        }
         ppn->wasProcessed = 1;
         precord->ppn = ppn;
         ellSafeAdd(&pnotifyPvt->waitList, &precord->ppnr->waitNode);
@@ -298,7 +295,7 @@ static void notifyCallback(CALLBACK *pcallback)
         return;
     }
     if(pnotifyPvt->state == notifyRestartCallbackRequested) {
-        processNotifyCommon(ppn, precord);
+        processNotifyCommon(ppn, precord, 0);
         return;
     }
     /* All done. Clean up and call userCallback */
@@ -382,7 +379,7 @@ void dbProcessNotify(processNotify *ppn)
         precord->ppnr->precord = precord;
         ellInit(&precord->ppnr->restartList);
     }
-    processNotifyCommon(ppn, precord);
+    processNotifyCommon(ppn, precord, 1);
 }
 
 void dbNotifyCancel(processNotify *ppn)
@@ -582,7 +579,7 @@ static void tpnThread(void *pvt)
     processNotify *ppn = (processNotify *) ptpnInfo->ppn;
 
     dbProcessNotify(ppn);
-    epicsEventWait(ptpnInfo->callbackDone);
+    epicsEventMustWait(ptpnInfo->callbackDone);
     dbNotifyCancel(ppn);
     epicsEventDestroy(ptpnInfo->callbackDone);
     dbChannelDelete(ppn->chan);
diff --git a/modules/database/src/ioc/dbStatic/dbStaticRun.c b/modules/database/src/ioc/dbStatic/dbStaticRun.c
index d3817ff..46cbf98 100644
--- a/modules/database/src/ioc/dbStatic/dbStaticRun.c
+++ b/modules/database/src/ioc/dbStatic/dbStaticRun.c
@@ -179,7 +179,7 @@ long dbFreeRecord(DBENTRY *pdbentry)
     if(!pdbRecordType) return(S_dbLib_recordTypeNotFound);
     if(!precnode) return(S_dbLib_recNotFound);
     if(!precnode->precord) return(S_dbLib_recNotFound);
-    free(CONTAINER(precnode->precord, dbCommonPvt, common));
+    free(dbRec2Pvt(precnode->precord));
     precnode->precord = NULL;
     return(0);
 }
diff --git a/modules/database/test/std/rec/asyncproctest.c b/modules/database/test/std/rec/asyncproctest.c
index a8b09a9..7b8fca7 100644
--- a/modules/database/test/std/rec/asyncproctest.c
+++ b/modules/database/test/std/rec/asyncproctest.c
@@ -9,6 +9,7 @@
 
 #include <testMain.h>
 #include <dbUnitTest.h>
+#include <errlog.h>
 #include <dbCommon.h>
 #include <dbAccess.h>
 #include <epicsEvent.h>
@@ -16,6 +17,7 @@
 #include <iocsh.h>
 #include "registryFunction.h"
 #include <subRecord.h>
+#include <dbScan.h>
 
 epicsEventId done;
 static int waitFor;
@@ -28,11 +30,17 @@ long doneSubr(subRecord *prec)
     return 0;
 }
 
+static
+void dummydone(void *usr, struct dbCommon* prec)
+{
+    epicsEventMustTrigger(done);
+}
+
 void asyncproctest_registerRecordDeviceDriver(struct dbBase *);
 
 MAIN(asyncproctest)
 {
-    testPlan(21);
+    testPlan(27);
 
     done = epicsEventMustCreate(epicsEventEmpty);
 
@@ -99,6 +107,40 @@ MAIN(asyncproctest)
     testdbGetFieldEqual("chain3", DBF_LONG, 7);
     testdbGetFieldEqual("chain3.A", DBF_LONG, 2);
 
+    testDiag("===== Chain 4 ======");
+
+    {
+        dbCommon *dummy=testdbRecordPtr("chain4_dummy");
+
+        testdbPutFieldOk("chain4_pos.PROC", DBF_LONG, 0);
+
+        /* sync once queue to wait for any queued RPRO */
+        scanOnceCallback(dummy, dummydone, NULL);
+
+        if (epicsEventWaitWithTimeout(done, 10.0) != epicsEventOK)
+            testAbort("Processing timed out");
+
+        testdbGetFieldEqual("chain4_pos", DBF_SHORT, 1);
+        testdbGetFieldEqual("chain4_rel", DBF_SHORT, 1);
+        testdbGetFieldEqual("chain4_lim", DBF_SHORT, 1);
+    }
+
+    testDiag("===== Chain 5 ======");
+
+    {
+        dbCommon *dummy=testdbRecordPtr("chain4_dummy");
+
+        testdbPutFieldOk("chain5_cnt.PROC", DBF_LONG, 0);
+
+        /* sync once queue to wait for any queued RPRO */
+        scanOnceCallback(dummy, dummydone, NULL);
+
+        if (epicsEventWaitWithTimeout(done, 10.0) != epicsEventOK)
+            testAbort("Processing timed out");
+
+        testdbGetFieldEqual("chain5_cnt", DBF_SHORT, 1);
+    }
+
     testIocShutdownOk();
 
     testdbCleanup();
diff --git a/modules/database/test/std/rec/asyncproctest.db b/modules/database/test/std/rec/asyncproctest.db
index 352b6ca..87fe5d6 100644
--- a/modules/database/test/std/rec/asyncproctest.db
+++ b/modules/database/test/std/rec/asyncproctest.db
@@ -53,3 +53,41 @@ record(sub, "done3") {
     field(FLNK, "chain3")
     field(TPRO, "$(TPRO=)")
 }
+
+
+# loop breaking regression
+# should _not_ RPRO
+record(calcout,"chain4_pos") {
+    field(CALC, "E:=E+1;E")
+    field(OUT,"chain4_rel.A PP")
+    field(TPRO, "$(TPRO=)")
+}
+
+record(calc,"chain4_rel") {
+    field(CALC, "E:=E+1;E")
+    field(FLNK,"chain4_lim")
+    field(TPRO, "$(TPRO=)")
+}
+
+record(calc,"chain4_lim") {
+    field(CALC, "E:=E+1;E")
+    field(INPA,"chain4_pos PP")
+    field(INPB,"chain4_pos.HIGH PP")
+    field(INPC,"chain4_pos.LOW PP")
+    field(FLNK,"chain4_pos")
+    field(TPRO, "$(TPRO=)")
+}
+
+record(bo, "chain4_dummy") {
+field(TPRO, "$(TPRO=)")
+}
+
+# loop breaking regression part 2
+# selft link should _not_ RPRO
+record(calcout,"chain5_cnt") {
+    field(CALC, "E:=E+1;E")
+    field(INPA,"chain5_cnt.A PP")
+    field(OUT,"chain5_cnt.A PP")
+    field(FLNK,"chain5_cnt")
+    field(TPRO, "$(TPRO=)")
+}