Patches to be uploaded shortly.

I decided to try a different approach for stack classes, using the AST matcher library instead of the RAV. It's nice since I can encapsulate "this is a stack class" in a matcher and then use that to filter down to all the uses... but AST matchers aren't quite powerful enough to do all the logic together.

Comment on attachment 731237 [details] [diff] [review] Part 2a: Use MOZ_STACK_CLASS in content r=me

Everything is migrated, so this declaration is dead...

On closer inspection, the logic here seems wrong. At present, including <algorithm> will nab you a lot of extra warnings thanks to the random number generators. So feedback for now, if this is a wanted analysis.

The other analyses are the outparams analysis, must-flow-through, and frame verification, which had been previously decided to be not worth migrating over. So there's no point keeping around horribly broken stuff in our build system.

Comment on attachment 731246 [details] [diff] [review] Part 2g: Use MOZ_STACK_CLASS in xpconnect Review of attachment 731246 [details] [diff] [review]: ----------------------------------------------------------------- I don't really know what those NS_SUPPRESS_STACK_CHECK things are about, but presumably you do. rs=me

A note on all the Part 2* patches: most of that stuff is a fairly mechanical s/NS_STACK_CLASS/MOZ_STACK_CLASS/ change, plus necessary fixes to get the tree to build. Mostly, that was adding MOZ_STACK_CLASS attributes, but two were removed, one on nsAutoString and one on ... something with SVG, I forgot :-)

Comment on attachment 731238 [details] [diff] [review] Part 2b: Use MOZ_STACK_CLASS in dom Review of attachment 731238 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to claim I can do this...

Comment on attachment 731252 [details] [diff] [review] Part 4: Move clang static-initializers detection over Review of attachment 731252 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/tests/TestStaticInit.cpp @@ +1,1 @@ > +__attribute__((constructor)) void foo() { // expected-warning {{function 'foo' called during static initialization}} This isn't called anymore

Comment on attachment 731236 [details] [diff] [review] Part 1: Implement the MOZ_STACK_CLASS analysis Review of attachment 731236 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Comment on attachment 731243 [details] [diff] [review] Part 2e: Use MOZ_STACK_CLASS in layout r=mats

Comment on attachment 731251 [details] [diff] [review] Part 3: Remove NS_STACK_CLASS I really very much want the NS_STACK_CLASS tests to continue to exist, even if they move to build/clang-plugin/tests

Perhaps I marked r+ too quickly. It appears that there is a fundamental difference between NS_STACK_CLASS and MOZ_STACK_CLASS, and I'm not sure it's good! It appears that MOZ_STACK_CLASS does not automatically propagate to classes that inherit or include an instance member of the marked class? Instead you have to explicitly annotate every class in the tree as a stack class? That seems pretty unfortunate.

Comment on attachment 731252 [details] [diff] [review] Part 4: Move clang static-initializers detection over I believe that static-initializer checking was added for performance reasons in bug 573786. I have no particular love nor hate for it, so glandium should probably decide whether it is still relevant/useful.

(In reply to :Ms2ger from comment #17) > Comment on attachment 731252 [details] [diff] [review] > Part 4: Move clang static-initializers detection over > > Review of attachment 731252 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/clang-plugin/tests/TestStaticInit.cpp > @@ +1,1 @@ > > +__attribute__((constructor)) void foo() { // expected-warning {{function 'foo' called during static initialization}} > > This isn't called anymore It doesn't matter (and it wasn't called before)

(In reply to Benjamin Smedberg [:bsmedberg] from comment #20) > I really very much want the NS_STACK_CLASS tests to continue to exist, even > if they move to build/clang-plugin/tests They are retained, in similar form, by the code in part 1. (In reply to Benjamin Smedberg [:bsmedberg] from comment #21) > Perhaps I marked r+ too quickly. It appears that there is a fundamental > difference between NS_STACK_CLASS and MOZ_STACK_CLASS, and I'm not sure it's > good! It appears that MOZ_STACK_CLASS does not automatically propagate to > classes that inherit or include an instance member of the marked class? > Instead you have to explicitly annotate every class in the tree as a stack > class? That seems pretty unfortunate. The decision for the latter point is that, since most of the MOZ_STACK_CLASS values are basically RAII things, it's probably better to explicitly annotate that a class must be located on the stack rather than having it be automatically inferred and getting surprised later. I'm personally of the opinion that's it better to be explicit about what should be going on than letting the compiler infer it and get a mysterious error later down the line.

This just removes the attributes from nscore.h

Parts 1-3 checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/d565aefe20b9 https://hg.mozilla.org/integration/mozilla-inbound/rev/772ef35c06d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d3abbee125 https://hg.mozilla.org/integration/mozilla-inbound/rev/250fc9026b5a https://hg.mozilla.org/integration/mozilla-inbound/rev/abc42da07933 https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5bc945e72f https://hg.mozilla.org/integration/mozilla-inbound/rev/3a772aaaa017 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a343ec4a75c https://hg.mozilla.org/integration/mozilla-inbound/rev/ece8b0253775 https://hg.mozilla.org/integration/mozilla-inbound/rev/13c33b6d787d

https://hg.mozilla.org/mozilla-central/rev/d565aefe20b9 https://hg.mozilla.org/mozilla-central/rev/772ef35c06d3 https://hg.mozilla.org/mozilla-central/rev/a0d3abbee125 https://hg.mozilla.org/mozilla-central/rev/250fc9026b5a https://hg.mozilla.org/mozilla-central/rev/abc42da07933 https://hg.mozilla.org/mozilla-central/rev/8b5bc945e72f https://hg.mozilla.org/mozilla-central/rev/3a772aaaa017 https://hg.mozilla.org/mozilla-central/rev/2a343ec4a75c https://hg.mozilla.org/mozilla-central/rev/ece8b0253775 https://hg.mozilla.org/mozilla-central/rev/13c33b6d787d

Is there anything else to be done here?

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #28) > Is there anything else to be done here? I never ported the detecting static constructors patch to the clang-based approach, since I could never quite get it working without false positives. As a result, I also never checked in the cleanup of the dehydra analyses.

(In reply to Joshua Cranmer [:jcranmer] from comment #29) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #28) > > Is there anything else to be done here? > > I never ported the detecting static constructors patch to the clang-based > approach, since I could never quite get it working without false positives. Weren't you also getting false negatives due to the difference in how gcc and clang emit static constructors? (gcc emitting more)

(In reply to Mike Hommey [:glandium] from comment #30) > (In reply to Joshua Cranmer [:jcranmer] from comment #29) > > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > > #28) > > > Is there anything else to be done here? > > > > I never ported the detecting static constructors patch to the clang-based > > approach, since I could never quite get it working without false positives. > > Weren't you also getting false negatives due to the difference in how gcc > and clang emit static constructors? (gcc emitting more) I don't recall. I know the patch on this bug for that topic has a false positive in an STL file which fired on about 2/3 of our code (or, basically, any templated class).

Close as wontfix for the remaining part?

Yeah, I think we can close this one out now.