Last Comment Bug 512726 - Add NS_MUST_OVERRIDE static annotation
: Add NS_MUST_OVERRIDE static annotation
Status: RESOLVED FIXED
[fixed-lorentz]
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: (dormant account)
:
Mentors:
Depends on:
Blocks: FramePoisoning 500870 540472
  Show dependency treegraph
 
Reported: 2009-08-26 10:34 PDT by Zack Weinberg (:zwol)
Modified: 2010-04-07 06:11 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4-fixed


Attachments
patch (7.29 KB, patch)
2009-08-26 10:34 PDT, Zack Weinberg (:zwol)
benjamin: review-
Details | Diff | Splinter Review
Zack's must-override stuff (5.20 KB, patch)
2009-09-17 11:41 PDT, (dormant account)
benjamin: review+
Details | Diff | Splinter Review
more test cases (1.30 KB, patch)
2009-09-17 13:19 PDT, Zack Weinberg (:zwol)
no flags Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2009-08-26 10:34:35 PDT
Created attachment 396752 [details] [diff] [review]
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 dwitte@gmail.com 2009-08-26 11:05:02 PDT
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) 2009-08-26 11:16:31 PDT
(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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-08-26 13:05:07 PDT
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) 2009-08-26 13:41:03 PDT
(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 dwitte@gmail.com 2009-08-26 14:20:31 PDT
(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 dwitte@gmail.com 2009-08-26 14:21:25 PDT
.memberOf.name, that is.
Comment 7 Zack Weinberg (:zwol) 2009-08-26 18:01:03 PDT
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 dwitte@gmail.com 2009-08-26 18:10:07 PDT
Somewhere in dehydra itself would be nice, perhaps Taras can suggest where.
Comment 9 (dormant account) 2009-08-26 18:50:12 PDT
dehydra.js?
Comment 10 (dormant account) 2009-09-17 11:41:41 PDT
Created attachment 401242 [details] [diff] [review]
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) 2009-09-17 11:43:30 PDT
renaming bug, since nscore.h already has the NS_MUST_OVERRIDE stuff.
Comment 12 (dormant account) 2009-09-17 13:03:29 PDT
just realized i hooked this in a treehydra module, i'll fix it in next revision
Comment 13 Zack Weinberg (:zwol) 2009-09-17 13:19:43 PDT
Created attachment 401268 [details] [diff] [review]
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) 2009-09-17 13:24:54 PDT
(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) 2009-09-18 10:29:28 PDT
http://hg.mozilla.org/mozilla-central/rev/2047ed35d947
Comment 16 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-03-24 08:11:02 PDT
http://hg.mozilla.org/projects/firefox-lorentz/rev/f30ff14a2a42
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-06 16:11:04 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-04-07 06:11:54 PDT
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Note You need to log in before you can comment on or make changes to this bug.