Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1724533002:
clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` (Closed)
Descriptionclang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo`
The compiler only needs the INCLUDE environment var, and those contents
can just be passed via flags. clang-cl's -imsvc flag adds include directories
as-if they're from INCLUDE (i.e. they're in the right place in the directory
search order, and they're treated as system headers that don't emit warnings).
In 64-bit builds, this would also work for MSVC, but MSVC happily warns about
questionable code in system headers if enough warnings are turned on, so we would
just use /I there (and make sure the flags are early on the compile command so
that these directories are searched at the same time as they would be with INCLUDE).
However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit
bin directories to load dlls. Since invoking the compiler so differently for 32-bit
and 64-bit is strange, only do this simplification for clang-cl for now.
goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning
about this clang-cl flag (https://b//28179421).
Build time impact:
64-bit debug builds symbol_level=1, building some binary ("gn")
before:
clang-cl: 34.9s, 35.1s, 35.1s
cl: 26.3s, 26.1s, 27.6s
after:
clang-cl: 33.8s, 33.7s, 34s
cl: 27s, 34.5s, 26.7s
So no discernible build perf difference, but fewer processes
and less reliance on these environment files seems like a good
change anyhow.
It also helps with a potential cross-compile of chrome/win, since
ninja's -t msvc only exists in ninja/win.
BUG=588831
Committed: https://crrev.com/8435ab74b05929d290a11808a0a94ea7536531d7
Cr-Commit-Position: refs/heads/master@{#392624}
Patch Set 1 #Patch Set 2 : . #
Total comments: 7
Patch Set 3 : no msvc #Patch Set 4 : comment, nicer, put in rspfile #Patch Set 5 : actually #
Total comments: 7
Patch Set 6 : foo #
Total comments: 2
Patch Set 7 : comment #Patch Set 8 : durrrr #Patch Set 9 : poc #
Total comments: 4
Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : imsvc #Patch Set 13 : formatting #
Messages
Total messages: 36 (9 generated)
Description was changed from
==========
no -t msvc -e no more
BUG=
==========
to
==========
no -t msvc -e no more
64-bit debug builds symbol_level=1, building some binary ("gn")
before:
clang-cl: 34.9s, 35.1s, 35.1s
cl: 26.3s, 26.1s, 27.6s
after:
clang-cl: 33.8s, 33.7s, 34s
cl: 27s, 34.5s, 26.7s
So no discernible build perf difference, but fewer processes
and less reliance on these environment files seems like a good
change anyhow.
BUG=
==========
Description was changed from
==========
no -t msvc -e no more
64-bit debug builds symbol_level=1, building some binary ("gn")
before:
clang-cl: 34.9s, 35.1s, 35.1s
cl: 26.3s, 26.1s, 27.6s
after:
clang-cl: 33.8s, 33.7s, 34s
cl: 27s, 34.5s, 26.7s
So no discernible build perf difference, but fewer processes
and less reliance on these environment files seems like a good
change anyhow.
BUG=
==========
to
==========
gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo`
The compiler only needs the INCLUDE environment var, and those contents
can just be passed via flags.
clang-cl considers headers in INCLUDE as "system headers" and doesn't print
warnings for questionable code in these directories, so use -isystem= there.
MSVC happily warns about questionable code in system headers if enough warnings
are turned on, just use /I there (and make sure the flags are early on the
compile command so that these directories are searched at the same time as they
would be with INCLUDE).
clang-cl only recently exposed -isystem=, so landing this is blocked on a clang
roll. Also, goma doesn't know yet that clang-cl learned about this flag, so
this is also blocked on goma learning about clang-cl learning about this
(http://b/27283528).
Build time impact:
64-bit debug builds symbol_level=1, building some binary ("gn")
before:
clang-cl: 34.9s, 35.1s, 35.1s
cl: 26.3s, 26.1s, 27.6s
after:
clang-cl: 33.8s, 33.7s, 34s
cl: 27s, 34.5s, 26.7s
So no discernible build perf difference, but fewer processes
and less reliance on these environment files seems like a good
change anyhow.
It also helps with a potential cross-compile of chrome/win, since
ninja's -t msvc only exists in ninja/win.
BUG=588831
==========
thakis@chromium.org changed reviewers: + scottmg@chromium.org
I think this is pretty neat. Once a json-env-equipped toolchain lands, it should be fairly easy to no longer require manually-set INCLUDE env vars for cross builds after this (and another change to hook up the json) -- but I think this is a very nice change even without the cross build motivation.
(ps: Take your time with this one; as mentioned in the CL description landing it is blocked on a bunch of stuff anyhow.)
https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" I don't understand how cl.exe gets PATH to be set properly in this case now. Or are you saying it doesn't need that? Oh, maybe because we only use the x64 cross or x64 native, we can get away without adding any directories? https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:190: f.write(env_block) Could you fix the indent on this while you're here? https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:195: assert include Maybe assert " not in include. But I guess we don't elsewhere anyway.
thanks! https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > I don't understand how cl.exe gets PATH to be set properly in this case now. Or > are you saying it doesn't need that? Oh, maybe because we only use the x64 cross > or x64 native, we can get away without adding any directories? We don't need the path. cl is set to `cl = "${goma_prefix}\"${x86_toolchain_data.vc_bin_dir}/cl.exe\""` below, which is a qualified path to cl. cl doesn't invoke any subprograms (or looks at them relative to itself). vc_bin_dir is also returned by setup_toolchain.py https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... File build/toolchain/win/setup_toolchain.py (right): https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:190: f.write(env_block) On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > Could you fix the indent on this while you're here? Will do. https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/set... build/toolchain/win/setup_toolchain.py:195: assert include On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > Maybe assert " not in include. But I guess we don't elsewhere anyway. Will do or not, I guess. It'll be a surprise!
https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" On 2016/02/25 03:45:41, Nico wrote: > On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > > I don't understand how cl.exe gets PATH to be set properly in this case now. > Or > > are you saying it doesn't need that? Oh, maybe because we only use the x64 > cross > > or x64 native, we can get away without adding any directories? > > We don't need the path. cl is set to `cl = > "${goma_prefix}\"${x86_toolchain_data.vc_bin_dir}/cl.exe\""` below, which is a > qualified path to cl. cl doesn't invoke any subprograms (or looks at them > relative to itself). vc_bin_dir is also returned by setup_toolchain.py Are you sure? Looking in VC\bin\amd64_x86, there's a compiler and its direct DLLs, but other DLLs like msobj120.dll and mspdb120.dll that I believe it needs to load are only in the root (x86) dir and in amd64.
On 2016/02/25 18:16:23, scottmg (slow until 25feb) wrote: > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > File build/toolchain/win/BUILD.gn (right): > > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags > /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" > On 2016/02/25 03:45:41, Nico wrote: > > On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > > > I don't understand how cl.exe gets PATH to be set properly in this case now. > > Or > > > are you saying it doesn't need that? Oh, maybe because we only use the x64 > > cross > > > or x64 native, we can get away without adding any directories? > > > > We don't need the path. cl is set to `cl = > > "${goma_prefix}\"${x86_toolchain_data.vc_bin_dir}/cl.exe\""` below, which is a > > qualified path to cl. cl doesn't invoke any subprograms (or looks at them > > relative to itself). vc_bin_dir is also returned by setup_toolchain.py > > Are you sure? Looking in VC\bin\amd64_x86, there's a compiler and its direct > DLLs, but other DLLs like msobj120.dll and mspdb120.dll that I believe it needs > to load are only in the root (x86) dir and in amd64. Also vcvarsamd64_x86.bat does add (many) extra dirs to the PATH, including amd64_x86 and amd64.
On 2016/02/25 18:16:23, scottmg (slow until 25feb) wrote: > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > File build/toolchain/win/BUILD.gn (right): > > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags > /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" > On 2016/02/25 03:45:41, Nico wrote: > > On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > > > I don't understand how cl.exe gets PATH to be set properly in this case now. > > Or > > > are you saying it doesn't need that? Oh, maybe because we only use the x64 > > cross > > > or x64 native, we can get away without adding any directories? > > > > We don't need the path. cl is set to `cl = > > "${goma_prefix}\"${x86_toolchain_data.vc_bin_dir}/cl.exe\""` below, which is a > > qualified path to cl. cl doesn't invoke any subprograms (or looks at them > > relative to itself). vc_bin_dir is also returned by setup_toolchain.py > > Are you sure? Looking in VC\bin\amd64_x86, there's a compiler and its direct > DLLs, but other DLLs like msobj120.dll and mspdb120.dll that I believe it needs > to load are only in the root (x86) dir and in amd64. Hm, I only tried doing a 64-bit build, which worked fine. I'll try doing a 32-bit build too.
On 2016/02/25 18:16:23, scottmg (slow until 25feb) wrote: > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > File build/toolchain/win/BUILD.gn (right): > > https://codereview.chromium.org/1724533002/diff/20001/build/toolchain/win/BUI... > build/toolchain/win/BUILD.gn:78: command = "$cl /nologo $include_flags > /showIncludes /FC @$rspfile /c {{source}} /Fo{{output}} /Fd$pdbname" > On 2016/02/25 03:45:41, Nico wrote: > > On 2016/02/25 03:40:15, scottmg (slow until 25feb) wrote: > > > I don't understand how cl.exe gets PATH to be set properly in this case now. > > Or > > > are you saying it doesn't need that? Oh, maybe because we only use the x64 > > cross > > > or x64 native, we can get away without adding any directories? > > > > We don't need the path. cl is set to `cl = > > "${goma_prefix}\"${x86_toolchain_data.vc_bin_dir}/cl.exe\""` below, which is a > > qualified path to cl. cl doesn't invoke any subprograms (or looks at them > > relative to itself). vc_bin_dir is also returned by setup_toolchain.py > > Are you sure? Looking in VC\bin\amd64_x86, there's a compiler and its direct > DLLs, but other DLLs like msobj120.dll and mspdb120.dll that I believe it needs > to load are only in the root (x86) dir and in amd64. Hm, I only tried doing a 64-bit build, which worked fine. I'll try doing a 32-bit build too.
Ok, this does indeed not work for 32-bit builds. Options: * Keep using environment file for 32-bit msvc builds * Keep using environment file for 64-bit msvc builds too * Use it for both msvc and clang-cl builds Making 32-bit and 64-bit builds behave this differently is a bit weird, but so is making clang-cl and cl builds behaving differently. I weakly lean towards using the environment files only for 32-bit msvs builds, but if you want me to always use them with msvs that sounds ok too. Using them with clang-cl seems unnecessary though.
On 2016/02/25 20:55:50, Nico wrote: > Ok, this does indeed not work for 32-bit builds. Options: > > * Keep using environment file for 32-bit msvc builds > * Keep using environment file for 64-bit msvc builds too > * Use it for both msvc and clang-cl builds > > Making 32-bit and 64-bit builds behave this differently is a bit weird, but so > is making clang-cl and cl builds behaving differently. > > I weakly lean towards using the environment files only for 32-bit msvs builds, > but if you want me to always use them with msvs that sounds ok too. Using them > with clang-cl seems unnecessary though. OK, darn. I think it'd be best to make all MSVCs still use ninja -t msvc -e since even amd64 expects to run that way, and clang-cl is likely to have more other divergences than cl does with itself for 64/32. Getting rid of it for clang-cl seems nice though. (We could actually fiddle our toolchain packaging to duplicate some dlls to make it work for packaged toolchain msvc x86 builds too, but that's pretty ugly.)
scottmg@chromium.org changed reviewers: + brucedawson@chromium.org
+bruce for fyi on comments ~8-13.
On 2016/02/25 21:07:04, scottmg wrote: > +bruce for fyi on comments ~8-13. It seems like a bad idea to rearrange Microsoft's DLLs when creating packages. We're already dropping CRT DLLs in the middle of their install image, but moving/copying their VC++ DLLs seems like a step to far. I think I'd go with what I think is Scott's suggestion of using ninja -t msvc -e for all msvs builds. It's easy to justify launching clang-cl compiler tasks differently. It's hard to justify launching 32-bit and 64-bit msvs builds differently.
On 2016/02/25 21:11:59, brucedawson wrote: > On 2016/02/25 21:07:04, scottmg wrote: > > +bruce for fyi on comments ~8-13. > > It seems like a bad idea to rearrange Microsoft's DLLs when creating packages. > We're already dropping CRT > DLLs in the middle of their install image, but moving/copying their VC++ DLLs > seems like a step to far. > > I think I'd go with what I think is Scott's suggestion of using ninja -t msvc -e > for all msvs builds. It's easy to justify launching clang-cl compiler tasks > differently. It's hard to justify launching 32-bit and 64-bit msvs builds > differently. "It works in 64-bit, and the env file is gross" There! :-) (Or, a more technical argument, the 32-bit build is a cross build, the 64-bit build isn't.) But ok, I'll change this to only make the build saner for clang-cl builds.
Description was changed from ========== gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl considers headers in INCLUDE as "system headers" and doesn't print warnings for questionable code in these directories, so use -isystem= there. MSVC happily warns about questionable code in system headers if enough warnings are turned on, just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). clang-cl only recently exposed -isystem=, so landing this is blocked on a clang roll. Also, goma doesn't know yet that clang-cl learned about this flag, so this is also blocked on goma learning about clang-cl learning about this (http://b/27283528). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ========== to ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl considers headers in INCLUDE as "system headers" and doesn't print warnings for questionable code in these directories, so use -isystem= there. In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. clang-cl only recently exposed -isystem=, so landing this is blocked on a clang roll. Also, goma doesn't know yet that clang-cl learned about this flag, so this is also blocked on goma learning about clang-cl learning about this (http://b/27283528). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ==========
Ok, now clang-cl-only. Makes the build so much nicer, msvc is really missing out here :-)
https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:67: # %PATH) -- e.g. 32-bit builds require %PATH% to be set and just passing "%PATH%" in the first instance. https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:67: # %PATH) -- e.g. 32-bit builds require %PATH% to be set and just passing "32-bit cl.exe builds..." https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:69: if (defined(invoker.sys_include_flags)) { I think this would be more clear as is_clang.
thanks! https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:67: # %PATH) -- e.g. 32-bit builds require %PATH% to be set and just passing On 2016/02/25 22:56:37, scottmg wrote: > "%PATH%" in the first instance. Done. https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:67: # %PATH) -- e.g. 32-bit builds require %PATH% to be set and just passing On 2016/02/25 22:56:37, scottmg wrote: > "32-bit cl.exe builds..." Done. https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:69: if (defined(invoker.sys_include_flags)) { On 2016/02/25 22:56:37, scottmg wrote: > I think this would be more clear as is_clang. Hm, but then if we ever get rid of the 32-bit build and move the 64-bit msvs build (if it's still around then) to this system too, then we have to change two lines instead of just one…I kind of like that this doesn't duplicate the logic. I added an `assert(!is_clang)` to the else block, how's that? (Just hitting * while the cursor is on sys_include_flags finds quickly where this is set.) If you really do want is_clang I'll change it though, of course :-)
lgtm https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:69: if (defined(invoker.sys_include_flags)) { On 2016/02/26 03:00:44, Nico wrote: > On 2016/02/25 22:56:37, scottmg wrote: > > I think this would be more clear as is_clang. > > Hm, but then if we ever get rid of the 32-bit build and move the 64-bit msvs Wow, I feel like we'll add a 128-bit build before that happens... :) > build (if it's still around then) to this system too, then we have to change two > lines instead of just one…I kind of like that this doesn't duplicate the logic. > I added an `assert(!is_clang)` to the else block, how's that? (Just hitting * > while the cursor is on sys_include_flags finds quickly where this is set.) > > If you really do want is_clang I'll change it though, of course :-) assert sgtm. https://codereview.chromium.org/1724533002/diff/100001/build/toolchain/win/BU... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/100001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:73: assert(!is_clang) # clang-cl doesn't need this env hoop, so omit it there. Should this be invoker.is_clang?
Thanks, done. I also changed setup_toolchain.py to always expect 7 args since it's always called with 7 in practice now and that's a tiny bit simpler. Almost all blockers are resolved by now, only a goma client update still needs to happen (a goma CL has landed, the fix is just not deployed yet). So I'll hopefully be able to tick cq some time early next week. https://codereview.chromium.org/1724533002/diff/100001/build/toolchain/win/BU... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/100001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:73: assert(!is_clang) # clang-cl doesn't need this env hoop, so omit it there. On 2016/02/26 03:32:03, scottmg wrote: > Should this be invoker.is_clang? Oh, good catch, thanks, done! That's what I get for tweaking Windows CLs on my Mac -- would've taken me a while to figure this out this morning without this comment :-)
Actually, I didn't test this carefully enough and this isn't ready yet :-/ See diff from patch set 7 to 8; this is now also blocked on https://llvm.org/PR26751 (and on goma learning about dealing with that, I suppose).
Patch set 9 seems to work, but it's kind of ugly. But I might want to land that until a nicer fix is done, since fixes have a bit of a long dependency chain.
lgtm-ish https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:72: # -nostdlibinc so that clang-cl _only_ looks in sys_include_flags, instead -nostdlibinc doesn't appear to be mentioned in the flags below. https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:75: sys_include_flags = "-isystem ..\..\third_party\llvm-build\Release+Asserts\lib\clang\3.9.0\include ${invoker.sys_include_flags} " I guess we should have an assert(invoker.is_clang) in this branch now.
https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:72: # -nostdlibinc so that clang-cl _only_ looks in sys_include_flags, instead On 2016/02/26 19:55:22, scottmg wrote: > -nostdlibinc doesn't appear to be mentioned in the flags below. Yes, clang-cl doesn't expose that flag yet. It's not needed, it just makes things not work by accident (by looking at sys includes) if I misspell -isystem like I originally did. https://codereview.chromium.org/1724533002/diff/160001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:75: sys_include_flags = "-isystem ..\..\third_party\llvm-build\Release+Asserts\lib\clang\3.9.0\include ${invoker.sys_include_flags} " On 2016/02/26 19:55:22, scottmg wrote: > I guess we should have an assert(invoker.is_clang) in this branch now. Yes, if I go ahead with that I'll add that. But it's kinda gross to hardcode this path here anyhow, so I'll think about different approaches for a bit first.
Description was changed from ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl considers headers in INCLUDE as "system headers" and doesn't print warnings for questionable code in these directories, so use -isystem= there. In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. clang-cl only recently exposed -isystem=, so landing this is blocked on a clang roll. Also, goma doesn't know yet that clang-cl learned about this flag, so this is also blocked on goma learning about clang-cl learning about this (http://b/27283528). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ========== to ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl's -imsvc flag adds include directories as-if they're from INCLUDE (i.e. they're in the right place in the directory search order, and they're treated as system headers that don't emit warnings). In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning about this clang-cl flag (https://b//28179421). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ==========
Ok, I've regrouped bit: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160411/155377.html The patch now uses the new -imsvc flag, which doesn't require the yucky hardcoded compiler include dir. It's still blocked on goma learning about that new flag, but I'm reasonably happy with this now.
lgtm
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1724533002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1724533002/240001
Message was sent while issue was closed.
Description was changed from ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl's -imsvc flag adds include directories as-if they're from INCLUDE (i.e. they're in the right place in the directory search order, and they're treated as system headers that don't emit warnings). In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning about this clang-cl flag (https://b//28179421). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ========== to ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl's -imsvc flag adds include directories as-if they're from INCLUDE (i.e. they're in the right place in the directory search order, and they're treated as system headers that don't emit warnings). In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning about this clang-cl flag (https://b//28179421). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl's -imsvc flag adds include directories as-if they're from INCLUDE (i.e. they're in the right place in the directory search order, and they're treated as system headers that don't emit warnings). In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning about this clang-cl flag (https://b//28179421). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 ========== to ========== clang/gn/win: Stop running the compiler through `ninja -t msvc -e environment.foo` The compiler only needs the INCLUDE environment var, and those contents can just be passed via flags. clang-cl's -imsvc flag adds include directories as-if they're from INCLUDE (i.e. they're in the right place in the directory search order, and they're treated as system headers that don't emit warnings). In 64-bit builds, this would also work for MSVC, but MSVC happily warns about questionable code in system headers if enough warnings are turned on, so we would just use /I there (and make sure the flags are early on the compile command so that these directories are searched at the same time as they would be with INCLUDE). However, in 32-bit builds, MSVC needs PATH to contain both the 32-bit and the 64-bit bin directories to load dlls. Since invoking the compiler so differently for 32-bit and 64-bit is strange, only do this simplification for clang-cl for now. goma doesn't yet know the -imsvc flag flag, so this is blocked on goma learning about this clang-cl flag (https://b//28179421). Build time impact: 64-bit debug builds symbol_level=1, building some binary ("gn") before: clang-cl: 34.9s, 35.1s, 35.1s cl: 26.3s, 26.1s, 27.6s after: clang-cl: 33.8s, 33.7s, 34s cl: 27s, 34.5s, 26.7s So no discernible build perf difference, but fewer processes and less reliance on these environment files seems like a good change anyhow. It also helps with a potential cross-compile of chrome/win, since ninja's -t msvc only exists in ninja/win. BUG=588831 Committed: https://crrev.com/8435ab74b05929d290a11808a0a94ea7536531d7 Cr-Commit-Position: refs/heads/master@{#392624} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/8435ab74b05929d290a11808a0a94ea7536531d7 Cr-Commit-Position: refs/heads/master@{#392624} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
