Closed Bug 856108 Opened 7 years ago Closed 2 years ago

Port the remaining dehydra analyses to clang

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

(Whiteboard: [leave open])

Attachments

(12 files, 1 obsolete file)

10.88 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
27.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.11 KB, patch
Ms2ger
: review+
Details | Diff | Splinter Review
10.94 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
37.87 KB, patch
mats
: review+
Details | Diff | Splinter Review
8.24 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
6.40 KB, patch
bholley
: review+
Details | Diff | Splinter Review
11.39 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
6.90 KB, patch
glandium
: feedback+
Details | Diff | Splinter Review
93.30 KB, patch
Details | Diff | Splinter Review
1.19 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
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.
Attachment #731236 - Flags: review?(ehsan)
Attachment #731237 - Flags: review?(bzbarsky)
Attachment #731238 - Flags: review?(khuey)
Comment on attachment 731237 [details] [diff] [review]
Part 2a: Use MOZ_STACK_CLASS in content

r=me
Attachment #731237 - Flags: review?(bzbarsky) → review+
Attachment #731243 - Flags: review?(dbaron)
Attachment #731245 - Flags: review?(benjamin)
Attachment #731246 - Flags: review?(bobbyholley+bmo)
Attached patch Part 3: Remove NS_STACK_CLASS (obsolete) — Splinter Review
Everything is migrated, so this declaration is dead...
Attachment #731251 - Flags: review?(benjamin)
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.
Attachment #731252 - Flags: feedback?(benjamin)
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
Attachment #731246 - Flags: review?(bobbyholley+bmo) → review+
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...
Attachment #731238 - Flags: review?(khuey) → review+
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!
Attachment #731236 - Flags: review?(ehsan) → review+
Attachment #731240 - Flags: review?(ehsan) → review+
Attachment #731243 - Flags: review?(dbaron) → review?(matspal)
Comment on attachment 731243 [details] [diff] [review]
Part 2e: Use MOZ_STACK_CLASS in layout

r=mats
Attachment #731243 - Flags: review?(matspal) → review+
Attachment #731245 - Flags: review?(benjamin) → review+
Attachment #731250 - Flags: review?(benjamin) → review+
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
Attachment #731251 - Flags: review?(benjamin) → review-
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.
Attachment #731252 - Flags: feedback?(benjamin) → feedback?(mh+mozilla)
Attachment #731252 - Flags: feedback?(mh+mozilla) → feedback+
(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.
Blocks: 859523
This just removes the attributes from nscore.h
Attachment #731251 - Attachment is obsolete: true
Attachment #734910 - Flags: review?(benjamin)
Attachment #734910 - Flags: review?(benjamin) → review+
Is there anything else to be done here?
Flags: needinfo?(Pidgeot18)
(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.
Flags: needinfo?(Pidgeot18)
(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).
Product: Core → Firefox Build System
Close as wontfix for the remaining part?
Flags: needinfo?(nika)
Yeah, I think we can close this one out now.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(nika)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.