From 0b47c5c886b68d86802bfd685888e71d4847ee41 Mon Sep 17 00:00:00 2001 From: Maksim Ivanov Date: Tue, 21 Jan 2025 02:29:45 +0000 Subject: [PATCH] Fix hang when buggy (unaltered) OK comes after STOP Make sure that we don't proceed with an unmodified file - for this, consistently validate the pass' results on both codepaths (in process_done_futures() and wait_for_first_success()). Besides fixing the hang that was possible when a pass misbehaved, there are a couple of other side effects: * more cases of ERRORs in a pass will be reported - previously only the ones on the first codepath were handled; * max_improvement is enforced stricter now - previously the second codepath allowed a larger reduction to sneak in. This fixes #156. --- cvise/tests/test_test_manager.py | 22 +++++++++ cvise/utils/testing.py | 80 +++++++++++++++++++++----------- 2 files changed, 74 insertions(+), 28 deletions(-) diff --git a/cvise/tests/test_test_manager.py b/cvise/tests/test_test_manager.py index e2bd67d1..ff568333 100644 --- a/cvise/tests/test_test_manager.py +++ b/cvise/tests/test_test_manager.py @@ -2,6 +2,7 @@ import os import pytest import shutil +import time from unittest.mock import patch from cvise.passes.abstract import AbstractPass, PassResult # noqa: E402 @@ -80,6 +81,18 @@ def transform(self, test_case, state, process_event_notifier): return (PassResult.OK, state) +class SlowUnalteredThenStoppingPass(StubPass): + """Attempts to simulate the "STOP event observed before a buggy OK" scenario.""" + + DELAY_SECS = 1 # the larger the number, the higher the chance of catching bugs + + def transform(self, test_case, state, process_event_notifier): + if state == 0: + time.sleep(self.DELAY_SECS) + return (PassResult.OK, state) + return (PassResult.STOP, state) + + def read_file(path): with open(path) as f: return f.read() @@ -203,3 +216,12 @@ def test_halt_on_unaltered(input_file, manager): assert read_file(input_file) == INPUT_DATA # This number of "failed to modify the variant" reports were to be created. assert bug_dir_count() == testing.TestManager.MAX_CRASH_DIRS + 1 + + +def test_halt_on_unaltered_after_stop(input_file, manager): + """Check that we quit after the pass' stop, even if it interleaved with a misbehave.""" + p = SlowUnalteredThenStoppingPass() + manager.run_pass(p) + assert read_file(input_file) == INPUT_DATA + # Whether the misbehave ("failed to modify the variant") is detected depends on timing. + assert bug_dir_count() <= 1 diff --git a/cvise/utils/testing.py b/cvise/utils/testing.py index b185d0c1..99d3829a 100644 --- a/cvise/utils/testing.py +++ b/cvise/utils/testing.py @@ -1,5 +1,6 @@ from concurrent.futures import FIRST_COMPLETED, wait import difflib +from enum import auto, Enum, unique import filecmp import logging import math @@ -32,6 +33,14 @@ MAX_PASS_INCREASEMENT_THRESHOLD = 3 +@unique +class PassCheckingOutcome(Enum): + """Outcome of checking the result of an invocation of a pass.""" + ACCEPT = auto() + IGNORE = auto() + STOP = auto() + + def rmfolder(name): assert 'cvise' in str(name) try: @@ -406,33 +415,14 @@ def process_done_futures(self): raise future.exception() test_env = future.result() - if test_env.success: - if self.max_improvement is not None and test_env.size_improvement > self.max_improvement: - logging.debug(f'Too large improvement: {test_env.size_improvement} B') - else: - # Report bug if transform did not change the file - if filecmp.cmp(self.current_test_case, test_env.test_case_path): - if not self.silent_pass_bug: - if not self.report_pass_bug(test_env, 'pass failed to modify the variant'): - quit_loop = True - else: - quit_loop = True - new_futures.add(future) - else: - self.pass_statistic.add_failure(self.current_pass) - if test_env.result == PassResult.OK: - assert test_env.exitcode - if self.also_interesting is not None and test_env.exitcode == self.also_interesting: - self.save_extra_dir(test_env.test_case_path) - elif test_env.result == PassResult.STOP: - quit_loop = True - elif test_env.result == PassResult.ERROR: - if not self.silent_pass_bug: - self.report_pass_bug(test_env, 'pass error') - quit_loop = True - if not self.no_give_up and test_env.order > self.GIVEUP_CONSTANT: - self.report_pass_bug(test_env, 'pass got stuck') - quit_loop = True + outcome = self.check_pass_result(test_env) + if outcome == PassCheckingOutcome.ACCEPT: + quit_loop = True + new_futures.add(future) + elif outcome == PassCheckingOutcome.IGNORE: + pass + elif outcome == PassCheckingOutcome.STOP: + quit_loop = True else: new_futures.add(future) @@ -446,13 +436,46 @@ def wait_for_first_success(self): for future in self.futures: try: test_env = future.result() - if test_env.success: + outcome = self.check_pass_result(test_env) + if outcome == PassCheckingOutcome.ACCEPT: return test_env # starting with Python 3.11: concurrent.futures.TimeoutError == TimeoutError except (TimeoutError, concurrent.futures.TimeoutError): pass return None + def check_pass_result(self, test_env): + if test_env.success: + if self.max_improvement is not None and test_env.size_improvement > self.max_improvement: + logging.debug(f'Too large improvement: {test_env.size_improvement} B') + return PassCheckingOutcome.IGNORE + if not filecmp.cmp(self.current_test_case, test_env.test_case_path): + return PassCheckingOutcome.ACCEPT + # Report bug if transform did not change the file + if not self.silent_pass_bug: + if not self.report_pass_bug(test_env, 'pass failed to modify the variant'): + return PassCheckingOutcome.STOP + return PassCheckingOutcome.IGNORE + + self.pass_statistic.add_failure(self.current_pass) + if test_env.result == PassResult.OK: + assert test_env.exitcode + if self.also_interesting is not None and test_env.exitcode == self.also_interesting: + self.save_extra_dir(test_env.test_case_path) + elif test_env.result == PassResult.STOP: + return PassCheckingOutcome.STOP + elif test_env.result == PassResult.ERROR: + if not self.silent_pass_bug: + self.report_pass_bug(test_env, 'pass error') + return PassCheckingOutcome.STOP + + if not self.no_give_up and test_env.order > self.GIVEUP_CONSTANT: + if not self.giveup_reported: + self.report_pass_bug(test_env, 'pass got stuck') + self.giveup_reported = True + return PassCheckingOutcome.STOP + return PassCheckingOutcome.IGNORE + @classmethod def terminate_all(cls, pool): pool.stop() @@ -464,6 +487,7 @@ def run_parallel_tests(self): with pebble.ProcessPool(max_workers=self.parallel_tests) as pool: order = 1 self.timeout_count = 0 + self.giveup_reported = False while self.state is not None: # do not create too many states if len(self.futures) >= self.parallel_tests: