0

I have some doubts in this code> Any discussion will be very helpful to understand the things:

class Singleton
{
private:
    static Singleton *single;
    Singleton() {}
    ~Singleton() {}
public:
        static Singleton* getInstance()
        {
                if (!single)
                        single = new Singleton();
                return single;
        }
        void method()
        {
                cout << "Method of the singleton class" << endl;
        }
        static void destroy()
        {
                delete single;
                single = NULL;
        }
};


Singleton* Singleton::single = NULL;

int main()
{
    Singleton *sc2;
            sc2 = Singleton::getInstance();  // sc2 is pointing to some memory location
    {
        Singleton *sc1 = Singleton::getInstance(); // sc1 and sc2 pointing to same memory location
        sc1->method();
        Singleton::destroy();   // memory location deleted.
        cout << sc1;
    }

    sc2->method();   // ??? how this is working fine??

    return 0;
}

inside this block, we are deleting the memory in "Singleton::destroy()";

{
Singleton *sc1 = Singleton::getInstance();
    sc1->method();
Singleton::destroy();
cout << sc1;
}

Then how this call "sc2->method();" is successful??

Devesh

Devesh Agrawal
  • 8,982
  • 16
  • 82
  • 131

2 Answers2

2

Why is it working?

Because the function you are calling is statically defined so the this pointer is not required to call it and the function itself doesn't use the this pointer. Therefore there would be no reason for the function to crash.

However, as you mentioned the class is being misused.

A safer way to still allow for a destroy() function is to use shared_ptr<>() for the Singleton pointer. That way, the destroy becomes:

single.reset();

And if anyone else still has a pointer, it remains valid. The problem will only be if you call the getInstance() again. At that point you have TWO versions of your "singleton". Although you have another possible way to fix that problem: you can prevent any calls to getInstance() once the destroy was called.

getInstance() ... if(destroyed) throw std::runtime_error("singleton destroyed"); ...
destroy() ... destroyed = true; ...

Update

There is a copy of the assembly for the call to method as per g++ / objdump -d (under Linux, although it should work in cygwin too):

400978:       48 8b 45 f0             mov    -0x10(%rbp),%rax
40097c:       48 89 c7                mov    %rax,%rdi
40097f:       e8 ae 00 00 00          callq  400a32 <_ZN9Singleton6methodEv>

(P.S. objdump disassemble with registers inverted from the usual INTEL syntax.)

As we can see, the compiler uses a "callq". The this pointer is in %rax. The "callq" does NOT make use of %rax. As far as the assembly code is concerned, that function is currently static.

Inside method(), %rax is not used so whatever its value it does not matter:

0000000000400a32 <_ZN9Singleton6methodEv>:
400a32:       55                      push   %rbp
400a33:       48 89 e5                mov    %rsp,%rbp
400a36:       48 83 ec 10             sub    $0x10,%rsp
400a3a:       48 89 7d f8             mov    %rdi,-0x8(%rbp)
400a3e:       be 44 0b 40 00          mov    $0x400b44,%esi
400a43:       bf 80 10 60 00          mov    $0x601080,%edi
400a48:       e8 b3 fd ff ff          callq  400800 <_ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc@plt>
400a4d:       be 30 08 40 00          mov    $0x400830,%esi
400a52:       48 89 c7                mov    %rax,%rdi
400a55:       e8 c6 fd ff ff          callq  400820 <_ZNSolsEPFRSoS_E@plt>
400a5a:       c9                      leaveq 
400a5b:       c3                      retq
Alexis Wilke
  • 19,179
  • 10
  • 84
  • 156
  • I am calling this function. sc2->method(); This function is not static. so i think it will use this pointer. just clearing my doubt. Devesh – Devesh Agrawal Oct 31 '13 at 03:42
  • 1
    @DeveshAgrawal Although the function isn't declared as static, it is equivalent to a static function since it doesn't access any data members. Alexis is suggesting that the compiler treats it the same as a static function for this reason. In fact you should declare it static to avoid confusing your users - by declaring it non-`static`, and omitting the `const` keyword at the end, you are suggesting to the user that the function is going to mutate data members. – JBentley Oct 31 '13 at 04:07
  • @JBentley Thanks for response. Now the things are bit clear. One more thing, if it use any data then it won't be treated as static by compiler. So the second call "sc2->method();" should crash. because by the time method tries to access data, its gone, because memory is deleted. But i am not seeing any crash. what can be the reason?? – Devesh Agrawal Oct 31 '13 at 04:16
  • @DeveshAgrawal `method` is not accessing any data, it's sending a string literal to `cout`. No memory needs to be accessed for that function to complete. – JBentley Oct 31 '13 at 04:20
  • @DeveshAgrawal Because the memory is still there, it's just MARKED free. If you run your program under a debugging tool like `valgrind` for example, then that access should get caught and reported to you – Adam Oct 31 '13 at 04:20
  • @JBentley If you also have `method` simply print `this` it will still work. There's nothing staticy at work here. – Adam Oct 31 '13 at 04:23
  • @Adam I'm not sure I agree - if `method` prints `this` then the object is being observed, and the compiler will be forced to pass the `this` pointer. If on the other hand, `method` doesn't touch `this` and instead outputs a string literal, then the fact that it crashes has nothing to do with the fact that the memory is still there but marked free, because it never goes near that memory anyway. In this scenario the reason it doesn't crash is because the function is deterministic regardless of whether the memory has been deleted or not. – JBentley Oct 31 '13 at 04:27
  • @JBentley You're half right. If no members are observed then `this` doesn't have to be valid. `static_cast(NULL)->method()` works too (it does, I tried it). However, the function signature still says that a `this` needs to be passed, and Alexis is suggesting that the compiler is doing a very aggressive optimization in taking it out altogether. Maybe it is, but he's got to show that it does. – Adam Oct 31 '13 at 04:33
  • @Adam Fair enough, you've convinced me that the wording on this answer is not entirely accurate. The answer could be improved by removing the words, *"the function you are calling is statically defined so the this pointer is not required to call it and"* from the first paragraph. – JBentley Oct 31 '13 at 04:36
