1479672 - Local hazard builds have bitrotted a bit

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

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

Various minor things. There's one more critically important fix to be able to run local builds, necessary because of the default build switching to clang, but that's dependent on another bug so I'll leave it for later.

Comment 1

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

Attachment: Remove --enable-stdcxx-compat option from JS shell being analyzed for hazards

This option is used for release builds to ensure that they run on ancient versions of linux distros, but it causes checker errors on local builds and We Just Don't Care.

Comment 2

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

Attachment: Reduce the number of parallel compiles for JS shell hazard analysis

Not worth a review. My laptop can have problems running this many parallel compile jobs.

Comment 3

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

Attachment: More verbose output from hazard commands

Not worth reviewing. This will print out commands for running the various substeps, which is really handy for cutting & pasting to rerun them. (Expected usage: run the whole thing locally, then cut & paste these commands to run the parts, possibly under a JS debugger.)

Comment 4

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

Attachment: Write callgraph.txt via JS shell redirection instead of bash redirection

It's slightly nicer for using the JS debugger if output doesn't go via shell redirection. (Without this change, it's common to run computeCallgraph.js under the JS debugger, and have it spend >90% of its runtime printing the output to the terminal. With this change, it will go straight to the appropriate output file.)

Comment 5

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

Attachment: Report which test failed, and suppress the spew on saying subprocess.CalledProcessError with a messy stack

I guess I'll borrow this bug for some more random stuff.

Comment 6

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

Attachment: Quote all GC functions, even if field calls

Ok, this is an abuse of this bug. This is really a fix to the test suite; when parsing the results, it wasn't working for unquoted field calls.

Comment 7

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

Attachment: Use template string for compact code,

No need to review; with the callgraph.txt output change, this really isn't needed anymore, but it also contains a switch to using template strings that looks better. So why not.

Comment 8

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

Attachment: Comment what is going on in the exceptions test so I don't waste time figuring it out again

When running locally under rr, I was baffled for a while as to why cc1plus was running twice. I thought there must be some bug in my test harness that compiled everything twice.

It turns out that it's intentional behavior for this particular test, which handles a subtlety related to exception handling. (Constructors can be interrupted with exceptions, and will run cleanup code in that case. But not if you're compiling with -fno-exceptions.)

Comment 9

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

Attachment: Force use of gcc for hazard analysis

This is a tricky one, and I need to run it through try now that I *think* the prerequisite bug has landed.

The problem is that I was clearing out any $CC/$CXX that might be set to a wrapper, and letting it use the default that will at this point in the processing be found in $PATH.

Except that now the default compiler is clang, so it'll try to use that and fail.

But... the rust compilation setup had a bug where it processes --with-compiler-wrapper stuff and mangles it if it's not in its whitelist, and somehow that gets involved here. I *think* that is fixed in bug 1409276. But until it is, this breaks the gecko tree rust compilation stuff. (Not the main rust compilation, it's when the rust compilation setup tries to compile c/c++ code.)

Anyway, it's messy and hopefully fixed, but I haven't tried it out yet. I'm not really happy with the particular fix here; I should really be doing something smarter with any existing CC/CXX settings. But this fixes the main problem, so as long as it doesn't introduce another one, it seems good enough.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e438cdb3c51
Remove --enable-stdcxx-compat option from JS shell being analyzed for hazards, r=jonco

Comment 11

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

Comment on attachment 8996204 [details] [diff] [review]
Reduce the number of parallel compiles for JS shell hazard analysis

Marking trivial patches r=me

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0e438cdb3c51

Comment 13

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

Attachment: Remove need to set GECKO_DIR when running build-haz-linux.sh, and fix shell lint warnings

With this, it is now possible to run build-haz-linux.sh with no environment variables set and have it work!

Well, once you have all of the prerequisites downloaded and placed into the directory you pass in, anyway.

Usage after this patch (quick version, checking only the JS shell):

  taskcluster/scripts/builder/build-haz-linux.sh --no-tooltool --project shell /some/work/dir

