Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2310513002:
Use -fno-delete-null-pointer-checks with gcc builds (Closed)
DescriptionUse -fno-delete-null-pointer-checks with gcc builds
R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org
BUG=v8:3782
Committed: https://crrev.com/dbefc8ee2e9ee6e41b83f3d09c788c34bc923b43
Cr-Commit-Position: refs/heads/master@{#39286}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address nits and add GN config as well #Messages
Total messages: 30 (12 generated)
bmeurer@chromium.org changed required reviewers: + jochen@chromium.org, machenbach@chromium.org
LGTM from my side
Please link also to the BUG in the CL desc. https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi File gypfiles/toolchain.gypi (right): https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi#new... gypfiles/toolchain.gypi:1105: # https://bugs.chromium.org/p/v8/issues/detail?id=3782 nit: short link: crbug.com/v8/3782 https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi#new... gypfiles/toolchain.gypi:1107: 'cflags': [ '-fno-delete-null-pointer-checks' ] Please also add the GN configs as our bots testing this are about to switch or already switched. You'd need to add here: https://cs.chromium.org/chromium/src/v8/BUILD.gn?l=194 (somewhere in config("toolchain")) something like: if (!is_win && !is_clang) { cflags += [ "-fno-delete-null-pointer-checks" ] } Then please also include trybots on the chromium side like android_arm64_dbg_recipe.
will pre-gcc-6 just swallow this setting or will it bail out?
On 2016/09/05 09:46:29, jochen wrote: > will pre-gcc-6 just swallow this setting or will it bail out? pre-gcc-6 do swallow this setting. Tested with gcc-4.8, which is the minimum required to build V8.
Description was changed from ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG= ========== to ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG=v8:3782 ==========
Addressed comments. PTAL. https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi File gypfiles/toolchain.gypi (right): https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi#new... gypfiles/toolchain.gypi:1105: # https://bugs.chromium.org/p/v8/issues/detail?id=3782 On 2016/09/05 06:53:39, machenbach (slow) wrote: > nit: short link: crbug.com/v8/3782 Done. https://codereview.chromium.org/2310513002/diff/1/gypfiles/toolchain.gypi#new... gypfiles/toolchain.gypi:1107: 'cflags': [ '-fno-delete-null-pointer-checks' ] On 2016/09/05 06:53:39, machenbach (slow) wrote: > Please also add the GN configs as our bots testing this are about to switch or > already switched. > > You'd need to add here: > https://cs.chromium.org/chromium/src/v8/BUILD.gn?l=194 (somewhere in > config("toolchain")) something like: > > if (!is_win && !is_clang) { > cflags += [ "-fno-delete-null-pointer-checks" ] > } > > Then please also include trybots on the chromium side like > android_arm64_dbg_recipe. Done.
lgtm
lgtm
The CQ bit was checked by ofrobots@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org Link to the patchset: https://codereview.chromium.org/2310513002/#ps20001 (title: "address nits and add GN config as well")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12338) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8437)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ofrobots@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG=v8:3782 ========== to ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG=v8:3782 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG=v8:3782 ========== to ========== Use -fno-delete-null-pointer-checks with gcc builds R=bmeurer@chromium.org, jochen@chromium.org, machenbach@chromium.org BUG=v8:3782 Committed: https://crrev.com/dbefc8ee2e9ee6e41b83f3d09c788c34bc923b43 Cr-Commit-Position: refs/heads/master@{#39286} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dbefc8ee2e9ee6e41b83f3d09c788c34bc923b43 Cr-Commit-Position: refs/heads/master@{#39286}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2328563002/ by ofrobots@google.com. The reason for reverting is: Fails on MIPS: https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder....
Message was sent while issue was closed.
On 2016/09/08 15:55:27, ofrobots wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2328563002/ by mailto:ofrobots@google.com. > > The reason for reverting is: Fails on MIPS: > https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder.... I had to revert. Does the mips builder use an out-of-date gcc version?
Message was sent while issue was closed.
On 2016/09/08 16:00:48, ofrobots wrote: > On 2016/09/08 15:55:27, ofrobots wrote: > > A revert of this CL (patchset #2 id:20001) has been created in > > https://codereview.chromium.org/2328563002/ by mailto:ofrobots@google.com. > > > > The reason for reverting is: Fails on MIPS: > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder.... > > I had to revert. Does the mips builder use an out-of-date gcc version? At least on arm it looks like it's clang that complains: https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20builder/... On arm and mips (and also android) the host_clang variable is used in gyp to compile with clang on the host. Maybe you need to put the flag into a toolset==target condition?
Message was sent while issue was closed.
On 2016/09/08 18:53:35, machenbach (OOO 26 Sep) wrote: > On 2016/09/08 16:00:48, ofrobots wrote: > > On 2016/09/08 15:55:27, ofrobots wrote: > > > A revert of this CL (patchset #2 id:20001) has been created in > > > https://codereview.chromium.org/2328563002/ by mailto:ofrobots@google.com. > > > > > > The reason for reverting is: Fails on MIPS: > > > > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder.... > > > > I had to revert. Does the mips builder use an out-of-date gcc version? > > At least on arm it looks like it's clang that complains: > https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20builder/... > > On arm and mips (and also android) the host_clang variable is used in gyp to > compile with clang on the host. > Maybe you need to put the flag into a toolset==target condition? Is there a way for me to run a trybot on these builds? which build specifically?
Message was sent while issue was closed.
On 2016/09/26 19:58:48, ofrobots wrote: > On 2016/09/08 18:53:35, machenbach (OOO 26 Sep) wrote: > > On 2016/09/08 16:00:48, ofrobots wrote: > > > On 2016/09/08 15:55:27, ofrobots wrote: > > > > A revert of this CL (patchset #2 id:20001) has been created in > > > > https://codereview.chromium.org/2328563002/ by mailto:ofrobots@google.com. > > > > > > > > The reason for reverting is: Fails on MIPS: > > > > > > > > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder.... > > > > > > I had to revert. Does the mips builder use an out-of-date gcc version? > > > > At least on arm it looks like it's clang that complains: > > > https://build.chromium.org/p/client.v8.ports/builders/V8%20Arm%20-%20builder/... > > > > On arm and mips (and also android) the host_clang variable is used in gyp to > > compile with clang on the host. > > Maybe you need to put the flag into a toolset==target condition? > > Is there a way for me to run a trybot on these builds? which build specifically? Sorry - no trybots for arm or mips cross-compile. You'd need to try locally. For arm that's not difficult. But by now the bot is on GN and things might have changed. The mips builder uses gyp. You'd need to get the mips toolchain and then set CC, CXX, LINK yourself as in: https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder... Also use these gyp variables: GYP_CHROMIUM_NO_ACTION: 0 GYP_CROSSCOMPILE: 1 GYP_DEFINES: target_arch=mips v8_enable_i18n_support=0 v8_target_arch=mips v8_use_snapshot=false |
