New issue
Advanced search Search tips

Issue 753488 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

VS2017 Only Build Break in obj/third_party/webrtc/call/call/call.obj

Project Member Reported by robliao@chromium.org, Aug 8 2017

Issue description

>ninja -C out\Debug obj/third_party/webrtc/call/call/call.obj

ninja: Entering directory `out\Debug'
[ 1/1 - 1 processes @ 0.4/s : 2.504s ] CXX obj/third_party/webrtc/call/call/call.obj
FAILED: obj/third_party/webrtc/call/call/call.obj
ninja -t msvc -e environment.x86 -- "d:\src\third_party\depot_tools\win_toolchain\vs_files\1f52d730755ac72dddaf121b73c9d6fd5c24ddf8\vc\tools\msvc\14.11.25503\bin\hostx64\x86/cl.exe" /nologo /showIncludes  @obj/third_party/webrtc/call/call/call.obj.rsp /c ../../third_party/webrtc/call/call.cc /Foobj/third_party/webrtc/call/call/call.obj /Fd"obj/third_party/webrtc/call/call_cc.pdb"
D:\src\third_party\webrtc/modules/video_coding/sequence_number_util.h(90): error C2672: 'rtc::SafeLt': no matching overloaded function found
D:\src\third_party\webrtc/modules/video_coding/sequence_number_util.h(118): note: see reference to class template instantiation 'webrtc::SeqNumUnwrapper<T,M>' being compiled
ninja: build stopped: subcommand failed.

VC2015 = Okay
VC2017 = Broken
VC2017 Prerelease = Broken
Clang = Okay

An isolated test case suggests that the static_assert should be playing okay.

VC2017 isn't outputting the full template instantiation, which makes it tricky to diagnose the issue.
 
VC2017 isn't outputting the full template instantiation because there is no instantiation of SegNumUnwrapper. Weird. Here is a minimal repro:

// call.cc
#include <inttypes.h>
#include <limits>
struct LtOp { template <typename T1, typename T2> static constexpr bool Op(T1 a, T2 b) { return a < b; } };
template <typename T1, typename T2> constexpr bool SafeLt(const T1& a, const T2& b) { return safe_cmp_impl::LeOp::Op(a, b); }
template <typename T, T M = 0>
  class SeqNumUnwrapper {
    static_assert(SafeLt(std::numeric_limits<T>::max(),
        std::numeric_limits<uint64_t>::max()),
        "Type unwrapped must be an unsigned integer smaller than uint64_t.");
  };



Compile thusly - it works with VC++ 2015 but fails with VC++ 2017. I'll file a bug.

>cl /nologo /c call.cc /EHsc
call.cc
call.cc(8): error C2672: 'SafeLt': no matching overloaded function found
call.cc(10): note: see reference to class template instantiation 'SeqNumUnwrapper<T,M>' being compiled


The repro can be reduced further - I'll probably do that first.

Hmmm. My repro has bugs in it (LeOp versus LtOp) - maybe that's the problem with the original code. Please hold.
Having thought about it some more...

The problem happens when VC++ 2017 is analyzing SeqNumUnwrapper when it has not been instantiated. And, the problem has to do with std::numeric_limits<T>::max(). If this expression is replaced with an integer constant then the code compiles. It appears that VC++ 2017 has problems evaluating std::numeric_limits<T>::max() when T is not known. I tried casting the result of std::numeric_limits<T>::max() to uint64_t but that didn't help.

Here are some workarounds that do work:


  static_assert(
      std::is_unsigned<T>::value &&
          rtc::SafeLt(sizeof(T),
                      sizeof(uint64_t)),
      "Type unwrapped must be an unsigned integer smaller than uint64_t.");


  static_assert(
      std::is_unsigned<T>::value &&
          std::numeric_limits<T>::max() < std::numeric_limits<uint64_t>::max(),
      "Type unwrapped must be an unsigned integer smaller than uint64_t.");


Since there is no need for the SafeLt indirection I think I'll land the second workaround.

Independent of this compiler issue, the rtc code should really be replaced with the equivalent functionality from base/numerics. Just looking at safe_compare.h, it wastes unnecessary cycles on double-width type promotions for range checking that can be resolved at compile-time (as StrictNumeric does). Maybe I'll file a bug.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/452ea0d4d9f0c75065753ee04e1449f4c545955e

commit 452ea0d4d9f0c75065753ee04e1449f4c545955e
Author: brucedawson <brucedawson@chromium.org>
Date: Wed Aug 09 17:00:11 2017

Workaround VC++ 2017 template bug

When compiling webrtc's call.cc with VC++ 2017 (is_clang = false) the
following compile error occurs:

sequence_number_util.h(90): error C2672: 'rtc::SafeLt': no matching
overloaded function found
note: see reference to class template instantiation
'webrtc::SeqNumUnwrapper<T,M>' being compiled

This error is not associated with any particular instantiation of
SeqNumUnwrapper (there isn't one) and this undefined nature of 'T' seems
to be what confuses the compiler. When it tries to locate SafeLt for an
undefined type 'T' it gets confused.

SafeLt is unnecessary in this context and changing it to use the '<'
operator directly avoids the problem.

The bug has been reported to Microsoft.

BUG= chromium:753488 

Review-Url: https://codereview.webrtc.org/2997623002
Cr-Commit-Position: refs/heads/master@{#19292}

[modify] https://crrev.com/452ea0d4d9f0c75065753ee04e1449f4c545955e/webrtc/modules/video_coding/sequence_number_util.h

Status: Fixed (was: Assigned)
The patch appears to work for my build after the sync.

Sign in to add a comment