New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 904337 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Sign in to add a comment

roll clang again

Project Member Reported by h...@chromium.org, Nov 12

Issue description

Previous roll:  Issue 894363 
 
Blockedon: 905289
Blocking: 904324
Want rL347072 for (a somewhat cleaner way of doing) issue 904324.
Blockedon: 908360
Blockedon: 908372
Blockedon: 908832
Blockedon: 908846
Blockedon: 910096
https://bugs.llvm.org/show_bug.cgi?id=39836
Significant build time regression since SVN r347776, "[LICM] Reapply r347190 "Make LICM able to hoist phis" with fix"
Sounds a little worrying.
Yeah...probably want to build new packages at r347867+ :-/
> Yeah...probably want to build new packages at r347867+ :-/

Already started :-)
https://chromium-review.googlesource.com/c/chromium/src/+/1355181
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 29

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

commit cc84b69c3a6ec0f918a2812ec2794b55e7399ea1
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Nov 29 15:31:30 2018

Clang update script: bump gnuwin version to include sort.exe

LLVM r347097 introduced a lit test that requires sort.

Bug:  904337 
Change-Id: I2daaea742207a3b5a77a0b95571352928641c287
Reviewed-on: https://chromium-review.googlesource.com/c/1354939
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612202}
[modify] https://crrev.com/cc84b69c3a6ec0f918a2812ec2794b55e7399ea1/tools/clang/scripts/update.py

Blockedon: 908937
(in case the roll above won't stick) Need r347803 for  issue 908937 .
I doubt it's going to work because the ASan NtTerminateThread patch is still in tree. I was debugging it yesterday, but it makes me want to pull my hair out.
I pulled it back out in r347933 and started packages for that for tomorrow here: https://chromium-review.googlesource.com/1356132
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 30

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

commit 1c5fa74f45fcc511536eb347d86bcaa39fc679cb
Author: David Staessens <dstaessens@chromium.org>
Date: Fri Nov 30 02:11:19 2018

re-land "media/gpu/test: Cleanup and split test helpers build target."

> This change splits up the test helpers target in separate encode, decode and
> other helpers. Tests now only depend on what they actually need. This change
> doesn't introduce any new code, but some things are moved to avoid weird
> dependancies (such as encode helpers depending on frame mapper which depends on
> decode helpers).

> This change also moves code in the video_accelerator_unittest_helpers.h file
> from the media to the media/test namespace.
>
> TEST=ran video/jpeg encode/decode tests on nocturne
>
> CQ-DEPEND=CL:1345670
>
> BUG=879065, 904337 
>
> Change-Id: I17704399c5724cbacc5611578e0dec22191e7a50
> Reviewed-on: https://chromium-review.googlesource.com/c/1345701
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: David Staessens <dstaessens@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#610757}

BUG=879065

Change-Id: I3c478745acdecc7bcbea089a079603c0c2472cdd
Reviewed-on: https://chromium-review.googlesource.com/c/1351193
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: David Staessens <dstaessens@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612518}
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/BUILD.gn
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/jpeg_decode_accelerator_unittest.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/jpeg_encode_accelerator_unittest.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/BUILD.gn
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/rendering_helper.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/video_accelerator_unittest_helpers.h
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/video_decode_accelerator_unittest_helpers.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/video_frame_validator.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/test/video_frame_validator.h
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/1c5fa74f45fcc511536eb347d86bcaa39fc679cb/media/gpu/video_encode_accelerator_unittest.cc

> I pulled it back out in r347933 and started packages for that for tomorrow here: https://chromium-review.googlesource.com/1356132

Thanks! Trying that now.
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 30

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

commit 0bdd3c25975f763d121802a092c1ef0925049976
Author: Hans Wennborg <hans@chromium.org>
Date: Fri Nov 30 15:50:07 2018

Roll clang 346388:347933.

Bug:  904337 
Change-Id: I8fad51cde2b93b83dfa1a14c61a992f659d827be
Reviewed-on: https://chromium-review.googlesource.com/c/1356710
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612660}
[modify] https://crrev.com/0bdd3c25975f763d121802a092c1ef0925049976/tools/clang/scripts/update.py

Project Member

Comment 18 by bugdroid1@chromium.org, Nov 30

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

commit 835d0fdea0f19000edf8766a4e299704586be44d
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Fri Nov 30 17:42:49 2018

Revert "Roll clang 346388:347933."

This reverts commit 0bdd3c25975f763d121802a092c1ef0925049976.

Reason for revert: Suspected to cause compile failure
on the Google Chrome Linux x64 bot  crbug.com/910644 

Original change's description:
> Roll clang 346388:347933.
> 
> Bug:  904337 
> Change-Id: I8fad51cde2b93b83dfa1a14c61a992f659d827be
> Reviewed-on: https://chromium-review.googlesource.com/c/1356710
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612660}

TBR=thakis@chromium.org,hans@chromium.org,rnk@chromium.org

