New issue
Advanced search Search tips

Issue 628560 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner: ----
Closed: Jul 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Feature



Sign in to add a comment

Analyze if strict-aliasing can be used in V8

Project Member Reported by machenb...@chromium.org, Jul 15 2016

Issue description

Chromium passes -fno-strict-aliasing, but there's some evidence that V8 code might work with -fstrict-aliasing.

Here an analysis what is passed currently.

GYP - v8 in chromium:
- Linux: [-fno-strict-aliasing]
- Mac: [-fstrict-aliasing, -fno-strict-aliasing]

GYP - v8 stand-alone:
- Linux: []
- Mac: [-fstrict-aliasing, -fno-strict-aliasing]

GN - v8 in chromium:
- Linux: [-fno-strict-aliasing]
- Mac: [-fno-strict-aliasing]

GN - v8 stand-alone:
- Linux: [-fno-strict-aliasing]
- Mac: [-fno-strict-aliasing]

Thoughts:
- We shipped chromium, passing -fstrict-aliasing _and_ -fno-strict-aliasing on mac, it is unclear which one took precedence
- We didn't switch it off on linux stand-alone, but it is unclear if it is on by default with clang and -O3 (with gcc it is on by default)
- The v8 tests pass on our linux gcc bot, which is a sign that strict-aliasing might work with linux
- It is unclear which performance impact strict-aliasing might have
- Maybe we could add -Wstrict-aliasing everywhere to make sure we build, but without switching on the optimizations?

I'd like to avoid:
- to switch strict-aliasing on (if it was off), if it leads to undefined behavior
- to switch strict-aliasing off (if it was on), if it causes performance regressions
 

Comment 1 by thakis@chromium.org, Jul 15 2016

When there are multiple of theses flags, the last one wins. Looks like the last one is the disable flag always atm?

Comment 2 by thakis@chromium.org, Jul 15 2016

(the last one on the compile command line that is)
https://paste.googleplex.com/4873217768423424

Disable comes last. So we always had strict-aliasing off on mac.
Blocking: -626064
Cc: -thakis@chromium.org
Labels: -Type-Bug -Pri-2 -Needs-Feedback Pri-3 Type-Feature
Keeping the bug open to analyze if we could switch strict-aliasing on on linux. First we should check if there's any performance gain in doing so.
Labels: OS-Linux
I think it's actually quite dangerous to turn it on. We do all kinds of crazy pointer casting and misuse (like Smi) in v8. If we tell the compiler to rely on strict aliasing, this can cause weird bugs stemming from the compiler exploiting this (wrong) knowledge.

I just uploaded a CL to disable strict-aliasing in gyp, because it was causing a compile error on our mips bot:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder/builds/10732

CL: https://chromium-review.googlesource.com/c/571806
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/db302014b6dcfd82722c28eb7a6e9446bc546542

commit db302014b6dcfd82722c28eb7a6e9446bc546542
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Fri Jul 14 13:16:52 2017

Disable strict-aliasing for gyp

In gn, strict aliasing is disabled anyway. Enabling it for gyp can cause
the compilation to fail on bots that still use gyp, or (even worse) can
lead to miscompilation because the compiler exploits strict aliasing
assumptions.

R=machenbach@chromium.org

Bug:  chromium:628560 
Change-Id: Ib756b8126a10d52f8c807ceda42dfc6dbda80ea6
Reviewed-on: https://chromium-review.googlesource.com/571806
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46678}
[modify] https://crrev.com/db302014b6dcfd82722c28eb7a6e9446bc546542/gypfiles/standalone.gypi

Status: WontFix (was: Available)
Thanks for the analysis. We're probably not going to switch it on.
For the record (and in case this bug is resurrected in the future),  v8 also uses reinterpret_cast between 'unsigned short' and 'char16_t' pointers as well when calling ICU APIs (https://chromium-review.googlesource.com/c/v8/v8/+/987953 ). ICU has a small wrapper class / helper method to make the conversion work even with strict-aliasing turned on in gcc/clang.

At first, v8 used those helper class/method, but went back to reinterpret_cast because I was told that v8 uses -fno-strict-aliasing. 

Sign in to add a comment