838029 - Land sixgill-based GC rooting static analysis in tree

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Brian Hackett's static rooting analysis for GC hazards is currently hosted externally on a github repo. Pulling it into the tree makes it a bit easier for people to find and run it and share updates. Note that this still depends on installing sixgill separately.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Land GC static analysis

I haven't yet done any configure gunk for finding or specifying the sixgill location, so this isn't quite ready yet.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Add --with-sixgill option

Allow passing the path to a sixgill tree to configure. This will not be used by anything that is part of the build, but will be used in an auxiliary Makefile that runs that analysis.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Add --with-sixgill option and Makefile for static analysis

On second thought, I really ought to put more build bits into this patch.

The Makefile is a dummy, because really the contents of the rules are tied into the analysis. And it's not part of the build anyway, so build peers probably don't care much.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Import bhackett static rooting analysis into tree

Scripts are copied from https://github.com/bhackett1024/gcAnalysis.git

Makefile fires off an analysis job. It is likely you'd want to override many of the options. The Makefile is *not* part of the standard build; it's just available if you want to run the analysis with it.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 721342 [details] [diff] [review]
Import bhackett static rooting analysis into tree

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

Can you rename the directory to devtools/rootAnalysis?

::: js/src/devtools/analysis/README.txt
@@ +9,5 @@
> +  - [sfink] I needed a couple of patches to get it work on Fedora 16/17/18.
> +    Ask me if you need them.
> +
> +2. Compile an optimized JS shell that includes the patch at
> +   <http://people.mozilla.org/~sfink/data/bug-835552-cwd-snarf>. This does not

I think you should land this patch (or something providing equivalent functionality) to the tree as it will quickly go out of date.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 721341 [details] [diff] [review]
Add --with-sixgill option and Makefile for static analysis

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

I'm not sure I'm a fan of this patch. If this really is the most straightforward way then please let me know and I'll r+ it so as to unblock you.

::: js/src/devtools/analysis/Makefile.in
@@ +9,5 @@
> +# Makefile will use $PATH to subvert compiler invocations to add in the sixgill
> +# plugin, and then do a regular build of whatever portion of the tree you are
> +# analyzing. The plugins will dump out several xdb database files. Various
> +# analysis scripts, written in JS, will run over those database files to
> +# produce the final analysis output.

This feels like a really awkward way to do an analysis-enabled build. I don't fully understand how the sixgill stuff works, but wouldn't it be easier to simply have --with-sixgill setup the compiler and such to build an instrumented build, and then just add a "make sixgill-check" or something that runs the analysis on the instrumented build?

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Comment on attachment 721341 [details] [diff] [review]
> Add --with-sixgill option and Makefile for static analysis
> 
> Review of attachment 721341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure I'm a fan of this patch. If this really is the most
> straightforward way then please let me know and I'll r+ it so as to unblock
> you.
> 
> ::: js/src/devtools/analysis/Makefile.in
> @@ +9,5 @@
> > +# Makefile will use $PATH to subvert compiler invocations to add in the sixgill
> > +# plugin, and then do a regular build of whatever portion of the tree you are
> > +# analyzing. The plugins will dump out several xdb database files. Various
> > +# analysis scripts, written in JS, will run over those database files to
> > +# produce the final analysis output.
> 
> This feels like a really awkward way to do an analysis-enabled build. I
> don't fully understand how the sixgill stuff works, but wouldn't it be
> easier to simply have --with-sixgill setup the compiler and such to build an
> instrumented build, and then just add a "make sixgill-check" or something
> that runs the analysis on the instrumented build?

If we wanted to properly integrate it as part of the build system, yes, that would be the way to go. Sixgill does it the way it does because it wants to be able to work with large projects with arbitrarily funky build systems. This patch is really two independent things: (1) the analysis scripts, landing in the tree just so they're easier to find than in bhackett's separate repo; and (2) a basic hacky Makefile to run the analysis. Neither part really has anything to do with the mozilla build system other than borrowing its ability to make a directory under $objdir and fill in a couple of paths.

We *could* integrate this more properly, as you outline above, but I'm not sure it really makes sense right now for a few reasons:

1. The analysis depends on a specific compiler and version that we don't have on any of our releng systems currently, nor on most people's desktops. (And of the people who do, some have it as a special gcc build that is only used for this.)

2. The tree containing this analysis is often not the one you're interested in analyzing. This really is just a pile of scripts and a Makefile to duct-tape them together. The configure goop I added is just a way of recording a path to sixgill so you don't have to enter it every time you run the analysis.

3. The way this stuff runs is going to change quite a bit very soon. (It's switching to using ctypes instead of running binaries.) I don't plan on passing those changes by a build peer, since this really isn't a part of the build except during configuration.

> This feels like a really awkward way to do an analysis-enabled build. I
> don't fully understand how the sixgill stuff works, but wouldn't it be
> easier to simply have --with-sixgill setup the compiler and such to build an
> instrumented build, and then just add a "make sixgill-check" or something
> that runs the analysis on the instrumented build?

It's not an "analysis-enabled build". The build *is* the first step of the analysis. You never run any of the binaries produced by that build. (Well, you could, since they're the same as binaries produced without the plugin. But that wouldn't have anything to do with the static analysis.)

But yes, you could have a --with-sixgill that enabled the plugin (without the gcc $PATH hacks), and add a make sixgill-check that ran the scripts on the plugin's output. But I don't see much benefit to tying the analysis into the build system; they really don't have much to do with each other. (In other words, a 'sixgill-check' target doesn't seem that useful.)

It *would* make sense for --with-sixgill to enable the plugin in a nicer way (eg via compiler flags or whatever), but that feels more brittle and depends on having more smarts about our build system. (eg, would the proper settings get inherited by parts of the tree like libffi that have their own configure and don't necessarily inherit the compiler from the main build? And does it complicate things that sometimes we only want to analyze the JS shell, and sometimes we want the full tree?) Basically, I'm not doing that because I don't want to have to think about the build system, and sixgill already has an effective if hacky mechanism for injecting itself into arbitrary build systems.

Maybe I'll change my mind when/if I decide I want a full-browser analysis build on tbpl, but there's a bunch of releng work to do to get to that point.

Hopefully this makes a little more sense now.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 721341 [details] [diff] [review]
Add --with-sixgill option and Makefile for static analysis

(same patch)

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 721341 [details] [diff] [review]
Add --with-sixgill option and Makefile for static analysis

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

Okay, I guess I still don't understand enough about how sixgill works to usefully comment on it. Can you put up a wiki doc somewhere and link it in the source?

Comment 10

[Steve Fink [:sfink] [:s:]]

Green try run (with partial tests) at https://tbpl.mozilla.org/?tree=Try&rev=5808a878f449

http://hg.mozilla.org/integration/mozilla-inbound/rev/c132f3957827
http://hg.mozilla.org/integration/mozilla-inbound/rev/5d18c9bfd37b

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d18c9bfd37b
https://hg.mozilla.org/mozilla-central/rev/c132f3957827