with /some/work/dir containing installs of gcc, sixgill, clang, and rustc (I'm not sure if the latter two are used at all yet.)

Those can be installed by running once by setting something like

MOZ_TOOLCHAINS="public/build/clang.tar.xz@FQ6FV3QdRI-SvJ-OyT_GeQ public/build/gcc.tar.xz@BYmAixHnT-mG2qmATEonbw public/build/sixgill.tar.xz@e8vnR-YETq6YR9B3c76oBg public/build/rustc.tar.xz@dA6thFKRQEum6HxaV9p2NA public/build/node.tar.xz@T0hi2BEGS7GPDVJY3FujQg"

and then running without the --no-tooltool option, though those will change with later updates. There's supposed to be a mach artifact toolchain command for finding the latest of these by name, but it's broken for reasons I forget, and I'm failing to find the bug number right now.

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 8996886 [details] [diff] [review]
Force use of gcc for hazard analysis

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

::: js/src/devtools/rootAnalysis/run_complete
@@ +251,5 @@
>  
>          # update the PATH so that the build will see the wrappers.
>          if (exists $ENV{CC}) {
>              $ENV{PATH} = dirname($ENV{CC}) . ":$ENV{PATH}";
> +            $ENV{CC} = "gcc";

This seems strange that we're adding the directory containing $ENV{CC} to the path but then not using it, but if you say so...

Comment 15

[Jon Coppeard (:jonco)]

Comment on attachment 8997567 [details] [diff] [review]
Remove need to set GECKO_DIR when running build-haz-linux.sh, and fix shell lint warnings

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

::: taskcluster/scripts/builder/build-haz-linux.sh
@@ +52,5 @@
>  export MOZ_OBJDIR="$WORKSPACE/obj-analyzed"
>  mkdir -p "$MOZ_OBJDIR"
>  
>  if [ -n "$DO_TOOLTOOL" ]; then
> +  ( cd "$TOOLTOOL_DIR"; "$GECKO_DIR"/mach artifact toolchain -v${TOOLTOOL_MANIFEST:+ --tooltool-url https://tooltool.mozilla-releng.net/ --tooltool-manifest "$GECKO_DIR/$TOOLTOOL_MANIFEST"} --cache-dir "$TOOLTOOL_CACHE"${MOZ_TOOLCHAINS:+ ${MOZ_TOOLCHAINS}} )

If it's possible to format this so it's more readable that would be great.

Comment 16

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

(In reply to Jon Coppeard (:jonco) from comment #14)
> Comment on attachment 8996886 [details] [diff] [review]
> Force use of gcc for hazard analysis
> 
> Review of attachment 8996886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/devtools/rootAnalysis/run_complete
> @@ +251,5 @@
> >  
> >          # update the PATH so that the build will see the wrappers.
> >          if (exists $ENV{CC}) {
> >              $ENV{PATH} = dirname($ENV{CC}) . ":$ENV{PATH}";
> > +            $ENV{CC} = "gcc";
> 
> This seems strange that we're adding the directory containing $ENV{CC} to
> the path but then not using it, but if you say so...

It's complicated and I'm not sure I completely understand it.

But 2 lines after this is

        $ENV{"PATH"} = "$wrap_dir:" . $ENV{"PATH"};

So actually, it's *not* invoking the original $ENV{CC}; we're invoking $wrap_dir/gcc. Though I wonder if $ENV{CC} = basename($ENV{CC}) would have been better here...? Anyway, $wrap_dir/gcc is a shell script that is going to look for the real compiler and invoke it with additional arguments. It finds the compiler by removing its own directory from $PATH and then basically doing `which gcc` (it gets "gcc" from whatever name you invoked the wrapper script with.)

The simpler approach would be to set $ENV{WRAPPED_CC} to $ENV{CC}, and have the wrapper script look for $WRAPPED_CC. I wonder why I didn't do that in the first place... it's tricky, because this stuff gets invoked in a number of different ways (via mach build, via plain make, via this run_complete wrapper script that sets up a common server to receive results then invokes whatever actual build command you're using, ... maybe more? We don't actually need all of these, but I didn't want to break them with any changes in the sixgill repo.)

Anyway, I will at least comment what's going on here, and maybe I'll take a stab at a $WRAPPED_CC. It won't handle all the cases, but as long as it handles this one then it doesn't really matter; the other ones won't be running this script and as long as they don't depend on $WRAPPED_CC, they shouldn't change.

Comment 17

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

Oh, right. $WRAPPED_CC doesn't handle everything that's going on, though it would simplify things a bit.

The problem is that we need to run $wrap_dir/gcc, but the build will probably run whatever $CC says, so we either have to set up $PATH and blow away $CC, or set $CC to $wrap_dir/gcc and arrange for the wrapper compiler to find the original compiler (whether it started out in $CC or CC was unset and it was found via $PATH).

Comment 18

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

Turns out there already is something like $WRAPPED_CC: $GCCDIR. So I switched to that, which makes the sequence of actions much simpler to follow. And it seems to work both locally and on try, so I'll use it instead.

Comment 19

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

(In reply to Jon Coppeard (:jonco) from comment #15)
> > +  ( cd "$TOOLTOOL_DIR"; "$GECKO_DIR"/mach artifact toolchain -v${TOOLTOOL_MANIFEST:+ --tooltool-url https://tooltool.mozilla-releng.net/ --tooltool-manifest "$GECKO_DIR/$TOOLTOOL_MANIFEST"} --cache-dir "$TOOLTOOL_CACHE"${MOZ_TOOLCHAINS:+ ${MOZ_TOOLCHAINS}} )
> 
> If it's possible to format this so it's more readable that would be great.

What do you mean? That's beautiful! The very picture of clarity and simplicity! What more could you possibly ask for?

Anyway... yeah, I backslash-newlined my way to something a little more readable.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbf72dcf175
Reduce the number of parallel compiles for JS shell hazard analysis. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b699f010a6
More verbose output from hazard commands. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e7080fdf6c
Force use of gcc for hazard analysis, r=jonco

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fbbf72dcf175
https://hg.mozilla.org/mozilla-central/rev/45b699f010a6
https://hg.mozilla.org/mozilla-central/rev/07e7080fdf6c

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d67c4217db0b
Write callgraph.txt via JS shell redirection instead of bash redirection, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc260a940fb
Remove need to set GECKO_DIR when running build-haz-linux.sh, and fix shell lint warnings, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8308ccadecfa
Report which test failed, and suppress the spew on saying subprocess.CalledProcessError with a messy stack
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0d7eda05f7
Quote all GC functions, even if field calls, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38d3c565e75
Use template string for compact code, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d9c24f99d7
Comment what is going on in the exceptions test so I don't waste time figuring it out again, r=me

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d67c4217db0b
https://hg.mozilla.org/mozilla-central/rev/5dc260a940fb
https://hg.mozilla.org/mozilla-central/rev/8308ccadecfa
https://hg.mozilla.org/mozilla-central/rev/de0d7eda05f7
https://hg.mozilla.org/mozilla-central/rev/a38d3c565e75
https://hg.mozilla.org/mozilla-central/rev/47d9c24f99d7