Problem 1557 - Invalid code in G4CascadeParameters.cc; happens to work on Mac and Linux
Summary: Invalid code in G4CascadeParameters.cc; happens to work on Mac and Linux
Status: RESOLVED FIXED
Alias: None
Product: Geant4
Classification: Unclassified
Component: processes/hadronic/models/cascade (show other problems)
Version: 9.6
Hardware: All All
: P5 critical
Assignee: Michael Kelsey
URL:
Depends on:
Blocks:
 
Reported: 2014-01-04 23:44 CET by Tom Roberts
Modified: 2014-01-06 22:01 CET (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this problem.
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.