re-enabling strict aliasing

[Paweł Hajdan, Jr.]

I took a look at http://code.google.com/p/chromium/issues/detail?id=32204 and produced https://codereview.chromium.org/12091004/ which re-enables strict aliasing for most build targets.

The old thread from 2010 about it is https://groups.google.com/d/msg/chromium-dev/QCWxN3l7gns/mOy4agn23N0J

I think the concern about weird crashes is valid. I got one while working on the CL (was caused by issues in third party code for which warnings are not fatal - I've solved it by disabling strict aliasing for such third party code) was totally bizarre.

On the other hand, reinterpret_casts that compiler warns about with strict aliasing can easily have bugs as well. bit_cast looks generally cleaner and safer.

Actually I've discovered a case of casting (reinterpret_cast) a const reference to std::vector<A> to const reference to std::vector<B>, where A and B have the same size and member layout (content/browser/renderer_host/render_widget_host_view_aura.cc):

  // ui::CompositionUnderline should be identical to
  // WebKit::WebCompositionUnderline, so that we can do reinterpret_cast safely.
  COMPILE_ASSERT(sizeof(ui::CompositionUnderline) ==
                 sizeof(WebKit::WebCompositionUnderline),
                 ui_CompositionUnderline__WebKit_WebCompositionUnderline_diff);

  // TODO(suzhe): convert both renderer_host and renderer to use
  // ui::CompositionText.
  const std::vector<WebKit::WebCompositionUnderline>& underlines =
      reinterpret_cast<const std::vector<WebKit::WebCompositionUnderline>&>(
          composition.underlines);

Now even with strict aliasing compiler can be tricked into accepting only slightly changed code (https://codereview.chromium.org/12091004/diff/36001/content/browser/renderer_host/render_widget_host_view_aura.cc), but at least there is one more reminder from compiler that the code is suspicious and fragile.

After this, do you think we should go forward with re-enabling strict aliasing, or just keep it disabled and close the bug?

Paweł

[Ami Fischman]

I recently found out the hard way that webrtc requires unstrict aliasing.  If you take the approach in your CL then webrtc will break (at the very least have video corruption (263657)) because today it builds with chromium_code=1.  That particular case is fixable by suppressing strict-aliasing in the webrtc gyp forest (or fixing the aliasing violations!) but I just thought I'd point out this example in case there are other third_party-flavored codebases that are in the same boat.

(I am in favor of strict-aliasing in general, FWIW)

Cheers,

-a

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Mike Frysinger]

+1 to enabling aliasing support.  doing it on a repo by repo basis would probably be saner (restrict it to places where we build with -Werror).

-mike

--

[Paweł Hajdan, Jr.]

On Thu, Aug 15, 2013 at 1:35 PM, Mike Frysinger <vap...@chromium.org> wrote:

+1 to enabling aliasing support.  doing it on a repo by repo basis would probably be saner (restrict it to places where we build with -Werror).

This is my current plan (only build targets with -Werror).

On Thu, Aug 15, 2013 at 1:00 PM, Ami Fischman <fisc...@chromium.org> wrote:

I recently found out the hard way that webrtc requires unstrict aliasing.  If you take the approach in your CL then webrtc will break (at the very least have video corruption (263657)) because today it builds with chromium_code=1.  That particular case is fixable by suppressing strict-aliasing in the webrtc gyp forest (or fixing the aliasing violations!) but I just thought I'd point out this example in case there are other third_party-flavored codebases that are in the same boat.

(I am in favor of strict-aliasing in general, FWIW)

Ami, did compiler issue a warning for that strict aliasing problem? I'll look more into how chromium_code is handled, I'd just like to make sure that warnings flag such issues - otherwise it'd seem too risky to enable.

Paweł 

[Ami Fischman]

The links I included lead to http://build.chromium.org/p/client.webrtc/builders/Mac64%20Release/builds/296/, among others, which can show you the entire compile stdio.  AFAICT no warnings.

[Paweł Hajdan, Jr.]

This is worrying. I also noticed some Mozilla bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=657806 , https://bugzilla.mozilla.org/show_bug.cgi?id=413253 , https://bugzilla.mozilla.org/show_bug.cgi?id=567627 . The last one was also a violation gcc didn't warn about.

It looks like there are no big performance benefits (I haven't measured it for Chrome, but Mozilla guys apparently didn't see any), and there are known problems created by enabling strict aliasing that the compiler doesn't warn about.

I'm leaning more and more towards just using -fno-strict-aliasing, and enabling corresponding warnings if possible (i.e. warn about detected violations of strict aliasing to flag suspicious code, but do not change how code generation / optimization works).

Paweł

[Peter Kasting]

On Mon, Aug 19, 2013 at 1:04 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'm leaning more and more towards just using -fno-strict-aliasing, and enabling corresponding warnings if possible (i.e. warn about detected violations of strict aliasing to flag suspicious code, but do not change how code generation / optimization works).

The ex-compiler-guy in me does not like this proposal.

Usually, if your code cannot compile correctly with strict aliasing on, you have some scary code that may not be buggy now, but can easily lead to bugs later.  "Strict aliasing OK" is something of a code sanity check that the code doesn't do as many questionable pointer transforms and the like.  (Admittedly, it's often possible to "fix" strict aliasing problems by replacing one bit of questionable logic with another, so it's not a guarantee.)

The performance angle is hard to predict.  Strict aliasing can buy performance when it enables more aggressive loop transforms.  Otherwise it may not gain performance.   But I don't see performance as the primary reason to enable the flag.

PK

[JF Bastien]

Usually, if your code cannot compile correctly with strict aliasing on, you have some scary code that may not be buggy now, but can easily lead to bugs later.  "Strict aliasing OK" is something of a code sanity check that the code doesn't do as many questionable pointer transforms and the like.  (Admittedly, it's often possible to "fix" strict aliasing problems by replacing one bit of questionable logic with another, so it's not a guarantee.)

The performance angle is hard to predict.  Strict aliasing can buy performance when it enables more aggressive loop transforms.  Otherwise it may not gain performance.   But I don't see performance as the primary reason to enable the flag.

 If there is no perf win then there is no reason to enable the optimizer flag. Enable the diagnostic ones, but not the optimization. Strict aliasing is something people get wrong often, AFAIK static analysis doesn't always succeed with warning properly, finding bugs it creates is tricky, and my current understanding (from talking to someone who implemented it in LLVM) is that it's got ill-specified quirks in the language.

[Jeffrey Yasskin]

FWIW, even -Wstrict-aliasing has been too buggy to use in the past:
http://gcc.gnu.org/PR41838 and http://gcc.gnu.org/PR39891.

Also, it's not surprising that -fstrict-aliasing would mis-optimize
some buggy code that -Wstrict-aliasing doesn't catch. If the compiler
could detect all type-based aliasing on its own, there would be no
need for the language rule.

If Mozilla didn't find any performance improvement, I don't see a good
reason to risk the bugs.

Jeffrey

P.S. What's the word for "optimize some code into a behavior the
author didn't expect because the author wrote incorrect code"? ;)

[Peter Kasting]

On Tue, Aug 20, 2013 at 12:35 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

If Mozilla didn't find any performance improvement, I don't see a good

reason to risk the bugs.

Are we compiling the same code they were?  If not it might be nice to check if _we_ see any perf win.

P.S. What's the word for "optimize some code into a behavior the
author didn't expect because the author wrote incorrect code"? ;)

"Undefined behavior" is the best I can do.

At Green Hills we used to joke that when we detected standard-violating code, we should output an executable that reformatted the hard drive.

PK

[Nico Weber]

Do you have any performance / code size numbers on how much this would buy us?

Nico

--

[Jeffrey Yasskin]

Also beware naive performance numbers: it's possible you'd just be
measuring the effect of undefined behavior, rather than the actual
benefits of -fstrict-aliasing on top of well-defined code. Until all
the tests pass, they're an upper bound on the possible improvement.

Otherwise, pkasting's point about measuring the effect on Chrome seems
reasonable.

[Paweł Hajdan, Jr.]

I did some benchmarks using tools/perf/run_measurement (dromaeo, octane, image decoding) and the results are pretty much the same.

Code size is different - we could save 35 KB on out/Release/chrome after enabling strict aliasing (139805400 bytes vs. 139841800 bytes).

IMHO not worth it, especially given known problems with no warnings in our codebase, other problems with it pointed out by Jeffrey, and our results generally seem to match Mozilla's experience.

My current plan is to land parts of my CL that replace some reinterpret_casts with bit_casts for more safety, and that'd be it.

What do you think?

Paweł

[Paweł Hajdan, Jr.]

I committed https://codereview.chromium.org/26406002 and WontFixed the bug.

Paweł