[mwild]

dotFoam and getEnv are used in static initializers, but rely on static data themselves which resulted in undefined behaviour since the order of initialization is not specified. The static data these functions relied on was default-constructed instances of Foam::fileName and Foam::string, so I replaced the references to these static instances with default-constructed temporaries.

This also fixes the SIGSEGV when libOpenFOAM is loaded and the WM_PROJECT_DIR
or WM_PROJECT_INST_DIR environment variables where not defined.

Quite probably there are more such bugs present.

0001-Fixed-static-init-order-bugs-in-getEnv-and-dotFoam.patch

[henry]

The issue of the order of initialization is not being specified is being handled by having such inter-dependencies all included and compiled into a single file: global.C and this works on all the systems we have tried. Under what conditions does this not work for you?

H

[henry]

I have now checked through the code and your patch and agree with your conclusions: global.Cver does not include string.C and fileName.C and so there is currently a global construction order problem and I am surprised we have not seen a problem with it before. Under what conditions do you see the problem?

Your solution is an option and would only cause problems if the return of the functions were to be compared with string::null or fileName::null by pointer (probably not a good idea anyway). However, for consistency with the way these inter-dependencies are currently being handled the offending constructions of string::null, fileName::null and word::null should be transferred to global.Cver which should also solve the problem you currently see, let me know what you think.

H

[mwild]

I see this problem on Mac OS X (I know, that one is not supported...), under Ubuntu 8.10 it doesn't show up.

I'm not sure I personally like transferring all these initializers into global.Cver.

