Coverage: Failed to compile components/viz/host/hit_test/hit_test_query.cc with coverage intrumentation |
|||||||
Issue descriptionClang++ fails to compile components/viz/host/hit_test/hit_test_query.cc with coverage instrumentation, and this is causing a lot of test targets fail to build, such as unit_tests, browser_tests. I'll investigate this issue some time this week.
,
Oct 2
,
Oct 4
This is pretty bad :( Since the latest clang roll ( issue 888476 ), the build is failing for a bunch of targets on the coverage bots: ERROR [Thu Oct 4 17:20:22 UTC 2018]: Restarting the loop as too many targets failed to build: accessibility_unittests app_shell_unittests aura_unittests blink_heap_unittests blink_platform_unittests browser_tests cc_unittests chrome_app_unittests components_browsertests components_unittests compositor_unittests content_browsertests content_unittests extensions_browsertests extensions_unittests headless_browsertests headless_unittests interactive_ui_tests media_blink_unittests media_unittests message_center_unittests nacl_loader_unittests pdf_unittests printing_unittests remoting_unittests services_unittests skia_unittests snapshot_unittests sync_integration_tests ui_touch_selection_unittests unit_tests views_unittests viz_unittests vr_common_unittests webkit_unit_tests wm_unittests blink_tests appcache_manifest_parser_fuzzer audio_decoder_fuzzer ax_tree_fuzzer blink_harfbuzz_shaper_fuzzer blink_html_tokenizer_fuzzer blink_http_parsers_fuzzer blink_json_parser_fuzzer blink_png_decoder_fuzzer blink_text_codec_UTF_8_fuzzer blink_text_codec_WINDOWS_1252_fuzzer cast_message_fuzzer clear_site_data_fuzzer content_security_policy_fuzzer css_parser_fast_paths_fuzzer css_parser_proto_fuzzer csv_reader_fuzzer feature_policy_fuzzer fingerprint_fuzzer form_structure_fuzzer form_structure_process_query_response_fuzzer hit_test_manager_fuzzer hit_test_query_fuzzer html_preload_scanner_fuzzer lookup_affiliation_response_parser_fuzzer merkle_integrity_source_stream_fuzzer mhtml_parser_fuzzer origin_policy_parser_fuzzer origin_trial_token_fuzzer paint_op_buffer_eq_fuzzer paint_op_buffer_fuzzer password_manager_form_parser_fuzzer password_manager_form_parser_generic_fuzzer password_manager_form_parser_proto_fuzzer password_manager_form_parser_proto_generic_fuzzer payment_method_manifest_fuzzer payment_web_app_manifest_fuzzer renderer_fuzzer sequence_manager_fuzzer signed_exchange_certificate_chain_fuzzer signed_exchange_envelope_fuzzer signed_exchange_signature_header_field_fuzzer stylesheet_contents_fuzzer template_url_parser_fuzzer text_resource_decoder_fuzzer transfer_cache_fuzzer v4_get_hash_protocol_manager_fuzzer v8_serialized_script_value_fuzzer web_icon_sizes_fuzzer I guess we should add many more targets to the ToT testbots in order to prevent issues like this.
,
Oct 4
Yep, I filed a separate bug to add unit_tests to the bot after this is fixed. Hey Max, I'm curious how did you get the logs for unfinished builds?
,
Oct 4
From code-coverage-linux-0004 bot from /home/coverage-bot/bot.log file.
,
Oct 4
> I guess we should add many more targets to the ToT testbots in order to prevent issues like this. Yes, let's file a bug for that. It should probably wait until after the LUCI migration. I repro'd the crash and I'll make a reduction.
,
Oct 4
I'm going to try to put together a roll and fix this forward, so let's block it on the roll bug.
,
Oct 4
Thanks rnk@!
,
Oct 4
It looks like this wasn't a regression from the last roll. The reduction is still going.
,
Oct 4
Wow, but the builds broke on the bots exactly 2 days ago... Based on what I see in the logs, the regression range in Chromium should be 595540:595761: https://chromium.googlesource.com/chromium/src/+log/31e4dccfb1777b8a6c31255872682d10bc32492c..0530624701d02b73eda440b0efc127f70098f513?pretty=fuller&n=10000 0001: + REVISION=595463 + REVISION= + REVISION=595807 ERROR [Tue Oct 2 16:32:22 UTC 2018]: Restarting the loop as too many targets failed to build: 0002: + REVISION=595472 + REVISION= + REVISION=595824 ERROR [Tue Oct 2 17:27:37 UTC 2018]: Restarting the loop as too many targets failed to build: 0003: + REVISION=595479 + REVISION= + REVISION=595850 ERROR [Tue Oct 2 19:07:15 UTC 2018]: Restarting the loop as too many targets failed to build: + REVISION= 0004: + REVISION=595540 + REVISION= + REVISION=595915 ERROR [Tue Oct 2 21:33:48 UTC 2018]: Restarting the loop as too many targets failed to build: 0005: + REVISION=595424 + REVISION= + REVISION=595761 ERROR [Tue Oct 2 12:18:50 UTC 2018]: Restarting the loop as too many targets failed to build:
,
Oct 4
https://chromium-review.googlesource.com/c/chromium/src/+/1226099 might be the culprit then, at least for the failure mentioned in c#1. Let me see where the other targets are breaking.
,
Oct 4
Well, apparently that's the only problem: mmoroz@code-coverage-linux-0001:/chromium/src/logs$ cat *.log | egrep 'clang\+\+: error: unable to execute command: Aborted' -c 62 mmoroz@code-coverage-linux-0001:/chromium/src/logs$ cat *.log | egrep 'components/viz/host/hit_test/hit_test_query.cc:33:19: Generating code for declaratio\n' -c 62 Sadrul, could you please either fix or revert your CL https://chromium-review.googlesource.com/c/chromium/src/+/1226099 ? We should also try to make a smaller reproducer out of it and report it to LLVM, but to keep things working in Chromium we would have to change the code a little bit.
,
Oct 4
Actually, it's zandershah@'s CL, not sadrul@'s
,
Oct 4
Reverting the CL, as it's already 6:30 PM for zandershah@. https://chromium-review.googlesource.com/c/chromium/src/+/1262926 Reid, thank you for jumping on this! Sorry for the false alarm, who knew that things can collide so weirdly...
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/37dda234a8f8245675c7e4f7d64402fe21926d16 commit 37dda234a8f8245675c7e4f7d64402fe21926d16 Author: Max Moroz <mmoroz@chromium.org> Date: Thu Oct 04 23:37:52 2018 Revert "viz: HitTest debug logging." This reverts commit 535e31ec622d308b10ce7e7ac913595c76bcee1e. Reason for revert: Broke code coverage builds: crbug.com/891320 Bug: 891320 Original change's description: > viz: HitTest debug logging. > > Ctrl+Shift+H to trigger. Requires --enable-viz-hit-test-debug flag and logging level of 1 on hit_test_debug_key_event_observer. > > Adds observer to render_widget_host_impl to act on OnInputEventAck. > > Example: https://pastebin.com/pcxrZv6A > > R=riajiang@chromium.org > > Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel > Change-Id: I1c9a0867debe5b4aa670a51f85fe24909073374c > Reviewed-on: https://chromium-review.googlesource.com/1226099 > Commit-Queue: Alexander Shah <zandershah@google.com> > Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org> > Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> > Reviewed-by: Ria Jiang <riajiang@chromium.org> > Cr-Commit-Position: refs/heads/master@{#595563} TBR=sadrul@chromium.org,nzolghadr@chromium.org,riajiang@chromium.org,zandershah@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: I51780234c0ee6ed9e4cdae71ef839cc5b4f633f7 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/c/1262926 Commit-Queue: Max Moroz <mmoroz@chromium.org> Reviewed-by: Max Moroz <mmoroz@chromium.org> Cr-Commit-Position: refs/heads/master@{#596925} [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/components/viz/common/hit_test/hit_test_region_list.h [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/components/viz/host/hit_test/hit_test_query.cc [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/components/viz/host/hit_test/hit_test_query.h [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/content/browser/BUILD.gn [delete] https://crrev.com/d2187d5fd9cb3ce36583a36af3633badb1a753df/content/browser/renderer_host/hit_test_debug_key_event_observer.cc [delete] https://crrev.com/d2187d5fd9cb3ce36583a36af3633badb1a753df/content/browser/renderer_host/hit_test_debug_key_event_observer.h [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/content/browser/renderer_host/render_widget_host_view_event_handler.cc [modify] https://crrev.com/37dda234a8f8245675c7e4f7d64402fe21926d16/content/browser/renderer_host/render_widget_host_view_event_handler.h
,
Oct 5
Is that config covered on the CQ? If not, can it be?
,
Oct 5
@sadrul, hard to say, probably not, I bet coverage builds are slow. The waterfall bot itself didn't actually uncover the bug to begin with.
I got a reduction:
$ cat hit.cpp
class a {
public:
a(const char *);
bool ak();
};
a c() {
a an = "";
#define ao(b) an.ak() ? #b : "" #b
ao();
return an;
}
$ ./bin/clang -c -fprofile-instr-generate -fcoverage-mapping hit.cpp
clang-8: /usr/local/google/home/rnk/llvm-project/clang/lib/CodeGen/CoverageMappingGen.cpp:592: void (anonymous namespace)::CounterCoverageMappingBuilder::popRegions(size_t): Assertion `SpellingRegion(SM, Region).isInSourceOrder()' failed.
...
I'll file a bug for it upstream. Fixing it doesn't seem trivial, so I'm not going to try to get a fix into this clang roll. We'll probably wait for the next one.
,
Oct 5
I am afraid I don't completely understand. What do we need to do before we re-submit the CL?
,
Oct 5
The way the CL uses xmacro #defines happens to cause clang's code coverage emission code to crash. You can resubmit the CL with a workaround, or wait for us to fix the bug in the compiler and roll in a new version. It usually takes us around two weeks to fix a bug and push a new compiler version. In the mean time, if you avoid the xmacro pattern for the switch/case in the CL, you can probably resubmit the change without breaking the code coverage build. If you still prefer the #define version you can leave a TODO to this bug to put back the macros after we fix the compiler bug.
,
Oct 9
Re c#16, Reid is right, coverage builds are quite slow, so we don't have them in the CQ.
,
Oct 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d commit 77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d Author: Alexander Shah <zandershah@google.com> Date: Thu Oct 11 20:33:45 2018 Reland viz: HitTest debug logging. Original CL: https://chromium-review.googlesource.com/c/chromium/src/+/1226099 Only change is a fix for the xdefine macro issue in hit_test_query.cc. Bug: 891320 R=riajiang@chromium.org Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel Change-Id: Ief766fbd9a9bc5c41a0a7b087c318af5e09e8d81 Reviewed-on: https://chromium-review.googlesource.com/c/1269760 Reviewed-by: Ria Jiang <riajiang@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: Alexander Shah <zandershah@google.com> Cr-Commit-Position: refs/heads/master@{#598917} [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/components/viz/common/hit_test/hit_test_region_list.h [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/components/viz/host/hit_test/hit_test_query.cc [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/components/viz/host/hit_test/hit_test_query.h [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/content/browser/BUILD.gn [add] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/content/browser/renderer_host/hit_test_debug_key_event_observer.cc [add] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/content/browser/renderer_host/hit_test_debug_key_event_observer.h [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/content/browser/renderer_host/render_widget_host_view_event_handler.cc [modify] https://crrev.com/77c6f62e39c351d19c460e4d0fbb9b65c5ccff5d/content/browser/renderer_host/render_widget_host_view_event_handler.h |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by liaoyuke@chromium.org
, Oct 2