512726 - Add NS_MUST_OVERRIDE static annotation

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

The "frame poisoning" project (bug 497495) needs to be able to make sure that every class in a hierarchy defines its own version of a couple of methods, rather than inheriting them from a superclass.  AFAIK there is no way to enforce this in standard C++ so I've written a static analysis module that does it.  This patch adds the module to xpcom/analysis/ and the annotation macro to nscore.h.

The semantics of the annotation are documented in comments in both files.

I don't know how to hook this up so that the static-analysis tinderbox runs it.

There are whitespace cleanups to nscore.h in here, I figured better to do them at the same time as touching nscore.h for some substantive reason.  I can take them out if you want.

Comment 1

[[e-mail]]

Comment on attachment 396752 [details] [diff] [review]
patch

It's probably mentioned in the other bug, but I'm guessing that using pure virtual declarations won't work because you want base classes to be able to implement these methods also.

>+function get_must_overrides(cls)
>+{
>+  let mos = {};
>+  for each (let base in cls.bases)
>+    for each (let m in base.type.members)
>+      if (has_member_attribute(m, 'NS_must_override'))
>+       mos[unqualified_name(m.name)] = m;

You could also guard this on |m.isVirtual|, in case people misuse the attribute.

To hook this up as an analysis pass, take a look at xpcom/tests/static-checker/Makefile.in. You can add two cpp's, one that passes this check and one that fails, and list them under STATIC_PASS_TESTCASES/STATIC_FAILURE_TESTCASES resp. Putting the analysis script under xpcom/analysis is sufficient for it to run. r=me on the script if you want it :)

Comment 2

[Zack Weinberg (:zwol)]

(In reply to comment #1)
> (From update of attachment 396752 [details] [diff] [review])
> It's probably mentioned in the other bug, but I'm guessing that using pure
> virtual declarations won't work because you want base classes to be able to
> implement these methods also.

Correct.  The frame-object hierarchy has many classes that are used both directly and as superclasses.  They need to implement my special methods but still force subclasses to override them.

> >+      if (has_member_attribute(m, 'NS_must_override'))
> >+       mos[unqualified_name(m.name)] = m;
> 
> You could also guard this on |m.isVirtual|, in case people misuse the
> attribute.

No, I want to allow it for non-virtual methods and even for statics.  For instance, I'm currently using it for |operator new|.

> To hook this up as an analysis pass, take a look at
> xpcom/tests/static-checker/Makefile.in. You can add two cpp's, one that passes
> this check and one that fails, and list them under
> STATIC_PASS_TESTCASES/STATIC_FAILURE_TESTCASES resp. Putting the analysis
> script under xpcom/analysis is sufficient for it to run.

Thanks, I'll update the patch with that.

Comment 3

[Benjamin Smedberg]

Comment on attachment 396752 [details] [diff] [review]
patch

>diff --git a/xpcom/analysis/must-override.js b/xpcom/analysis/must-override.js

>+function unqualified_name(name)
>+{
>+  return name.split("::").pop();
>+}

This sounds iffy to me... what does this actually give you for qualified names? I think we have a signaturesMatch function somewhere that uses .shortName and the .parameters array to test.

It doesn't look like this actually adds the analysis to config/static-checking-config.mk... how did you test this?

And you should add static-checking tests to xpcom/tests/static-checking in the manner of the tests already there.

Comment 4

[Zack Weinberg (:zwol)]

(In reply to comment #3)
> (From update of attachment 396752 [details] [diff] [review])
> >diff --git a/xpcom/analysis/must-override.js b/xpcom/analysis/must-override.js
> 
> >+function unqualified_name(name)
> >+{
> >+  return name.split("::").pop();
> >+}
> 
> This sounds iffy to me... what does this actually give you for qualified names?

It gives the short method name including prototype, e.g. "A::f(int)" becomes "f(int)".  I could make this more efficient with lastIndexOf and slice, but it does exactly what I need for this analysis.

> I think we have a signaturesMatch function somewhere that uses .shortName and
> the .parameters array to test.

In type-printer.js ? That appears to look only at the prototype, not the name itself.  I suppose I could copy it, if there's a reason I ought to be comparing the parameters array instead of the stringified prototype.

What exactly does .shortName contain?  Just the base name, or the name plus the prototype?

> It doesn't look like this actually adds the analysis to
> config/static-checking-config.mk... how did you test this?

I manually rebuilt layout with CXX=".../hydra-gcc/g++ -fplugin=.../gcc_dehydra.so -fplugin-arg=.../must-override.js -DNS_STATIC_CHECKING".

I can mess with static-checking-config.mk, but I don't have a plugin-supporting GCC on this computer so I won't be able to try it till Monday.

> And you should add static-checking tests to xpcom/tests/static-checking in the
> manner of the tests already there.

Will be in the next revision, but I won't be able to confirm it works till Monday, for the same reason.

Comment 5

[[e-mail]]

(In reply to comment #4)
> It gives the short method name including prototype, e.g. "A::f(int)" becomes
> "f(int)".  I could make this more efficient with lastIndexOf and slice, but it
> does exactly what I need for this analysis.

Hmm, it breaks if your paramlist contains "::", though. Consider:

namespace n {
  typedef int foo;
  struct bar { void baz(foo i); };
};

Then .name for baz will be "n::bar::baz(n::foo)". (.shortName would be "baz".)

In the absence of a proper signaturesMatch utility function, you could just strip .memberOf off the front of .name, which would give you "baz(n::foo)".

Comment 6

[[e-mail]]

.memberOf.name, that is.

Comment 7

[Zack Weinberg (:zwol)]

Am considering copying signaturesMatch from type-printer.js into must-override.js, but before i do that, do we have anywhere to put utility functions?

Comment 8

[[e-mail]]

Somewhere in dehydra itself would be nice, perhaps Taras can suggest where.

Comment 9

[(dormant account)]

dehydra.js?

Comment 10

[(dormant account)]

Attachment: Zack's must-override stuff

Moved signaturesMatch to a common location. Made it check shortName for completeness(even if it isn't useful atm).

Made Zack's script use .shortName and library functions.

Comment 11

[(dormant account)]

renaming bug, since nscore.h already has the NS_MUST_OVERRIDE stuff.

Comment 12

[(dormant account)]

just realized i hooked this in a treehydra module, i'll fix it in next revision

Comment 13

[Zack Weinberg (:zwol)]

Attachment: more test cases

Here, have a more comprehensive "pass" test and a third "fail" test. I wrote these a while ago and never uploaded them.

Comment 14

[(dormant account)]

(In reply to comment #13)
> Created an attachment (id=401268) [details]
> more test cases
> 
> Here, have a more comprehensive "pass" test and a third "fail" test. I wrote
> these a while ago and never uploaded them.

k, i'll fold them in

Comment 15

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/2047ed35d947

Comment 16

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/f30ff14a2a42

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 18

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430