1340588 - use GCC-style dependency generation with clang-cl, rather than the cl.py wrapper we have now

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

We have this wrapper that invokes cl for us:

http://dxr.mozilla.org/mozilla-central/source/config/config.mk#122
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/cl.py#1

so that we can get at the includes via /showincludes and write the GCC-style dependency files we want for |make|.  Doing this adds an extra process invocation on every compiler invocation on Windows, which is reasonably expensive.

We can avoid this with clang-cl, though!  clang-cl is just clang, after all, and clang knows how to write out GCC-style dependency files directly.  We can therefore ditch our wrapper (and /showincludes) and win back some compilation time.

Passing the necessary options will probably have to be done through -Xclang, as IIRC the clang-cl binary doesn't understand the options we need.

Comment 1

[(Away)]

The compiler's not happy about forwarding the arguments:

$ clang -MP -MD -MF bar foo.cpp

$ clang-cl -Xclang -MP -Xclang -MD -Xclang -MF -Xclang bar foo.cpp
error: unknown argument: '-MD'
error: unknown argument: '-MF'

$ clang --driver-mode=cl -Xclang -MP -Xclang -MD -Xclang -MF -Xclang bar ../foo.cpp
error: unknown argument: '-MD'
error: unknown argument: '-MF'

Why no complaint about -MP? The difference in those flags (https://github.com/llvm-mirror/clang/blob/master/include/clang/Driver/Options.td#L348-L364) is that -MP has "Flags<[CC1Option]>" while the others don't.

I'm out of my expertise here, so I'm guessing: does this mean -MD and -MF are gcc-driver options that can't be used because the clang-cl pipeline is just "cl-driver -> cc1" ?

Comment 2

[(Away)]

Attachment: Failed patch (don't land)

Comment 3

[(not currently active) Ted Mielczarek]

I asked about this on cfe-users (I hope that's a useful place to ask):
http://lists.llvm.org/pipermail/cfe-users/2018-March/001261.html

Comment 4

[Nathan Froyd [:froydnj]]

I remember making this work once upon a time, but I don't remember exactly what I did.  This is what I was able to make work this morning after looking at some clang driver code:

clang-cl -Xclang -MP -Xclang -MG -Xclang -dependency-file -Xclang bar -Xclang -MT -Xclang x.obj x.cpp -c

It seems to generate "bar" appropriately.  Testing a patch.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: enable clang-cl to generate depfiles directly, rather than using a wrapper

We use a wrapper script when compiling with MSVC to parse the
/showIncludes output and thereby generate a Makefile dependency
fragment.  This fragment enables us to do correct and faster incremental
builds.  But the cost of invoking the wrapper script can be significant;
it's an extra process or two to launch for every single compilation.

Instead, let's have clang-cl generate the dependencies directly, which
should be somewhat faster.

This seems to do the right thing on try (compilation completes successfully);
dmajor, do you want to try giving this a spin for incremental compilation?
Something like:

1. Build.
2. Touch some header.
3. Build.

And every file affected by that header should be rebuilt.

Comment 6

[(Away)]

I was attempting to test this, but in the middle of my build I bluescreened, and re-running |mach build| on my next boot gave me tons of things like:

 0:14.43 .deps/MurmurHash3.obj.pp:1: warning: NUL character seen; rest of line ignored
 0:14.43 .deps/MurmurHash3.obj.pp:2: warning: NUL character seen; rest of line ignored
 0:14.45 .deps/AttributeMap.obj.pp:1: warning: NUL character seen; rest of line ignored
 0:14.45 .deps/AttributeMap.obj.pp:11: warning: NUL character seen; rest of line ignored
 0:14.45 .deps/Buffer.obj.pp:1: warning: NUL character seen; rest of line ignored

And then the obj files associated with those would fail to build until I deleted the broken .pp's.

I don't know if this is relevant. Maybe I would be in just as much trouble without this change.

Comment 7

[Nathan Froyd [:froydnj]]

It's possible; the clang source has things like:

    if (Arg *MF = Args.getLastArg(options::OPT_MF)) {
      DepFile = MF->getValue();
      C.addFailureResultFile(DepFile, &JA);
    }

where addFailureResultFile has an explanatory comment:

  /// addFailureResultFile - Add a file to remove if we crash, and returns its
  /// argument.

so it looks like this hacky way doesn't inform whatever's invoking cc1 that we might need to remove things if the compilation falls over.

Then again, if you bluescreened, I don't know that doing things the non-hacky way would have succeeded...

Comment 8

[(Away)]

Comment on attachment 8956916 [details] [diff] [review]
enable clang-cl to generate depfiles directly, rather than using a wrapper

Ok, this time without bluescreens, it works! Tested js/src/wasm/WasmBaselineCompile.h and toolkit/crashreporter/nsExceptionHandler.h.

Comment 9

[Nathan Froyd [:froydnj]]

The clang-cl folks seem to think that -dependency-file is the way to go for now:

http://lists.llvm.org/pipermail/cfe-users/2018-March/001268.html

I filed a bug for real options, as that would make our lives easier:

https://bugs.llvm.org/show_bug.cgi?id=36701

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30dc6c484d42
enable clang-cl to generate depfiles directly, rather than using a wrapper; r=build-peer

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/30dc6c484d42