From dbc27529e48d8ea1793349be31434953f4719fb5 Mon Sep 17 00:00:00 2001 From: Jan Nidzwetzki Date: Thu, 13 Aug 2026 22:44:29 +0200 Subject: [PATCH 1/3] Make: Improve Java error message --- makefile | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/makefile b/makefile index 041313e30..b770ef6e3 100755 --- a/makefile +++ b/makefile @@ -41,10 +41,22 @@ endif include ./Javagui/makefile.inc compileJava := "$(shell $(JAVAC) -help > /dev/null 2>&1; if [ $$? != 0 ]; then echo "false"; else echo "true"; fi)" +# The probe above quotes its answer while callers pass it unquoted +# (compileJava=false), so every comparison uses this normalized copy -- +# otherwise "compileJava=true" would read as "not true" and skip the GUI. +compileJavaFlag := $(subst ",,$(compileJava)) + +# Whether the caller asked for the GUI to be skipped, as opposed to the probe +# above finding no working javac. Only the second is a fault, and java2 below +# turns it into a hard error rather than a message that scrolls past. +javaSkipRequested := $(if $(filter command line environment,\ + $(origin compileJava)),yes) + define javac-msg @echo "" - @echo "Warning: The command \"$(JAVAC) -help\" returned an error!" - @echo " Please check your Java 2 SDK configuration an run again." + @echo "Error: The command \"$(JAVAC) -help\" returned an error!" + @echo " Please check your Java 2 SDK configuration and run again," + @echo " or pass compileJava=false to skip the GUI on purpose." endef @@ -149,12 +161,15 @@ java: java2 update-config .PHONY: java2 java2: -ifeq ($(compileJava),"true") +ifeq ($(compileJavaFlag),true) @echo ; echo " *** Compiling the java based GUI *** "; echo $(MAKE) -C Javagui all +else ifeq ($(javaSkipRequested),yes) + @echo ; echo "The java based GUI was not compiled (compileJava=false)." else @echo ; echo "The java based GUI was not compiled!" $(javac-msg) + @exit 1 endif From 2de6678d40dbc3bae16cb78d7039adc49b45b4f4 Mon Sep 17 00:00:00 2001 From: Jan Nidzwetzki Date: Thu, 13 Aug 2026 23:06:15 +0200 Subject: [PATCH 2/3] Fix errors found by the thread sanitizer --- Algebras/Distributed2/ConnectionInfo.cpp | 63 ++++++++++++------- Algebras/Distributed2/ConnectionInfo.h | 13 +++- Algebras/Distributed2/Distributed2Algebra.cpp | 37 +++++++---- Algebras/Standard-C++/StandardAlgebra.cpp | 16 ++--- Tools/Flob/FlobManager.cpp | 7 ++- Tools/Utilities/Application.cpp | 28 +++++++++ include/StandardTypes.h | 24 ++++--- 7 files changed, 132 insertions(+), 56 deletions(-) diff --git a/Algebras/Distributed2/ConnectionInfo.cpp b/Algebras/Distributed2/ConnectionInfo.cpp index 16f6e592e..aa36464e6 100644 --- a/Algebras/Distributed2/ConnectionInfo.cpp +++ b/Algebras/Distributed2/ConnectionInfo.cpp @@ -328,7 +328,8 @@ void ConnectionInfo::simpleCommand(std::string command1, stopTimeout(true); } } else { - cmdLog->insert(this, command); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, command); serr.code = 0; serr.msg = "command not evaluated"; resList = mynl->TheEmptyList(); @@ -431,7 +432,8 @@ bool ConnectionInfo::switchDatabase(const std::string& dbname, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -445,7 +447,8 @@ bool ConnectionInfo::switchDatabase(const std::string& dbname, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -458,7 +461,8 @@ bool ConnectionInfo::switchDatabase(const std::string& dbname, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -513,7 +517,8 @@ void ConnectionInfo::simpleCommand(const std::string& command1, if(!cmdLog || forceExec){ si->Secondo(command, myResList, serr); } else { - cmdLog->insert(this, command); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, command); myResList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -583,7 +588,8 @@ void ConnectionInfo::simpleCommandFromList(const std::string& command1, if(!cmdLog || forceExec){ si->Secondo(cmd, myResList, serr); } else { - cmdLog->insert(this, mynl->ToString(cmd)); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, mynl->ToString(cmd)); myResList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -658,7 +664,8 @@ void ConnectionInfo::simpleCommand(const std::string& command1, if(!cmdLog || forceExec){ si->Secondo(command, myResList, serr); } else { - cmdLog->insert(this, command); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, command); myResList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -847,7 +854,8 @@ bool ConnectionInfo::createOrUpdateObject(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -868,7 +876,8 @@ bool ConnectionInfo::createOrUpdateObject(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -967,7 +976,8 @@ bool ConnectionInfo::createOrUpdateRelationFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -986,7 +996,8 @@ bool ConnectionInfo::createOrUpdateRelationFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1011,7 +1022,8 @@ bool ConnectionInfo::createOrUpdateRelationFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1074,7 +1086,8 @@ bool ConnectionInfo::createOrUpdateAttributeFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1093,7 +1106,8 @@ bool ConnectionInfo::createOrUpdateAttributeFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1111,7 +1125,8 @@ bool ConnectionInfo::createOrUpdateAttributeFromBinFile(const std::string& name, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1252,7 +1267,8 @@ bool ConnectionInfo::retrieve(const std::string& objName, if(!cmdLog || forceExec){ si->Secondo(cmd, myResList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); myResList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1391,7 +1407,8 @@ bool ConnectionInfo::retrieveRelationInFile(const std::string& fileName, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1444,7 +1461,8 @@ bool ConnectionInfo::retrieveRelationInFile(const std::string& fileName, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1500,7 +1518,8 @@ bool ConnectionInfo::retrieveRelationFile(const std::string& objName, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1528,7 +1547,8 @@ bool ConnectionInfo::retrieveRelationFile(const std::string& objName, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; @@ -1559,7 +1579,8 @@ bool ConnectionInfo::retrieveRelationFile(const std::string& objName, if(!cmdLog || forceExec){ si->Secondo(cmd, resList, serr); } else { - cmdLog->insert(this, cmd); + if (CommandLogger* clog = cmdLog.load()) + clog->insert(this, cmd); resList = mynl->TheEmptyList(); serr.code = 0; serr.msg = "command not executed"; diff --git a/Algebras/Distributed2/ConnectionInfo.h b/Algebras/Distributed2/ConnectionInfo.h index 90018fa94..81f5fc1cd 100644 --- a/Algebras/Distributed2/ConnectionInfo.h +++ b/Algebras/Distributed2/ConnectionInfo.h @@ -29,6 +29,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA #ifndef ALGEBRAS_DISTRIBUTED2_CONNECTIONINFO_H_ #define ALGEBRAS_DISTRIBUTED2_CONNECTIONINFO_H_ +#include #include #include @@ -287,12 +288,12 @@ class ConnectionInfo void setLogger(CommandLogger* cmdlog){ - this->cmdLog = cmdlog; + this->cmdLog.store(cmdlog); } CommandLogger* getLogger() const{ - return cmdLog; + return cmdLog.load(); } void setNum(const int num){ @@ -352,7 +353,13 @@ class ConnectionInfo mutex_type simtx; // mutex for synchronizing // access to the interface - CommandLogger* cmdLog; // if this is nor null, commands are + // Atomic because it is written and read from several worker threads at + // once: getWorkerConnection installs the caller's logger on a connection + // that other workers are already running commands through. Which logger + // wins was always last-writer-wins; this only makes the access defined + // rather than a data race. + std::atomic cmdLog; + // if this is nor null, commands are // written to log instead of sending // to the server int num; // some number that can be used to store additional information diff --git a/Algebras/Distributed2/Distributed2Algebra.cpp b/Algebras/Distributed2/Distributed2Algebra.cpp index 3fa6170cd..2231e012d 100644 --- a/Algebras/Distributed2/Distributed2Algebra.cpp +++ b/Algebras/Distributed2/Distributed2Algebra.cpp @@ -8627,17 +8627,22 @@ template class dsummarizeRelInfo : public dsummarizeRelListener { boost::mutex mtx; void start() { - for (size_t i = 0; i < array->getSize(); i++) { + const size_t slots = array->getSize(); + + // Every entry exists before any thread does. A worker reports its result + // into filenames[] through fileAvailable as soon as it starts, holding + // mtx -- but the push_back that used to follow held nothing and may + // reallocate, which moves the very strings a running worker is assigning + // to. Sizing the vectors up front and starting the threads afterwards + // removes the race and the write through the freed buffer together. + filenames.resize(slots); + getters.assign(slots, nullptr); + runners.assign(slots, nullptr); + + for (size_t i = 0; i < slots; i++) { if (array->isSlotUsed(i)) { - RelationFileGetter *getter = - new RelationFileGetter(array, i, this); - getters.push_back(getter); - filenames.push_back(""); - runners.push_back(new boost::thread(*getter)); - } else { // unused slot - filenames.push_back(""); - runners.push_back(nullptr); - getters.push_back(nullptr); + getters[i] = new RelationFileGetter(array, i, this); + runners[i] = new boost::thread(*getters[i]); } } } @@ -11509,11 +11514,19 @@ template class Mapper { rtype *r = new rtype(ci, dbname, i, this); ci->deleteIfAllowed(); w.push_back(r); - boost::thread *runner = new boost::thread(&rtype::run, r); - runners.push_back(runner); } } + // The threads start only once every logger above has been set. Several + // slots can share one worker connection, so starting them inside the loop + // meant a later iteration wrote ConnectionInfo::cmdLog while an already + // running worker read it. Creating a thread synchronizes with everything + // the creating thread did first, so this removes the race outright + // instead of locking around it. + for (size_t k = 0; k < w.size(); k++) { + runners.push_back(new boost::thread(&rtype::run, w[k])); + } + // wait for finishing the threads and delete them for (size_t i = 0; i < runners.size(); i++) { runners[i]->join(); diff --git a/Algebras/Standard-C++/StandardAlgebra.cpp b/Algebras/Standard-C++/StandardAlgebra.cpp index 56944e073..c76020b9c 100755 --- a/Algebras/Standard-C++/StandardAlgebra.cpp +++ b/Algebras/Standard-C++/StandardAlgebra.cpp @@ -301,8 +301,8 @@ Within this implementation, we don't use the flag but maintain it in order to demonstrate how to handle complex objects. */ -long CcInt::intsCreated = 0; -long CcInt::intsDeleted = 0; +std::atomic CcInt::intsCreated(0); +std::atomic CcInt::intsDeleted(0); /* @@ -471,8 +471,8 @@ The following type constructor, ~REAL~, is defined in the same way as ~INT~. */ -long CcReal::realsCreated = 0; -long CcReal::realsDeleted = 0; +std::atomic CcReal::realsCreated(0); +std::atomic CcReal::realsDeleted(0); /* @@ -601,8 +601,8 @@ Within this implementation, we don't use the flag but maintain it in order to demonstrate how to handle complex objects. */ -long CcBool::boolsCreated = 0; -long CcBool::boolsDeleted = 0; +std::atomic CcBool::boolsCreated(0); +std::atomic CcBool::boolsDeleted(0); /* @@ -743,8 +743,8 @@ TypeConstructor ccBool( CcBool::BasicType(), CcBoolProperty, 3.5 Type constructor *CcString* */ -long CcString::stringsCreated = 0; -long CcString::stringsDeleted = 0; +std::atomic CcString::stringsCreated(0); +std::atomic CcString::stringsDeleted(0); bool CcString::Adjacent( const Attribute* arg ) const { diff --git a/Tools/Flob/FlobManager.cpp b/Tools/Flob/FlobManager.cpp index ab765a052..e7f5b1986 100644 --- a/Tools/Flob/FlobManager.cpp +++ b/Tools/Flob/FlobManager.cpp @@ -11,6 +11,7 @@ of that class. #include "SecondoSMI.h" #include +#include #include #include #include "Flob.h" @@ -72,8 +73,10 @@ FlobManager instance. #define FM_useStats #ifdef FM_useStats -static size_t createdFlobs; -static size_t reusedFlobs; +// Atomic because Flobs are created from several threads at once while the +// increment of createdFlobs sits outside the manager's mutex. +static std::atomic createdFlobs; +static std::atomic reusedFlobs; #endif diff --git a/Tools/Utilities/Application.cpp b/Tools/Utilities/Application.cpp index ced9a508b..57365d5a5 100755 --- a/Tools/Utilities/Application.cpp +++ b/Tools/Utilities/Application.cpp @@ -339,6 +339,13 @@ Application::Application( int argc, const char** argv ) Application::~Application() { + // Reset the pointer, because the signal handlers this class installs stay + // installed for the life of the process and every one of them reaches + // through appPointer. Leaving it set meant that a signal arriving during + // or after teardown read a destroyed object. ThreadSanitizer caught + // exactly that on the server shutdown path. + Application::appPointer = 0; + if(Application::relocationInfo != NULL) { free(Application::relocationInfo); Application::relocationInfo = NULL; @@ -455,6 +462,17 @@ This is the default signal handler for all signals that would abort the process if not handled otherwise. */ + // The instance may already be gone: these handlers stay installed for the + // whole process, while appPointer is cleared by ~Application. With nothing + // left to record the signal for, hand it to the default handler rather than + // dereference a destroyed object. + if ( appPointer == 0 ) + { + signal( sig, SIG_DFL ); + raise( sig ); + return; + } + // When the application opted in, a terminating signal is only recorded here; // the shutdown itself runs later from the main loop, via // ProcessPendingSignals. Everything else -- and every signal when the opt-in @@ -492,6 +510,11 @@ abort the process if not handled otherwise. void Application::UserSignalHandler ( int sig ) { + if ( Application::appPointer == 0 ) + { + return; + } + // SIGUSR1 is used to cancel running queries if ( sig == SIGUSR1 ) { @@ -562,6 +585,11 @@ Application::RemoteSignalHandler() BOOL Application::AbortOnSignalHandler( DWORD sig ) { + if ( Application::appPointer == 0 ) + { + return (FALSE); + } + Application::appPointer->lastSignal = sig; if ( sig == CTRL_C_EVENT || sig == CTRL_BREAK_EVENT || diff --git a/include/StandardTypes.h b/include/StandardTypes.h index 80bf6e18c..a6480ad52 100755 --- a/include/StandardTypes.h +++ b/include/StandardTypes.h @@ -68,6 +68,7 @@ by the ~StandardAlgebra~: +#include #include #include @@ -609,8 +610,11 @@ CcInt operator--(int){ inline virtual StorageType GetStorageType() const { return Core; } - static long intsCreated; - static long intsDeleted; + // Atomic: these are bumped in every constructor and destructor, and + // objects of these types are created concurrently by the worker + // threads in Distributed2. + static std::atomic intsCreated; + static std::atomic intsDeleted; private: inttype intval; @@ -759,8 +763,8 @@ class CcReal : public Attribute return (os << realval); } - static long realsCreated; - static long realsDeleted; + static std::atomic realsCreated; + static std::atomic realsDeleted; inline bool operator==(const CcReal& rhs) const { @@ -1049,8 +1053,8 @@ class CcBool : public Attribute } - static long boolsCreated; - static long boolsDeleted; + static std::atomic boolsCreated; + static std::atomic boolsDeleted; virtual std::string getCsvStr() const{ if(!IsDefined()){ @@ -1297,8 +1301,8 @@ class CcString : public Attribute return (os << "\"" << stringval << "\""); } - static long stringsCreated; - static long stringsDeleted; + static std::atomic stringsCreated; + static std::atomic stringsDeleted; virtual std::string getCsvStr() const{ if(!IsDefined()){ @@ -1575,8 +1579,8 @@ class CcString : public Attribute return (os << "\"" << stringval << "\""); } - static long stringsCreated; - static long stringsDeleted; + static std::atomic stringsCreated; + static std::atomic stringsDeleted; virtual std::string getCsvStr() const{ if(!IsDefined()){ From 449f82c31b13ee5ca4eb9b684273c0219905c014 Mon Sep 17 00:00:00 2001 From: Jan Nidzwetzki Date: Thu, 13 Aug 2026 23:39:49 +0200 Subject: [PATCH 3/3] CI: Added full thread sanitizer test --- .github/workflows/build.yml | 253 ++++++++++++++++++++++++++++++------ .gitignore | 3 +- CM-Scripts/run-tests.sh | 58 +++++---- CM-Scripts/tsan.supp | 33 +++++ Tests/csloadtest/README.md | 77 ++++++++--- Tests/csloadtest/makefile | 129 ++++++------------ makefile.env | 24 ++-- 7 files changed, 400 insertions(+), 177 deletions(-) create mode 100644 CM-Scripts/tsan.supp diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 8dc750079..22be5f554 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -136,10 +136,9 @@ jobs: # moves a chunk once it is published and keeps its counters atomic, and # nothing but a test says so. # - # Plain first, because the value checks catch corruption on their own; - # then under ThreadSanitizer, which catches the unsynchronized access - # before it corrupts anything. Needs no server, so it runs here rather - # than with the connection tests further down. + # The value checks catch corruption on their own. The accesses that race + # without corrupting anything yet are ThreadSanitizer's to find, and it + # runs over the whole tree in the build-linux-tsan job rather than here. # # --list-mem is what makes this reach the storage layer: it sizes the # tables to a single page, so BigArray runs out of room within the first @@ -155,24 +154,21 @@ jobs: run: | make ./app --mode shared --threads 8 --rounds 5 --list-mem 1 - make tsan-check-sources - make tsan - TSAN_OPTIONS="halt_on_error=0 history_size=4" \ - ./app-tsan --mode shared --threads 4 --rounds 2 --list-mem 1 # The other half of the contract -- "operations on the same list need # the caller's own lock" -- is the caller's to keep, so NestedList # ships a checker that turns breaking it into an immediate abort - # naming both threads and the node. Built here from the same sources - # (TSAN_FLAGS is just the flag set for that self-contained build; no - # sanitizer this time, the checker is the instrument). + # naming both threads and the node. "make app-check" is that build: + # NestedList.cpp and the test recompiled with -DNL_CHECK_CONCURRENCY + # over the stock libsecondo.a. No sanitizer -- the checker is the + # instrument, and it looks for overlap in time rather than for the + # missing happens-before that TSan reports. # # Both directions are asserted, because a checker that never fires # and a checker that always fires both look like a passing build from # one side only. - rm -rf tsan app-tsan - make tsan TSAN_FLAGS="-g -O1 -DNL_CHECK_CONCURRENCY" - ./app-tsan --mode shared --threads 8 --rounds 5 --list-mem 1 + make app-check + ./app-check --mode shared --threads 8 --rounds 5 --list-mem 1 # # ...and one list passed from thread to thread, each one joined # before the next starts. That keeps the contract, so it has to run @@ -181,7 +177,7 @@ jobs: # and forgets to unmark it gets wrong. Such a checker still aborts on # the unsynchronised mode below and still passes the mode above, so # this is the direction that has to be asserted separately. - ./app-tsan --mode handover --threads 4 --rounds 5 --list-mem 1 + ./app-check --mode handover --threads 4 --rounds 5 --list-mem 1 # What the contract run must produce is the checker's *report*, not # merely a non-zero exit: a crash, a binary that was never built, and @@ -192,7 +188,7 @@ jobs: # anything kept this step green. fired="" for attempt in $(seq 1 10); do - if out=$(./app-tsan --mode contract --threads 4 --rounds 1 \ + if out=$(./app-check --mode contract --threads 4 --rounds 1 \ --list-mem 1 2>&1); then echo "$out" echo "FAIL: the concurrency checker did not fire although four" \ @@ -212,13 +208,6 @@ jobs: exit 1 fi - # Leave nothing instrumented-looking behind: the connection test - # further down builds its own app-tsan in this same directory, and - # make would find these objects up to date and reuse them. They were - # built without -fsanitize=thread, so that step would run a blind - # binary and pass no matter what. - rm -rf tsan app-tsan - - name: Setup Node uses: actions/setup-node@v7 with: @@ -342,34 +331,19 @@ jobs: # The client library has to tolerate several connections in one process: # the WebUI bridge opens one per browser session and runs them in # parallel. Tests/csloadtest drives it from many threads and checks the - # answers; under ThreadSanitizer it also checks that nothing shared is - # touched without synchronization. Runs against the monitor started above. + # answers. Runs against the monitor started above. The same test runs + # under ThreadSanitizer in the build-linux-tsan job, where both this + # process and the server it talks to are instrumented. # ----------------------------------------------------------------------- - name: Test concurrent client connections working-directory: Tests/csloadtest - # These two drive a server over a socket, so a wedged connection hangs - # rather than fails; and see the cap on the nested-list step above for - # how a sanitizer run can spin instead of reporting. + # This drives a server over a socket, so a wedged connection hangs + # rather than fails. timeout-minutes: 10 run: | make ./app --db berlintest --threads 10 --rounds 5 --expect-binary - - name: Test concurrent client connections under ThreadSanitizer - working-directory: Tests/csloadtest - timeout-minutes: 10 - run: | - # Fails if the list of instrumented sources has drifted from the - # archive: an object we forget to instrument makes TSan quietly blind. - make tsan-check-sources - make tsan - # TSan exits 66 when it reports anything, which fails this step. - # halt_on_error=0 so that a failing run lists every race, not just - # the first. Fewer threads and rounds than above: the sanitizer is - # slow, and the races it looks for do not need a large workload. - TSAN_OPTIONS="halt_on_error=0 history_size=4" \ - ./app-tsan --db berlintest --threads 4 --rounds 2 - - name: Stop the WebUI stack if: always() run: | @@ -387,6 +361,199 @@ jobs: echo "########## WebUI backend" cat "${RUNNER_TEMP}/webui-backend.log" 2>/dev/null || true + # ------------------------------------------------------------------------ + # The whole tree under ThreadSanitizer. + # + # This used to be two steps in build-linux that instrumented a hand-listed + # mirror of the client library's sources -- NestedList, BigArray, SocketIO + # and the CS interface, and nothing else. The QueryProcessor, every algebra + # and the four server daemons were outside it, which is precisely where the + # threads that serve users run. Instrumenting a subset is also the one thing + # that makes TSan report races that do not exist, because it cannot see the + # synchronization in the objects it was not compiled into. + # + # "make SECONDO_TSAN=1" (see makefile.env) instruments everything instead, + # so the mirror and the guard that kept it from drifting are both gone. One + # runner is enough: the sanitizer finds races in the code, not in the + # platform, so running it on all three Ubuntu versions would cost three + # times as much to learn the same thing. + # ------------------------------------------------------------------------ + build-linux-tsan: + runs-on: ubuntu-26.04 + env: + # halt_on_error=0 so one run lists every race rather than only the first; + # TSan still exits 66, which fails the step that produced it. history_size + # trades memory for how far back the "previous access" stack can reach. + # detect_deadlocks=0 because Berkeley DB takes its locks out of order on + # purpose and resolves the result with its own deadlock detector -- that + # is what DB_LOCK_DETECT is for. TSan cannot know that and reported every + # occurrence: one restore of berlintest produced 20919 lock-order + # inversions, all inside libdb_cxx, a 1.2 million line log, and not one + # data race. Races are what this job is for, and they are still detected; + # only the lock-order checker is off. + TSAN_OPTIONS: >- + halt_on_error=0 + history_size=4 + detect_deadlocks=0 + suppressions=${{ github.workspace }}/CM-Scripts/tsan.supp + steps: + - name: Checkout repo + uses: actions/checkout@v7 + + - name: Setup ccache + uses: hendrikmuhs/ccache-action@v1.2.23 + with: + # Its own key: -fsanitize=thread and -O1 change every command line, so + # these objects would never hit the build-linux cache -- sharing a key + # would only make the two jobs evict each other. Larger, too, because + # instrumented objects are bigger. + key: tsan-ubuntu-26.04 + max-size: 2G + + - name: Update packages + run: | + sudo apt-get update + + - name: Install packages + run: | + sudo apt-get -y install flex bison gcc g++ libdb5.3 libdb5.3-dev libdb5.3++ libdb5.3++-dev db5.3-util libjpeg-dev libgsl0-dev libreadline-dev librecode-dev libgmp-dev libncurses-dev libxml2-dev libboost-all-dev build-essential default-jdk wget swi-prolog-nox swi-prolog-java swi-prolog libquadmath0 libgmp-dev libgmp10 libboost-all-dev libbison-dev libfl-dev nlohmann-json3-dev + + - name: Detect SECONDO build environment + run: | + { + echo "SECONDO_BUILD_DIR=${GITHUB_WORKSPACE}" + echo "SECONDO_CC=ccache gcc" + echo "SECONDO_CPP=ccache g++" + } >> "$GITHUB_ENV" + make -s -f makefile.detect print-vars + make -f makefile.detect check-env + + - name: Show ccache statistics (before build) + run: ccache -s + + # compileJava=false skips the Java GUI: javac never sees a sanitizer flag, + # so building it here would only cost time. + - name: Build SECONDO with ThreadSanitizer + run: | + make -j $(nproc) SECONDO_TSAN=1 compileJava=false + + - name: Show ccache statistics (after build) + run: ccache -s + + # The top-level make does not build libsecondo.a, and Tests/csloadtest + # links it. + - name: Build the SECONDO client library + run: make -C apis/api_cpp/cs SECONDO_TSAN=1 + + # A binary that was never instrumented reports nothing and looks exactly + # like a clean run, so check the server binaries carry TSan's calls before + # trusting anything below. SecondoMonitor is the one the old job could not + # reach at all. + - name: Check that the server binaries are instrumented + run: | + rc=0 + for b in bin/SecondoMonitor bin/SecondoListener bin/SecondoRegistrar \ + bin/SecondoCheckpoint bin/SecondoBDB bin/SecondoCS; do + n=$(nm -C "$b" | grep -c __tsan_ || true) + echo "$b: $n __tsan_ references" + if [ "$n" -lt 10 ]; then + echo "FAIL: $b is not instrumented"; rc=1 + fi + done + exit $rc + + - name: Test the nested list storage layer + run: | + make -C Tests tnestedlist SECONDO_TSAN=1 + Tests/tnestedlist + + - name: Test the BigArray chunked storage + run: | + make -C Tests tbigarray SECONDO_TSAN=1 + Tests/tbigarray + + # Several threads on one list, and one list handed from thread to thread. + # Needs no server. Fewer threads and rounds than in build-linux: the + # sanitizer is slow, and the races it looks for do not need a large + # workload. + - name: Test concurrent access to one nested list + working-directory: Tests/csloadtest + timeout-minutes: 30 + run: | + make SECONDO_TSAN=1 + ./app --mode shared --threads 4 --rounds 2 --list-mem 1 + ./app --mode handover --threads 4 --rounds 5 --list-mem 1 + + - name: Restore the berlintest database + run: | + echo "${GITHUB_WORKSPACE}/bin" >> "$GITHUB_PATH" + echo "SECONDO_CONFIG=${GITHUB_WORKSPACE}/bin/SecondoConfig.ini" >> "$GITHUB_ENV" + echo "SECONDO_PARAM_SecondoHome=${RUNNER_TEMP}/tsan-dbs" >> "$GITHUB_ENV" + export PATH="${GITHUB_WORKSPACE}/bin:${PATH}" + export SECONDO_CONFIG="${GITHUB_WORKSPACE}/bin/SecondoConfig.ini" + export SECONDO_PARAM_SecondoHome="${RUNNER_TEMP}/tsan-dbs" + mkdir -p "$SECONDO_PARAM_SecondoHome" + cd bin + printf 'create database berlintest;\nrestore database berlintest from berlintest;\nclose database;\nquit;\n' \ + | ./SecondoTTYBDB + + # The replacement for the old app-tsan connection test, and strictly more + # than it was: the client process and the server process on the other end + # of the socket are now both instrumented. + - name: Test concurrent client connections + timeout-minutes: 30 + run: | + cd bin + ./SecondoMonitor --daemon \ + --pidfile "${RUNNER_TEMP}/secondo-monitor.pid" \ + --log "${RUNNER_TEMP}/secondo-monitor.log" + ./SecondoMonitor --health + cd ../Tests/csloadtest + rc=0 + ./app --db berlintest --threads 4 --rounds 2 || rc=$? + # Shut the monitor down before run-tests.sh, whose suites start + # monitors of their own and expect the default port to be free. + SecondoMonitor --shutdown \ + --pidfile "${RUNNER_TEMP}/secondo-monitor.pid" || true + exit $rc + + # The .test suites and the .examples files are breadth over the operator + # set, run in one process, and TSan has nothing to say about them -- they + # would only multiply the job's run time. What is left is the part that + # runs real SecondoMonitor process trees driven by parallel clients. + # The optimizer is skipped because SecondoBDB embeds an uninstrumented + # libswipl that threads, garbage-collects and shifts stacks on its own. + - name: Test SECONDO + run: | + cd CM-Scripts/ + SECONDO_SKIP_TESTRUNNER_TESTS=true \ + SECONDO_SKIP_ALGEBRA_TESTS=true \ + SECONDO_SKIP_OPTIMIZER_TESTS=true \ + ./run-tests.sh -tty /tmp/secondotest-tsan 3600 + + - name: Show failed test logs + if: failure() + run: | + for f in "${GITHUB_WORKSPACE}"/bin/*.test.log; do + [ -f "$f" ] || continue + echo "########## $f"; tail -80 "$f" + done + find /tmp/secondotest-tsan -type f 2>/dev/null | while read -r f; do + echo "########## $f"; tail -80 "$f" + done || true + for d in "${TMPDIR:-/tmp}"/TestClientServer.* /tmp/TestClientServer.*; do + [ -d "$d" ] || continue + find "$d" -type f 2>/dev/null | while read -r f; do + echo "########## $f"; tail -80 "$f" + done + done || true + for f in "${GITHUB_WORKSPACE}"/bin/Checkpoint.msg \ + "${GITHUB_WORKSPACE}"/bin/server.msg/* \ + "${RUNNER_TEMP}/secondo-monitor.log"; do + [ -f "$f" ] || continue + echo "########## $f"; tail -80 "$f" + done || true + build-macos: # Builds the C++ kernel + TTY, the Java GUI and the embedded-Prolog # optimizer engine, then runs the full test suite including the optimizer diff --git a/.gitignore b/.gitignore index 7431ee7f0..34460df9c 100644 --- a/.gitignore +++ b/.gitignore @@ -224,7 +224,8 @@ makefile.algebras /Tests/Testspecs/temp_nested_list /Tests/Testspecs/*/temp_nested_list /Tests/csloadtest/app -/Tests/csloadtest/app-tsan +/Tests/csloadtest/app-check +/Tests/csloadtest/check/ # Selftest /Selftest/result*_err diff --git a/CM-Scripts/run-tests.sh b/CM-Scripts/run-tests.sh index 6196c16df..bfe256348 100755 --- a/CM-Scripts/run-tests.sh +++ b/CM-Scripts/run-tests.sh @@ -118,35 +118,49 @@ fi declare -i error=0 # -# Tests executed by the TestRunner -# -# The first test create databases -dbTest="createdb.test" -dbFile="$buildDir/bin/$dbTest" +# Tests executed by the TestRunner (skippable via SECONDO_SKIP_TESTRUNNER_TESTS). +# These drive a kernel linked into TestRunner and are, apart from createdb.test +# which sets up the databases the rest of this section shares, single threaded. +# The sections below each create their own databases, so skipping this one does +# not disturb them. +# +if [ "${SECONDO_SKIP_TESTRUNNER_TESTS:-}" == "true" ]; then + echo "*** Skipping test suites (SECONDO_SKIP_TESTRUNNER_TESTS=true) ***" +else + # The first test create databases + dbTest="createdb.test" + dbFile="$buildDir/bin/$dbTest" -testSuites=$(find $buildDir/Tests -wholename "*.test") + testSuites=$(find $buildDir/Tests -wholename "*.test") -#echo -e "$testSuites" -#echo "ldd: "$(ldd $SECONDO_BUILD_DIR/bin/SecondoBDB) + #echo -e "$testSuites" + #echo "ldd: "$(ldd $SECONDO_BUILD_DIR/bin/SecondoBDB) -echo "*** Executing test suites ***" -for testName in $dbFile $testSuites; do - runDir=${testName%/*} - testFile=${testName##*/} - runTest $runDir $testFile "time $runnerCmd -i ${testFile}" $timeOutMax -done + echo "*** Executing test suites ***" + for testName in $dbFile $testSuites; do + runDir=${testName%/*} + testFile=${testName##*/} + runTest $runDir $testFile "time $runnerCmd -i ${testFile}" $timeOutMax + done +fi # -# Algebra tests +# Algebra tests (skippable via SECONDO_SKIP_ALGEBRA_TESTS). Selftest runs each +# operator's example queries in one process, so like the section above this is +# breadth over the operator set rather than concurrency coverage. # -echo "*** Executing algebra tests ***" -exampleFiles=$(find $buildDir/bin/tmp -wholename "*.examples") -for testName in $exampleFiles; do - runDir=${testName%/*} - testFile=${testName##*/} - runTest $runDir $testFile "cd $buildDir/bin/; time Selftest tmp/${testFile}" $timeOutMax -done +if [ "${SECONDO_SKIP_ALGEBRA_TESTS:-}" == "true" ]; then + echo "*** Skipping algebra tests (SECONDO_SKIP_ALGEBRA_TESTS=true) ***" +else + echo "*** Executing algebra tests ***" + exampleFiles=$(find $buildDir/bin/tmp -wholename "*.examples") + for testName in $exampleFiles; do + runDir=${testName%/*} + testFile=${testName##*/} + runTest $runDir $testFile "cd $buildDir/bin/; time Selftest tmp/${testFile}" $timeOutMax + done +fi # diff --git a/CM-Scripts/tsan.supp b/CM-Scripts/tsan.supp new file mode 100644 index 000000000..112d700b8 --- /dev/null +++ b/CM-Scripts/tsan.supp @@ -0,0 +1,33 @@ +# ThreadSanitizer suppressions for the build-linux-tsan CI job. +# +# The job builds the whole tree with "make SECONDO_TSAN=1", so everything +# SECONDO itself compiles is instrumented. What is not instrumented is the +# third-party code linked into the same binaries -- Berkeley DB above all, +# which arbitrates with its own test-and-set mutexes in mmap'd shared regions +# rather than with pthread primitives. TSan intercepts pthread calls, so it +# sees the synchronization of libraries that use them; it cannot see BDB's, +# and reports the accesses it protects as races. +# +# Add an entry here only for a report that is genuinely not ours to fix, and +# say in a comment why. A suppression for SECONDO code is a bug being hidden: +# fix the race instead. Entries are matched against the demangled frames of a +# report, so keep them as narrow as the report allows. +# +# Format (see https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions): +# race: +# deadlock:<...> +# called_from_lib: +# +# This file must exist even when empty -- TSAN_OPTIONS names it, and TSan +# aborts at startup if the path cannot be opened. +# +# Nothing is suppressed here yet, and the one class of noise seen so far is +# handled outside this file. Berkeley DB acquires locks out of order by design +# and untangles the result with its own deadlock detector (DB_LOCK_DETECT), so +# TSan's lock-order checker reported every one: a single restore of berlintest +# produced 20919 inversions inside libdb_cxx and no data race at all. That is +# switched off wholesale with detect_deadlocks=0 in the workflow rather than +# matched here, because the frames come out of a stripped system library and a +# "deadlock:libdb_cxx" entry would depend on the module name resolving. If the +# lock-order checker is ever wanted for SECONDO's own mutexes, drop that option +# and try such an entry here instead. diff --git a/Tests/csloadtest/README.md b/Tests/csloadtest/README.md index a0ad5b7f9..e7b7f0491 100644 --- a/Tests/csloadtest/README.md +++ b/Tests/csloadtest/README.md @@ -20,13 +20,28 @@ Host and port default to `SecondoHost`/`SecondoPort` in `Config.ini`, which is copied from `bin/SecondoConfig.ini` by the makefile, so they cannot drift out of step with the installation. -Two workloads, selected with `--mode`: +The workloads, selected with `--mode` (default `both` = `connect` + `parse`): - `connect` opens and closes connections from several threads while running commands on them. This is the path with the process-wide state: runtime flags, the message centre's handler list, the connection counter. Needs a server. - `parse` builds and re-reads nested lists, each thread on its own `NestedList`. This is the path through `NLParser`, and needs no server. +- `shared` puts every thread on *one* `NestedList`, which is what the running + system does -- `Distributed2` copies each worker's result into the global + list and reads it back, and every `Out` function an operator runs in a worker + thread uses the global `nl` too. Needs no server. +- `handover` passes one list from thread to thread, each joined before the next + starts. That keeps the threading contract, so it must run clean. +- `contract` breaks the contract on purpose and must abort. Needs a build with + `-DNL_CHECK_CONCURRENCY`; see below. + +Use `--list-mem 1` with the last three: it sizes the tables to a single page, +so `BigArray` runs out of room within the first rounds and has to `ftruncate` +and `mremap` while other threads read through the mapping it is moving. At the +default sizes the whole workload fits in the mapping made at construction and +never grows it. The run prints how far the node table grew and fails if it did +not outgrow that first mapping. `--expect-binary` / `--expect-text` assert which list transfer mode the server announced. The mode is negotiated, so the *server* decides it: to cover the @@ -36,21 +51,51 @@ from quietly testing the same path twice. ## ThreadSanitizer - make tsan # ./app-tsan - TSAN_OPTIONS="halt_on_error=0" ./app-tsan --mode connect --threads 4 --rounds 2 - -The client sources are recompiled with `-fsanitize=thread` into `tsan/` and -archived there, so the objects of the normal build are left alone and the -linker pulls in archive members exactly as the real build does. Everything the -test links is instrumented; mixing instrumented and uninstrumented objects is -what produces phantom reports. - -`make tsan-check-sources` fails if the source list has drifted from `CSLIBS` in -`apis/api_cpp/cs/makefile` -- an archive member we forget to instrument would -make the sanitizer quietly blind. - -Instrument this harness rather than the Python bridge in `WebUI/backend`: -CPython is not instrumented and would bury real findings in noise. +There is no sanitizer build here any more. This directory used to recompile a +hand-listed mirror of the client library's sources with `-fsanitize=thread`, +which left the `QueryProcessor`, every algebra and the four server daemons +uninstrumented -- and instrumenting a subset is also what produces phantom +reports, because TSan cannot see the synchronization in objects it was not +compiled into. + +Instead the whole tree is built with it, into a clean tree: + + make SECONDO_TSAN=1 compileJava=false + make -C apis/api_cpp/cs SECONDO_TSAN=1 + cd Tests/csloadtest && make && ./app --mode shared --threads 4 --rounds 2 --list-mem 1 + +`./app` is then itself instrumented, and so is the server it connects to. See +the `SECONDO_TSAN` block in `makefile.env`; objects built with and without it +must never be mixed, and it forces `-O1` where the tree otherwise builds +without any `-O`, so every object changes. The `build-linux-tsan` CI job does +exactly this, with `TSAN_OPTIONS` and `CM-Scripts/tsan.supp`. + +Instrument SECONDO rather than the Python bridge in `WebUI/backend`: CPython is +not instrumented and would bury real findings in noise. + +## The threading contract checker + +TSan reports accesses that race but do not overlap in time. The other half of +`NestedList`'s contract -- "operations on the same list need the caller's own +lock" -- is about accesses that *do* overlap, and it is the caller's to keep, +so it cannot be checked by construction: + + make app-check # ./app-check + ./app-check --mode shared --threads 8 --rounds 5 --list-mem 1 # must pass + ./app-check --mode handover --threads 4 --rounds 5 --list-mem 1 # must pass + ./app-check --mode contract --threads 4 --rounds 1 --list-mem 1 # must abort + +This is not a sanitizer build. It recompiles `NestedList.cpp` and this test with +`-DNL_CHECK_CONCURRENCY` over the stock `libsecondo.a` -- only those two, since +`NodeAccessGuard` is a free class rather than a `NestedList` member, so the +class layout does not change, and the `NL_WRITING_NODE` / `NL_READING_NODE` +macros appear in no other translation unit. The objects are named ahead of +`-lsecondo`, so the archive's own copy of `NestedList.o` is never pulled in. + +Assert both directions. A checker that never fires and one that always fires +look alike from one side only, and what the `contract` run has to produce is +the report `NestedList threading contract was broken` -- not merely a non-zero +exit, which a crash or a binary built without the define gives just as well. ## Where it stands diff --git a/Tests/csloadtest/makefile b/Tests/csloadtest/makefile index 95102e8dd..c2f6185f9 100644 --- a/Tests/csloadtest/makefile +++ b/Tests/csloadtest/makefile @@ -40,100 +40,55 @@ ClientLoadTest.o: ClientLoadTest.cpp ######################################################################## # -# ThreadSanitizer build: "make tsan", then ./app-tsan +# Concurrency contract checker: "make app-check", then ./app-check # -# Checks that the client library tolerates several connections in one -# process. Everything the test links has to be instrumented for the report -# to mean anything, so the client sources are recompiled here rather than -# taken from libsecondo.a -- into ./tsan/, so that the objects the normal -# build produced are left alone. Mixing instrumented and uninstrumented -# objects is what produces phantom reports. +# The other half of NestedList's threading contract -- "operations on the +# same list need the caller's own lock" -- is the caller's to keep, so it +# cannot be checked by construction. -DNL_CHECK_CONCURRENCY compiles in a +# checker that turns breaking it into an immediate abort naming both threads +# and the node, and --mode contract breaks it on purpose. +# +# It is not a sanitizer build: ThreadSanitizer catches accesses that race but +# do not overlap in time, this catches overlapping ones, and the two are +# complementary. TSan comes from a whole-tree "make SECONDO_TSAN=1" instead +# (see makefile.env), which is why nothing here has to be instrumented. +# +# The flag reaches exactly two translation units. NodeAccessGuard is a free +# class, not a NestedList member, so the class layout does not change and +# the rest of libsecondo.a can be used as built; and the NL_WRITING_NODE / +# NL_READING_NODE macros appear only in NestedList.cpp. The test itself needs +# it for --mode contract. # ######################################################################## BD=$(SECONDO_BUILD_DIR) -# The sources behind libsecondo.a. Mirrors CSLIBS in apis/api_cpp/cs/makefile; -# "make tsan-check-sources" fails if the two have drifted apart. -CS_SOURCES = \ - $(BD)/ClientServer/SecondoInterfaceCS.cpp \ - $(BD)/ClientServer/SocketIO.cpp \ - $(BD)/QueryProcessor/SecondoInterfaceGeneral.cpp \ - $(BD)/Tools/BigArray/BigArray.cpp \ - $(BD)/Tools/NestedLists/NestedList.cpp \ - $(BD)/Tools/NestedLists/NList.cpp \ - $(BD)/Tools/NestedLists/NLParser.cpp \ - $(BD)/Tools/NestedLists/NLScanner.cpp \ - $(BD)/Tools/NestedLists/NLLex.cpp \ - $(BD)/Tools/NestedLists/NLParser.tab.cpp \ - $(BD)/Tools/Utilities/WinUnix.cpp \ - $(BD)/Tools/Utilities/Environment.cpp \ - $(BD)/Tools/Utilities/FileSystem.cpp \ - $(BD)/Tools/Utilities/UtilFunctions.cpp \ - $(BD)/Tools/Utilities/Application.cpp \ - $(BD)/Tools/Utilities/Profiles.cpp \ - $(BD)/Tools/Utilities/Messenger.cpp \ - $(BD)/Tools/Utilities/StringUtils.cpp \ - $(BD)/Tools/Utilities/Base64.cpp \ - $(BD)/Tools/Utilities/AlmostEqual.cpp \ - $(BD)/Tools/Utilities/satof.cpp \ - $(BD)/Tools/Flob/ExternalFileCache.cpp \ - $(BD)/Tools/Flob/FlobManager.cpp \ - $(BD)/Tools/Flob/PersistentFlobCache.cpp \ - $(BD)/UserInterfaces/cmsg.cpp \ - $(BD)/UserInterfaces/SQLLanguage.cpp - -TSAN_LIB_OBJECTS = $(addprefix tsan/,$(notdir $(CS_SOURCES:.cpp=.o))) - -# -O1 keeps the stack frames that make a TSan report readable; the sanitizer -# dominates the run time in any case. -TSAN_FLAGS = -fsanitize=thread -fno-omit-frame-pointer -g -O1 -TSAN_INCLUDES = -I$(BD)/Tools/NestedLists -I../include -I$(BDBINCLUDE) - -vpath %.cpp $(sort $(dir $(CS_SOURCES))) - -tsan/%.o: %.cpp - @mkdir -p tsan - $(CPPC) -c -o $@ $(CCFLAGS) $(TSAN_FLAGS) $(TSAN_INCLUDES) $< \ - -MMD -MP -MF tsan/$*.dep - -tsan/ClientLoadTest.o: ClientLoadTest.cpp - @mkdir -p tsan - $(CPPC) -c -o $@ $(CCFLAGS) $(TSAN_FLAGS) $(TSAN_INCLUDES) $< \ - -MMD -MP -MF tsan/ClientLoadTest.dep - --include $(wildcard tsan/*.dep) - -.PHONY: tsan -tsan: Config.ini app-tsan$(EXEEXT) - -# Archived rather than linked object by object, exactly as libsecondo.a is: -# several members (the Flob caches) reference the storage interface, which a -# client never calls. Out of an archive the linker simply does not pull them -# in; listing every object instead would demand symbols nothing uses. -tsan/libsecondo-tsan.a: $(TSAN_LIB_OBJECTS) - $(AR) -rs $@ $(TSAN_LIB_OBJECTS) - -app-tsan$(EXEEXT): tsan/ClientLoadTest.o tsan/libsecondo-tsan.a - $(CPPC) -o $@ tsan/ClientLoadTest.o -Ltsan -lsecondo-tsan $(TSAN_FLAGS) \ - -L$(BDB_LIB_DIR) -l$(BERKELEY_DB_LIB) -lpthread -lstdc++ \ - $(COMMON_LD_FLAGS) - -# Guards the list above against the archive growing a member we do not -# instrument -- a silently uninstrumented object makes TSan quietly blind. -.PHONY: tsan-check-sources -tsan-check-sources: - @ar t $(BD)/apis/api_cpp/cs/lib/libsecondo.a | sort > /tmp/cslt_ar.$$$$; \ - for s in $(CS_SOURCES); do basename $$s .cpp; done \ - | sed 's/$$/.o/' | sort > /tmp/cslt_ours.$$$$; \ - if diff -u /tmp/cslt_ar.$$$$ /tmp/cslt_ours.$$$$; then \ - echo "tsan source list matches libsecondo.a"; rc=0; \ - else \ - echo "tsan source list has drifted from apis/api_cpp/cs/makefile"; rc=1; \ - fi; \ - rm -f /tmp/cslt_ar.$$$$ /tmp/cslt_ours.$$$$; exit $$rc +CHECK_FLAGS = -DNL_CHECK_CONCURRENCY +CHECK_INCLUDES = -I$(BD)/Tools/NestedLists -I../include -I$(BDBINCLUDE) + +check/ClientLoadTest.o: ClientLoadTest.cpp + @mkdir -p check + $(CPPC) -c -o $@ $(CCFLAGS) $(CHECK_FLAGS) $(CHECK_INCLUDES) $< \ + -MMD -MP -MF check/ClientLoadTest.dep + +check/NestedList.o: $(BD)/Tools/NestedLists/NestedList.cpp + @mkdir -p check + $(CPPC) -c -o $@ $(CCFLAGS) $(CHECK_FLAGS) $(CHECK_INCLUDES) $< \ + -MMD -MP -MF check/NestedList.dep + +-include $(wildcard check/*.dep) + +# Our NestedList.o is named ahead of -lsecondo (which LDFLAGS supplies), so +# every NestedList symbol is already resolved by the time the linker reaches +# the archive and its own uninstrumented copy is never pulled in. +app-check$(EXEEXT): check/ClientLoadTest.o check/NestedList.o + $(CPPC) -o $@ check/ClientLoadTest.o check/NestedList.o \ + $(LDFLAGS) $(DEFAULTLIB) $(COMMON_LD_FLAGS) + +.PHONY: check +check: Config.ini app-check$(EXEEXT) .PHONY:clean clean: rm -f Config.ini app$(EXEEXT) ClientLoadTest.o - rm -rf tsan app-tsan$(EXEEXT) + rm -rf check app-check$(EXEEXT) diff --git a/makefile.env b/makefile.env index f17684b20..1527bfab9 100755 --- a/makefile.env +++ b/makefile.env @@ -236,19 +236,27 @@ endif DEFAULTCCFLAGS += -pthread -DTHREAD_SAFE COMMON_LD_FLAGS += -lboost_thread -# Build with ThreadSanitizer: "make SECONDO_TSAN=1". Used to check that the -# client library really tolerates several connections in one process -- -# Tests/csloadtest drives it. Only what the test links has to be instrumented, -# so in practice this is set for the objects of the client library -# (apis/api_cpp/cs) and the test itself, not for a whole tree build. +# Build with ThreadSanitizer: "make SECONDO_TSAN=1". Meant for a whole tree +# build -- the kernel, every algebra and the server processes -- which is what +# the build-linux-tsan CI job does. Instrumenting only part of a binary is what +# produces phantom reports, so there is no supported way to sanitize a subset. +# +# Both link variables are set because neither reaches every binary on its own: +# COMMON_LD_FLAGS covers the ClientServer daemons, the UserInterfaces apps, +# Tests/ and SpecParser, while a handful of makefiles (Algebras/Temporal2, +# Raster2/Import, HadoopParallel/askFlob, Optimizer/MemoryAllocation, +# InteractiveQueryEditor) name only LDFLAGS. Appending to LDFLAGS is safe only +# here, below the include of makefile.$(platform), which assigns it with ":=". # # -O1 rather than -O2 because TSan keeps more of the stack frames that make a -# report readable, and the sanitizer dominates the run time anyway. Objects -# built with and without it must not be mixed: build into a clean tree, or -# "make clean" first. +# report readable, and the sanitizer dominates the run time anyway. Note that +# the tree is otherwise built without any -O, so this changes every object -- +# and objects built with and without it must not be mixed: build into a clean +# tree, or "make clean" first. ifdef SECONDO_TSAN DEFAULTCCFLAGS += -fsanitize=thread -fno-omit-frame-pointer -g -O1 COMMON_LD_FLAGS += -fsanitize=thread +LDFLAGS += -fsanitize=thread endif # the macro below should be used in local makefiles to