663917 - Extend gcClasses.as to flag instances of multiple inheritance (of the base class of interest)

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

In Bug 663159, comment 14, Lars suggests using gcClasses.as to scan the source tree to find instances of multiple inheritance.  (And he also notes that something using clang/dehydra would be better.)

I plan to patch gcClasses.as accordingly.  (It won't be anything too smart; any instance of multiple inheritance in the tree will just be flagged accordingly.  The legend says the output flag 'M' is already taken, what a shame.  Maybe I'll go with 'm'.)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

In fact, AFAICT, the regexps in gcClasses.as are written *assuming* that all inheritance is single-inheritance.  E.g., if you select base class B in the script, and there's a defintion for D that looks like:
  class D : public A, public B
then the script won't include D as one of the subclasses of B.

So some of the data gathered using the script could well have been overlooking points in the class hierarchy.

I'll obviously have to fix this as part of this ticket (that, or abandon the ticket and switch to a different analysis tool; which would not be crazy).

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patchA: add MI handling to gcClasses.as

The main change in this patch is to add an extraBases member to the LineInfo class, so that the LineInfos can carry information about whether multiple-inheritance is present, and if so, what the other bases are, and then using that information to (1.) improve the accuracy of the printed hierarcy, and (2.) add a 'm' tag for the classes in the hierarcy that have more than one immediate base.

There are a couple drive-by fixes that I made along the way as I was exploring how the script handled my selftest (which I will be posting next).  They are:

-- Add support for matching class declarations where the base class is not preceded by public|private|virtual; and when the base class is preceded by one of those keywords, require an intervening space character.

-- Add filtering of friend declarations of the form "friend class Foo::Bar::Baz"

-- Don't match a double-colon as a single-colon in a class definition.

If pushed I will remove the drive-by fixes, though it seems like the first is a no-brainer if its reasonably sound, and at least one of the last two bullets is necessitated by the introduction of the first bullet.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: test/exploratory code for gcClasses.as

Here's the test I used to test my changes to gcClasses.as

Before applying patchA (attachment 539024 [details] [diff] [review]), it prints the following:

Legend:
  D - at least one of the definitions has an explicit destructor
  E - at least one of the definitions is exactly traced
  M - multiple definitions in the tree (in different namespaces or nested within classes)
  # - continuation line for M with additional locations
  X - definitions of the same name exist outside the tree
  @ - continuation line for @ with locations

       GCRoot                                             (internal):0
         BadClass1                                        extensions/FakeDefsForBugzilla663159.cpp:7
         GoodClass                                        extensions/FakeDefsForBugzilla663159.cpp:3
           BadClass3                                      extensions/FakeDefsForBugzilla663159.cpp:11
           BarDoesNotExtendGoodClass                      extensions/FakeDefsForBugzilla663159.cpp:28
           CuriosityKillsFelixTheCat2                     extensions/FakeDefsForBugzilla663159.cpp:41
         GoodClass2                                       extensions/FakeDefsForBugzilla663159.cpp:39
           CuriosityKillsFelixTheCat1                     extensions/FakeDefsForBugzilla663159.cpp:40
         Trickery                                         extensions/FakeDefsForBugzilla663159.cpp:33
           ExtensionOfpublicTrickeryNotTrickery           extensions/FakeDefsForBugzilla663159.cpp:34
         publicTrickery                                   extensions/FakeDefsForBugzilla663159.cpp:32

Classes: 11
Explicit destructors: 0
Exactly traced: 0 (0%)

After applying the patch, it prints:

Legend:
  D - at least one of the definitions has an explicit destructor
  E - at least one of the definitions is exactly traced
  M - multiple definitions in the tree (in different namespaces or nested within classes)
  m - at least one of the definitions has > 1 base.
  # - continuation line for M with additional locations
  X - definitions of the same name exist outside the tree
  @ - continuation line for @ with locations

        GCRoot                                             (internal):0
   m      BadClass1                                        extensions/FakeDefsForBugzilla663159.cpp:7
   m      BadClass2                                        extensions/FakeDefsForBugzilla663159.cpp:9
          GoodClass                                        extensions/FakeDefsForBugzilla663159.cpp:3
   m        BadClass3                                      extensions/FakeDefsForBugzilla663159.cpp:11
   m        BadClass4                                      extensions/FakeDefsForBugzilla663159.cpp:13
   m        BadClass5                                      extensions/FakeDefsForBugzilla663159.cpp:15
   m        CuriosityKillsFelixTheCat1                     extensions/FakeDefsForBugzilla663159.cpp:40
          GoodClass2                                       extensions/FakeDefsForBugzilla663159.cpp:39
   m        CuriosityKillsFelixTheCat2                     extensions/FakeDefsForBugzilla663159.cpp:41
          NonPubClass                                      extensions/FakeDefsForBugzilla663159.cpp:5
          Trickery                                         extensions/FakeDefsForBugzilla663159.cpp:33
          publicTrickery                                   extensions/FakeDefsForBugzilla663159.cpp:32
            ExtensionOfpublicTrickeryNotTrickery           extensions/FakeDefsForBugzilla663159.cpp:34

