Closed Bug 885954 Opened 6 years ago Closed 6 years ago

Disable copy constructor and copy assignment for HeapPtr

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(2 files, 1 obsolete file)

Luke suggested that we could do this to make invalid use of HeapPtr impossible. I don't remember why we did not do this long ago.
We did not do this because it is incorrect. Copy and assignment of Heap*Ptr* create a duplicate edge in the reference graph: this is fine. The operator we want to delete is HeapPtr(MoveRef<HeapPtr>), which implicitly destroys a reference in the heap that the GC is also tracking. This will hopefully also disable use of these classes in unsafe contexts that explicitly use Move, such as HashTable and Vector.
I just found this bug from last year!  Here's a patch.
Assignee: general → jcoppeard
Attachment #8363696 - Flags: review?(terrence)
Unfortunately if I try to do this for Heap<T> the code doesn't compile because we do use these in Vectors.
Comment on attachment 8363696 [details] [diff] [review]
heap-ptr-move-constructors

Review of attachment 8363696 [details] [diff] [review]:
-----------------------------------------------------------------

Great! I've been meaning to revisit this bug for ages. Now that we're using && instead of Move, it looks much more straightforward. r=me

For Vector<Heap<T>>, maybe it would be possible to just make the move constructor work by updating the store buffer?
Attachment #8363696 - Flags: review?(terrence) → review+
So after reading up a bit more about move semantics, it seems that because we have defined a copy constructor (amongst other things) then no move constructor/assignment operator will be implicitly generated.  In that case, the copy constructor/assignment operator will be used instead, and here they actually do what we want anyway - the new Heap<T> will be created in the normal way, and the original one will remove its store buffer entry when it is destroyed.

So I think we don't need to do anything here.  Does that sound right to you?
Flags: needinfo?(terrence)
That sounds reasonable, although I'm not sure I'm really the person to ask. Jim implemented move semantics in HashTable and Vector, so he'll probably have a much better idea if what we have currently is safe.
Flags: needinfo?(terrence) → needinfo?(jimb)
I believe that is correct --- that defining an explicit copy constructor inhibits the generation of an implicitly defined move constructor (on the grounds, I guess, that if the implicitly defined copy constructor isn't appropriate, then the implicitly defined move constructor probably wouldn't be either?), so there's no need to explicitly delete it.

Here's a typescript that shows the addition of an explicit copy constructor inhibiting the declaration of an implicit move constructor:

$ cat move.cc
#include <utility>

using std::move;

struct S {
  int x;
  S() : x(0) { }
#ifdef EXPLICIT_COPY // Compiles fine if not #defined; error if #defined
  S(S &rhs) : x(rhs.x) { }
#endif
};

int main(int argc, char **argv) {
  S s1;
  S s2(s1);
  S s3(move(s1));
};
$ g++ -std=c++11 move.cc -o move
$ g++ -std=c++11 move.cc -o move -D EXPLICIT_COPY
move.cc: In function ‘int main(int, char**)’:
move.cc:16:16: error: no matching function for call to ‘S::S(std::remove_reference<S&>::type)’
   S s3(move(s1));
                ^
move.cc:16:16: note: candidates are:
move.cc:9:3: note: S::S(S&)
   S(S &rhs) : x(rhs.x) { }
   ^
move.cc:9:3: note:   no known conversion for argument 1 from ‘std::remove_reference<S&>::type {aka S}’ to ‘S&’
move.cc:7:3: note: S::S()
   S() : x(0) { }
   ^
move.cc:7:3: note:   candidate expects 0 arguments, 1 provided
$
Flags: needinfo?(jimb)
But note that you get better error messages (and your intentions are clearer!) if you do explicitly delete the move constructor:

$ cat move.cc
#include <utility>

using std::move;

struct S {
  int x;
  S() : x(0) { }
  S(S &rhs) : x(rhs.x) { }
  S(S &&rhs) = delete;
};

int main(int argc, char **argv) {
  S s1;
  S s2(s1);
  S s3(move(s1));
};
$ g++ -std=c++11 move.cc -o move
move.cc: In function ‘int main(int, char**)’:
move.cc:15:16: error: use of deleted function ‘S::S(S&&)’
   S s3(move(s1));
                ^
move.cc:9:3: error: declared here
   S(S &&rhs) = delete;
   ^
$ 

