PSA: Don't try to hold onto temporaries with references
If a function returns a temporary, you probably don't want to hold onto it with a "const Foo& foo". The temporary will get deallocated and then you'll be left with a reference to dead memory, which is bad new bears: http://trac.webkit.org/changeset/68984 http://trac.webkit.org/changeset/68985 This kind of thing can be tricky to spot in code reviews because it's not obvious at the call site whether a function turns a temporary or a reference to some existing object. Adam
On Oct 3, 2010, at 2:21 AM, Adam Barth wrote:
If a function returns a temporary, you probably don't want to hold onto it with a "const Foo& foo". The temporary will get deallocated and then you'll be left with a reference to dead memory, which is bad new bears:
I don’t understand why the crashes were happening here and why your code changes were helpful. What you say here about object lifetime is not correct. I thought the same thing a year or so back. But the C++ language keeps these objects alive until the end of the block. Some other programmers on the project challenged me when I made this assertion, and we found that I was wrong and they were right. But that doesn’t explain why the code was crashing! -- Darin
On Sun, Oct 3, 2010 at 10:31 AM, Darin Adler <darin@apple.com> wrote:
On Oct 3, 2010, at 2:21 AM, Adam Barth wrote:
If a function returns a temporary, you probably don't want to hold onto it with a "const Foo& foo". The temporary will get deallocated and then you'll be left with a reference to dead memory, which is bad new bears:
I don’t understand why the crashes were happening here and why your code changes were helpful.
What you say here about object lifetime is not correct. I thought the same thing a year or so back. But the C++ language keeps these objects alive until the end of the block. Some other programmers on the project challenged me when I made this assertion, and we found that I was wrong and they were right.
But that doesn’t explain why the code was crashing!
Well, the tests are still crashing after these changes, so I'm certainly willing to believe there's more going on here. I'll keep investigating. Adam
On 10/04/2010 01:31 AM, Darin Adler wrote:
What you say here about object lifetime is not correct. I thought the same thing a year or so back. But the C++ language keeps these objects alive until the end of the block. Some other programmers on the project challenged me when I made this assertion, and we found that I was wrong and they were right.
ah, I was not aware that it is until the end of the block, I thought it is more local.
On Sun, Oct 3, 2010 at 10:31 AM, Darin Adler <darin@apple.com> wrote:
What you say here about object lifetime is not correct. I thought the same thing a year or so back. But the C++ language keeps these objects alive until the end of the block.
Correct. One helpful section from the standard (12.2/5 "Temporary objects"): "The temporary to which the reference is bound or the temporary that is the complete object to a subobject of which the temporary is bound persists for the lifetime of the reference except as specified below. A temporary bound to a reference member in a constructor’s ctor-initializer (12.6.2) persists until the constructor exits. A temporary bound to a reference parameter in a function call (5.2.2) persists until the completion of the full expression containing the call." Adam's changes will not make any functional difference. PK
Thanks Peter and Darin. In summary, looking at code like this B& b = c->foo(); ... b.m(); If c->foo() returns a temporary ("return B();"), then it is safe. If c->foo() returns a reference to a member variable ("return m_b;"), then it is up to the lifetime of of "c->m_b". The cases that Adam changed were instances of the former. dave On Mon, Oct 4, 2010 at 12:36 PM, Peter Kasting <pkasting@chromium.org>wrote:
On Sun, Oct 3, 2010 at 10:31 AM, Darin Adler <darin@apple.com> wrote:
What you say here about object lifetime is not correct. I thought the same thing a year or so back. But the C++ language keeps these objects alive until the end of the block.
Correct. One helpful section from the standard (12.2/5 "Temporary objects"):
"The temporary to which the reference is bound or the temporary that is the complete object to a subobject of which the temporary is bound persists for the lifetime of the reference except as specified below. A temporary bound to a reference member in a constructor’s ctor-initializer (12.6.2) persists until the constructor exits. A temporary bound to a reference parameter in a function call (5.2.2) persists until the completion of the full expression containing the call."
Adam's changes will not make any functional difference.
PK
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes). So you will be returning a reference to a temporary which, I think, no longer is valid. I made a quick test to be sure and the destructor of B is indeed called. Why is it safe?
On Mon, Oct 4, 2010 at 12:23 PM, Leandro Graciá Gil <leandrogracia@chromium.org> wrote:
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes).
Actually, the temporary object ceases to exist as soon as *the expression containing the call completes*, as Peter Kasting pointed out. So this should be ok: B b = c->foo(); // foo() returns a reference to a temporary, and the temporary is then copied to b, then destroyed And this too: c->foo().m(); But not this: B& b = c->foo(); // the temporary is gone now b.m(); // trouble Hans
On Mon, Oct 4, 2010 at 12:41 PM, Hans Wennborg <hans@chromium.org> wrote:
On Mon, Oct 4, 2010 at 12:23 PM, Leandro Graciá Gil <leandrogracia@chromium.org> wrote:
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes).
Actually, the temporary object ceases to exist as soon as *the expression containing the call completes*, as Peter Kasting pointed out. So this should be ok:
B b = c->foo(); // foo() returns a reference to a temporary, and the temporary is then copied to b, then destroyed
And this too:
c->foo().m();
But not this:
B& b = c->foo(); // the temporary is gone now b.m(); // trouble
Hans
I take it all back! I read that standards quote a bit too fast :) "A temporary bound to the returned value in a function return statement persists until the function exists". I suppose that says it all. Leandro is right, I think.
________________________________
Date: Mon, 4 Oct 2010 12:23:06 +0100 From: leandrogracia@chromium.org To: levin@google.com CC: webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] PSA: Don't try to hold onto temporaries with references
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes). So you will be returning a reference to a temporary which, I think, no longer is valid. I made a quick test to be sure and the destructor of B is indeed called. Why is it safe?
ok, I avoided saying anything since I have yet to contrib code and quite frankly don't have an authoritiative answer but I do recall when I was learning cpp the compiler would warn if you even tried to return a ref to a temp( maybe this is just msvc or you need to try it with -Wall ). If the thing it points to is on the stack, and presumably the stack gets popped on return, you would probably need to generate some really odd code to save this reference once the thing for which it is a synonym goes out of scope. If you are defending this as safe it may be nice to show what kind of code the compiler generates to keep this thing valid.
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
The standard is clear on this. The temporary does persist for the lifetime of the reference. See <https://bugs.webkit.org/show_bug.cgi?id=47055#c4>, a comment I posted yesterday morning, for the reference to the appropriate section in the C++ standard. -- Darin
On Mon, Oct 4, 2010 at 4:23 AM, Leandro Graciá Gil < leandrogracia@chromium.org> wrote:
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes).
foo() is returning a temp by value. On the caller side, that value is copied to a (hidden) temp whose lifetime is the same as the lifetime of |b|, and then |b| is set to be a reference to that temp. By contrast, if foo were returning a temp by reference, then the reference would be invalid on return because the (foo()-scoped) temp it referred to would be destroyed when foo() exited. PK
On Tue, Oct 5, 2010 at 3:42 AM, Peter Kasting <pkasting@chromium.org> wrote:
On Mon, Oct 4, 2010 at 4:23 AM, Leandro Graciá Gil < leandrogracia@chromium.org> wrote:
In summary, looking at code like this
B& b = c->foo(); ... b.m();
If c->foo() returns a temporary ("return B();"), then it is safe.
Maybe I'm wrong, but are you completely sure about this one? I would say that the temporary object created in return B() will cease to exist as soon as it returns (just after the constructor finishes).
foo() is returning a temp by value. On the caller side, that value is copied to a (hidden) temp whose lifetime is the same as the lifetime of |b|, and then |b| is set to be a reference to that temp.
By contrast, if foo were returning a temp by reference, then the reference would be invalid on return because the (foo()-scoped) temp it referred to would be destroyed when foo() exited.
Thanks Darin and Peter. I left out an important detail: the full function signature .(I mentally used my standard way of writing such code.) #1 was "B foo() { return B();}" vs #2 was "const B& foo() { return m_b; }" I suspect that the the example code written to test it looked like this: B& foo() { return B();} PK
participants (8)
-
Adam Barth
-
Darin Adler
-
David Levin
-
Hans Wennborg
-
Holger Freyther
-
Leandro Graciá Gil
-
Mike Marchywka
-
Peter Kasting