Closed Bug 1340588 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

All
Windows
defect
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files)

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.
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" ?
Product: Core → Firefox Build System
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
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.
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.
Attachment #8956916 - Flags: review?(core-build-config-reviews)
Attachment #8956916 - Flags: feedback?(dmajor)
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.
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 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.
Attachment #8956916 - Flags: feedback?(dmajor) → feedback+
Assignee: nobody → nfroyd
Attachment #8956916 - Flags: review?(core-build-config-reviews) → review+
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
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30dc6c484d42
enable clang-cl to generate depfiles directly, rather than using a wrapper; r=build-peer
https://hg.mozilla.org/mozilla-central/rev/30dc6c484d42
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.