1

To briefly summarize what Jesse's link says:

You get lucky. sc2 still points to that old instance. Its memory is freed, but that doesn't mean it's being cleared out. If you access that memory the results are undefined. It might appear to work (like it does for you), it might hit some debugging feature that might alert you to the problem, or it might make the computer grow legs and run out the door. It's undefined and it's up to you to make sure it doesn't happen.

Adam
  • 16,808
  • 7
  • 52
  • 98
  • Although it is correct that the memory is not cleared, in this case it does not matter. The function is static and does not use `this`. That's a better explanation. – Alexis Wilke Oct 31 '13 at 03:39
  • 1
    `method` is not static – Adam Oct 31 '13 at 03:39
  • The function itself is treated as a static function by the compiler. It does not require 'this' to "find" the pointer. The pointer is hard coded at the location of the call (because it is not virtual.) In assembly it becomes the equivalent of `method(this)`. – Alexis Wilke Oct 31 '13 at 03:41
  • It's *not* "treated as static". The mechanism you describe is how all non-virtual, non-static member functions are called. That does not make them static. `method` certainly gets a `this` pointer like you describe, and that's exactly what makes it non-static. If it was static it wouldn't get a `this`. – Adam Oct 31 '13 at 04:16
  • @Adam Can you clarify why the compiler would pass a `this` pointer to `method` when it isn't using it? Would the compiler not see that it never uses the `this` pointer, and optimise it away (into a static function)? – JBentley Oct 31 '13 at 04:24
  • @AlexisWilke Because that's the signature. Maybe there's some level of optimization that will optimize it out, but -O3 doesn't seem to. And yes, I tried adding a `cout << this;` to `method` and nothing changes. Sorry man, but the burden of proof is on you. – Adam Oct 31 '13 at 04:26
  • In any case, regardless of whether the compiler optimises away the `this` pointer or not, why is he getting lucky? You say, *"If you access that memory the results are undefined"* but where is he accessing the memory? – JBentley Oct 31 '13 at 04:32
  • I updated my answer which shows the resulting assembly code. I'm sorry if you have not yet understood that C++ was calling all functions, except virtual functions, by using this as the first parameter (i.e. `method(this)`) and thus the fact that `this` is not required to call the function once converted to assembly. (the object pointer is important in the C++ code for the compiler to determine which function you're actually calling! but it does not get used for the `callq` assembly instruction.) [maybe I should have written: `method(sc2)`; I use `this` to mean the object pointer...] – Alexis Wilke Oct 31 '13 at 04:45
  • Another thing: `cout << this` is fine too because you just read a pointer and write its value in stdout. That will work. You are not dereferencing it. – Alexis Wilke Oct 31 '13 at 04:52
  • @AlexisWilke for a non-virtual call the address of the function is not dependent on `this`, but instead only on the type. The compiler builds a table with pointers to the functions (one table per class), and when you call it it's just an offset into that table. That's where your `400a32` comes from, I believe. But that does not make the function static. a `__thiscall` function still gets a `this` argument, that's the only thing that says whether it's static or not. You've got the right idea but you're calling it the wrong thing. – Adam Oct 31 '13 at 04:54
  • @AlexisWilke yes, that print is not dereferencing it. The point of that is to show that the compiler doesn't have to optimize away the argument (aka make it static, as you're calling it). – Adam Oct 31 '13 at 04:55
  • It's done the same way as in Python, except Python makes it explicit: A non-static function looks like this: `def func(self, args)` while a static one looks like this `def func(args)`. It's exactly the same way in C++ except the compiler adds that `self` (`this`) for you automatically. – Adam Oct 31 '13 at 04:58
  • And yes OP does get lucky because I think (I don't have the standard in front of me to check) that the pointer on which you call a member still has to be valid. So technically this is undefined behavior. The fact that it works is purely implementation specific. – Adam Oct 31 '13 at 05:00
  • The `400a32` is the address. The .text section includes it: ` 12 .text 000002e4 0000000000400850 0000000000400850 00000850 2**4 CONTENTS, ALLOC, LOAD, READONLY, CODE`. There are 3 cases when tables are used: virtual functions, dynamically linked libraries, switches (at times). Now you are correct that I use the term 'static' in a way contrary to the C++ specifications. But whether the code crashes (my first paragraph) is because it is all the same, in that very case. And you are correct that he got lucky because he's not using `this` inside the function and the function is not virtual. – Alexis Wilke Oct 31 '13 at 05:06