Closed Bug 1159433 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(4 files, 12 obsolete files)

3.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.20 KB, patch
Details | Diff | Splinter Review
51.14 KB, patch
Details | Diff | Splinter Review
1.58 KB, patch
Details | Diff | Splinter Review
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.
Flags: needinfo?(ehsan)
Depends on: 1163397
All of these types can store pointers to their other members, so they are
not safe to be moved in memory.
Attachment #8603891 - Flags: review?(nfroyd)
These containers can all reallocate and memmove the contents of their
storage buffers.
Attachment #8603892 - Flags: review?(nfroyd)
Flags: needinfo?(ehsan)
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.
Attachment #8603892 - Flags: review?(nfroyd)
Attachment #8603890 - Flags: review?(nfroyd) → review+
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.
Attachment #8603891 - Flags: review?(nfroyd) → review+
Attachment #8603889 - Flags: review?(jmuizelaar)
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.
Flags: needinfo?(ehsan)
Depends on: 1166282
(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...
Flags: needinfo?(ehsan)
Attachment #8603889 - Attachment is obsolete: true
Attachment #8622506 - Flags: review?(jmuizelaar)
All of these types can store pointers to their other members, so they are
not safe to be moved in memory.
Attachment #8622507 - Flags: review?(nfroyd)
These containers can all reallocate and memmove the contents of their
storage buffers.
Attachment #8622508 - Flags: review?(nfroyd)
Attachment #8603891 - Attachment is obsolete: true
Attachment #8603892 - Attachment is obsolete: true
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?
Attachment #8622507 - Flags: review?(nfroyd) → review+
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.
Attachment #8622508 - Flags: review?(nfroyd)
(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...
The containers derived from nsTArray_base can all reallocate and memmove
the contents of their storage buffers.
Attachment #8622508 - Attachment is obsolete: true
Attachment #8622594 - Flags: review?(nfroyd)
(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! :)
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
Attachment #8622506 - Flags: review?(jmuizelaar)
Attachment #8622594 - Flags: review?(nfroyd)
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?
Attachment #8622506 - Attachment is obsolete: true
Attachment #8624505 - Flags: feedback?(nfroyd)
Comment on attachment 8622594 [details] [diff] [review]
Part 4: Make nsTArray_base only accept memmovable argument types

># HG changeset patch
># User Ehsan Akhgari <ehsan@mozilla.com>
>
>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:
Attachment #8622594 - Attachment is obsolete: true
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?
Attachment #8624505 - Flags: feedback?(nfroyd) → feedback+
I've changed the tests to make it (hopefully) more clear what they are testing.
Attachment #8624505 - Attachment is obsolete: true
Attachment #8627212 - Flags: feedback?(nfroyd)
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. :)
Attachment #8627212 - Flags: feedback?(nfroyd) → feedback+
Attachment #8627212 - Flags: review?(ehsan)
Attachment #8624507 - Flags: review?(ehsan)
Attachment #8624506 - Flags: review?(ehsan)
Attachment #8624507 - Flags: review?(ehsan) → review?(nfroyd)
Attachment #8624506 - Flags: review?(ehsan) → review?(nfroyd)
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!!!)
Attachment #8627212 - Flags: review?(ehsan) → review+
auto => const CXXRecordDecl*
Attachment #8627212 - Attachment is obsolete: true
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. :)
Attachment #8624506 - Flags: review?(nfroyd) → review+
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?
Attachment #8624507 - Flags: review?(nfroyd) → review+
Slightly more accurate commit message
Attachment #8624506 - Attachment is obsolete: true
(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.
Has this passed try server?
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.
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.
Depends on: 1123386
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.