856108 - Port the remaining dehydra analyses to clang

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Joshua Cranmer [:jcranmer]]

Patches to be uploaded shortly.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Implement the MOZ_STACK_CLASS analysis

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 2

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2a: Use MOZ_STACK_CLASS in content

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2b: Use MOZ_STACK_CLASS in dom

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2c: Use MOZ_STACK_CLASS in editor

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2d: Use MOZ_STACK_CLASS in gfx

Comment 7

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2e: Use MOZ_STACK_CLASS in layout

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2f: Use MOZ_STACK_CLASS in xpcom

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2g: Use MOZ_STACK_CLASS in xpconnect

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2h: Use MOZ_STACK_CLASS everywhere else

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Remove NS_STACK_CLASS

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

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: Part 4: Move clang static-initializers detection over

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.

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Part 5: Remove all vestiges of dehydra from our tree

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 14

[Bobby Holley (:bholley)]

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

Comment 15

[Joshua Cranmer [:jcranmer]]

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 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 18

[(no longer active)]

Comment on attachment 731236 [details] [diff] [review]
Part 1: Implement the MOZ_STACK_CLASS analysis

Review of attachment 731236 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 19

[Mats Palmgren (inactive)]

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

r=mats

Comment 20

[Benjamin Smedberg]

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

Comment 21

[Benjamin Smedberg]

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 22

[Benjamin Smedberg]

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.

Comment 23

[Mike Hommey [:glandium]]

(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)

Comment 24

[Joshua Cranmer [:jcranmer]]

(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.

Comment 25

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Remove NS_STACK_CLASS

This just removes the attributes from nscore.h

Comment 26

[Joshua Cranmer [:jcranmer]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[(no longer active)]

Is there anything else to be done here?

Comment 29

[Joshua Cranmer [:jcranmer]]

(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.

Comment 30

[Mike Hommey [:glandium]]

(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)

Comment 31

[Joshua Cranmer [:jcranmer]]

(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).

Comment 32

[Tom Ritter [:tjr]]

Close as wontfix for the remaining part?

Comment 33

[Nika Layzell [:nika] (ni? for response)]

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