From 443c7449af68ac2e30ae75871cdf167a6d3e425d Mon Sep 17 00:00:00 2001 From: Stefano Cianciulli Date: Fri, 23 Jun 2023 15:35:33 +0000 Subject: [PATCH] Add check for read barriers in Checker One of the consequences of having the new uffd GC is that the read barrier is completely removed, causing some Checker tests (e.g., 1004-checker-volatile-ref-load) that are explicitly checking for its present to fail. This CL emits the information on what type of read barrier is present in the Control-Flow Graph and adds a readBarrierType('...') check that allows to have different assertions for different read barrier types. Bug: 283780888 Test: atest -a art-run-test-1004-checker-volatile-ref-load Test: art/tools/checker/run_unit_tests.py Change-Id: Ie57839f933b82c5134697368db9143b7b23af599 --- compiler/optimizing/optimizing_compiler.cc | 12 +- test/1004-checker-volatile-ref-load/info.txt | 2 +- .../src/Main.java | 31 ++- test/knownfailures.json | 12 - tools/checker/README | 6 +- .../file_format/c1visualizer/parser.py | 6 + .../file_format/c1visualizer/struct.py | 7 +- tools/checker/match/file.py | 17 +- tools/checker/match/line.py | 1 + tools/checker/match/test.py | 214 +++++++++++++++++- 10 files changed, 265 insertions(+), 43 deletions(-) diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc index e4c9b27236..79bfa42a4c 100644 --- a/compiler/optimizing/optimizing_compiler.cc +++ b/compiler/optimizing/optimizing_compiler.cc @@ -414,11 +414,19 @@ void OptimizingCompiler::DumpInstructionSetFeaturesToCfg() const { std::string isa_string = std::string("isa:") + GetInstructionSetString(features->GetInstructionSet()); std::string features_string = "isa_features:" + features->GetFeatureString(); + std::string read_barrier_type = "none"; + if (gUseReadBarrier) { + if (art::kUseBakerReadBarrier) + read_barrier_type = "baker"; + else if (art::kUseTableLookupReadBarrier) + read_barrier_type = "tablelookup"; + } + std::string read_barrier_string = ART_FORMAT("read_barrier_type:{}", read_barrier_type); // It is assumed that visualizer_output_ is empty when calling this function, hence the fake // compilation block containing the ISA features will be printed at the beginning of the .cfg // file. - *visualizer_output_ - << HGraphVisualizer::InsertMetaDataAsCompilationBlock(isa_string + ' ' + features_string); + *visualizer_output_ << HGraphVisualizer::InsertMetaDataAsCompilationBlock( + isa_string + ' ' + features_string + ' ' + read_barrier_string); } bool OptimizingCompiler::CanCompileMethod([[maybe_unused]] uint32_t method_idx, diff --git a/test/1004-checker-volatile-ref-load/info.txt b/test/1004-checker-volatile-ref-load/info.txt index 1ba4a5225d..80ef530621 100644 --- a/test/1004-checker-volatile-ref-load/info.txt +++ b/test/1004-checker-volatile-ref-load/info.txt @@ -2,5 +2,5 @@ Regression test for b/140507091: Check that an explicit null check is emitted for a volatile field load on ARM64 at the beginning of the Baker read barrier thunk, so that a null holder object is properly caught (and throws a NullPointerException as expected), instead of -trigerring a SIGSEGV, when the Concurrent Copying GC is in the marking +triggering a SIGSEGV, when the Concurrent Copying GC is in the marking phase. diff --git a/test/1004-checker-volatile-ref-load/src/Main.java b/test/1004-checker-volatile-ref-load/src/Main.java index 6455e3ec3f..c3d55ce535 100644 --- a/test/1004-checker-volatile-ref-load/src/Main.java +++ b/test/1004-checker-volatile-ref-load/src/Main.java @@ -36,23 +36,20 @@ public static void main(String[] args) { /// CHECK: <> InstanceFieldGet [{{l\d+}}] field_name:Main.foo field_type:Reference loop:<> /// CHECK: NullCheck [<>] dex_pc:<> loop:<> /// CHECK-NEXT: InstanceFieldGet [<>] dex_pc:<> field_name:Foo.bar field_type:Reference loop:<> - /* The following following Checker assertions are only valid when the compiler is emitting Baker - read barriers, i.e. when ART is using the Concurrent Copying (CC) garbage collector. - - TODO(b/283392413, b/283780888): Re-enable the following Checker assertions (by replacing the - double forward slash comments with triple forward slash ones) when b/283780888 is resolved. - - // CHECK-NEXT: add w<>, {{w\d+}}, #0x8 (8) - // CHECK-NEXT: adr lr, #+0x{{c|10}} - // The following instruction (generated by - // `art::arm64::CodeGeneratorARM64::EmitBakerReadBarrierCbnz`) checks the - // Marking Register (X20) and goes into the Baker read barrier thunk if MR is - // not null. The null offset (#+0x0) in the CBNZ instruction is a placeholder - // for the offset to the Baker read barrier thunk (which is not yet set when - // the CFG output is emitted). - // CHECK-NEXT: cbnz x20, #+0x0 - // CHECK-NEXT: ldar {{w\d+}}, [x<>] - */ + /// CHECK-IF: readBarrierType('baker') + /// CHECK-NEXT: add w<>, {{w\d+}}, #0x8 (8) + /// CHECK-NEXT: adr lr, #+0x{{c|10}} + // The following instruction (generated by + // `art::arm64::CodeGeneratorARM64::EmitBakerReadBarrierCbnz`) checks the + // Marking Register (X20) and goes into the Baker read barrier thunk if MR is + // not null. The null offset (#+0x0) in the CBNZ instruction is a placeholder + // for the offset to the Baker read barrier thunk (which is not yet set when + // the CFG output is emitted). + /// CHECK-NEXT: cbnz x20, #+0x0 + /// CHECK-ELSE: + /// CHECK-NEXT: add x<>, {{x\d+}}, #0x8 (8) + /// CHECK-FI: + /// CHECK-NEXT: ldar {{w\d+}}, [x<>] public void test() { // Continually check that reading field `foo.bar` throws a diff --git a/test/knownfailures.json b/test/knownfailures.json index adf201c738..11ca93ed96 100644 --- a/test/knownfailures.json +++ b/test/knownfailures.json @@ -1333,12 +1333,6 @@ "variant": "baseline", "description": [ "Working as intended tests that don't pass with baseline." ] }, - { - "tests": ["1004-checker-volatile-ref-load"], - "env_vars": {"ART_USE_READ_BARRIER": "false"}, - "bug": "b/140507091", - "description": ["Test containing Checker assertions expecting Baker read barriers."] - }, { "tests": ["2040-huge-native-alloc"], "env_vars": {"ART_USE_READ_BARRIER": "false"}, @@ -1346,12 +1340,6 @@ "bug": "b/242181443", "description": ["Test fails due to delay delebrately added in the userfaultfd GC between marking and compaction."] }, - { - "tests": ["1004-checker-volatile-ref-load"], - "env_vars": {"ART_READ_BARRIER_TYPE": "TABLELOOKUP"}, - "bug": "b/140507091", - "description": ["Test containing Checker assertions expecting Baker read barriers."] - }, { "tests": ["689-zygote-jit-deopt"], "variant": "gcstress", diff --git a/tools/checker/README b/tools/checker/README index b04b0d8253..e01f0d0750 100644 --- a/tools/checker/README +++ b/tools/checker/README @@ -17,15 +17,15 @@ Matching of check lines is carried out in the order of appearance in the source file. There are five types of check lines. Branching instructions are also supported and documented later in this file. - CHECK: Must match an output line which appears in the output group - later than lines matched against any preceeding checks. Output + later than lines matched against any preceding checks. Output lines must therefore match the check lines in the same order. These are referred to as "in-order" checks in the code. - CHECK-DAG: Must match an output line which appears in the output group - later than lines matched against any preceeding in-order checks. + later than lines matched against any preceding in-order checks. In other words, the order of output lines does not matter between consecutive DAG checks. - CHECK-NOT: Must not match any output line which appears in the output group - later than lines matched against any preceeding checks and + later than lines matched against any preceding checks and earlier than lines matched against any subsequent checks. Surrounding non-negative checks (or boundaries of the group) therefore create a scope within which the statement is verified. diff --git a/tools/checker/file_format/c1visualizer/parser.py b/tools/checker/file_format/c1visualizer/parser.py index 55efbd74ea..26087cfdf0 100644 --- a/tools/checker/file_format/c1visualizer/parser.py +++ b/tools/checker/file_format/c1visualizer/parser.py @@ -77,6 +77,12 @@ def _parse_c1_line(c1_file, line, line_no, state, filename): features[feature_name] = is_enabled c1_file.set_isa_features(features) + + # Check what type of read barrier is used + match = re.search(r"read_barrier_type:(\w+)", method_name) + if match: + c1_file.set_read_barrier_type(match.group(1)) + else: state.last_method_name = method_name elif line == "end_compilation": diff --git a/tools/checker/file_format/c1visualizer/struct.py b/tools/checker/file_format/c1visualizer/struct.py index 9428a0e5f6..7f3032016c 100644 --- a/tools/checker/file_format/c1visualizer/struct.py +++ b/tools/checker/file_format/c1visualizer/struct.py @@ -25,10 +25,14 @@ def __init__(self, filename): self.full_file_name = filename self.passes = [] self.instruction_set_features = ImmutableDict() + self.read_barrier_type = "none" def set_isa_features(self, features): self.instruction_set_features = ImmutableDict(features) + def set_read_barrier_type(self, read_barrier_type): + self.read_barrier_type = read_barrier_type + def add_pass(self, new_pass): self.passes.append(new_pass) @@ -41,7 +45,8 @@ def find_pass(self, name): def __eq__(self, other): return (isinstance(other, self.__class__) and self.passes == other.passes - and self.instruction_set_features == other.instruction_set_features) + and self.instruction_set_features == other.instruction_set_features + and self.read_barrier_type == other.read_barrier_type) class C1visualizerPass(PrintableMixin): diff --git a/tools/checker/match/file.py b/tools/checker/match/file.py index a573ccccdc..33bb9d1abe 100644 --- a/tools/checker/match/file.py +++ b/tools/checker/match/file.py @@ -318,14 +318,21 @@ def handle(self, statement): self.last_variant = variant -def match_test_case(test_case, c1_pass, instruction_set_features): +def match_test_case( + test_case, + c1_pass, + instruction_set_features, + read_barrier_type): """ Runs a test case against a C1visualizer graph dump. Raises MatchFailedException when a statement cannot be satisfied. """ assert test_case.name == c1_pass.name - initial_variables = {"ISA_FEATURES": instruction_set_features} + initial_variables = { + "ISA_FEATURES": instruction_set_features, + "READ_BARRIER_TYPE": read_barrier_type + } state = ExecutionState(c1_pass, initial_variables) test_statements = test_case.statements + [None] for statement in test_statements: @@ -351,7 +358,11 @@ def match_files(checker_file, c1_file, target_arch, debuggable_mode, print_cfg): Logger.start_test(test_case.name) try: - match_test_case(test_case, c1_pass, c1_file.instruction_set_features) + match_test_case( + test_case, + c1_pass, + c1_file.instruction_set_features, + c1_file.read_barrier_type) Logger.test_passed() except MatchFailedException as e: line_no = c1_pass.start_line_no + e.line_no diff --git a/tools/checker/match/line.py b/tools/checker/match/line.py index 16144e1050..987ae61186 100644 --- a/tools/checker/match/line.py +++ b/tools/checker/match/line.py @@ -129,6 +129,7 @@ def evaluate_line(checker_line, variables): assert checker_line.is_eval_content_statement() # Required for eval. hasIsaFeature = lambda feature: variables["ISA_FEATURES"].get(feature, False) + readBarrierType = lambda barrier_type: variables["READ_BARRIER_TYPE"] == barrier_type eval_string = "".join(get_eval_text(expr, variables, checker_line) for expr in checker_line.expressions) diff --git a/tools/checker/match/test.py b/tools/checker/match/test.py index adebd6da61..a2bb6f4d19 100644 --- a/tools/checker/match/test.py +++ b/tools/checker/match/test.py @@ -101,7 +101,10 @@ def test_VariableContentEscaped(self): class MatchFiles_Test(unittest.TestCase): - def assertMatches(self, checker_string, c1_string, isa=None, instruction_set_features=None): + def assertMatches(self, checker_string, c1_string, + isa=None, + instruction_set_features=None, + read_barrier_type="none"): checker_string = \ """ /// CHECK-START: MyMethod MyPass @@ -118,6 +121,10 @@ def assertMatches(self, checker_string, c1_string, isa=None, instruction_set_fea name if present else "-" + name for name, present in instruction_set_features.items()) meta_data += "isa_features:" + joined_features + if meta_data: + meta_data += " " + meta_data += f"read_barrier_type:{read_barrier_type}" + meta_data_string = "" if meta_data: meta_data_string = \ @@ -145,11 +152,17 @@ def assertMatches(self, checker_string, c1_string, isa=None, instruction_set_fea c1_file = parse_c1_visualizer_stream("", io.StringIO(c1_string)) assert len(checker_file.test_cases) == 1 assert len(c1_file.passes) == 1 - match_test_case(checker_file.test_cases[0], c1_file.passes[0], c1_file.instruction_set_features) + match_test_case(checker_file.test_cases[0], + c1_file.passes[0], + c1_file.instruction_set_features, + c1_file.read_barrier_type) - def assertDoesNotMatch(self, checker_string, c1_string, isa=None, instruction_set_features=None): + def assertDoesNotMatch(self, checker_string, c1_string, + isa=None, + instruction_set_features=None, + read_barrier_type="none"): with self.assertRaises(MatchFailedException): - self.assertMatches(checker_string, c1_string, isa, instruction_set_features) + self.assertMatches(checker_string, c1_string, isa, instruction_set_features, read_barrier_type) def assertBadStructure(self, checker_string, c1_string): with self.assertRaises(BadStructureException): @@ -1009,3 +1022,196 @@ def test_hasIsaFeature(self): "some_isa", ImmutableDict({"feature1": False, "feature2": True}) ) + + def test_readBarrierType(self): + # CheckEval assertions with no read barrier + self.assertMatches( + """ + /// CHECK-EVAL: readBarrierType('none') + """, + """ + foo + """, + None, + read_barrier_type="none" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('none') + """, + """ + foo + """, + None, + read_barrier_type="baker" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('none') + """, + """ + foo + """, + None, + read_barrier_type="tablelookup" + ) + + # CheckEval assertions with "baker" read barrier + self.assertMatches( + """ + /// CHECK-EVAL: readBarrierType('baker') + """, + """ + foo + """, + None, + read_barrier_type="baker" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('baker') + """, + """ + foo + """, + None, + read_barrier_type="none" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('baker') + """, + """ + foo + """, + None, + read_barrier_type="tablelookup" + ) + + # CheckEval assertions with "tablelookup" read barrier + self.assertMatches( + """ + /// CHECK-EVAL: readBarrierType('tablelookup') + """, + """ + foo + """, + None, + read_barrier_type="tablelookup" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('tablelookup') + """, + """ + foo + """, + None, + read_barrier_type="none" + ) + self.assertDoesNotMatch( + """ + /// CHECK-EVAL: readBarrierType('tablelookup') + """, + """ + foo + """, + None, + read_barrier_type="baker" + ) + + # CheckIf assertions with no read barrier + self.assertMatches( + """ + /// CHECK-IF: readBarrierType('none') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar1 + """, + None, + read_barrier_type="none" + ) + self.assertMatches( + """ + /// CHECK-IF: not readBarrierType('none') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar2 + """, + None, + read_barrier_type="none" + ) + + # CheckIf assertions with 'baker' read barrier + self.assertMatches( + """ + /// CHECK-IF: readBarrierType('baker') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar1 + """, + None, + read_barrier_type="baker" + ) + self.assertMatches( + """ + /// CHECK-IF: not readBarrierType('baker') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar2 + """, + None, + read_barrier_type="baker" + ) + + # CheckIf assertions with 'tablelookup' read barrier + self.assertMatches( + """ + /// CHECK-IF: readBarrierType('tablelookup') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar1 + """, + None, + read_barrier_type="tablelookup" + ) + self.assertMatches( + """ + /// CHECK-IF: not readBarrierType('tablelookup') + /// CHECK: bar1 + /// CHECK-ELSE: + /// CHECK: bar2 + /// CHECK-FI: + """, + """ + foo + bar2 + """, + None, + read_barrier_type="tablelookup" + )