New issue
Advanced search Search tips

Issue 687259 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

ClangToT Windows bots failing browser_tests

Project Member Reported by h...@chromium.org, Jan 31 2017

Issue description

Comment 1 by h...@chromium.org, Jan 31 2017

Green at #447072, red at #447092
Changes: http://test-results.appspot.com/revision_range?start=447072&end=447092

Comment 2 by h...@chromium.org, Jan 31 2017

Hmm, maybe that range is wrong. Some bots seem to have started failing earlier.

Comment 3 by thakis@chromium.org, Jan 31 2017

Failing on pinned bots too: https://build.chromium.org/p/chromium.fyi/console?category=win%20clang So certainly due to a chromium-side change

Comment 4 by h...@chromium.org, Jan 31 2017

Since the "shared" and "dbg" bots don't do official builds it can't be an official build thing, but that explains why the bots list slightly different test failures.

It doesn't explain the ranges though.

Anyway, looking at https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28shared%29%20tester suggests
Green: #446978
Red: #446997
http://test-results.appspot.com/revision_range?start=446978&end=446997

Many tests have backtrace pointing to Skia, and there is a Skia roll in there:
https://codereview.chromium.org/2660813004
It only picked up one commit:
https://skia.googlesource.com/skia.git/+/40fd7c94c24bb30d888c3d85a79cbb96c7fbf800%5E%21/

Comment 6 by thakis@chromium.org, Jan 31 2017

does it repro locally?

Comment 7 by h...@chromium.org, Jan 31 2017

I'm syncing up to find out :-)

But the Skia stuff feels sketchy so I'm hoping that's the cause.

Look at the constructors here:
https://skia.googlesource.com/skia.git/+/40fd7c94c24bb30d888c3d85a79cbb96c7fbf800/src/gpu/effects/GrSimpleTextureEffect.h
They invoke the base ctor like this:
    GrSimpleTextureEffect(GrTexture* texture,
                          sk_sp<GrColorSpaceXform> colorSpaceXform,
                          const SkMatrix& matrix,
                          GrSamplerParams::FilterMode filterMode)
            : INHERITED(texture, std::move(colorSpaceXform), matrix, filterMode,
                        ModulationFlags(texture->config())) {
        this->initClassID<GrSimpleTextureEffect>();
    }

Note the std::move() and not using braced-initializer style.

The .cpp file carefully uses braces for the constructors it defines:

https://skia.googlesource.com/skia.git/+/40fd7c94c24bb30d888c3d85a79cbb96c7fbf800/src/gpu/effects/GrSimpleTextureEffect.cpp
GrSimpleTextureEffect::GrSimpleTextureEffect(GrContext* ctx, sk_sp<GrTextureProxy> proxy,
                                             sk_sp<GrColorSpaceXform> colorSpaceXform,
                                             const SkMatrix& matrix,
                                             GrSamplerParams::FilterMode filterMode)
        : INHERITED{ctx,
                    ModulationFlags(proxy->config()),
                    GR_PROXY_MOVE(proxy),
                    std::move(colorSpaceXform),
                    matrix,
                    filterMode} {
    this->initClassID<GrSimpleTextureEffect>();
}


I have no idea what this means, but it smells of c++ subtlety.

Comment 8 by h...@chromium.org, Jan 31 2017

Status: Started (was: Assigned)
I'm still building locally, but it looks like clang-cl might simply be wrong here:

  int f();
  int g();
  int h();

  struct Base {
    Base(int a, int b, int c);
  };

  struct Derived : public Base {
    Derived();
  };

  Derived::Derived() : Base{f(), g(), h()} {}


MSVC: (https://godbolt.org/g/7T91wC)
f(), g(), h()

clang-cl: (https://godbolt.org/g/Xmb51J)
h(), g(), f()

clang linux: (https://godbolt.org/g/5aFLCF)
f(), g(), h()

Comment 10 by h...@chromium.org, Jan 31 2017

I have a workaround for Skia, but failing to upload the patch :-(( https://bugs.chromium.org/p/skia/issues/detail?id=6182

Comment 11 by h...@chromium.org, Feb 1 2017

Cc: halcanary@chromium.org
+halcanary, the current Skia sheriff.

Can you please upload the fix from https://bugs.chromium.org/p/skia/issues/detail?id=6182 ?

Comment 12 by h...@chromium.org, Feb 1 2017

Managed to upload the patch: https://skia-review.googlesource.com/c/7831/

Comment 13 by h...@chromium.org, Feb 1 2017

Landed the Clang fix in r293732
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 1 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/d689f7a5313a7fd501c2572c493fe6d91a88cba4

commit d689f7a5313a7fd501c2572c493fe6d91a88cba4
Author: Hans Wennborg <hans@chromium.org>
Date: Wed Feb 01 00:14:24 2017

GR_PROXY_MOVE: Work around Win/Clang eval order bug

BUG= chromium:687259 

Change-Id: I145dac240a3c4f89cf1b6bf6ff54ba73cd110ebf
Reviewed-on: https://skia-review.googlesource.com/7831
Reviewed-by: Hans Wennborg <hwennborg@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>

[modify] https://crrev.com/d689f7a5313a7fd501c2572c493fe6d91a88cba4/src/gpu/effects/GrProxyMove.h

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 1 2017

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

commit f3359876535eb294692b53ad472d52bb7d6abdd7
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Wed Feb 01 15:00:32 2017

Roll src/third_party/skia/ a54ccb221..d689f7a53 (3 commits).

https://skia.googlesource.com/skia.git/+log/a54ccb22111e..d689f7a5313a

$ git log a54ccb221..d689f7a53 --date=short --no-merges --format='%ad %ae %s'
2017-01-31 hans GR_PROXY_MOVE: Work around Win/Clang eval order bug
2017-02-01 borenet Move presubmit bot to skia.swarmbucket
2017-01-31 robertphillips Add more pre-checks to surfaceProxy creation

BUG= 687259 ,687174

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
TBR=halcanary@google.com

Review-Url: https://codereview.chromium.org/2670473005
Cr-Commit-Position: refs/heads/master@{#447512}

[modify] https://crrev.com/f3359876535eb294692b53ad472d52bb7d6abdd7/DEPS

Comment 16 by h...@chromium.org, Feb 1 2017

Status: Fixed (was: Started)
browser_tests are turning green on the pinned bots: https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/11961

Sign in to add a comment