1123907 - Add an analysis to ensure that a class doesn't have a vtable, and use it to verify the EntryType argument of nsTHashtable

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Comment 1

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

Attachment: Part 1: Add an analysis to ensure that a class marked MOZ_NEEDS_NO_VTABLE_TYPE cannot be instantiated by a class with a VTable

This analysis works by looking at every class template instantiation which is marked with MOZ_NEEDS_NO_VTABLE_TYPE and checking if it has a template parameter which has a VTable. The plugin considers a type to have a VTable if it is a CXXRecordDecl, and CXXRecordDecl::isDynamicClass() returns true. This occurs when there is a virtual base class, or there is a virtual method on the class.

Comment 2

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

Attachment: Part 2: Add MOZ_NEEDS_NO_VTABLE_TYPE to Attributes.h, and use it to verify the EntryType argument of nsTHashtable

Comment 3

[(no longer active)]

Comment on attachment 8624980 [details] [diff] [review]
Part 1: Add an analysis to ensure that a class marked MOZ_NEEDS_NO_VTABLE_TYPE cannot be instantiated by a class with a VTable

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

This looks great!  r=me with the below fixed.  Thanks!

::: build/clang-plugin/clang-plugin.cpp
@@ +1022,5 @@
> +    QualType argType = args[i].getAsType();
> +    while (const ArrayType *arrTy = argType->getAsArrayTypeUnsafe())
> +      argType = arrTy->getElementType();
> +    offender = argType->getAsCXXRecordDecl();
> +    if (offender && offender->isDynamicClass()) {

Please don't duplicate the logic here.  Right before the AST_MATCHERs in this file, there are similar helpers.  Please create a small helper like typeHasVTable() or some such that takes a QualType and returns a bool.  In this case, you don't need to add an overload for the class case similar to the rest of the helpers there.

::: build/clang-plugin/tests/TestNeedsNoVTableType.cpp
@@ +36,5 @@
> +  virtual void e();
> +};
> +struct F : E {
> +  virtual void e() final;
> +};

Another interesting case to test would be pure virtual functions, but of course for that you should modify the class bodies to hold a T* or some such.

@@ +43,5 @@
> +  {
> +    PickyConsumer<A> a1;
> +    PickyConsumerWrapper<A> a2;
> +    PickyConsumerSubclass<A> a3;
> +    NonPickyConsumer<A> a4;

I like the choice of variable names.  :-)

Comment 4

[(no longer active)]

Comment on attachment 8624980 [details] [diff] [review]
Part 1: Add an analysis to ensure that a class marked MOZ_NEEDS_NO_VTABLE_TYPE cannot be instantiated by a class with a VTable

OK, actually r=me this time!

Comment 5

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

Attachment: Part 1: Add an analysis to ensure that a class marked MOZ_NEEDS_NO_VTABLE_TYPE cannot be instantiated by a class with a VTable

Added tests and refactored out typeHasVTable

Comment 6

[Carsten Book [:Tomcat]]

can we get a try run for this changes ?

Comment 7

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

Oops, got this mixed up with another static analysis. This one is actually blocked on bug 1123386

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9c47646824
https://hg.mozilla.org/integration/mozilla-inbound/rev/3822943d1e31

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf9c47646824
https://hg.mozilla.org/mozilla-central/rev/3822943d1e31