From 7136ea89d2a073301894ff1faa5c26e91ca242ef Mon Sep 17 00:00:00 2001 From: Michael Achenbach Date: Mon, 5 Dec 2022 15:40:21 +0100 Subject: [PATCH] [gcmole] Fix and simplify test-run mode The test-run mode was broken after output improvements and the introduction of pathlib. This fixes the string concatenation with paths and updates the test output to match the status quo. This also changes the test-run mode to run exclusively when the --test-run option is passed. Now it's either a test run or a normal run. Like that we can add the test run as a separate test step on a bot. If both are needed in sequence for something, gcmole could be called twice. Bug: v8:12660 Change-Id: I58179d50950fa76d8f66b974325a8fed84dc91b6 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4075727 Commit-Queue: Michael Achenbach Reviewed-by: Leszek Swirski Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/main@{#84655} --- tools/gcmole/gcmole.py | 67 +++++----- tools/gcmole/gcmole_test.py | 15 +-- tools/gcmole/test-expectations.txt | 192 +++++++++++++++++++++++++---- 3 files changed, 213 insertions(+), 61 deletions(-) diff --git a/tools/gcmole/gcmole.py b/tools/gcmole/gcmole.py index 4ac8cb5b30..0e3b7b7fc1 100755 --- a/tools/gcmole/gcmole.py +++ b/tools/gcmole/gcmole.py @@ -6,12 +6,15 @@ # This is main driver for gcmole tool. See README for more details. # Usage: CLANG_BIN=clang-bin-dir python tools/gcmole/gcmole.py [arm|arm64|ia32|x64] +from contextlib import contextmanager +from contextlib import redirect_stderr from multiprocessing import cpu_count from pathlib import Path import argparse import collections import difflib +import io import json import os import re @@ -221,7 +224,7 @@ def invoke_clang_plugin_for_each_file(filenames, plugin, plugin_args, options): # ----------------------------------------------------------------------------- -def build_file_list(options, for_test): +def build_file_list(options): """Calculates the list of source files to be checked with gcmole. The list comprises all files from marked source sections in the @@ -236,7 +239,7 @@ def build_file_list(options, for_test): Returns: List of file paths (of type Path). """ - if for_test: + if options.test_run: return [options.v8_root_dir / "tools/gcmole/gcmole-test.cc"] result = [] gn_files = [ @@ -431,8 +434,8 @@ def write_gcmole_results(collector, options, dst): # Analysis -def check_correctness_for_arch(options, for_test): - files = build_file_list(options, for_test) +def check_correctness_for_arch(options): + files = build_file_list(options) if not options.reuse_gcsuspects: generate_gc_suspects(files, options) @@ -441,7 +444,6 @@ def check_correctness_for_arch(options, for_test): processed_files = 0 errors_found = False - output = "" log("Searching for evaluation order problems " + (' and dead variables' if options.dead_vars else '') + "for" + @@ -459,26 +461,29 @@ def check_correctness_for_arch(options, for_test): if not errors_found: errors_found = re.search("^[^:]+:\d+:\d+: (warning|error)", stderr, re.MULTILINE) is not None - if for_test: - output = output + stderr - else: - sys.stdout.write(stderr) + sys.stderr.write(stderr) log("Done processing {} files.", processed_files) log("Errors found" if errors_found else "No errors found") - return errors_found, output + return errors_found -def test_run(options): +def has_unexpected_errors(options, errors_found, file_io): + """Returns True if error state isn't as expected, False otherwise. + + In test-run mode, we expect certain errors and return False if expectations + are met. + """ if not options.test_run: - return True + return errors_found + log("Test Run") - errors_found, output = check_correctness_for_arch(options, True) + output = file_io.getvalue() if not errors_found: log("Test file should produce errors, but none were found. Output:") print(output) - return False + return True new_file = options.out_dir / "test-expectations-gen.txt" with open(new_file, "w") as f: @@ -494,9 +499,9 @@ def test_run(options): print("#" * 79) log("Output mismatch from running tests.") log("Please run gcmole manually with --test-run --verbose.") - log("Expected: " + expected_file) - log("New: " + new_file) - log("*Diff:* " + diff_file) + log(f"Expected: {expected_file}") + log(f"New: {new_file}") + log(f"*Diff:* {diff_file}") print("#" * 79) for line in difflib.unified_diff( expectations.splitlines(), @@ -509,17 +514,17 @@ def test_run(options): print("#" * 79) log("Full output") - log("Expected: " + expected_file) - log("Diff: " + diff_file) - log("*New:* " + new_file) + log(f"Expected: {expected_file}") + log(f"Diff: {diff_file}") + log(f"*New*: {new_file}") print("#" * 79) print(output) print("#" * 79) - return False + return True log("Tests ran successfully") - return True + return False # ============================================================================= @@ -637,15 +642,17 @@ def main(argv): options.func(options) -def full_run(options): - any_errors_found = False - if not test_run(options): - any_errors_found = True - else: - errors_found, output = check_correctness_for_arch(options, False) - any_errors_found = any_errors_found or errors_found +@contextmanager +def maybe_redirect_stderr(options): + file_io = io.StringIO() if options.test_run else sys.stderr + with redirect_stderr(file_io) as f: + yield f - sys.exit(1 if any_errors_found else 0) + +def full_run(options): + with maybe_redirect_stderr(options) as file_io: + errors_found = check_correctness_for_arch(options) + sys.exit(has_unexpected_errors(options, errors_found, file_io)) def verify_and_convert_dirs(parser, options, default_tools_gcmole_dir, diff --git a/tools/gcmole/gcmole_test.py b/tools/gcmole/gcmole_test.py index 9bec6e1c62..2baf6532b5 100644 --- a/tools/gcmole/gcmole_test.py +++ b/tools/gcmole/gcmole_test.py @@ -17,7 +17,8 @@ import gcmole TESTDATA_PATH = os.path.join( os.path.dirname(os.path.abspath(__file__)), 'testdata', 'v8') -Options = collections.namedtuple('Options', ['v8_root_dir', 'v8_target_cpu']) +Options = collections.namedtuple( + 'Options', ['v8_root_dir', 'v8_target_cpu', 'test_run']) def abs_test_file(f): @@ -27,13 +28,13 @@ def abs_test_file(f): class FilesTest(unittest.TestCase): def testFileList_for_testing(self): - options = Options(Path(TESTDATA_PATH), 'x64') + options = Options(Path(TESTDATA_PATH), 'x64', True) self.assertEqual( - gcmole.build_file_list(options, True), + gcmole.build_file_list(options), list(map(abs_test_file, ['tools/gcmole/gcmole-test.cc']))) def testFileList_x64(self): - options = Options(Path(TESTDATA_PATH), 'x64') + options = Options(Path(TESTDATA_PATH), 'x64', False) expected = [ 'file1.cc', 'file2.cc', @@ -45,11 +46,11 @@ class FilesTest(unittest.TestCase): 'test/cctest/test-x64-file2.cc', ] self.assertEqual( - gcmole.build_file_list(options, False), + gcmole.build_file_list(options), list(map(abs_test_file, expected))) def testFileList_arm(self): - options = Options(Path(TESTDATA_PATH), 'arm') + options = Options(Path(TESTDATA_PATH), 'arm', False) expected = [ 'file1.cc', 'file2.cc', @@ -59,7 +60,7 @@ class FilesTest(unittest.TestCase): 'arm/file2.cc', ] self.assertEqual( - gcmole.build_file_list(options, False), + gcmole.build_file_list(options), list(map(abs_test_file, expected))) diff --git a/tools/gcmole/test-expectations.txt b/tools/gcmole/test-expectations.txt index 92256b3e18..fa07e159a5 100644 --- a/tools/gcmole/test-expectations.txt +++ b/tools/gcmole/test-expectations.txt @@ -1,73 +1,217 @@ -tools/gcmole/gcmole-test.cc:30:10: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:30:10: warning: Possibly stale variable due to GCs. return obj; ^ -tools/gcmole/gcmole-test.cc:48:3: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:28:20: note: Call might cause unexpected GC. + isolate->heap()->CollectGarbage(OLD_SPACE, GarbageCollectionReason::kTesting); + ^ +./src/heap/heap.h:983:21: note: GC call here. + V8_EXPORT_PRIVATE bool CollectGarbage( + ^ +tools/gcmole/gcmole-test.cc:48:3: warning: Possible problem with evaluation order with interleaved GCs. TwoArgumentsFunction(*CauseGC(obj1, isolate), *CauseGC(obj2, isolate)); ^ -tools/gcmole/gcmole-test.cc:60:3: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:48:25: note: Call might cause unexpected GC. + TwoArgumentsFunction(*CauseGC(obj1, isolate), *CauseGC(obj2, isolate)); + ^ +tools/gcmole/gcmole-test.cc:21:1: note: GC call here. +Handle CauseGC(Handle obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:60:3: warning: Possible problem with evaluation order with interleaved GCs. TwoSizeTArgumentsFunction(sizeof(*CauseGC(obj1, isolate)), ^ -tools/gcmole/gcmole-test.cc:85:7: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:60:37: note: Call might cause unexpected GC. + TwoSizeTArgumentsFunction(sizeof(*CauseGC(obj1, isolate)), + ^ +tools/gcmole/gcmole-test.cc:21:1: note: GC call here. +Handle CauseGC(Handle obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:85:7: warning: Possible problem with evaluation order with interleaved GCs. so->Method(*CauseGC(obj1, isolate)); ^ -tools/gcmole/gcmole-test.cc:87:7: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:85:15: note: Call might cause unexpected GC. + so->Method(*CauseGC(obj1, isolate)); + ^ +tools/gcmole/gcmole-test.cc:21:1: note: GC call here. +Handle CauseGC(Handle obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:87:7: warning: Possible problem with evaluation order with interleaved GCs. so->Method(CauseGCRaw(*obj1, isolate)); ^ -tools/gcmole/gcmole-test.cc:131:14: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:85:15: note: Call might cause unexpected GC. + so->Method(*CauseGC(obj1, isolate)); + ^ +tools/gcmole/gcmole-test.cc:21:1: note: GC call here. +Handle CauseGC(Handle obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:131:14: warning: Possible problem with evaluation order with interleaved GCs. so_handle->Method(*derived.VirtualCauseGC(obj1, isolate)); ^ -tools/gcmole/gcmole-test.cc:133:14: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:131:30: note: Call might cause unexpected GC. + so_handle->Method(*derived.VirtualCauseGC(obj1, isolate)); + ^ +tools/gcmole/gcmole-test.cc:115:3: note: GC call here. + Handle VirtualCauseGC(Handle obj, Isolate* isolate) override { + ^ +tools/gcmole/gcmole-test.cc:133:14: warning: Possible problem with evaluation order with interleaved GCs. so_handle->Method(*base->VirtualCauseGC(obj1, isolate)); ^ -tools/gcmole/gcmole-test.cc:154:14: warning: Possible problem with evaluation order. +tools/gcmole/gcmole-test.cc:131:30: note: Call might cause unexpected GC. + so_handle->Method(*derived.VirtualCauseGC(obj1, isolate)); + ^ +tools/gcmole/gcmole-test.cc:115:3: note: GC call here. + Handle VirtualCauseGC(Handle obj, Isolate* isolate) override { + ^ +tools/gcmole/gcmole-test.cc:154:14: warning: Possible problem with evaluation order with interleaved GCs. so_handle->Method(*SomeClass::StaticCauseGC(obj1, isolate)); ^ -tools/gcmole/gcmole-test.cc:164:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:154:22: note: Call might cause unexpected GC. + so_handle->Method(*SomeClass::StaticCauseGC(obj1, isolate)); + ^ +tools/gcmole/gcmole-test.cc:140:3: note: GC call here. + static Handle StaticCauseGC(Handle obj, Isolate* isolate) { + ^ +tools/gcmole/gcmole-test.cc:164:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:172:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:161:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:172:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:198:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:169:3: note: Call might cause unexpected GC. + Safepoint(); + ^ +tools/gcmole/gcmole-test.cc:19:1: note: GC call here. +void Safepoint() { LocalHeap::Current()->Safepoint(); } +^ +tools/gcmole/gcmole-test.cc:198:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:224:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:195:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:224:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:235:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:221:3: note: Call might cause unexpected GC. + Safepoint(); + ^ +tools/gcmole/gcmole-test.cc:19:1: note: GC call here. +void Safepoint() { LocalHeap::Current()->Safepoint(); } +^ +tools/gcmole/gcmole-test.cc:235:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:242:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:233:3: note: Call might cause unexpected GC. + Safepoint(); + ^ +tools/gcmole/gcmole-test.cc:19:1: note: GC call here. +void Safepoint() { LocalHeap::Current()->Safepoint(); } +^ +tools/gcmole/gcmole-test.cc:242:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:252:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:233:3: note: Call might cause unexpected GC. + Safepoint(); + ^ +tools/gcmole/gcmole-test.cc:19:1: note: GC call here. +void Safepoint() { LocalHeap::Current()->Safepoint(); } +^ +tools/gcmole/gcmole-test.cc:252:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:262:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:250:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:262:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:265:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:260:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:265:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:271:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:260:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:271:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:287:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:269:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:287:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:295:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:285:3: note: Call might cause unexpected GC. + TestGuardedDeadVarAnalysisNested(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:268:1: note: GC call here. +void TestGuardedDeadVarAnalysisNested(JSObject raw_obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:295:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:302:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:293:3: note: Call might cause unexpected GC. + TestGuardedDeadVarAnalysisNested(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:268:1: note: GC call here. +void TestGuardedDeadVarAnalysisNested(JSObject raw_obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:302:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:319:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:300:3: note: Call might cause unexpected GC. + TestGuardedDeadVarAnalysisNested(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:268:1: note: GC call here. +void TestGuardedDeadVarAnalysisNested(JSObject raw_obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:319:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:338:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:317:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:338:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ -tools/gcmole/gcmole-test.cc:349:3: warning: Possibly dead variable. +tools/gcmole/gcmole-test.cc:334:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ +tools/gcmole/gcmole-test.cc:349:3: warning: Possibly stale variable due to GCs. raw_obj.Print(); ^ +tools/gcmole/gcmole-test.cc:345:3: note: Call might cause unexpected GC. + CauseGCRaw(raw_obj, isolate); + ^ +tools/gcmole/gcmole-test.cc:27:1: note: GC call here. +Object CauseGCRaw(Object obj, Isolate* isolate) { +^ 24 warnings generated.