Bug in Singleton class

Hi!

I'm working with Visual Studio 2005 and have the following problem in my code:

I have several Structs with member variables. Those Structs are used by several classes as "Parameter classes", so if a member variable is changed, all classes should get the changed variable the next time they call it.
To implement this behaviour I used a Singleton Holder class. Here is the layout of the classes:

In SingletonHolder.h

Code:

---

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

---

The Parameter classes are defined in parameter.h

Code:

---

struct Params
{
private:
  double m_val1;
  double m_val2;

public:
  double getVal1() const { return m_val1; }
  double getVal2() const { return m_val2; }
  void setVal1(double val) { m_val1 = val; }
  void setVal2(double val) { m_val2 = val; }

  Params();
};

struct OtherParams
{
private:
  double m_otherVal1;
  double m_otherVal2;

public:
  double getOtherVal1() const { return m_otherVal1; }
  double getOtherVal2() const { return m_otherVal2; }
  void setOtherVal1(double val) { m_val1 = val; }
  void setOtherVal2(double val) { m_val2 = val; }

  OtherParams();
};

// only those Types are used by Client classes:
typedef SingletonHolder<Params> SingletonParams;
typedef SingletonHolder<OtherParams> SingletonOtherParams;

---

The implementation of Params and OtherParams in parameter.cpp:

Code:

---

Params::Params()
: m_val1(1)
, m_val1(2)
{
  // should be called only once (when static object is created)!
  cout << "Calling Params()" << endl;
}

OtherParams::OtherParams()
: m_val1(10)
, m_val1(20)
{
  // should be called only once (when static object is created)!
  cout << "Calling OtherParams()" << endl;
}

---

Classes can now access those parameters through:

Code:

---

double val1 = SingletonParams::Instance().getVal1();
double val2 = SingletonOtherParams::Instance().getOtherVal2();
...

---

Please note, that there should be during program execution only exactly one instance of Params and OtherParams!

My Problem is, that in Debug Mode (in VS2005) everything works as expected but in Release Mode there is one function where suddenly a new Object is created (constructor was called and a second cout notification happens). The code looks like this:

Code:

---

SingletonParams::Instance().setVal1(5);
...
cout << SingletonParams::Instance().getVal1(); // OK! is 5

updateSomething(); // Still OK in this function

// first for loop
for (...)
{
    cout << SingletonParams::Instance().getVal1(); // Still OK.
}

// some other loop
for (...)
{
    cout << SingletonParams::Instance().getVal1(); // Ups, new Object and c'tor is called!
}

---

It doesn't matter how the loops look like the thing is the c'tors of Params and OtherParams should never be called twice. As I said problems start in Release Mode and not in Debug mode.

Is there something fundamentally wrong with my code? Are the typedefs in the right place? I now that there are other possibilities to implement the wanted behaviour but I'd like to now what my mistake was.

Maybe somebody has an idea?

Thanks!

Marquito

Make the Params constructor private and friend SingletonHolder<Params>. Then the compiler should tell you if any other place in your code wants to default-construct a Params.

Quote:

---

Originally Posted by CornedBee

Make the Params constructor private and friend SingletonHolder<Params>. Then the compiler should tell you if any other place in your code wants to default-construct a Params.

---

Nice trick. I did that but compilation was fine.

Any other thoughts?

Yes. Reduce the problem to a minimal test case that exhibits it (tricky business - you'd have to remove code until the problem no longer occurs, then re-add the last thing you removed), then post that here.

Code:

---

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
inline T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

---

I would try adding the "inline" there. Multiple translation units that include that header could be generating more than one instance of that function, thus more than one instance of "static T obj". I don't know if the compiler would be right or wrong in doing so however.

gg

It would be wrong. The compiler is supposed to merge all template implementations into a single one.

On the other hand, the mechanism doesn't work across dynamic library boundaries. Do you have both a main program and a DLL, both using this mechanism?

Quote:

---

Originally Posted by Codeplug

Code:

---

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
inline T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

---

I would try adding the "inline" there. Multiple translation units that include that header could be generating more than one instance of that function, thus more than one instance of "static T obj". I don't know if the compiler would be right or wrong in doing so however.

gg

---

Thanks, that did the trick. Could somebody explain to me why? Is it a bug in the compiler?

There was no other dll using this mechanism. There were three classes using this mechanism. In 2 it fully worked but in the third one only until it reached the middle of a function. There the compiler must have created a new instance (because of optimization?).

Thanks for helping!

One last question:

Am I right when i say that a program shouldn't change its behaviour when the keyword inline is added or removed?

Inline tells the compiler that you want to make the code inline (eg copy the code to wherever your function call is). The outcome of the executable doesn't change, but speed may become faster and size increase.

Only if the compiler cares.

You're absolutely right that the inline keyword must never affect semantics. This still reeks of a compiler bug.

Quote:

---

Originally Posted by CornedBee

Only if the compiler cares.

---

Yes, hence "may" become bigger and faster.

marquito:
This is obviously not your main code - why don't you provide your code so we can test?
The stub you provided does not exhibit the problem you are describing.
Also try a rebuild to see if that works.

Quote:

---

Originally Posted by Elysia

Yes, hence "may" become bigger and faster.

marquito:
This is obviously not your main code - why don't you provide your code so we can test?
The stub you provided does not exhibit the problem you are describing.
Also try a rebuild to see if that works.

---

or bigger & slower (because of page faults) if the optimizer is having a bad day.

Quote:

---

Originally Posted by Elysia

marquito:
This is obviously not your main code - why don't you provide your code so we can test?
The stub you provided does not exhibit the problem you are describing.
Also try a rebuild to see if that works.

---

Sorry but its impossible. The main code is a few 1000 lines long... and the slightest change removes the bug so i can't give you a minimal example.
Without the inline fix a simple loop like

Code:

---

for(int i=0; i<10; ++i)
{
}

---

"triggers" the new instantiation - if commented out the behaviour is fine. I tried a rebuild but nothing changed. I can try to turn off optimization and see if anything changes.

Yes, in that case I suggest you try removing optimization and see if it helps. If it does, you can remove optimizations of single functions with pragmas (also disable specific optimizations for functions). See if you can narrow it down.

It is possible to attach code files?

Are you sure you don't have undefined behaviour elsewhere in the code?

Quote:

---

Originally Posted by anon

It is possible to attach code files?

Are you sure you don't have undefined behaviour elsewhere in the code?

---

Sorry, but I can't post the code files. We must assume that I'm right :-)

Code:

---

pragma optimize ("t", off)

---

before said function removed the bug as well.

Basically, it's an optimization bug, then?

Quote:

---

Originally Posted by Elysia

Basically, it's an optimization bug, then?

---

Ok, there is one class declared this way:

Code:

---

class __declspec(dllimport) ...

---

The c'tor of this class calls the Singleton classes the first time. Now there are other classes calling the member function of the Singletons and without optimization it works but with optimization it doesn't. I narrowed one function down to:

Code:

---

for (int i=0; i<10; ++i)
{
}
for (int i=0; i<10; ++i)
{
}
double d = Params::Instance().getVal1();

---

where a new Instance is only created if there are two or more of those dummy loops (optimization turned on). I don't now if the dll import has something to do with it or not. Imho it doesn't explain the random behaviour described above.

Inlining still works perfectly with or without optimization.

Thanks for your help!