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

Issue 646539 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 645953



Sign in to add a comment

`content_browsertests --gtest_filter=CaptureScreenshotTest.CaptureScreenshot` and other tests failing on clang tot bots

Project Member Reported by thakis@chromium.org, Sep 13 2016

Issue description

See eg https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux%20tester/builds/2067

thakis@thakis:~/src/chrome/src$ out/gn/content_browsertests --gtest_filter=CaptureScreenshotTest.CaptureScreenshot 
IMPORTANT DEBUGGING NOTE: each test is run inside its own process.
For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with either
--single_process (to run the test in one launcher/browser process) or
--single-process (to do the above, and also run Chrome in single-process mode).
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
Note: Google Test filter = CaptureScreenshotTest.CaptureScreenshot
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CaptureScreenshotTest, where TypeParam = 
[ RUN      ] CaptureScreenshotTest.CaptureScreenshot
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:403: Failure
Expected: (1) >= (std::abs(0x12-(int)(((color) >> 16) & 0xFF))), actual: 1 vs 18
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:404: Failure
Expected: (1) >= (std::abs(0x34-(int)(((color) >> 8) & 0xFF))), actual: 1 vs 52
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:405: Failure
Expected: (1) >= (std::abs(0x56-(int)(((color) >> 0) & 0xFF))), actual: 1 vs 86
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:407: Failure
Expected: (1) >= (std::abs(0x12-(int)(((color) >> 16) & 0xFF))), actual: 1 vs 18
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:408: Failure
Expected: (1) >= (std::abs(0x34-(int)(((color) >> 8) & 0xFF))), actual: 1 vs 52
../../content/browser/devtools/protocol/devtools_protocol_browsertest.cc:409: Failure
Expected: (1) >= (std::abs(0x56-(int)(((color) >> 0) & 0xFF))), actual: 1 vs 86
[  FAILED  ] CaptureScreenshotTest.CaptureScreenshot, where TypeParam =  and GetParam() =  (1458 ms)


I can repro this locally with trunk clang but not with pinned clang, with the following args.gn:

is_component_build = true
is_debug = false
symbol_level = 1
clang_use_chrome_plugins = false
clang_base_path = rebase_path("../../../../../llvm-build", "//", "//")
llvm_force_head_revision = true

(For pinned clang, I omit the last 3 and instead insert `use_goma = true`)

So I guess some codegen thing regressed upstream.

Mac looks unhappy too, with similar issues: https://build.chromium.org/p/chromium.fyi/builders/ClangToTMac%20tester

I'll try to find which llvm commit is to blame. (Maybe related to the same root cause as  bug 645989 ? probably not tho)
 

Comment 3 by thakis@chromium.org, Sep 13 2016

thakis@thakis:~/src/llvm-rw$ svn log -r281160:281163  https://nico@llvm.org/svn/llvm-project/
------------------------------------------------------------------------
r281160 | jamesm | 2016-09-11 04:07:30 -0400 (Sun, 11 Sep 2016) | 7 lines

[SimplifyCFG] Harden up the profitability heuristic for block splitting during sinking

Exposed by PR30244, we will split a block currently if we think we can sink at least one instruction. However this isn't right - the reason we split predecessors is so that we can sink instructions that otherwise couldn't be sunk because it isn't safe to do so - stores, for example.

So, change the heuristic to only split if it thinks it can sink at least one non-speculatable instruction.

Should fix PR30244.
------------------------------------------------------------------------
r281161 | jamesm | 2016-09-11 04:24:04 -0400 (Sun, 11 Sep 2016) | 3 lines

[AArch64] Fixup test after r281160

How I missed this locally is beyond me. I suspect llc didn't recompile. This is just changing the CHECK line back to what it was before r280364.
------------------------------------------------------------------------
r281162 | jamesm | 2016-09-11 05:00:03 -0400 (Sun, 11 Sep 2016) | 5 lines

[SimplifyCFG] Be even more conservative in SinkThenElseCodeToEnd

This should *actually* fix PR30244. This cranks up the workaround for PR30188 so that we never sink loads or stores of allocas.

The idea is that these should be removed by SROA/Mem2Reg, and any movement of them may well confuse SROA or just cause unwanted code churn. It's not ideal that the midend should be crippled like this, but that unwanted churn can really cause significant regressions in important workloads (tsan).
------------------------------------------------------------------------
r281163 | jamesm | 2016-09-11 05:13:32 -0400 (Sun, 11 Sep 2016) | 3 lines

Fixup failing debuginfo test for change in SimplifyCFG.

This reverts this test back to its original pre-r280364 behaviour as we don't sink allocas any more.
------------------------------------------------------------------------




Comment 4 by thakis@chromium.org, Sep 13 2016

Test passes when built with clang r281161 but fails with clang r281162

Comment 7 by thakis@chromium.org, Sep 14 2016

https://codereview.chromium.org/2345543002/ has some debugging notes. It looks like things go wrong in ComputedStyle::colorIncludingFallback. If I compile webkit_unit_tests with that CL applied, then TextPainterTest.* passes when built with clang r281161 but doesn't with 281162. The disassembly of the two function versions is pretty different due to the two clangs assigning different registers to locals. I'll try to reduce it some more.

Comment 8 by thakis@chromium.org, Sep 16 2016

Status: Attached a fairly reduced standalone repro to https://llvm.org/bugs/show_bug.cgi?id=30373#c6

Comment 9 by krasin@chromium.org, Sep 17 2016

Cc: krasin@chromium.org
Owner: thakis@chromium.org
Status: Fixed (was: Untriaged)
James Molloy fixed in r281889 with that reduced repro.
Cc: p...@chromium.org
 Issue 647264  has been merged into this issue.

Sign in to add a comment