Change-Id: Ia95868dd39daaa60e204a6ec64df6eabfafacb94
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  904337 ,  910644 
Reviewed-on: https://chromium-review.googlesource.com/c/1357012
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612687}
[modify] https://crrev.com/835d0fdea0f19000edf8766a4e299704586be44d/tools/clang/scripts/update.py

Blockedon: 910644
Blocking: 910200
Blockedon: 912021
Blockedon: 912040
Blockedon: 912571
Blocking: 908474
Want r348515 for  issue 908474 
Blockedon: -912021
Blocking: 910599
Blockedon: 912878
Blockedon: 912919

Comment 29 Deleted

Want r348569 for the thunk miscompile, and then r348577 for mac lit test fix.

I think the only hard blocker is  crbug.com/912878 . It's a week old regression, so not a straight-forward revert.
Blocking: 908937
Blockedon: -908937
Blocking: 912021
Blockedon: 913423
Blockedon: 913702
Blockedon: 913704
The MIPS folks want rLLD347742 after https://chromium-review.googlesource.com/c/chromium/src/+/1361321
Blockedon: -912919
Blocking: 914947
r349115 for  bug 914947 
r349216 for b/121046331.
IIUC the only blocker is tsan + interactive_ui_tests ( https://crbug.com/913702 ). After reading the bug, it seems to me that maybe it's not a blocker after all, since the test was already flaky?

In any case, I mashed the CQ button on the last set of packages in https://chromium-review.googlesource.com/c/chromium/src/+/1369628, which has r348614, so it doesn't have the fixes for c#40 and c#41 yet.
> In any case, I mashed the CQ button [...]

I mean, the dry run button to see if we can get green try jobs. Maybe that doesn't retry tsan, I'll futz with it some more.
The test is not flaky, it fails very consistently on our shiny new tsan tot bot: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ToTLinuxTSan
Cc: vtsyrklevich@chromium.org
Project Member

Comment 46 by bugdroid1@chromium.org, Dec 20

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

commit bd324208d892ab793b062049b1bd642bcee631d2
Author: Nico Weber <thakis@chromium.org>
Date: Thu Dec 20 09:52:35 2018

Roll clang 346388:348507.

Bug:  904337 
Change-Id: Icbc3e6fda8bcbaabedc0c823b91395dab3daf28b
Reviewed-on: https://chromium-review.googlesource.com/c/1366078
Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618154}
[modify] https://crrev.com/bd324208d892ab793b062049b1bd642bcee631d2/tools/clang/scripts/update.py

Project Member

Comment 47 by bugdroid1@chromium.org, Dec 20

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

commit f744168e409f017648ae5673c174b79e88fe26fd
Author: Juan Antonio Navarro Pérez <perezju@chromium.org>
Date: Thu Dec 20 11:56:56 2018

Revert "Roll clang 346388:348507."

This reverts commit bd324208d892ab793b062049b1bd642bcee631d2.

Reason for revert: Causing compile failures on multiple builders

Original change's description:
> Roll clang 346388:348507.
> 
> Bug:  904337 
> Change-Id: Icbc3e6fda8bcbaabedc0c823b91395dab3daf28b
> Reviewed-on: https://chromium-review.googlesource.com/c/1366078
> Reviewed-by: Hans Wennborg <hans@chromium.org>
> Commit-Queue: Hans Wennborg <hans@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#618154}

TBR=thakis@chromium.org,hans@chromium.org

Change-Id: Ib78eb22439b34da425a15478dfe88ee41c0b08a5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  904337 ,  916951 
Reviewed-on: https://chromium-review.googlesource.com/c/1386584
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618177}
[modify] https://crrev.com/f744168e409f017648ae5673c174b79e88fe26fd/tools/clang/scripts/update.py

The roll was reverted due to  Issue 916951  which sounds a lot like llvm.org/pr39890 which was supposedly fixed in Clang r349075

We already have a package past that: https://chromium-review.googlesource.com/c/chromium/src/+/1382232 so I'll give that a try.
I double checked that https://chromium-review.googlesource.com/c/chromium/src/+/1382232 doesn't have the  Issue 916951  problem.
Project Member

Comment 50 by bugdroid1@chromium.org, Dec 20

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

commit 82a6a90a82e37af925f7db86a631baafe31be18e
Author: Hans Wennborg <hans@chromium.org>
Date: Thu Dec 20 18:52:52 2018

Roll clang 346388:349417.

Binary-Size: compiler roll fluctuations
Bug:  904337 
Change-Id: I65932cdedefc4e640ea6938057977fba8f60fa4d
Reviewed-on: https://chromium-review.googlesource.com/c/1382232
Commit-Queue: Hans Wennborg <hans@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618281}
[modify] https://crrev.com/82a6a90a82e37af925f7db86a631baafe31be18e/tools/clang/scripts/update.py

Project Member

Comment 51 by bugdroid1@chromium.org, Dec 20

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

commit a108e622afdc4c14cef57cda4cd60277ede86bb7
Author: Nico Weber <thakis@chromium.org>
Date: Thu Dec 20 19:57:21 2018

Add nacl_helper_bootstrap to linux deterministic build whitelist after clang roll.

I'll investigate what's up later.

TBR=hans