Classes: 14
Explicit destructors: 0
Exactly traced: 0 (0%)

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #3)
SIGH.  its hard to read the output I pasted above.  Plus, I neglected to include the command lines I used.  Here's my attempt to rectify both problems.

% hg qpop && ~/bin/asc -strict utils/gcClasses.as > /dev/null && \
  $AVM utils/gcClasses.abc -- --base GCRoot fakedefs.cpp 
popping bz663159-mi-for-gcclasses-script
patch queue now empty

gcClasses.abc, 7370 bytes written
Legend:
  D - at least one of the definitions has an explicit destructor
  E - at least one of the definitions is exactly traced
  M - multiple definitions in the tree (in different namespaces or nested within classes)
  # - continuation line for M with additional locations
  X - definitions of the same name exist outside the tree
  @ - continuation line for @ with locations

       GCRoot                                             (internal):0
         BadClass1                                        fakedefs.cpp:7
         GoodClass                                        fakedefs.cpp:3
           BadClass3                                      fakedefs.cpp:11
           BarDoesNotExtendGoodClass                      fakedefs.cpp:28
           CuriosityKillsFelixTheCat2                     fakedefs.cpp:41
         GoodClass2                                       fakedefs.cpp:39
           CuriosityKillsFelixTheCat1                     fakedefs.cpp:40
         Trickery                                         fakedefs.cpp:33
           ExtensionOfpublicTrickeryNotTrickery           fakedefs.cpp:34
         publicTrickery                                   fakedefs.cpp:32

Classes: 11
Explicit destructors: 0
Exactly traced: 0 (0%)

% hg qpush && ~/bin/asc -strict utils/gcClasses.as > /dev/null && \
  $AVM utils/gcClasses.abc -- --base GCRoot fakedefs.cpp 
applying bz663159-mi-for-gcclasses-script
now at: bz663159-mi-for-gcclasses-script

gcClasses.abc, 8296 bytes written
Legend:
  D - at least one of the definitions has an explicit destructor
  E - at least one of the definitions is exactly traced
  M - multiple definitions in the tree (in different namespaces or nested within classes)
  m - at least one of the definitions has > 1 base.
  # - continuation line for M with additional locations
  X - definitions of the same name exist outside the tree
  @ - continuation line for @ with locations

        GCRoot                                             (internal):0
   m      BadClass1                                        fakedefs.cpp:7
   m      BadClass2                                        fakedefs.cpp:9
          GoodClass                                        fakedefs.cpp:3
   m        BadClass3                                      fakedefs.cpp:11
   m        BadClass4                                      fakedefs.cpp:13
   m        BadClass5                                      fakedefs.cpp:15
   m        CuriosityKillsFelixTheCat1                     fakedefs.cpp:40
          GoodClass2                                       fakedefs.cpp:39
   m        CuriosityKillsFelixTheCat2                     fakedefs.cpp:41
          NonPubClass                                      fakedefs.cpp:5
          Trickery                                         fakedefs.cpp:33
          publicTrickery                                   fakedefs.cpp:32
            ExtensionOfpublicTrickeryNotTrickery           fakedefs.cpp:34

Classes: 14
Explicit destructors: 0
Exactly traced: 0 (0%)
%

That should make it clear how different the two constructed hierarchies are. 

