2

I'm working with threads and I have question about how the compilers are allowed to optimize the following code:

void MyClass::f(){
    Parent* p = this->m_parent;
    this->m_done = true;
    p->function();
}

It is very important that p (on the stack or in a register) is used to call the function instead of this->m_parent. Since as soon as m_done becomes true then this may be deleted from another thread if it happens to run its cleanup pass (I have had actual crashes due to this), in which case m_parent could contain garbage, but the thread stack/registers would be intact.

My initial testing on GCC/Linux shows that I don't have a race condition but I would like to know if this will be the case on other compilers as well?

Is this a case for gulp volatile? I've looked at What kinds of optimizations does 'volatile' prevent in C++? and Is 'volatile' needed in this multi-threaded C++ code? but I didn't feel like either of them applied to my problem.

I feel uneasy relying on this as I'm unsure what the compiler is allowed to do here. I see the following cases:

  • No/Beneficial optimization, the pointer in this->m_parent is stored on the stack/register and this value is later used to call function() this is wanted behavior.
  • Compiler removes p but this->m_parent is by chance available in a register already and the compiler uses this for the call to function() would work but be unreliable between compilers. This is bad as it could expose bugs moving to a different platform/compiler version.
  • Compiler removes p and reads this->m_parent before the call to function(), this creates a race condition which I can't have.

Could some one please shed some light on what the compiler is allowed to do here?

Edit

I forgot to mention that this->m_done is a std::atomic<bool> and I'm using C++11.

Community
  • 1
  • 1
Emily L.
  • 5,673
  • 2
  • 40
  • 60
  • The `register` keyword would do what I want if the compiler would respect it... – Emily L. Jul 12 '13 at 16:09
  • this is not a problem of volatile / register. It's a problem of re-ordering. The compiler is allowed to set m_done to true before he actually loads m_parent. – Tobias Langner Jul 12 '13 at 16:12
  • Also `volatile` will force a memory read from the buss, which isn't really what (even though it probably could be used for solving my problem). I just want to tell the compiler "Pretty please keep this value in a register `cause it's going to dissapear soon and I need a copy" – Emily L. Jul 12 '13 at 16:14
  • @TobiasLangner `register` and a memory barrier would do it I guess but compiler won't necessarily respect `register` :( – Emily L. Jul 12 '13 at 16:15
  • I am inclined to say that the compiler is right and your code is broken. And even if your code can be made technically legal, it seems too tricky for comfort. –  Jul 12 '13 at 16:16
  • register doesn't help and is not required. You need a value in p - that's enough. And it needs to be there before m_done is true. – Tobias Langner Jul 12 '13 at 16:19
  • I believe that the proposed atomic change is enough and the use of the variable `p` is not needed at all. Depending on compiler `this` will be already put on the stack or in register and the memory pointed by it is not needed to retrieve the pointer. But if the `function` is virtual then this code is not safe at all, because it relies on the existence of the memory pointed by `this`. – bbonev Jul 18 '13 at 10:11

3 Answers3

2

This code will work perfectly as written in C++11 if m_done is std::atomic<bool>. The read of m_parent is sequenced-before the write to m_done which will synchronize with a hypothetical read of m_done in another thread that is sequenced-before a hypothetical write to m_parent. Taken together, that means that the standard guarantees that this thread's read of m_parent happens-before the other thread's write.

