1159433 - Add an analysis that will prevent non-memmovable classes to be passed as template arguments to classes that require their template argument to be memmovable

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

Here is the scenario that this analysis would catch:

template <class T>
class nsTArray {
  // T must be memmovable!
};

template <class T>
class nsAutoTArray MOZ_NON_MEMMOVABLE {
  // ...
};

class MiddleMan {
  nsAutoTArray<int> mMember;
};

void foo() {
  nsTArray<MiddleMan> badArray;
}

When the compiler sees badArray, it should scream and refuse to compile the code.

Comment 1

[(no longer active)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

Comment 2

[(no longer active)]

Attachment: Part 2: Add the MOZ_NON_MEMMOVABLE and MOZ_NEEDS_MEMMOVABLE_TYPE macros to MFBT

Comment 3

[(no longer active)]

Attachment: Part 3: Mark nsAutoTArray, AutoFallibleTArray and nsTAutoString_CharT as non-memmovable

All of these types can store pointers to their other members, so they are
not safe to be moved in memory.

Comment 4

[(no longer active)]

Attachment: Part 4: Make nsTArray, FallibleTArray, nsAutoTArray and AutoFallibleTArray only accept memmovable argument types

These containers can all reallocate and memmove the contents of their
storage buffers.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8603892 [details] [diff] [review]
Part 4: Make nsTArray, FallibleTArray, nsAutoTArray and AutoFallibleTArray only accept memmovable argument types

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

This isn't the right place to add this annotation.  The right place is here:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#655

I don't know whether or how the analysis takes into account template specialization, though.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8603891 [details] [diff] [review]
Part 3: Mark nsAutoTArray, AutoFallibleTArray and nsTAutoString_CharT as non-memmovable

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

Can you see about applying the annotation to these classes also?

http://mxr.mozilla.org/mozilla-central/search?string=ALLOW_MEMMOVE+%3D+false&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

You may want to see about applying it to mfbt/Vector.h as well.

Comment 7

[(no longer active)]

I'll see if I can attach an annotation just to a template class and not its specializations, which would be needed to move this annotation to nsTArray_CopyChooser.

Comment 8

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7)
> I'll see if I can attach an annotation just to a template class and not its
> specializations, which would be needed to move this annotation to
> nsTArray_CopyChooser.

I have this working now.

I'm planning to address comment 6 in a follow-up...

Comment 9