Bug: 429358, 904337 
Change-Id: Iea85c35ddbd9ceaf7c46a95e29b9e67a4bfe1d6a
Reviewed-on: https://chromium-review.googlesource.com/c/1387604
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618298}
[modify] https://crrev.com/a108e622afdc4c14cef57cda4cd60277ede86bb7/tools/determinism/deterministic_build_whitelist.pyl

Owner: h...@chromium.org
Status: Fixed (was: Available)
Next roll:  Issue 917419 
Cc: kjlubick@chromium.org
FYI, it looks like r347339 (within the roll range) has enabled some additional checks which broke skia and perfetto builds on OSS-Fuzz. That's not a big deal, but something that might be worth being aware of. Thanks kjlubick@ for spotting the breakage.

Cc: mtklein@chromium.org
r347339 adds -Wextra-semi-stmt, which is neither in -Wall nor -Wextra, so you only get it if you build with -Weverything. Doing that is explicitly discouraged. It looks like skia does use it (https://cs.chromium.org/search/?q=weverything&sq=package:chromium); I vaguely remember talking about this with mtklein and he said "we want this, we can handle it" (I think?). Projects that use -Weverything should expect more turbulences on compiler updates. That's not something we (the compiler folks) test or care much about -- if you don't want -Weverything and everything that comes with it, don't use it.
Cc: primiano@chromium.org
+primiano for perfetto too. You shouldn't use -Weverything unless you know what comes with it (in which case we shouldn't get comments like #53).
Yeah, we're still pretty happy using -Weverything and a bunch of -Wno-*, and don't mind that the fuzzers break temporarily.  Those OSS-Fuzz builds are outside our build system, so we're not really in the loop on when they break until we get emails.  Most of the Skia codebase got cleaned up for -Wextra-semi-stmt, but that cleanup missed a file only those fuzzers build.
Cc: fmayer@chromium.org
Re #55. 
In perfetto we set -Weverything only in the standalone build [1]  (outside of the chrome tree). When building in chrome, we use whatever the default flags are.
The intention (but let's figure out where this is going wrong) is that folks in my team are the only ones who pay the cost of this choice, not the rest of chrome or compiler folks. Breakages due to bleeding edge warnings should happen only when our team catches up with chrome's CLANG_REVISION in [2], but that is a process that should be invisible to everybody else.

** EDIT after re-reading 53 and drinking more coffee **
Ahh the problem here might be related with OSS-Fuzz (!= chrome clusterfuzz) and the fact that OSS-fuzz might be using the standalone build and hence using Weverything, that would explain.
+fmayer (who maintains our OSS-fuzz). I think the thing to fix is to make it so that -Weverything is not set if is_hermetic_clang=false in our GN args (is_hermetic_clang=false in the OSS-fuzz builds when I follow [3]).


[1] https://android.googlesource.com/platform/external/perfetto/+/master/gn/standalone/BUILD.gn#26
[2] https://android.googlesource.com/platform/external/perfetto/+/master/tools/install-build-deps#119
[3] https://android.googlesource.com/platform/external/perfetto/+/master/infra/oss-fuzz/build_fuzzers
(Same deal for Skia... -Weverything's only in our standalone builds.  If we want to prevent this in the future, and we're going to continue using the Chromium toolchain to build these fuzzers, it probably makes sense to build them against Skia in Chromium's GN tree, not Skia's own.  Or maybe just add "-w" to extra_cflags when building.)
> That's not something we (the compiler folks) test or care much about

Sure thing, I didn't mean that it might be a problem for you, just wanted to give a heads up in case anyone else would get an unexpected compilation failure and start to complain.
Removing Weverything from the OSS-fuzz build in https://android-review.googlesource.com/c/platform/external/perfetto/+/858176
Project Member

Comment 61 by bugdroid1@chromium.org, Dec 28

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

commit 36cb878b2bfbb2c16ef540f28f988b23b61b29bf
Author: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Date: Fri Dec 28 22:00:41 2018

Roll src/third_party/perfetto 5a17545b5ab9..8a5b9a8ad6fa (1 commits)

https://android.googlesource.com/platform/external/perfetto.git/+log/5a17545b5ab9..8a5b9a8ad6fa


git log 5a17545b5ab9..8a5b9a8ad6fa --date=short --no-merges --format='%ad %ae %s'
2018-12-28 primiano@google.com Disable -Weverything on fuzzer builds


Created with:
  gclient setdep -r src/third_party/perfetto@8a5b9a8ad6fa

The AutoRoll server is located here: https://autoroll.skia.org/r/perfetto-chromium-autoroll

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

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:904337 
TBR=perfetto-bugs@google.com

Change-Id: I524e1a6bcb8dac61a20f51d2030b4ce6f6e4014b
Reviewed-on: https://chromium-review.googlesource.com/c/1392290
Reviewed-by: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Commit-Queue: chromium-autoroll <chromium-autoroll@skia-public.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#619201}
[modify] https://crrev.com/36cb878b2bfbb2c16ef540f28f988b23b61b29bf/DEPS

Sign in to add a comment