1

This is a weird problem and I'm not sure what to make of it.

I have something like the following:

struct Parms
{
    const std::string value1;
    const std::string value2;

    std::string parm1;
    std::string parm2;

    Parms() : parm1(value1), parm2(value1) {}

    static const Parms& getDefaults()
    {
        static Parms defaults;
        return defaults;
    }
};

Which I generally use like so:

Parms myParms = Parms::getDefaults();
myParms.parm1 = "crap";
functionThatNeedsParms(myParms);

Pretty straightforward. This has never caused me any headaches, until I started trying to write unit tests that use this code, using CxxTest. I have two test suite classes in different files, and when I run them individually, everything is great.

When I run them together, I see two bad things. First, the whole thing core dumps trying to double free the static defaults variable. Secondly, if I look at the contents of defaults some time before it dies, but after I've started using it, the static const std::strings that are in there are corrupted (some letters have randomly changed, though it is always the same on every run).

What is going on?

Paul D.
  • 1,785
  • 2
  • 19
  • 25
  • Is the code just as above (ie getDefaults is implicitly inline because of definition inside the class). Is params built into one library and all the test code built into another library? Are value1/2 really just const members and not references or static or something else wierd. – Martin York Aug 31 '10 at 18:34
  • 'defaults' is not a good name. it very closely resembles 'default' which is a C++ keyword – Chubsdad Sep 01 '10 at 18:12
  • Why does the post say Singleton? It looks like this class is doing lot more things that just 'Singleton' functionality. – Chubsdad Sep 01 '10 at 18:20

3 Answers3

2

double free and core dump

I think I can explain the "double free and core dump" issue that you are having. I recently encountered the same thing and it sounds like you are doing the same thing I did.

From you description you said that when you "run them separately" they work fine but if you "run them together" you get the double free/core dump issue.

I found this to occur if the same global is declared twice.

In my case I had class foo, in one file I had a global class foo gFoo; and in a different file I had a global class foo gFoo;. (Yeah this sounds stupid, actually I was linking against a file X.cxx as well as a shared library that also included X.cxx -- the results where essentially the same.)

Now, I would have expected a compiler complaint about this, but apparently there are flags to enable or disable this check, and the code compiled fine. But when the program was terminating and calling all of its destructors, it called gFoo's destructurs twice and gave me the double free message along with a core dump.

Given that you stated it works fine independently but fails when combined, I'm betting you have the global defined in two separate files, and it works fine when they are compiled by themselves, but when you combine them to make a single test, you probably have the global declaration happening twice.

Check that out.

John Rocha
  • 1,656
  • 3
  • 19
  • 30
0

static variables in C and C++ are not thread safe. That means that race conditions (bad thing) can happen if two threads try to access your singleton object. One approach to fix your problem is to use Thread Local Storage. This is supported by the pthreads library and also some compilers directly support thread local storage.

The alternative, if your singleton MUST be global to all threads, is to provide lock to ensure that only one thread can access your data at a time.

Since however, the problem arises only in unit testing. I would suggest to not run multi-threaded unit tests unless you intend to use your singleton in multiple threads.

doron
  • 27,972
  • 12
  • 65
  • 103
  • I don't see any reference to multi threaded environment in the question so this answer is grasping at straws. You should have posted a comment to the question asking about multi-threaded before providing an answer. – Martin York Aug 31 '10 at 18:30
  • I am guessing from the nature of the problem and the phrase "running together" that this is in fact multi-threading. I may be wrong but the question is short on detail and this may be the issue. – doron Aug 31 '10 at 19:11
-1

This is highly dependent on the compiler and platform you are using and without actually seeing the tests I can only guess of what is going on.

I see some misconceptions in your code:
1) You are missing a copy operator and copy constructor You are copying the instances that contain std::string which might be implemented using reference counting. The reference counting is implemented in overloaded copy constructor/operator of std::string but these might not get called from the implicitly generated copy constructor of your class and therefore causing double freed memory and other nasty things. The copy operator/constructor should look like this:

// Copy constructor
Parms(const Parms& oth)  { parm1 = oth.parm1; parm2 = oth.parm2; }

// Copy operator
Parms& operator= (const Parms& oth)  { 
  if (&oth == this)  // Check for self-assignment
    return *this;
  parm1 = oth.parm1;
  parm2 = oth.parm2;
  return *this;
}



2) I don't quite understand the presence of value1 and value2. It seems you never initialize them, you just use them in the default constructor to copy their (empty) content into parm1 and parm2. You can avoid this completely as parm1 and parm2 are initialized automatically to an empty string when called.

3) You do not need to use the singleton pattern here. The getDefaults() method could be implemented as follows:

static Parms getParms() { return Parms(); }

The singleton pattern is meant for classes that only have one instance through the whole program run which doesn't seem to be the case of your class.

This way you can safely use the getParms() function from multiple threads and a smart compiler will optimize out the implied additional copy.

Karel Petranek
  • 15,005
  • 4
  • 44
  • 68
  • Sorry, I should have been more explicit in my original question. This is an issue I ran across at work, so I wasn't about to copy & paste the actual code, so what I put together was (an apparently not that clear) trivialized example. The singleton behavior is used because there is also a setDefaults() method, so you can set them once and then reuse them wherever. At any rate, the real problem was #1. I did not realize that the std::string implementation used refcounting, so I assumed a shallow copy would be valid. Thanks for your response. – Paul D. Aug 31 '10 at 17:42
  • Actually wait a second. Why would it being refcounted blow everything up? If I call getDefaults() and assign it into some of type Parms, it ought to just increase the refcount, and then later when the test ends and it goes out of scope, it ought to decrease it, and life should go on as normal. – Paul D. Aug 31 '10 at 17:49
  • Oh I see what you are saying. Is there some definitive spec to look at that nails down what all the possible copy constructors and assignment operators are? It seems odd to me that they would design things in such a way that you could accidentally invalidate the refcount. – Paul D. Aug 31 '10 at 17:52
  • Yes, this would work if the default copy operator called the corresponding string copy operator. Even though this should be the case (according to http://stackoverflow.com/questions/2009996/stdstring-in-struct-copy-assignment-issues) I found out some compilers misbehave and do an equivalent of a memcpy() (I think Visual C++ 6 did that and it was pretty hard to track it down). – Karel Petranek Aug 31 '10 at 18:07
  • Another thought: if you also have the function setDefaults, do you make the `Parms defaults` variable a static member of the class? The refcounting might or might not be the issue, I would need to see the full code to make better guesses :) – Karel Petranek Aug 31 '10 at 18:11
  • 1
    1) There is no need to have a copy constructor or assignment operator as the class does not manage resources and thus the defaults are perfectly safe. 2) Even if reference counting is used; std::string will never cause a double delete. 3) That is a horrible assignment operator (It does not provide the strong exception gurantee (Please look up `Copy and Swap Idiom`)) – Martin York Aug 31 '10 at 18:37
  • @Marting York Thanks for noting the flaws. Ad points 1 and 2: As I noted in the comments, I used to run in problems when I had a class without a copy operator that contained std::string (with ancient compilers though). Even though this may be rare, given the little information provided I considered mentioning it. Ad point 3: You are perfectly correct here. – Karel Petranek Aug 31 '10 at 19:06