Probably a more robust solution would be to transform these static "null" member variables into static member functions which dynamically allocate the "null_" variable on first call (initialization on first use: http://www.parashift.com/c++-faq-lit...html#faq-10.13):


namespace Foam
{
class fileName
****: public string
{
****// ...the whole bunch...

public:
****// public static member functions
********//- Returns the null constructed instance
********static fileName& null()
********{
************// gets called the first time 'round
************static fileName* fn = new fileName();
************// return the null constructed instance
************return *fn;
********}

****// ...more stuff...
};

} // end namespace Foam



Of course one would then have to transform all these string::null, fileName::null and word::null references into function calls (which can probably be automated by grep and Vim or sed).

But then again, as this doesn't seem to be the current coding practice and if including string.C and fileName.C in global.Cver instead of compiling them directly solves the problem, that's certainly fine with me.

What I'm not quite sure of is how the linker determines initialization order. Is it the order in which the files are listed? If so, why then doesn't it work under Mac OS X?

[henry]

> I'm not sure I personally like transferring all these initializers into global.Cver.

I am not a big fan but it was the easiest way to get around some complex inter-dependencies given that the language specification is not at all helpful on this point.

> initialization on first use

I think this is a good suggestion at least for this case and I will look into implementing it at least for version 1.6, for 1.5.x I will probably move the string.C, fileName.C and word.C into global.Cver just to avoid the problem for now.

> What I'm not quite sure of is how the linker determines initialization order. Is it the order in which the files are listed? If so, why then doesn't it work under Mac OS X?

It probably depends on some hash and it is surprising we have not seen this problem on Linux bou are right that the code does not currently conform to the standard and should be fixed.

H

[mwild]

>> initialization on first use

> I think this is a good suggestion at least for this case and I will look into implementing it at least for version 1.6, for 1.5.x I will probably move the string.C, fileName.C and word.C into global.Cver just to avoid the problem for now.

That's good news. I'm not sure how to do the same thing for the streams though. I don't think that something like

Info() << "Hello World\n";

is very nice, and (although I didn't check), I think that some static initializers rely on streams. The case is different for the FatalErrorIn and WarningIn functions, where you have the chance for initialization on first use.

>> What I'm not quite sure of is how the linker determines initialization order. Is it the order in which the files are listed? If so, why then doesn't it work under Mac OS X?

> It probably depends on some hash and it is surprising we have not seen this problem on Linux bou are right that the code does not currently conform to the standard and should be fixed.

Agreed. That's the whole point of me sending in these bug reports :-) To make OpenFOAM even better, more reliable and robust to platform changes than it is now.

Michael

[henry]

I tested the "initialization on first use" method but there is a problem: it doesn't work for inlined functions. While this isn't a show-stopper it is a bit annoying and would prevent some useful but not essential optimizations. Overall I still prefer the current approach of minimizing the inter-dependencies and those that are unavoidable combine into a single file. To this end could you check that including string.C, fileName.C and word.C into global.Cver solves your problems?

Thanks

H

[mwild]

I inserted

#include "string.C"
#include "word.C"
#include "fileName.C"

at the very top of global.Cver, removed the corresponding lines from src/OpenFOAM/Make/files and compiled a fresh debug-version from commit 1409cb2 (plus the patches v1-v3 by Bernhard and fixing the "uint" issue).

However, the problem persists. Replacing the XXX::null references in Unix.C by XXX() works, however.

Any further ideas?

Michael

[henry]

I am not aware of any issue with "uint", if you would like any fixes applied for this could you please post them here for review.

Unfortunately I cannot reproduce the problem you are getting so I can only guess what it might be. My guess is there is still an ordering problem and the calls to the functions using XXX::null need to also be included in global.Cver after string.C etc. if they have not already been.

H

[mwild]

The "uint" issue is simply due to the fact that on Mac OS X uint is not typedef'ed by default to "unsigned int" (probably because . The only change necessary is:


diff --git a/src/OpenFOAM/primitives/uint/uintIO.C b/src/OpenFOAM/primitives/uint/uintIO.C
index 26b6422..5729cfd 100644
--- a/src/OpenFOAM/primitives/uint/uintIO.C
+++ b/src/OpenFOAM/primitives/uint/uintIO.C
@@ -66,7 +66,7 @@ Istream& operator>>(Istream& is, unsigned int& i)

if (t.isLabel())
{
- i = uint(t.labelToken());
+ i = (unsigned int)(t.labelToken());
}
else
{


The alternative would be to include sys/types.h, where uint is typedef'ed. I prefer the first solution, since AFAIK uint is not standard C++.

To the issue at hand: You are probably right, but including Unix.C is unfortunately not possible, since it belongs to libOSspecific, right? A crude solution would be to provide "private" functions in global.Cver which return references to XXX:null, and then use those in Unix.C. But this is a bit obscure to say the least. And would require a lot of discipline and care in the future, because one always would have to check whether this particular reference to XXX::null might get used in a static initializer.

[mwild]

Hmmm, pretty strange: For testing I removed the '#include "XXX.C"' again and moved the static initializers of XXX:null from XXX.C into global.Cver, and this works... Going to dig deeper.

[henry]

I have checked the C++ standard document and as you say "uint" is not mentioned, but "unsigned" is equivalent to "unsigned int", perhaps you could try that on Mac OS X? Anyway, I will make the change you suggest as I think "unsigned int" is clearer than "unsigned".

Including Unix.C should not be necessary, only the static initializations which use the functions in Unix.C, are they already included in global.Cver?

H

[mwild]

Using "unsigned int" is Bernhards ideas. He posted it somewhere on this Forum, if I remember correctly. Strangely it is not included in his patches.

As I said, it works if I move the static initializers of XXX:null into global.Cver. So probably I must have done something wrong or there's something else amiss. I'm investigating right now...

[henry]

My preference is to include

#include "string.C"
#include "word.C"
#include "fileName.C"

in global.Cver rather than just the static initializers but this may cause other problems. Including just the static initializers relies on the null constructors being inlined but this is probably OK.

H

[mwild]

Mine too. But I now found the problem (or I think I have):

Take word.C for example:

#include <openfoam/word.h>
#include <openfoam/debug.h>

// * * * * * * * * * * * * * * Static Data Members * * * * * * * * * * * * * //

const char* const Foam::word::typeName = "word";
int Foam::word::debug(Foam::debug::debugSwitch(word::t ypeName, 0));
const Foam::word Foam::word::null;

You can see that this calls debugSwitch(), which itself requires debugSwitchesPtr_ to be present. This pointer, however, is only initialized afterwards, when debug.C is included. Reversing the order of inclusion doesn't solve the problem, since in that case debug.C relies on word::null which is not initialized until later.

I tested this by "splitting" string.C, word.C and fileName.C into two parts using #ifdef's and included them twice in global.Cver: once at the top, once below the inclusion of debug.C. Using the defines I made sure that in the first include everything except the debugSwitch() calls get processed, and in the second include only the debugSwitch() calls. As expected, this worked fine.

Would it be fine to move the static initializers of string, word and fileName into a file src/OpenFOAM/primitives/strings/stringsGlobal.C which is included in global.Cver?

Michael

[henry]

I agree, the static initializers will have to be split into a separate file and I agree with your suggestion of having a stringsGlobal.C included in global.Cver. Thanks for sorting out this nasty problem.

H

[mwild]

I propose attached patch to solve this issue:

0001-BUGFIX-Static-initialization-order.patch

Michael

[henry]

Thanks for the patch, I have applied it and pushed the corrected version to our 1.5.x git repository.

I will also apply the corresponding changes to our development version with will become 1.6.

H

[mwild]

Great news. Thanks for mentioning me in the commit message.

Another question: Will 1.6 be based on the 1.5.x repo? I.e. will there be a smooth transition? Or is it a completely unrelated repository?

Michael

[henry]

We will probably release 1.5.1 based on the 1.5.x git repo version early next year and 1.6 based on our internal development line later in the year. Our development version already contains significant restructuring and hence not consistent with the 1.5.x line although the equivalent of all bug-fixes and patches applied to 1.5.x are also applied to our development line.

H