1244825 - fix bugs in duplicate mRefCnt member analysis

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Nathan Froyd [:froydnj]]

I was writing some code today that inserted a:

  mRefCnt.get()

inside member functions.  Imagine my surprise when the compiler started throwing errors (a fair number of them) about ambiguous name lookups.  Even though we have a static analysis to catch this sort of thing (bug 602122), we fail to catch cases like:

class X
{
  NS_DECL_ISUPPORTS
  ...
};

class Y
{
  NS_DECL_ISUPPORTS_CYCLE_COLLECTION_WHATEVER_MACRO
  ...
};

class Z : public X, public Y
{
  ...
};

AFAICT, the existing static analysis doesn't check for cases where we inherit mRefCnt members through multiple superclasses.  Ehsan?

I am writing patches to get rid of duplicate refcounts, but it's tedious business.

Comment 1

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

Yeah, that's totally wrong! I search all superclasses for mRefCnt members, but I don't handle one mRefCnt member per superclass... Should be a really easy fix, as I currently just return early, but instead I can make it perform the correct check. I'll try to have a patch soon (but don't expect it to fix all of the new duplicate members which are found... as you said that's super tedious!)

Comment 2

[Nathan Froyd [:froydnj]]

Thanks Michael!  I guess Ehsan doesn't need to answer this ni? then.

Comment 3

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

Attachment: Detect classes with two superclasses with mRefCnt members

This is a basic implementation of the updated analysis with tests. I'm not super happy with the
error messages (for example in the case RC1->RC2->RC4 and RC1->RC3->RC4 where both superclasses
inherit the mRefCnt from the same superclass, it will look like the note 'Superclass 'RC1' has
a mRefCnt member' is duplicated. Perhaps there should be a 'via 'RC2'' or 'via 'RC3'' note attached
as well?)

Unsurprisingly this failed when I tried to build firefox with it. The clang-plugin tests pass locally though.

Comment 4

[(no longer active)]

Hmm, there is something here that I don't understand.  Why do you need to treat the case where there is only one base class differently than when there are multiple ones?  I'd expect to see something roughly like this:

class Base1 {
  int mRefCnt; // note: Found mRefCnt in a base class.
};
class Base2 {
  int mRefCnt; // note: Found mRefCnt in a base class.
};
class D : Base1, Base2 {
  int mRefCnt; // error: mRefCnt already declared in a base class.
};

Can you please clarify why you chose to do something else?

Comment 5

[(no longer active)]

Comment on attachment 8714660 [details] [diff] [review]
Detect classes with two superclasses with mRefCnt members

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

::: build/clang-plugin/clang-plugin.cpp
@@ +1324,4 @@
>  
>    // Check every superclass for whether it has a base with a refcnt member, and
>    // warn for those which do
> +  for (auto &base : D->bases()) {

If you agree with my suggestion, this loop can be simplified quite a bit!

@@ +1331,2 @@
>      if (baseRefCntMember) {
> +      if (refCntMember) {

Is there any case where refCntMember can be null?

Comment 6

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

(In reply to :Ehsan Akhgari from comment #4)
> Hmm, there is something here that I don't understand.  Why do you need to
> treat the case where there is only one base class differently than when
> there are multiple ones?  I'd expect to see something roughly like this:
> 
> class Base1 {
>   int mRefCnt; // note: Found mRefCnt in a base class.
> };
> class Base2 {
>   int mRefCnt; // note: Found mRefCnt in a base class.
> };
> class D : Base1, Base2 {
>   int mRefCnt; // error: mRefCnt already declared in a base class.
> };
> 
> Can you please clarify why you chose to do something else?

refCntMember is set to the field if the class directly has an mRefCnt member defined. However, a class may have multiple mRefCnt members without directly defining a mRefCnt field itself. for example:

class Base1 {
  int mRefCnt;
};
class Base2 {
  int mRefCnt;
};
class D : Base1, Base2 {};

Where D has two mRefCnt members, not because it and a superclass has an mRefCnt member, but because multiple of its superclasses have mRefCnt members. refCntMember is null in this case for D, because D doesn't directly have an mRefCnt field.

To make this happen, I no longer check in the matcher if the class has an mRefCnt member, instead I check all classes which either a) have an mRefCnt member or b) have multiple base classes. If they have an mRefCnt member, then if a superclass also has one then there is a conflict. If they have multiple base classes, then if at least two of those base classes has an mRefCnt member there is a conflict.

I'm not sure if there's a much simpler way to handle this than to handle it in that way.

Comment 7

[(no longer active)]

Comment on attachment 8714660 [details] [diff] [review]
Detect classes with two superclasses with mRefCnt members

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

Thanks for the explanation, makes sense now!

Comment 8

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

It seems like there is only one class in tree which fails this updated analysis: nsNavHistoryFolderResultNode. It has an mRefCnt from AsyncStatementCallback and from nsNavHistoryResultNode.

I'm not sure what the best way to fix this would be - presumably one of these classes needs to not directly implement the mRefCnt, instead causing their concrete subclasses to implement it.

mak, do you know much about this code and how hard it would be to deduplicate the mRefCnt property here?

Comment 9

[Marco Bonardo [:mak]]

changing nsNavHistoryResultNode could be more complex cause it participates to cycle collection..

On the other side AsyncStatementCallback has a lot of consumers... May we create a WeakAsyncStatementCallback, use that for nsNavHistoryFolderResultNode and have AsyncStatementCallback inherit from it and implement the refcnt?

Note I will be away for a couple weeks, if you need assistance you can ask :adw

Comment 10

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

Attachment: Detect classes with two superclasses with mRefCnt members

Rebased

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed23ebeb25b
Detect classes with two superclasses with mRefCnt members, r=ehsan

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4ed23ebeb25b