From 586744f25edd05a46daf5136b9cc7bbfb6d1c68d Mon Sep 17 00:00:00 2001
From: Frank Winklmeier <frank.winklmeier@cern.ch>
Date: Thu, 25 Aug 2022 09:30:20 +0200
Subject: [PATCH 1/2] G4AtlasAlg: enable thread-checker
Mostly suppressing warnings of "unsafe" code that is used only during
single-threaded execution.
---
.../G4AtlasAlg/G4AtlasAlg/ATLAS_CHECK_THREAD_SAFETY | 1 +
.../G4AtlasAlg/G4AtlasAlg/G4AtlasMTRunManager.h | 3 ++-
.../G4AtlasAlg/G4AtlasAlg/G4AtlasRunManager.h | 3 ++-
Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasAlg.cxx | 12 ++++++++----
.../G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx | 7 +++----
.../G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx | 7 +++----
.../G4AtlasAlg/src/G4AtlasWorkerRunManager.cxx | 2 +-
7 files changed, 20 insertions(+), 15 deletions(-)
create mode 100644 Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/ATLAS_CHECK_THREAD_SAFETY
diff --git a/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/ATLAS_CHECK_THREAD_SAFETY b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/ATLAS_CHECK_THREAD_SAFETY
new file mode 100644
index 000000000000..4818d903975b
--- /dev/null
+++ b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/ATLAS_CHECK_THREAD_SAFETY
@@ -0,0 +1 @@
+Simulation/G4Atlas/G4AtlasAlg
diff --git a/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasMTRunManager.h b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasMTRunManager.h
index bcf0586c9147..40af0672a17d 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasMTRunManager.h
+++ b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasMTRunManager.h
@@ -16,6 +16,7 @@
#include "GaudiKernel/ToolHandle.h"
#include "GaudiKernel/ServiceHandle.h"
#include "AthenaBaseComps/AthMessaging.h"
+#include "CxxUtils/checker_macros.h"
// G4Atlas includes
#include "G4AtlasInterfaces/IPhysicsListSvc.h"
@@ -38,7 +39,7 @@ class G4AtlasMTRunManager: public G4MTRunManager, public AthMessaging {
public:
/// Get the (pure) singleton instance
- static G4AtlasMTRunManager* GetG4AtlasMTRunManager();
+ static G4AtlasMTRunManager* GetG4AtlasMTRunManager ATLAS_NOT_THREAD_SAFE ();
/// G4 function called at end of run
void RunTermination() override final;
diff --git a/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasRunManager.h b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasRunManager.h
index da857be8d499..c4b50d29e6eb 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasRunManager.h
+++ b/Simulation/G4Atlas/G4AtlasAlg/G4AtlasAlg/G4AtlasRunManager.h
@@ -14,6 +14,7 @@
// Athena headers
#include "AthenaBaseComps/AthMessaging.h"
+#include "CxxUtils/checker_macros.h"
#include "G4AtlasInterfaces/ISensitiveDetectorMasterTool.h"
#include "G4AtlasInterfaces/IFastSimulationMasterTool.h"
#include "G4AtlasInterfaces/IPhysicsListSvc.h"
@@ -33,7 +34,7 @@ public:
virtual ~G4AtlasRunManager() {}
/// Retrieve the singleton instance
- static G4AtlasRunManager* GetG4AtlasRunManager();
+ static G4AtlasRunManager* GetG4AtlasRunManager ATLAS_NOT_THREAD_SAFE ();
/// Does the work of simulating an ATLAS event
bool ProcessEvent(G4Event* event);
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasAlg.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasAlg.cxx
index d7554c79da65..cd077b095d5d 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasAlg.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasAlg.cxx
@@ -8,6 +8,7 @@
#include "G4AtlasAlg/G4AtlasActionInitialization.h"
#include "AthenaKernel/RNGWrapper.h"
+#include "CxxUtils/checker_macros.h"
// Can we safely include all of these?
#include "G4AtlasAlg/G4AtlasMTRunManager.h"
@@ -114,7 +115,8 @@ void G4AtlasAlg::initializeOnce()
// Create the (master) run manager
if(m_useMT) {
#ifdef G4MULTITHREADED
- auto* runMgr = G4AtlasMTRunManager::GetG4AtlasMTRunManager();
+ auto* runMgr ATLAS_THREAD_SAFE = // protected by std::call_once above
+ G4AtlasMTRunManager::GetG4AtlasMTRunManager();
m_physListSvc->SetPhysicsList();
runMgr->SetDetGeoSvc( m_detGeoSvc.typeAndName() );
runMgr->SetFastSimMasterTool(m_fastSimTool.typeAndName() );
@@ -134,7 +136,8 @@ void G4AtlasAlg::initializeOnce()
}
// Single-threaded run manager
else {
- auto* runMgr = G4AtlasRunManager::GetG4AtlasRunManager();
+ auto* runMgr ATLAS_THREAD_SAFE = // safe because single-threaded
+ G4AtlasRunManager::GetG4AtlasRunManager();
m_physListSvc->SetPhysicsList();
runMgr->SetRecordFlux( m_recordFlux, std::make_unique<G4AtlasFluxRecorder>() );
runMgr->SetLogLevel( int(msg().level()) ); // Synch log levels
@@ -275,7 +278,7 @@ void G4AtlasAlg::finalizeOnce()
StatusCode G4AtlasAlg::execute()
{
- static int n_Event=0;
+ static std::atomic<unsigned int> n_Event=0;
ATH_MSG_DEBUG("++++++++++++ G4AtlasAlg execute ++++++++++++");
#ifdef G4MULTITHREADED
@@ -357,7 +360,8 @@ StatusCode G4AtlasAlg::execute()
#endif
}
else {
- auto* workerRM = G4AtlasRunManager::GetG4AtlasRunManager();
+ auto* workerRM ATLAS_THREAD_SAFE = // single-threaded case
+ G4AtlasRunManager::GetG4AtlasRunManager();
abort = workerRM->ProcessEvent(inputEvent);
}
if (abort) {
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
index 656c8686afe1..4fe05a827842 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
@@ -31,11 +31,10 @@ G4AtlasMTRunManager::G4AtlasMTRunManager()
{}
-G4AtlasMTRunManager* G4AtlasMTRunManager::GetG4AtlasMTRunManager()
+G4AtlasMTRunManager* G4AtlasMTRunManager::GetG4AtlasMTRunManager ATLAS_NOT_THREAD_SAFE ()
{
- static G4AtlasMTRunManager* thisManager = nullptr;
- if (!thisManager) { thisManager = new G4AtlasMTRunManager; }
- return thisManager;
+ static G4AtlasMTRunManager thisManager;
+ return &thisManager;
}
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
index 5ac05962741b..1190c90f2cda 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
@@ -30,11 +30,10 @@ G4AtlasRunManager::G4AtlasRunManager()
{ }
-G4AtlasRunManager* G4AtlasRunManager::GetG4AtlasRunManager()
+G4AtlasRunManager* G4AtlasRunManager::GetG4AtlasRunManager ATLAS_NOT_THREAD_SAFE ()
{
- static G4AtlasRunManager *thisManager = nullptr;
- if (!thisManager) { thisManager = new G4AtlasRunManager; } // Leaked
- return thisManager;
+ static G4AtlasRunManager thisManager;
+ return &thisManager;
}
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasWorkerRunManager.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasWorkerRunManager.cxx
index 7126ee071e15..2c14ef6341fc 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasWorkerRunManager.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasWorkerRunManager.cxx
@@ -95,7 +95,7 @@ void G4AtlasWorkerRunManager::InitializeGeometry()
const std::string methodName = "G4AtlasWorkerRunManager::InitializeGeometry";
// I don't think this does anything
- if(fGeometryHasBeenDestroyed) {
+ if(G4RunManager::IfGeometryHasBeenDestroyed()) {
G4TransportationManager::GetTransportationManager()->ClearParallelWorlds();
}
--
GitLab
From c0ccb7b18e07b8de9682b8a4ac437f52e2673bbb Mon Sep 17 00:00:00 2001
From: Frank Winklmeier <frank.winklmeier@cern.ch>
Date: Tue, 30 Aug 2022 11:59:44 +0200
Subject: [PATCH 2/2] G4AtlasAlg: revert fixes to RunManager singletons
Fixing the leaking RunManger singletons results in a crash during the
job termination. Likely some ordering issues in the cleanup of static
variables. See !56205 for the full Valgrind report.
---
Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx | 5 +++--
Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
index 4fe05a827842..1023a73b5c4d 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasMTRunManager.cxx
@@ -33,8 +33,9 @@ G4AtlasMTRunManager::G4AtlasMTRunManager()
G4AtlasMTRunManager* G4AtlasMTRunManager::GetG4AtlasMTRunManager ATLAS_NOT_THREAD_SAFE ()
{
- static G4AtlasMTRunManager thisManager;
- return &thisManager;
+ static G4AtlasMTRunManager* thisManager = nullptr;
+ if (!thisManager) { thisManager = new G4AtlasMTRunManager; }
+ return thisManager;
}
diff --git a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
index 1190c90f2cda..4f158fc6a511 100644
--- a/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
+++ b/Simulation/G4Atlas/G4AtlasAlg/src/G4AtlasRunManager.cxx
@@ -32,8 +32,9 @@ G4AtlasRunManager::G4AtlasRunManager()
G4AtlasRunManager* G4AtlasRunManager::GetG4AtlasRunManager ATLAS_NOT_THREAD_SAFE ()
{
- static G4AtlasRunManager thisManager;
- return &thisManager;
+ static G4AtlasRunManager *thisManager = nullptr;
+ if (!thisManager) { thisManager = new G4AtlasRunManager; } // Leaked
+ return thisManager;
}
--
GitLab