New issue
Advanced search Search tips

Issue 606810 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 607312

Blocking:
issue 82385



Sign in to add a comment

net_unittests QuicFramerTests failing on Windows Clang builds

Project Member Reported by h...@chromium.org, Apr 26 2016

Issue description

Comment 1 by h...@chromium.org, Apr 26 2016

All tests passed locally on my first attempt..

Comment 2 by wnwen@chromium.org, Apr 26 2016

Owner: wnwen@chromium.org
Status: Assigned (was: Available)
Fix in flight.

Comment 3 by wnwen@chromium.org, Apr 26 2016

Cc: wnwen@chromium.org
Owner: ----
Status: Available (was: Assigned)
Opps, may have jumped the gun a bit. My change broke all net_unittests on android, and your examples only failed on a few windows builders.

Comment 4 by h...@chromium.org, Apr 26 2016

It reproduces locally in a BUILD_TYPE=Official build..

wnwen: Do you have some more context? Like a link to your original change, fix, or what it's fixing?

Comment 5 by thakis@chromium.org, Apr 26 2016

Does it repro in buildtype=Official builds with msvc?

Comment 6 by wnwen@chromium.org, Apr 26 2016

This should have enough context on my bug: https://codereview.chromium.org/1919283002/

Comment 7 by h...@chromium.org, Apr 26 2016

> Does it repro in buildtype=Official builds with msvc?
Trying that now.

> This should have enough context on my bug: https://codereview.chromium.org/1919283002/
Thanks. The change you're referencing is more recent than this breakage, so probably not related.

Comment 8 by h...@chromium.org, Apr 26 2016

>> Does it repro in buildtype=Official builds with msvc?
> Trying that now.

Nope, they pass with MSVC in that mode :-/

Comment 9 by h...@chromium.org, Apr 26 2016

Also doesn't repro on Linux when built with:

GYP_DEFINES="BUILD_TYPE=Official target_arch=ia32"

Comment 10 by h...@chromium.org, Apr 26 2016

Classic Heisenbug: this patch makes the test failures go away:

diff --git a/net/quic/quic_framer_test.cc b/net/quic/quic_framer_test.cc
index 2039bd3..a1e7cc6 100644
--- a/net/quic/quic_framer_test.cc
+++ b/net/quic/quic_framer_test.cc
@@ -2645,6 +2645,7 @@ TEST_P(QuicFramerTest, AckFrame500NacksVersion32) {
   };
   // clang-format on

+  printf("framer_.version() = %d\n", framer_.version());
   if (framer_.version() <= QUIC_VERSION_31) {
     return;
   }


Seems like the if statement somehow gets optimized away, despite framer_.version() not being constant, but depending on the parameter of the test..

Value parameterized tests are documented here: https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests

Comment 11 by wnwen@chromium.org, Apr 26 2016

Cc: -wnwen@chromium.org

Comment 12 by h...@chromium.org, Apr 26 2016

Status: Started (was: Available)
It's starting to look like a codegen bug.. the test body starts off like this:


TEST_P(QuicFramerTest, AckFrame500NacksVersion32) {
  // clang-format off
  unsigned char packet[] = {
      // public flags (8 byte connection_id)
      static_cast<unsigned char>(
          framer_.version() > QUIC_VERSION_32 ? 0x38 : 0x3C),
      // connection_id
      0x10, 0x32, 0x54, 0x76, 0x98, 0xBA, 0xDC, 0xFE,
      // packet number
      0xA8, 0x9A, 0x78, 0x56, 0x34, 0x12,
      [some more bytes]
  };
  // clang-format on

  if (framer_.version() <= QUIC_VERSION_31) {
    return;
  }
  ... more test stuff below


The generated code looks like:


?TestBody@QuicFramerTest_AckFrame500NacksVersion32_Test@test@net@@UAEXXZ (public: virtual void __thiscall net::test::QuicFramerTest_AckFrame500NacksVersion32_Test::TestBody(void)):
  00000000: 83 B9 AC 00 00 00  cmp         dword ptr [ecx+0ACh],20h
            20
  00000007: B0 38              mov         al,38h
  00000009: 7F 02              jg          0000000D
  0000000B: B0 3C              mov         al,3Ch
  0000000D: 55                 push        ebp
  0000000E: 89 E5              mov         ebp,esp
  00000010: 53                 push        ebx
  00000011: 57                 push        edi
  00000012: 56                 push        esi
  00000013: 83 E4 F8           and         esp,0FFFFFFF8h
  00000016: 8D A4 24 88 FE FF  lea         esp,[esp-178h]
            FF
  0000001D: 88 44 24 30        mov         byte ptr [esp+30h],al
  00000021: C6 44 24 31 10     mov         byte ptr [esp+31h],10h
  00000026: C6 44 24 32 32     mov         byte ptr [esp+32h],32h
  0000002B: C6 44 24 33 54     mov         byte ptr [esp+33h],54h
  00000030: C6 44 24 34 76     mov         byte ptr [esp+34h],76h
  00000035: C6 44 24 35 98     mov         byte ptr [esp+35h],98h
  0000003A: C6 44 24 36 BA     mov         byte ptr [esp+36h],0BAh
  0000003F: C6 44 24 37 DC     mov         byte ptr [esp+37h],0DCh
  00000044: C6 44 24 38 FE     mov         byte ptr [esp+38h],0FEh
  00000049: C6 44 24 39 A8     mov         byte ptr [esp+39h],0A8h
  0000004E: C6 44 24 3A 9A     mov         byte ptr [esp+3Ah],9Ah
  00000053: C6 44 24 3B 78     mov         byte ptr [esp+3Bh],78h
  00000058: C6 44 24 3C 56     mov         byte ptr [esp+3Ch],56h
  0000005D: C6 44 24 3D 34     mov         byte ptr [esp+3Dh],34h
  00000062: C6 44 24 3E 12     mov         byte ptr [esp+3Eh],12h
  00000067: C6 44 24 3F 01     mov         byte ptr [esp+3Fh],1
  0000006C: 0F 28 05 00 00 00  movaps      xmm0,xmmword ptr [__xmm@ff00f30102000000123456789abfba6c]
            00
  00000073: 0F 11 44 24 40     movups      xmmword ptr [esp+40h],xmm0
  00000078: 0F 8C 47 08 00 00  jl          000008C5  <--- Jumps to return.
  .. more test stuff



It seems LLVM is cleverly re-using the "framer_.version() > QUIC_VERSION_32" comparison at 0x00 in the "jl" at 0x78, but the "and" at 0x13 has clobbered EFLAGS. This looks like shrink-wrapping gone wrong.

Comment 13 by h...@chromium.org, Apr 26 2016

Filed http://llvm.org/PR27531 upstream.

Comment 14 by h...@chromium.org, Apr 26 2016

Should be fixed by LLVM r267634. It'll take a while before the bots cycle, and then we'll need a roll.
Blocking: 82385

Comment 16 by h...@chromium.org, Apr 27 2016

Blockedon: 607312
Status: Fixed (was: Started)

Sign in to add a comment