Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1957523005:
clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. (Closed)
Descriptionclang/win/gn: Actually build 32-bit .obj files in 32-bit builds.
In clang builds, we use the same compiler binary for 32-bit and 64-bit
builds, so we need to tell that binary if it should produce 32-bit or
64-bit object files.
The //build/config/compiler:compiler config takes care of adding -m32
and -m64 flags, but the mini_installer target removed that config,
removing these flags (and other important compiler flags such as
-fmsc-version).
It looks like removing this config isn't really necessary -- the only
compiler flag that mini_installer wants to change is /GS-, to disable
buffer security checks. I first thought I'd move /GS into its own
config so that mini_installer can remove it, but it turns out /GS is
on by default. So don't pass /GS in :compiler, and do pass /GS- in
mini_installer.
No target should remove //build/config/compiler:compiler from its configs.
BUG=498033
Committed: https://crrev.com/7170da48a964aa08a16fbf03ce17dbd78b596500
Cr-Commit-Position: refs/heads/master@{#392175}
Patch Set 1 #
Total comments: 3
Patch Set 2 : different #
Messages
Total messages: 23 (10 generated)
thakis@chromium.org changed reviewers: + hans@chromium.org
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/1957523005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1957523005/1
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.gn File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.g... build/toolchain/win/BUILD.gn:285: cl = "${goma_prefix}$prefix/${clang_cl} -m32" Shouldn't this already be being set here: https://code.google.com/p/chromium/codesearch?q=build%2Fconfig%2Fwin%2FBUILD.... ?
The CQ bit was unchecked by thakis@chromium.org
https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.gn File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.g... build/toolchain/win/BUILD.gn:285: cl = "${goma_prefix}$prefix/${clang_cl} -m32" On 2016/05/06 15:53:58, Dirk Pranke wrote: > Shouldn't this already be being set here: > > https://code.google.com/p/chromium/codesearch?q=build%2Fconfig%2Fwin%2FBUILD.... > > ? Oh, great point! It seems to not always have an effect when it's set there. I unchecked cq and will investigate, thanks.
thakis@chromium.org changed reviewers: + brettw@chromium.org
(+brettw who added the only place where a target removes the compiler:compiler config) https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.gn File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/1957523005/diff/1/build/toolchain/win/BUILD.g... build/toolchain/win/BUILD.gn:285: cl = "${goma_prefix}$prefix/${clang_cl} -m32" On 2016/05/06 16:21:33, Nico wrote: > On 2016/05/06 15:53:58, Dirk Pranke wrote: > > Shouldn't this already be being set here: > > > > > https://code.google.com/p/chromium/codesearch?q=build%2Fconfig%2Fwin%2FBUILD.... > > > > ? > > Oh, great point! It seems to not always have an effect when it's set there. I > unchecked cq and will investigate, thanks. mini_installer does configs -= [ "//build/config/compiler:compiler" ] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/m... which looks a bit...uh, surprising to me. I guess that means "I mean it" flags like -m32 / -m64 need to be part of the toolchain instead of the compiler config then? :-/
From what I understand, mini_installer does this because it wants to build without /GS. I think that should be in a separate config that mini_installer can remove, instead of completely rolling its own compiler flags
Description was changed from ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. BUG=498033 ========== to ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. The //build/config/compiler:compiler config takes care of adding -m32 and -m64 flags, but the mini_installer target removed that config, removing these flags (and other important compiler flags such as -fmsc-version). It looks like removing this config isn't really necessary -- the only compiler flag that mini_installer wants to change is /GS-, to disable buffer security checks. I first thought I'd move /GS into its own config so that mini_installer can remove it, but it turns out /GS is on by default. So don't pass /GS in :compiler, and do pass /GS- in mini_installer. No target should remove //build/config/compiler:compiler from its configs. BUG=498033 ==========
Ok, I rewrote this. See patch set 2, and the update CL description. I believe this should be no behavior change, and I compared compiler flags for mini_installer locally (by looking at the generated .ninja file), and they seem to be identical.
so this is on by default w/ clang-cl, too? lgtm.
On 2016/05/06 21:11:50, Dirk Pranke wrote: > so this is on by default w/ clang-cl, too? Not yet, because the flag isn't implemented in clang-cl yet (tracked at https://crbug.com/598767)
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org Link to the patchset: https://codereview.chromium.org/1957523005/#ps20001 (title: "different")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1957523005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1957523005/20001
Message was sent while issue was closed.
Description was changed from ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. The //build/config/compiler:compiler config takes care of adding -m32 and -m64 flags, but the mini_installer target removed that config, removing these flags (and other important compiler flags such as -fmsc-version). It looks like removing this config isn't really necessary -- the only compiler flag that mini_installer wants to change is /GS-, to disable buffer security checks. I first thought I'd move /GS into its own config so that mini_installer can remove it, but it turns out /GS is on by default. So don't pass /GS in :compiler, and do pass /GS- in mini_installer. No target should remove //build/config/compiler:compiler from its configs. BUG=498033 ========== to ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. The //build/config/compiler:compiler config takes care of adding -m32 and -m64 flags, but the mini_installer target removed that config, removing these flags (and other important compiler flags such as -fmsc-version). It looks like removing this config isn't really necessary -- the only compiler flag that mini_installer wants to change is /GS-, to disable buffer security checks. I first thought I'd move /GS into its own config so that mini_installer can remove it, but it turns out /GS is on by default. So don't pass /GS in :compiler, and do pass /GS- in mini_installer. No target should remove //build/config/compiler:compiler from its configs. BUG=498033 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. The //build/config/compiler:compiler config takes care of adding -m32 and -m64 flags, but the mini_installer target removed that config, removing these flags (and other important compiler flags such as -fmsc-version). It looks like removing this config isn't really necessary -- the only compiler flag that mini_installer wants to change is /GS-, to disable buffer security checks. I first thought I'd move /GS into its own config so that mini_installer can remove it, but it turns out /GS is on by default. So don't pass /GS in :compiler, and do pass /GS- in mini_installer. No target should remove //build/config/compiler:compiler from its configs. BUG=498033 ========== to ========== clang/win/gn: Actually build 32-bit .obj files in 32-bit builds. In clang builds, we use the same compiler binary for 32-bit and 64-bit builds, so we need to tell that binary if it should produce 32-bit or 64-bit object files. The //build/config/compiler:compiler config takes care of adding -m32 and -m64 flags, but the mini_installer target removed that config, removing these flags (and other important compiler flags such as -fmsc-version). It looks like removing this config isn't really necessary -- the only compiler flag that mini_installer wants to change is /GS-, to disable buffer security checks. I first thought I'd move /GS into its own config so that mini_installer can remove it, but it turns out /GS is on by default. So don't pass /GS in :compiler, and do pass /GS- in mini_installer. No target should remove //build/config/compiler:compiler from its configs. BUG=498033 Committed: https://crrev.com/7170da48a964aa08a16fbf03ce17dbd78b596500 Cr-Commit-Position: refs/heads/master@{#392175} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7170da48a964aa08a16fbf03ce17dbd78b596500 Cr-Commit-Position: refs/heads/master@{#392175} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