[(no longer active)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

Comment 10

[(no longer active)]

Attachment: Part 3: Mark nsTArray_CopyChooser and nsTAutoString_CharT as non-memmovable

All of these types can store pointers to their other members, so they are
not safe to be moved in memory.

Comment 11

[(no longer active)]

Attachment: Part 4: Make nsTArray, FallibleTArray, nsAutoTArray and AutoFallibleTArray only accept memmovable argument types

These containers can all reallocate and memmove the contents of their
storage buffers.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8622507 [details] [diff] [review]
Part 3: Mark nsTArray_CopyChooser and nsTAutoString_CharT as non-memmovable

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

r=me with some tweaking of the documentation.

::: xpcom/glue/nsTArray.h
@@ +660,5 @@
>  //
>  // The default behaviour is to use memcpy/memmove for everything.
>  //
>  template<class E>
> +struct MOZ_NON_MEMMOVABLE nsTArray_CopyChooser

I think the documentation for MOZ_NON_MEMMOVABLE needs an update, if this is how the attribute gets used.  Since this copy chooser *is* memmovable, but the requirement for non-memmovability actually applies to the template argument, right?

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8622508 [details] [diff] [review]
Part 4: Make nsTArray, FallibleTArray, nsAutoTArray and AutoFallibleTArray only accept memmovable argument types

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

::: xpcom/glue/nsTArray.h
@@ +2042,5 @@
>  // file for more details.
>  //
>  template<class E>
> +class MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray :
> +  public nsTArray_Impl<E, nsTArrayInfallibleAllocator>

Wait, didn't we attach the attribute to the copy chooser, so we *wouldn't* need to annotate all the array types separately?  I am confused.

Comment 14

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> Comment on attachment 8622507 [details] [diff] [review]
> Part 3: Mark nsTArray_CopyChooser and nsTAutoString_CharT as non-memmovable
> 
> Review of attachment 8622507 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with some tweaking of the documentation.
> 
> ::: xpcom/glue/nsTArray.h
> @@ +660,5 @@
> >  //
> >  // The default behaviour is to use memcpy/memmove for everything.
> >  //
> >  template<class E>
> > +struct MOZ_NON_MEMMOVABLE nsTArray_CopyChooser
> 
> I think the documentation for MOZ_NON_MEMMOVABLE needs an update, if this is
> how the attribute gets used.  Since this copy chooser *is* memmovable, but
> the requirement for non-memmovability actually applies to the template
> argument, right?

It applies to the template arguments of the types that use nsTArray_CopyChooser.  Think of it this way: a MOZ_NON_MEMMOVABLE nsTArray_CopyChooser cannot be used as a template argument where MOZ_NEEDS_MEMMOVABLE_TYPE is specified.

Can you think of a better rewording for the documentation?  I'm at a loss for words here.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #13)
> Comment on attachment 8622508 [details] [diff] [review]
> Part 4: Make nsTArray, FallibleTArray, nsAutoTArray and AutoFallibleTArray
> only accept memmovable argument types
> 
> Review of attachment 8622508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/glue/nsTArray.h
> @@ +2042,5 @@
> >  // file for more details.
> >  //
> >  template<class E>
> > +class MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray :
> > +  public nsTArray_Impl<E, nsTArrayInfallibleAllocator>
> 
> Wait, didn't we attach the attribute to the copy chooser, so we *wouldn't*
> need to annotate all the array types separately?  I am confused.

Oops, sorry, this needs to go on nsTArray_base...

Comment 15

[(no longer active)]

Attachment: Part 4: Make nsTArray_base only accept memmovable argument types

The containers derived from nsTArray_base can all reallocate and memmove
the contents of their storage buffers.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> > Comment on attachment 8622507 [details] [diff] [review]
> > Part 3: Mark nsTArray_CopyChooser and nsTAutoString_CharT as non-memmovable
> > 
> > Review of attachment 8622507 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with some tweaking of the documentation.
> > 
> > ::: xpcom/glue/nsTArray.h
> > @@ +660,5 @@
> > >  //
> > >  // The default behaviour is to use memcpy/memmove for everything.
> > >  //
> > >  template<class E>
> > > +struct MOZ_NON_MEMMOVABLE nsTArray_CopyChooser
> > 
> > I think the documentation for MOZ_NON_MEMMOVABLE needs an update, if this is
> > how the attribute gets used.  Since this copy chooser *is* memmovable, but
> > the requirement for non-memmovability actually applies to the template
> > argument, right?
> 
> It applies to the template arguments of the types that use
> nsTArray_CopyChooser.  Think of it this way: a MOZ_NON_MEMMOVABLE
> nsTArray_CopyChooser cannot be used as a template argument where
> MOZ_NEEDS_MEMMOVABLE_TYPE is specified.

I agree that this wording of it makes sense, but consider:

struct nsTArray_CopyChooser<int>;

This is MOZ_NON_MEMMOVABLE, right?  This seems a little weird to me, because the whole point of this default traits structure is to move things around with memmove.  And saying traits structures are not memmovable is also odd.  (Maybe we should have a MOZ_REQUIRES_MEMMOVABLE_TEMPLATE_ARGUMENT(ARG_NAME, ...) ?  I realize that's very verbose, but it would make things clearer...) But OK, let's move on.

struct nsTArray_base<FallibleAllocator, nsTArray_CopyChooser<int>>;

since nsTArray_base is MOZ_NEEDS_MEMMOVABLE_TYPE and nsTArray_CopyChooser<int> is MOZ_NON_MEMMOVABLE, this...presumably works, because your tree still compiles. :)  But I don't see how, given the above.

Does having the attribute on nsTArray_base also complain about the case where we specialize nsTArray_CopyChooser for a non-memmovable type T?  That doesn't seem right, because the whole point of nsTArray_CopyChooser is so moving elements around uses proper C++ cdtors, which presumably update any interior pointers.

I don't think I get what the attribute on nsTArray_base is catching, or, come to think of it, why I r+'d MOZ_NON_MEMMOVABLE on nsTArray_CopyChooser--shouldn't that have been MOZ_NEEDS_MEMMOVABLE_TYPE?

There must either be a step missing from the above explanation, or I am just being super dense.  Which is it?

> Can you think of a better rewording for the documentation?  I'm at a loss
> for words here.

I'm at a loss too, because I don't understand what's going on yet! :)

Comment 17

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #16)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #14)
> > (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> > > Comment on attachment 8622507 [details] [diff] [review]
> > > Part 3: Mark nsTArray_CopyChooser and nsTAutoString_CharT as non-memmovable
> > > 
> > > Review of attachment 8622507 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > r=me with some tweaking of the documentation.
> > > 
> > > ::: xpcom/glue/nsTArray.h
> > > @@ +660,5 @@
> > > >  //
> > > >  // The default behaviour is to use memcpy/memmove for everything.
> > > >  //
> > > >  template<class E>
> > > > +struct MOZ_NON_MEMMOVABLE nsTArray_CopyChooser
> > > 
> > > I think the documentation for MOZ_NON_MEMMOVABLE needs an update, if this is
> > > how the attribute gets used.  Since this copy chooser *is* memmovable, but
> > > the requirement for non-memmovability actually applies to the template
> > > argument, right?
> > 
> > It applies to the template arguments of the types that use
> > nsTArray_CopyChooser.  Think of it this way: a MOZ_NON_MEMMOVABLE
> > nsTArray_CopyChooser cannot be used as a template argument where
> > MOZ_NEEDS_MEMMOVABLE_TYPE is specified.
> 
> I agree that this wording of it makes sense, but consider:
> 
> struct nsTArray_CopyChooser<int>;
> 
> This is MOZ_NON_MEMMOVABLE, right?

No!  This second iteration of my patches differentiates between the base template and its specializations.  The idea is to make the base nsTArray_CopyChooer MOZ_NON_MEMMOVABLE, but don't specify the attribute on the specializations, because presumably a properly written specialization will make the type safe to be stored in nsTArray.

If you read the test case I'm adding in part 1, things should be clearer.

> This seems a little weird to me, because
> the whole point of this default traits structure is to move things around
> with memmove.  And saying traits structures are not memmovable is also odd. 
> (Maybe we should have a MOZ_REQUIRES_MEMMOVABLE_TEMPLATE_ARGUMENT(ARG_NAME,
> ...) ?  I realize that's very verbose, but it would make things clearer...)
> But OK, let's move on.

I hope this makes sense with the above paragraph.

Honestly, this nsTArray_CopyChooser thing will be the exception, since almost nobody else seems to do things this way.  If I have to pick between choosing a name that makes sense for this case versus everything else, I would happily explain things for CopyChooser with a comment or something.

> struct nsTArray_base<FallibleAllocator, nsTArray_CopyChooser<int>>;
> 
> since nsTArray_base is MOZ_NEEDS_MEMMOVABLE_TYPE and
> nsTArray_CopyChooser<int> is MOZ_NON_MEMMOVABLE, this...presumably works,
> because your tree still compiles. :)  But I don't see how, given the above.

I think the rest of your questions should be answered by the above.  Please let me know if they aren't.

Comment 18

[(no longer active)]

Apparently in comment 5, Nathan was asking for the "needs movable" attribute to be added to the nsTArray_CopyChooser itself.  So I need to change how the analysis works again.

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

This is an updated version of the patch which should act more the way that you were asking. Can you take a look and make sure that it's what you're looking for?

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Make nsTArray_base only accept memmovable argument types

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Mark nsTAutoString_CharT as non-memmovable

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8622594 [details] [diff] [review]
Part 4: Make nsTArray_base only accept memmovable argument types

># HG changeset patch
># User Ehsan Akhgari <[e-mail]>
>
>Bug 1159433 - Part 4: Make nsTArray_base only accept memmovable argument types; r=froydnj
>
>The containers derived from nsTArray_base can all reallocate and memmove
>the contents of their storage buffers.
>
>diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h
>index 5c5a905..ca47de2 100644
>--- a/xpcom/glue/nsTArray.h
>+++ b/xpcom/glue/nsTArray.h
>@@ -338,17 +338,17 @@ struct nsTArray_SafeElementAtHelper<mozilla::dom::OwningNonNull<E>, Derived>
> };
> 
> //
> // This class serves as a base class for nsTArray.  It shouldn't be used
> // directly.  It holds common implementation code that does not depend on the
> // element type of the nsTArray.
> //
> template<class Alloc, class Copy>
>-class nsTArray_base
>+class MOZ_NEEDS_MEMMOVABLE_TYPE nsTArray_base
> {
>   // Allow swapping elements with |nsTArray_base|s created using a
>   // different allocator.  This is kosher because all allocators use
>   // the same free().
>   template<class Allocator, class Copier>
>   friend class nsTArray_base;
> 
> protected:

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8624505 [details] [diff] [review]
Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

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

My eyes are glazing over a bit at the similarity of all the tests.  Maybe define some of the wrapper classes closer to the place where they're being tested, more descriptive class names, more descriptive function names, or some comments to guide the reader as to what's going on?

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

I've changed the tests to make it (hopefully) more clear what they are testing.

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8627212 [details] [diff] [review]
Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

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

Still glazing over, but at least there are some nice comments to break up the monotony. :)

Comment 26

[(no longer active)]

Comment on attachment 8627212 [details] [diff] [review]
Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

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

This looks awesome, thanks for taking this on!

::: build/clang-plugin/clang-plugin.cpp
@@ +498,5 @@
> +
> +  // Look through all base cases to figure out if the parent is a non-memmovable class.
> +  for (CXXRecordDecl::base_class_const_iterator base = D->bases_begin();
> +       base != D->bases_end(); ++base) {
> +    auto result = isClassNonMemMovableWorker(base->getType());

Nit: please avoid using auto here and elsewhere as the clang version we use on automation is ancient and warns about auto usage since it was built before LLVM switched to using C++11.  (I know!!!)

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

auto => const CXXRecordDecl*

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8624506 [details] [diff] [review]
Part 3: Make nsTArray_base only accept memmovable argument types

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

\o/  You probably want to fix up the commit message to more closely reflect the reality of this patch, though. :)

Comment 29

[Nathan Froyd [:froydnj]]

Comment on attachment 8624507 [details] [diff] [review]
Part 4: Mark nsTAutoString_CharT as non-memmovable

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

Hooray.  I think we can get rid of the nsTArrayElementTraits specialization in this file now in a followup bug?

Comment 30

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Make nsTArray_CopyChooser only accept memmovable argument types by default

Slightly more accurate commit message

Comment 31

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #29)
> Hooray.  I think we can get rid of the nsTArrayElementTraits specialization
> in this file now in a followup bug?

We could remove it, but if we do then if you aren't compiling in clang, with clang plugins enabled (which AFAIK very few people are), then you will only get feedback on your error when it hits try. One of the advantages of abusing templates is that doing so works everywhere.

I can file the follow up bug if you want.

Comment 32

[(no longer active)]

Has this passed try server?

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Here's a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49275fbb1a04

I'm pretty sure it will pass though, as I don't actually touch any runtime code, only the static analysis.

Comment 34

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to ensure that some template arguments cannot be non-memmovable types

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d42819af459
s/nullptr/0 (*shudder*)

Comment 35

[Nika Layzell [:nika] (ni? for response)]

Nevermind - this is going to wait until we can use a newer version of clang to build the compiler plugin.  expected-error-re apparently doesn't work on the clang we are using on try.

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Mark nsTAutoString_CharT and nsAutoArrayBase as non-memmovable

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/871b7ba6d7f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/658f6fa34af0
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f7f29080831
https://hg.mozilla.org/integration/mozilla-inbound/rev/7680e6f688a6

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/871b7ba6d7f0
https://hg.mozilla.org/mozilla-central/rev/658f6fa34af0
https://hg.mozilla.org/mozilla-central/rev/1f7f29080831
https://hg.mozilla.org/mozilla-central/rev/7680e6f688a6