Problem 1557

Summary:	Invalid code in G4CascadeParameters.cc; happens to work on Mac and Linux
Product:	Geant4	Reporter:	Tom Roberts <tjrob>
Component:	processes/hadronic/models/cascade	Assignee:	Michael Kelsey <kelsey>
Status:	RESOLVED FIXED
Severity:	critical
Priority:	P5
Version:	9.6
Hardware:	All
OS:	All

Description Tom Roberts 2014-01-04 23:44:08 CET

G4CascadeParameters.cc assumes a specific load order, which is invalid C++.

The problem is that other static initializers call G4CascadeParameters.cc::Instance(), which has a static initializer. If one of the other files is loaded before G4CascadeParameters.cc, this crashes. Moreover this is VERY difficult to debug as it happens before main() is entered, and Microsoft debugging tools won't let you set a breakpoint before main.

On Mac and Linux, this HAPPENS to work, because for g++ the static initializer inside Instance() is executed on first call. But on Windows, VC++ puts it into the file's set of static initializers, and their order of execution depends on load order. Both methods are valid C++, and the code MUST NOT depend on which is used.

Fix: in G4CascadeParameters.cc:
  //TJR static const G4CascadeParameters theInstance;
  //TJR return &theInstance;
  static const G4CascadeParameters *theInstance = 0;
  if(theInstance == 0) theInstance = new G4CascadeParameters;
  return theInstance;

This ensures that the initialization always happens on first call.

It would be wise to proactively search for other such static initializers.

Comment 1 Tom Roberts 2014-01-05 00:20:55 CET

That fix is not complete, as this still crashes in G4CascadeParameters::G4CascadeParameters():
    messenger = new G4CascadeParamMessenger(this);

I'm tired of spending 2 days debugging your code (it's VERY difficult to debug static initializers in Windows), so I just replaced it with messenger=0.

I think you need to re-assess how your code uses static initializers. For sure in cascade, but perhaps throughout Geant4.

It would be wise to proactively search for other such static initializers.

Comment 2 Michael Kelsey 2014-01-05 05:32:55 CET

You are correct.  This should have been fixed as part of the multithreaded development for 10.0 (it's not multithread safe, either).  I don't know if there is going to be another 9.6 patch done, but we will make patch-compatible versions for both 9.6 and 10.0, to be used when patches are deployed.

The messenger itself should be safe, as it is a data member of the (singleton) class.

We do have multiple Windows platforms as part of our (nightly) software testing system (Windows 7 with VC++ 9, VC++ 10, and VC++ 11).  I am not familiar enough (at all) with Windows to understand why the situation you encountered was not caught.

Comment 3 Michael Kelsey 2014-01-06 22:01:50 CET

I have committed and tagged this fix, which is consistent with your suggestion above (and I note that your suggestion is the same as we recommend to our developers; I didn't follow our guidelines).  

Compared to your own changes above, my commit moves the singleton pointer out of the Instance() function definition to file scope via the anonymous namespace:

namespace {
  static const G4CascadeParameters* theInstance = 0;
}

Those three lines are placed immediately before the function definition (the "static" keyword is redundant with anon-namespace, but is not invalid, and helps human readers recognize that the object is a singleton.  This method is friendlier to multithreaded compilation and linking.

I am also including your suppression of the G4CascadeParamMessenger object.  I will need to review the thread safety of the two objects together and make a better fix, now that we are enforcing multithread compatibility in 10.0 and beyond.  In the mean time, the lack of Messenger (an unadvertised feature meant for G4 developers' use) will not affect end-user code.