constexpr compiler bug on msvc |
|||
Issue descriptionhttps://codereview.chromium.org/2132653002/ had some test failures on windows bots that seem to be the result of a bug in MSVC dealing with constexpr. The following reproduces the bug locally. I compile it as part of chromium, and call try_it() from MediaCodecLoop constructor (for convenience). I didn't fiddle with trying to build it standalone, and/or changing compiler flags. the gn flags for the build contained "use_goma = true", and that's it. // Copyright 2016 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include <stdio.h> class Foo { public: constexpr Foo(int x) : x_(x) {} static constexpr Foo Create(int x) { // Inlining ReallyCreate here avoids the bug. return ReallyCreate(x); } static constexpr Foo ReallyCreate(int x) { // Removing ?: avoids the bug. return x > 5 ? Foo(1) : Foo(2); } int x_; }; // Removing constexpr avoids the bug. // Calling ReallyCreate directly avoids the bug. constexpr Foo foo = Foo::Create(1); void try_it() { // Moving foo decl here from global scope avoids the bug. // Prints zero, but should print 2. fprintf(stderr, "foo.x_ == %d\n", foo.x_); }
,
Aug 9 2016
I tried with Update 3 and the bug still repros. I will report this to Microsoft but until then we will have to work around it.
,
Aug 10 2016
I filed this bug. Microsoft will probably investigate the bug soon, but it could be a long time before we get a fix. https://connect.microsoft.com/VisualStudio/feedback/details/3035054
,
Aug 11 2016
This potentially useful comment was added to the connect bug:
Explicitly defining the constexpr copy constructor also seems to fix the bug:
constexpr Foo(Foo const& x) : x_{ x.x_ } {}
At some point the copy constructor must fail to constexpr-execute, and the struct remains uninitialized. We can confirm this by adding
constexpr int bar = foo.x_;
below the definition of `foo`, at which point we get the compiler error
1>Source.cpp(34): error C2131: expression did not evaluate to a constant
1> Source.cpp(34): note: failure was caused by accessing a non-active member of a union
1> Source.cpp(34): note: see usage of 'Foo::x_'
,
Aug 11 2016
the original class is TimeDelta, which has no explicit copy constructor. i'll try adding one and see what happens. as an aside, i only vaguely follow why foo.x_ isn't a constexpr. does MSVC some sort of compile-time tracking for active members that the constexpr constructor would normally update?
,
Aug 11 2016
Thanks liberato. If you update TimeDelta, could you add a unittest for it that uses a global constexpr variable, which fails before your patch and passes afterwards? The error about foo.x_ is another bug in MSVC that's consistent with the other bug that it fails to initialize the global constants.
,
Aug 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f4831ddd84ec8f068fb934a6de94560e202c01eb commit f4831ddd84ec8f068fb934a6de94560e202c01eb Author: liberato <liberato@chromium.org> Date: Fri Aug 12 22:38:11 2016 Add explicit TimeDelta constexpr copy constructor. MSVC fails to initialize constexpr TimeDelta instances in some situations. It turns out (thanks brucedawson@) that including an explicit constexpr copy constructor works around this. BUG= 635974 Review-Url: https://codereview.chromium.org/2241603002 Cr-Commit-Position: refs/heads/master@{#411805} [modify] https://crrev.com/f4831ddd84ec8f068fb934a6de94560e202c01eb/base/time/time.h [modify] https://crrev.com/f4831ddd84ec8f068fb934a6de94560e202c01eb/base/time/time_win_unittest.cc
,
Aug 14 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6 commit db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6 Author: Max Morin <maxmorin@chromium.org> Date: Mon Aug 27 09:45:24 2018 Make TimeDelta TriviallyCopyable for std::atomic. We're having some awkwardness with working around the issue with stuff like std::atomic<int64_t> and conversions back and forth. It would be preferable to use std::atomic<TimeDelta> directly. Removes the workaround added for bug 635974 , since that was a bug in vs 2015 and we use clang now. Also fixes a couple of lint issues in time.h. Bug: 635974 , 851959, 761570 Change-Id: I4683f960b0c348748c5f0aaf222da4dda40256ec Reviewed-on: https://chromium-review.googlesource.com/1184781 Commit-Queue: Max Morin <maxmorin@chromium.org> Reviewed-by: Yuri Wiitala <miu@chromium.org> Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#586219} [modify] https://crrev.com/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6/base/time/time.h [modify] https://crrev.com/db5f2b6e74d6d2e2a9c6f530199067c1d6eff7b6/base/time/time_unittest.cc
,
Aug 27
Closing as MSVC isn't supported. |
|||
►
Sign in to add a comment |
|||
Comment 1 by jyasskin@chromium.org
, Aug 9 2016