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)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(2 files)
2.37 KB,
patch
|
Details | Diff | Splinter Review | |
3.53 KB,
patch
|
chmanchester
:
review+
away
:
feedback+
|
Details | Diff | Splinter Review |
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" ?
Updated•6 years ago
|
Product: Core → Firefox Build System
Comment 3•6 years ago
|
||
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
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 7•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8956916 -
Flags: review?(core-build-config-reviews) → review+
Assignee | ||
Comment 9•6 years ago
|
||
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•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30dc6c484d42
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•