Skip to content

Commit

Permalink
Add check for read barriers in Checker
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Stefano Cianciulli committed Jul 4, 2023
1 parent b1c6f57 commit 443c744
Show file tree
Hide file tree
Showing 10 changed files with 265 additions and 43 deletions.
12 changes: 10 additions & 2 deletions compiler/optimizing/optimizing_compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion test/1004-checker-volatile-ref-load/info.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
31 changes: 14 additions & 17 deletions test/1004-checker-volatile-ref-load/src/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,20 @@ public static void main(String[] args) {
/// CHECK: <<Foo:l\d+>> InstanceFieldGet [{{l\d+}}] field_name:Main.foo field_type:Reference loop:<<Loop:B\d+>>
/// CHECK: NullCheck [<<Foo>>] dex_pc:<<PC:\d+>> loop:<<Loop>>
/// CHECK-NEXT: InstanceFieldGet [<<Foo>>] dex_pc:<<PC>> field_name:Foo.bar field_type:Reference loop:<<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<<BaseRegNum:\d+>>, {{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<<BaseRegNum>>]
*/
/// CHECK-IF: readBarrierType('baker')
/// CHECK-NEXT: add w<<BaseRegNum:\d+>>, {{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<<BaseRegNum:\d+>>, {{x\d+}}, #0x8 (8)
/// CHECK-FI:
/// CHECK-NEXT: ldar {{w\d+}}, [x<<BaseRegNum>>]

public void test() {
// Continually check that reading field `foo.bar` throws a
Expand Down
12 changes: 0 additions & 12 deletions test/knownfailures.json
Original file line number Diff line number Diff line change
Expand Up @@ -1333,25 +1333,13 @@
"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"},
"variant": "debug",
"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",
Expand Down
6 changes: 3 additions & 3 deletions tools/checker/README
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 6 additions & 0 deletions tools/checker/file_format/c1visualizer/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down
7 changes: 6 additions & 1 deletion tools/checker/file_format/c1visualizer/struct.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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):
Expand Down
17 changes: 14 additions & 3 deletions tools/checker/match/file.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions tools/checker/match/line.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 443c744

Please sign in to comment.