(Of course, making this script smart enough to handle some corner cases like the ones in my drive-by fixes may be a mistake, since it may give clients of the script an impression that the script is solid-enough to be 100% trusted, and it really isn't ever going to be that solid compared to proper analysis tool.)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Hmm, hitting false matches when one extends template classes, like:

class A : public B<C, D>

(maybe Lars had the right idea matching on "public|private|virtual" before the class name.  But then again, we really don't want to be missing cases here...)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Also: need to extend the regexp to include "protected", as in:
  "public|private|protected|virtual"
and probably the "virtual" should be separate from the other three, or the pattern should be allowed to repeat (if it isn't already allowed to repeat).

Comment 7

[Lars T Hansen]

Comment on attachment 539024 [details] [diff] [review]
patchA: add MI handling to gcClasses.as

R+ but with some comments.

You do not call stripNamespaces on the subsequent base class names, why?

Catching BadClass7 (trailing comma without name is clear indication of partial information) will probably be a Good Public Service.

I'm fine with whatever drive-by fixes you want to land; basically the script is heuristic and we should make it as useful as we can, within reason.

(I see a fast C-based comment-stripping indexOf-running preprocessor for this script in the future to make it possible to do a significantly better job without going off the deep end runtime-wise, but maybe the time is better spent learning Clang or similar, and fixing the Player to compile with Clang.)

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #7)
> Comment on attachment 539024 [details] [diff] [review] [review]
> patchA: add MI handling to gcClasses.as
> 
> R+ but with some comments.
> 
> You do not call stripNamespaces on the subsequent base class names, why?

Ignorance.  I'll try adding it and see what happens.

> Catching BadClass7 (trailing comma without name is clear indication of
> partial information) will probably be a Good Public Service.

Good idea.

> ([...] maybe the time is
> better spent learning Clang or similar, and fixing the Player to compile
> with Clang.)

Would we be able to extract the occurrences of the exact-tracing macro forms when using Clang?

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> 
> > ([...] maybe the time is
> > better spent learning Clang or similar, and fixing the Player to compile
> > with Clang.)
> 
> Would we be able to extract the occurrences of the exact-tracing macro forms
> when using Clang?

TBD, and it we can't then maybe clang is not so useful.  That said, now that GC_POINTER is becoming obsolete when using GCMember, and DRC etc are being removed and replaced by GCMember globally (modulo GCC bugs...), the question is how many of the annotations that will need to remain, and whether those that do remain could be replaced by templated or other declarations that can be processed by Clang.

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #8)
> (In reply to comment #7)
> > Catching BadClass7 (trailing comma without name is clear indication of
> > partial information) will probably be a Good Public Service.
> 
> Good idea.

The good news is that this was a relatively easy check to add.

The bad news is that 462 lines (in 220 files) in the player source base cause the warning to fire.  :(

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Catching BadClass7 (trailing comma without name is clear indication of
> > > partial information) will probably be a Good Public Service.
> > 
> > Good idea.
> 
> The good news is that this was a relatively easy check to add.
> 
> The bad news is that 462 lines (in 220 files) in the player source base
> cause the warning to fire.  :(

(This probably over-estimates the real scope of the problem.  The actual count, if one filters out the third party directories, is probably more like 23 files.)

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: test/exploratory code for gcClasses.as

Added case exposing the necessity for stripNamespaces call for extra base classes.

Comment 13

[Lars T Hansen]

> > The bad news is that 462 lines (in 220 files) in the player source base
> > cause the warning to fire.  :(
> 
> (This probably over-estimates the real scope of the problem.  The actual
> count, if one filters out the third party directories, is probably more like
> 23 files.)

Tricky.  I suppose if we "cache" the warning and only print it if the class that we want to warn about appears in the tree then we risk missing classes altogether, not desirable at all.

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #13)
> > > The bad news is that 462 lines (in 220 files) in the player source base
> > > cause the warning to fire.  :(
> > 
> > (This probably over-estimates the real scope of the problem.  The actual
> > count, if one filters out the third party directories, is probably more like
> > 23 files.)
> 
> Tricky.  I suppose if we "cache" the warning and only print it if the class
> that we want to warn about appears in the tree then we risk missing classes
> altogether, not desirable at all.

Yeah, I think we are better off just emitting the warning.

An alternative could be to just pull in the next line and continue processing until we see either a '{' or a ';', no?  Sort of scary/hackish.

Probably better to just leave things alone until we get a chance to evaluate whether Clang will be able to see the macros (or if there's a way to put in breadcrumbs from the macros into the expanded code for Clang to see).

Comment 15

[Lars T Hansen]

(In reply to comment #14)
> (In reply to comment #13)
> > > > The bad news is that 462 lines (in 220 files) in the player source base
> > > > cause the warning to fire.  :(
> > > 
> > > (This probably over-estimates the real scope of the problem.  The actual
> > > count, if one filters out the third party directories, is probably more like
> > > 23 files.)
> > 
> > Tricky.  I suppose if we "cache" the warning and only print it if the class
> > that we want to warn about appears in the tree then we risk missing classes
> > altogether, not desirable at all.
> 
> Yeah, I think we are better off just emitting the warning.
> 
> An alternative could be to just pull in the next line and continue
> processing until we see either a '{' or a ';', no?  Sort of scary/hackish.

Yes, I thought about that.  Preprocessing gets in the way, for one thing, but it might still work more often than not.  Generally speaking this script (as well as exactgc.as) might be better off running a fast preprocessor that strips comments and joins lines and only leaves CPP lines alone; this would not truly guarantee any kind of correctness but would potentially boost accuracy.  But there are other problems with this, so I don't know whether it's worth it to pursue.

> Probably better to just leave things alone until we get a chance to evaluate
> whether Clang will be able to see the macros (or if there's a way to put in
> breadcrumbs from the macros into the expanded code for Clang to see).

Probably true.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: test/exploratory code for gcClasses.as

(slight refinement: includes illustration of explicit destructors.)

Comment 17

[Tamarin Bot]

changeset: 6405:c97a7a9e898d
user:      Felix S Klock II <[e-mail]>
summary:   Bug 663917: add MI to gcClasses.as plus other fixes (r=lhansen).

http://hg.mozilla.org/tamarin-redux/rev/c97a7a9e898d