Fix longstanding bug around implicit NPEs and GC, version 2.
The TODO has been there since M (so forever :)):
https://android-review.googlesource.com/c/platform/art/+/122794/13//COMMIT_MSG#13
We hardly see the issue in our tests as we need to have:
1) A GC happening while creating the NPE object.
2) ParallelMoves between the NullCheck and implicit null check operation
that moves references.
The CL piggy backs on the "IsEmittedAtUseSite" flag, to set implicit
null checks with it. The liveness analysis then special cases implicit
null checks to record environment uses at the location of the actual
instruction that will do the implicit null check.
Test: test.py --gcstress
Test: run-libcore-tests --gcstress
bug: 111545159
Change-Id: I3ecea4fe0d7e483e93db83281ca10db47da228c5
diff --git a/compiler/optimizing/ssa_liveness_analysis.cc b/compiler/optimizing/ssa_liveness_analysis.cc
index 2f782f3..62a70d6 100644
--- a/compiler/optimizing/ssa_liveness_analysis.cc
+++ b/compiler/optimizing/ssa_liveness_analysis.cc
@@ -103,9 +103,9 @@
ComputeLiveInAndLiveOutSets();
}
-static void RecursivelyProcessInputs(HInstruction* current,
- HInstruction* actual_user,
- BitVector* live_in) {
+void SsaLivenessAnalysis::RecursivelyProcessInputs(HInstruction* current,
+ HInstruction* actual_user,
+ BitVector* live_in) {
HInputsRef inputs = current->GetInputs();
for (size_t i = 0; i < inputs.size(); ++i) {
HInstruction* input = inputs[i];
@@ -131,11 +131,40 @@
// Check that the inlined input is not a phi. Recursing on loop phis could
// lead to an infinite loop.
DCHECK(!input->IsPhi());
+ DCHECK(!input->HasEnvironment());
RecursivelyProcessInputs(input, actual_user, live_in);
}
}
}
+void SsaLivenessAnalysis::ProcessEnvironment(HInstruction* current,
+ HInstruction* actual_user,
+ BitVector* live_in) {
+ for (HEnvironment* environment = current->GetEnvironment();
+ environment != nullptr;
+ environment = environment->GetParent()) {
+ // Handle environment uses. See statements (b) and (c) of the
+ // SsaLivenessAnalysis.
+ for (size_t i = 0, e = environment->Size(); i < e; ++i) {
+ HInstruction* instruction = environment->GetInstructionAt(i);
+ if (instruction == nullptr) {
+ continue;
+ }
+ bool should_be_live = ShouldBeLiveForEnvironment(current, instruction);
+ // If this environment use does not keep the instruction live, it does not
+ // affect the live range of that instruction.
+ if (should_be_live) {
+ CHECK(instruction->HasSsaIndex()) << instruction->DebugName();
+ live_in->SetBit(instruction->GetSsaIndex());
+ instruction->GetLiveInterval()->AddUse(current,
+ environment,
+ i,
+ actual_user);
+ }
+ }
+ }
+}
+
void SsaLivenessAnalysis::ComputeLiveRanges() {
// Do a post order visit, adding inputs of instructions live in the block where
// that instruction is defined, and killing instructions that are being visited.
@@ -186,32 +215,6 @@
current->GetLiveInterval()->SetFrom(current->GetLifetimePosition());
}
- // Process the environment first, because we know their uses come after
- // or at the same liveness position of inputs.
- for (HEnvironment* environment = current->GetEnvironment();
- environment != nullptr;
- environment = environment->GetParent()) {
- // Handle environment uses. See statements (b) and (c) of the
- // SsaLivenessAnalysis.
- for (size_t i = 0, e = environment->Size(); i < e; ++i) {
- HInstruction* instruction = environment->GetInstructionAt(i);
- if (instruction == nullptr) {
- continue;
- }
- bool should_be_live = ShouldBeLiveForEnvironment(current, instruction);
- // If this environment use does not keep the instruction live, it does not
- // affect the live range of that instruction.
- if (should_be_live) {
- CHECK(instruction->HasSsaIndex()) << instruction->DebugName();
- live_in->SetBit(instruction->GetSsaIndex());
- instruction->GetLiveInterval()->AddUse(current,
- environment,
- i,
- /* actual_user */ nullptr);
- }
- }
- }
-
// Process inputs of instructions.
if (current->IsEmittedAtUseSite()) {
if (kIsDebugBuild) {
@@ -224,6 +227,16 @@
DCHECK(!current->HasEnvironmentUses());
}
} else {
+ // Process the environment first, because we know their uses come after
+ // or at the same liveness position of inputs.
+ ProcessEnvironment(current, current, live_in);
+
+ // Special case implicit null checks. We want their environment uses to be
+ // emitted at the instruction doing the actual null check.
+ HNullCheck* check = current->GetImplicitNullCheck();
+ if (check != nullptr) {
+ ProcessEnvironment(check, current, live_in);
+ }
RecursivelyProcessInputs(current, current, live_in);
}
}