CFD Online Logo CFD Online URL
www.cfd-online.com
[Sponsors]
Home > Forums > Software User Forums > OpenFOAM > OpenFOAM Bugs

Fixed staticinitorder bugs in getEnv and dotFoam

Register Blogs Members List Search Today's Posts Mark Forums Read

Reply
 
LinkBack Thread Tools Search this Thread Display Modes
Old   December 25, 2008, 08:28
Default dotFoam and getEnv are used in
  #1
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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
mwild is offline   Reply With Quote

Old   December 26, 2008, 15:01
Default The issue of the order of init
  #2
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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 is offline   Reply With Quote

Old   December 26, 2008, 19:03
Default I have now checked through the
  #3
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 27, 2008, 09:56
Default I see this problem on Mac OS X
  #4
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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?
mwild is offline   Reply With Quote

Old   December 27, 2008, 15:11
Default > I'm not sure I personally li
  #5
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
> 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
henry is offline   Reply With Quote

Old   December 27, 2008, 16:03
Default >> initialization on first use
  #6
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
>> 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
mwild is offline   Reply With Quote

Old   December 27, 2008, 18:42
Default I tested the "initialization o
  #7
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 05:10
Default I inserted #include "string
  #8
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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
mwild is offline   Reply With Quote

Old   December 29, 2008, 05:57
Default I am not aware of any issue wi
  #9
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 06:52
Default The "uint" issue is simply due
  #10
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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 is offline   Reply With Quote

Old   December 29, 2008, 07:48
Default Hmmm, pretty strange: For test
  #11
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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.
mwild is offline   Reply With Quote

Old   December 29, 2008, 08:19
Default I have checked the C++ standar
  #12
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 08:31
Default Using "unsigned int" is Bernha
  #13
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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...
mwild is offline   Reply With Quote

Old   December 29, 2008, 09:17
Default My preference is to include
  #14
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 09:59
Default Mine too. But I now found the
  #15
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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
mwild is offline   Reply With Quote

Old   December 29, 2008, 10:16
Default I agree, the static initialize
  #16
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 11:23
Default I propose attached patch to so
  #17
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
I propose attached patch to solve this issue:

0001-BUGFIX-Static-initialization-order.patch

Michael
mwild is offline   Reply With Quote

Old   December 29, 2008, 12:27
Default Thanks for the patch, I have a
  #18
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Old   December 29, 2008, 12:55
Default Great news. Thanks for mention
  #19
Member
 
Michael Wild
Join Date: Mar 2009
Location: Bern, Switzerland
Posts: 79
Rep Power: 17
mwild is on a distinguished road
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
mwild is offline   Reply With Quote

Old   December 29, 2008, 14:40
Default We will probably release 1.5.1
  #20
Senior Member
 
Join Date: Mar 2009
Posts: 854
Rep Power: 22
henry is on a distinguished road
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
henry is offline   Reply With Quote

Reply

Thread Tools Search this Thread
Search this Thread:

Advanced Search
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are Off
Pingbacks are On
Refbacks are On


Similar Threads
Thread Thread Starter Forum Replies Last Post
Bugs in fvcsurfaceIntegrate su_junwei OpenFOAM Bugs 4 July 7, 2013 11:29
Bugs in FFT luca OpenFOAM Bugs 2 January 27, 2009 16:40
15x readme txt file of fixed bugs podallaire OpenFOAM Bugs 5 October 2, 2008 09:50
Wiki - Bugs Andy R Main CFD Forum 0 July 25, 2008 14:15
bugs in starcd 4.06 whitemelon Siemens 0 July 11, 2008 07:26


All times are GMT -4. The time now is 05:37.