Closed Bug 1123907 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan, Assigned: Nika)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
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.
Attachment #8624980 - Flags: review?(ehsan)
Assignee: nobody → michael
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 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!
Attachment #8624980 - Flags: review?(ehsan) → review+
Attachment #8624981 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
can we get a try run for this changes ?
Flags: needinfo?(michael)
Keywords: checkin-needed
Oops, got this mixed up with another static analysis. This one is actually blocked on bug 1123386
Depends on: 1123386
Flags: needinfo?(michael)
https://hg.mozilla.org/mozilla-central/rev/bf9c47646824
https://hg.mozilla.org/mozilla-central/rev/3822943d1e31
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.