So the status quo is probably best.
Oh, wait, now I think I actually understand what Jon was asking:

If you declare a 'const T &' copy constructor, then it can be used for both copy construction and when move construction is suggested. If that's what you want, then the explicit delete is not good:

$ cat move.cc
#include <iostream>
#include <utility>

using std::cout;
using std::move;

struct S {
  int x;
  S() : x(0) { }
  S(const S &rhs) : x(rhs.x) { cout << "const S &\n"; }
#ifdef EXPLICIT_DELETE
  S(S &&rhs) = delete;
#endif
};

int main(int argc, char **argv) {
  S s1;
  S s2(s1);
  S s3(move(s1));
};
$ g++ -std=c++11 move.cc -o move && ./move
const S &
const S &
$ g++ -std=c++11 move.cc -o move -D EXPLICIT_DELETE && ./move
move.cc: In function ‘int main(int, char**)’:
move.cc:19:16: error: use of deleted function ‘S::S(S&&)’
   S s3(move(s1));
                ^
move.cc:12:3: error: declared here
   S(S &&rhs) = delete;
   ^
$
Attached patch comment_moveref_behavior-v0.diff (obsolete) — Splinter Review
Thank you for doing that research! I believe the upshot of your explanation is that we are actually working correctly with our current declarations. I assure you, this is entirely by accident.

The C++ semantics here seem rather absurdly brittle, so I think we should add some substantial comments to help make sure we don't accidentally break it later. This patch contains those comments and some additional comments on our barriers classes that have been sorely lacking for far too long now.
Assignee: jcoppeard → terrence
Status: NEW → ASSIGNED
Attachment #8371639 - Flags: review?(jimb)
... I guess I don't really understand why move semantics are intrinsically meaningless on Heap<T> values. Anything can be moved, can't it?

The move can be any operation that:
1) makes the target work "the same as" the original, and
2) leaves the original inert but destructible.

Surely something like a Heap<T>, that has some serious machinery backing it up, could be moved more efficiently than it could be copied.
Comment on attachment 8371639 [details] [diff] [review]
comment_moveref_behavior-v0.diff

Review of attachment 8371639 [details] [diff] [review]:
-----------------------------------------------------------------

I suggested some changes to this after a long discussion (with a helpful explanation from terrence) on IRC. Clearing review.
Attachment #8371639 - Flags: review?(jimb)
Thanks for the feedback, Jim! This version should be much cleaner and less confusing.
Attachment #8371639 - Attachment is obsolete: true
Attachment #8372404 - Flags: review?(jimb)
Comment on attachment 8372404 [details] [diff] [review]
comment_moveref_behavior-v1.diff

Review of attachment 8372404 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. One final suggestion...

::: js/src/gc/Barrier.h
@@ +423,5 @@
> +     * Unlike RelocatablePtr<T>, HeapPtr<T> must be managed with GC lifetimes.
> +     * Specifically, the memory used by the pointer itself must be live until
> +     * at least the next minor GC. For that reason, move semantics are invalid
> +     * and are deleted here. Please note that not all containers support move
> +     * semantics, so this does not completely prevent invalid uses.

This has the effect of making this type not usable in vectors and hash tables --- which I think is the intention, so that's good.

But it still makes me a little uncomfortable: supporting move semantics is *not* a statement about the original's *lifetime* --- you can perfectly well assign a new value to some moved-from HeapPtr and continue to use it --- but you're deleting it here to prevent people from making lifetime mistakes (by using this type in vectors and hash tables, say).

This change gets errors when you want them because, *in those cases* one moves just before destroying the original, which is a right reserved by the GC. But if there were other uses of move in the code base that were *not* in anticipating of an impending destruction, this would also prevent it from being used there.

Well, now that I've articulated that it doesn't seem like quite a big deal. So letting this stand is fine. But it's just a little weird.

Instead of "For that reason, ... invalid uses", I would say:

"Attempting to move a value is a strong suggestion that some generic container is about to destruct the original; hash tables and vectors use move in this pattern. However, since HeapPtrs must only be destructed in coordination with the GC, putting them in such containers is almost certainly a bug. So, although moving these types is technically fine, we delete the move operations to help catch such mistakes. Naturally, we'll get false positives if someone uses move semantics without intending to destroy the original; and false negatives if a generic container simply copies and destructs, and never tries to move."
Attachment #8372404 - Flags: review?(jimb) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.