Casey
  • 41,449
  • 7
  • 95
  • 125
  • Thank you, that sells me! And I'll end this with a quote from Mc Hammer: "Na-na-na-na can't touch `this`!" (sorry I had to, I just found it hilarious, I know, I'm terribad) – Emily L. Jul 12 '13 at 16:37
  • @EmilyB.: Notice the lack of upvotes :) I think you were too quick to accept an answer (and dismiss Pete's answer); I'm tempted to believe it's correct, but I don't have any reference to indicate so. You might want to keep on checking the answers here just in case. – user541686 Jul 12 '13 at 16:39
  • 1
    "Perfectly as written" was meant in reference to the memory ordering only: the code is data-race-free. – Casey Jul 12 '13 at 16:40
  • I suppose that implies that the compiler may still optimize out the variable completely? I just realized I typoed the original code. `m_done` is a part of `m_parent` so m_parent has to be read into a register before m_done is written (`m_parent->m_done= true`) so it'll be in a register on all the platforms I care about (x86-64) yes? – Emily L. Jul 12 '13 at 17:01
  • The compiler can do anything it wants, as long is the value used for `p` is the value that was read from `m_parent` *before* the write to `m_done`. (Realistically yes, it will store `p` in a register - but don't worry about implementation details. Memory ordering is confusing enough without adding more detail ;) – Casey Jul 12 '13 at 17:32
  • I think you are overly obsessed by registers. "this" will probably be in a register. But this is no problem. m_done will not be in a register - it has to be written to memory. p can be in a register or not, but it does not matter. It is initialized from memory (m_parent) and then used to call p() again. No problem here. – Tobias Langner Jul 12 '13 at 17:44
  • I'm not obsessed by the registers :) I just worry that the compiler will emit instructions to read `this` from memory (in order to call `this->m_parent->function`) *after* the memory barrier (when it might have already been invalidated) because it decided that storing `p` on the stack (or a register) would be unnecessary. – Emily L. Jul 12 '13 at 17:52
  • he's not allowed due to the memory barrier. That's what it's for. As I said - it's not about register, it's about re-ordering – Tobias Langner Jul 12 '13 at 19:27
1

You might run into a re-ordering problem. Check memory barriers to solve this problem. Place it so that the loading of p and the setting of m_done is done in exactly that order (place it between both instructions) and you should be fine.

Implementations are available in C++11 and boost.

Tobias Langner
  • 10,634
  • 6
  • 46
  • 76
-1

Um, some other thread might delete the object out from under you while you're in one of its member functions? Undefined behavior. Plain and simple.

Pete Becker
  • 74,985
  • 8
  • 76
  • 165
  • Behaviour is perfectly defined as long as I don't touch `this` after it happens, the stack and register contents are valid and well defined. – Emily L. Jul 12 '13 at 16:33
  • @EmilyB. - no, that's a heuristic. The C++ language definition does not require this to work; that's what "undefined behavior" means. – Pete Becker Jul 12 '13 at 16:35
  • @Mehrdad - you want me to cite the place in the language definition that doesn't say that this has to work? Simple: the entire standard. – Pete Becker Jul 12 '13 at 16:40
  • @PeteBecker: Well, you're claiming that `this` for member functions is treated differently from regular pointers in free functions. It's plausible, but that doesn't make it true. What makes you think it's the case (and that this is UB)? – user541686 Jul 12 '13 at 16:41
  • @Mehrdad - show me where the standard says that a member function will work correctly after the end of the lifetime of the object that it was called on. – Pete Becker Jul 12 '13 at 16:44
  • @PeteBecker: Which part of the standard says that a free function will work correctly after the end of the lifetime of an object it has received a pointer to? It's probably the same section. – user541686 Jul 12 '13 at 16:46
  • @Mehrdad - surely you understand that member functions are not the same as free functions. If they were, they wouldn't need a different name. – Pete Becker Jul 12 '13 at 16:50
  • Is it undefined behavior to call a member function in the destructor? Object lifetime ends when the destructor starts (3.8p1). – Casey Jul 12 '13 at 16:52
  • @PeteBecker: They're both still functions, though. Can you point me to the part of the standard that states that *this* in particular is one of their differences between member and free functions? Just because they have a different name doesn't mean that *everything* is different about them. For all I know, the only difference between (non-virtual) member and free functions could be in their abilities to access `private`/`protected` variables, and ***nothing more***. If you're claiming otherwise then you should cite the part of the C++ standard that explains the difference you're referring to. – user541686 Jul 12 '13 at 16:53
  • 3
    In fact, [here's an answer](http://stackoverflow.com/a/3151000/541686) that indicates you're wrong. Read [**this**](http://www.parashift.com/c++-faq-lite/delete-this.html) too. – user541686 Jul 12 '13 at 16:58
  • Um, citing an answer that says, without authority, that `delete this` is well defined does not prove anything. It rests on the same assumptions as deleting from another thread: that if you don't touch any data members on the way out things are okay. There's nothing in the standard that says that. – Pete Becker Jul 12 '13 at 17:46
  • 12.7p4: "Member functions, including virtual functions (10.3), can be called during construction or destruction (12.6.2)." 3.8p1: "The lifetime of an object of type `T` ends when: if `T` is a class type with a non-trivial destructor (12.4), the destructor call starts, or the storage which the object occupies is reused or released." The standard does define behavior for invoking member functions after the object lifetime has ended. There are, however, *many* actions that are explicitly undefined behavior after the destructor completes mentioned all through 12.7. – Casey Jul 12 '13 at 17:47
  • @PeteBecker: Your answer doesn't point to any authoritative source about the differences between member and free functions either, and yet you thought "surely I understand" the difference. Well, same thing here. – user541686 Jul 12 '13 at 18:00
  • @Mehrdad - the starting point is that if the standard doesn't say what happens, then the behavior is undefined. That may sound like a tautology, but it's the definition of "undefined behavior". If you think the behavior of this code (or of `delete this`, which is essentially equivalent) is well defined, point out the part or parts of the standard that say what happens. Asserting that member functions are just like free functions in this regard is merely hand waving. – Pete Becker Jul 12 '13 at 19:55
  • @PeteBecker Am I to take your comments as indicating that you do not believe `delete this` to have defined behavior? I've seen quite a few statements to the contrary in the past by several committee members, including you. Has something changed in C++11 to make `delete this` undefined? – Casey Jul 12 '13 at 20:05
  • @PeteBecker: Ok, where does the standard say what happens for **free** functions with pointer arguments that get deleted? If you can't point me to a specific location (which you seem to be refusing to do) then by your reasoning that must be UB too?? – user541686 Jul 12 '13 at 20:05
  • @Casey - you have **never** seen a statement from me that `delete this` has well-defined behavior according to the language definition. I have never thought that that was the case. – Pete Becker Jul 12 '13 at 20:20
  • @Mehrdad - you're trying to change the subject. My assertion was that the behavior is undefined; you claim that it's well defined. As you said, "[citation needed]". – Pete Becker Jul 12 '13 at 20:22
  • @PeteBecker: Uh, no, I'm not changing the subject -- you're just ignoring the problem with your reasoning. If you can't point me to a part of the standard that talks about how `delete p;` behaves in *free* functions, how can you claim it behaves differently in *member* functions, where `p` *simply happens* to be `this`? If the standard doesn't differentiate between them then there obviously they're either both UB or both well-defined. – user541686 Jul 12 '13 at 20:34
  • @Mehrdad - huh? Let's start at the beginning. Is it your position that the standard defines the behavior of `delete this`? (It seems like that's the root of your position). If so, where in the standard does it tell you that? Again, "[citation needed]". – Pete Becker Jul 12 '13 at 20:38
  • 2
    Here's a golden oldie from 1995: ["...the answer is yes, delete this is valid, and compilers should support it."](http://collaboration.cmc.ec.gc.ca/science/rpn/biblio/ddj/Website/articles/CUJ/1995/9508/becker/becker.htm). [Here's an answer from 2012 in which you discuss the effect of `delete this` without mentioning undefined behavior](http://stackoverflow.com/a/12100365/923854). In any case, we should probably all take this to discussion, or post a new question. – Casey Jul 12 '13 at 20:40
  • @Casey - good catch (the first one); what can I say? We were all young and foolish at one time. `` And, of course, in 1995 there was no C++ standard. The second one, of course, has nothing to do with this discussion; it involved deleting a stack object. – Pete Becker Jul 12 '13 at 20:45
  • In any case, 12.7p4 allows *calling* member functions of an object after the end of its lifetime, if it's possible for such a function invocation to complete without undefined behavior, I can't see how a member function called *before* the end of the object's lifetime and returning after the end would behave differently. – Casey Jul 12 '13 at 20:55
  • 1
    @PeteBecker: OK, let's restart. **Yes**, I'm saying `delete this` is valid. But I'm telling you **no**, you **don't** need the standard to mention this explicitly, because `this` is a pointer, and **just as for any other pointer**, the standard explicitly says that `delete p;` works on **any** pointer `p` that was allocated with `new`. As far as I know, the standard **never differentiates** between the cases where `p == this` and `p != this`. You're claiming `delete p;` is invalid in the special case where `p == this`, which the standard never claims to be true, hence you need a citation. – user541686 Jul 12 '13 at 21:00