Closed Bug 1244825 Opened 4 years ago Closed 4 years ago

fix bugs in duplicate mRefCnt member analysis

Categories

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

defect
Not set

Tracking

(firefox47 affected, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox47 --- affected
firefox51 --- fixed

People

(Reporter: froydnj, Assigned: Nika)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(ehsan)
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!)
Assignee: nobody → michael
Thanks Michael!  I guess Ehsan doesn't need to answer this ni? then.
Flags: needinfo?(ehsan)
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.
Attachment #8714660 - Flags: review?(ehsan)
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 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?
Flags: needinfo?(michael)
(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.
Flags: needinfo?(michael)
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!
Attachment #8714660 - Flags: review?(ehsan) → review+
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?
Flags: needinfo?(mak77)
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
Flags: needinfo?(mak77)
Depends on: 1292224
Attachment #8714660 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed23ebeb25b
Detect classes with two superclasses with mRefCnt members, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/4ed23ebeb25b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.