Emit local changes accurately at all instruction points. Bug: 63243012 Change-Id: I874308618da6366b79b68e5b8eb0c0e1b08aa447
diff --git a/src/main/java/com/android/tools/r8/graph/DexCode.java b/src/main/java/com/android/tools/r8/graph/DexCode.java index 9b62842..3535bfa 100644 --- a/src/main/java/com/android/tools/r8/graph/DexCode.java +++ b/src/main/java/com/android/tools/r8/graph/DexCode.java
@@ -184,14 +184,12 @@ } int instructionNumber = 0; for (Instruction insn : instructions) { - StringUtils.appendLeftPadded(builder, Integer.toString(instructionNumber++), 5); - builder.append(": ") - .append(insn.toString(naming)) - .append('\n'); while (debugInfo != null && debugInfo.address == insn.getOffset()) { builder.append(" ").append(debugInfo).append("\n"); debugInfo = debugInfoIterator.hasNext() ? debugInfoIterator.next() : null; } + StringUtils.appendLeftPadded(builder, Integer.toString(instructionNumber++), 5); + builder.append(": ").append(insn.toString(naming)).append('\n'); } if (debugInfoIterator.hasNext()) { throw new Unreachable("Could not print all debug information.");
diff --git a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java index 2342b3b..c468613 100644 --- a/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java +++ b/src/main/java/com/android/tools/r8/graph/DexDebugEventBuilder.java
@@ -5,14 +5,18 @@ import com.android.tools.r8.dex.Constants; import com.android.tools.r8.graph.DexDebugEvent.StartLocal; +import com.android.tools.r8.ir.code.Argument; +import com.android.tools.r8.ir.code.DebugLocalsChange; import com.android.tools.r8.ir.code.DebugPosition; -import com.google.common.collect.ImmutableMap; +import com.android.tools.r8.ir.code.Instruction; +import it.unimi.dsi.fastutil.ints.Int2ReferenceAVLTreeMap; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMap.Entry; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMaps; +import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap; +import it.unimi.dsi.fastutil.ints.Int2ReferenceSortedMap; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.SortedSet; -import java.util.TreeSet; /** * Builder for constructing a list of debug events suitable for DexDebugInfo. @@ -25,135 +29,226 @@ private static final int NO_PC_INFO = -1; private static final int NO_LINE_INFO = -1; - private static class PositionState { - int pc = NO_PC_INFO; - int line = NO_LINE_INFO; - DexString file = null; - ImmutableMap<Integer, DebugLocalInfo> locals = null; - } - - private final DexMethod method; - - private final DexItemFactory dexItemFactory; - - // Previous and current position info to delay emitting position changes. - private final PositionState previous; - private final PositionState current; + private final DexEncodedMethod method; + private final DexItemFactory factory; // In order list of non-this argument locals. - private int lastArgumentRegister = -1; - private final List<DebugLocalInfo> arguments; - - // Mapping from register to local for currently open/visible locals. - private final Map<Integer, DebugLocalInfo> openLocals = new HashMap<>(); + private ArrayList<DebugLocalInfo> arguments; // Mapping from register to the last known local in that register (See DBG_RESTART_LOCAL). - private final Map<Integer, DebugLocalInfo> lastKnownLocals = new HashMap<>(); + private Int2ReferenceMap<DebugLocalInfo> lastKnownLocals; - // Flushed events. + // Mapping from register to local for currently open/visible locals. + private Int2ReferenceMap<DebugLocalInfo> pendingLocals = null; + + // Conservative pending-state of locals to avoid some equality checks on locals. + // pendingLocalChanges == true ==> localsEqual(emittedLocals, pendingLocals). + private boolean pendingLocalChanges = false; + + // State of pc, line, file and locals in the emitted event stream. + private int emittedPc = NO_PC_INFO; + private int emittedLine = NO_LINE_INFO; + private DexString emittedFile = null; + private Int2ReferenceMap<DebugLocalInfo> emittedLocals; + + // If lastMoveInstructionPc != NO_PC_INFO, then the last pc-advancing instruction was a + // move-exception at lastMoveInstructionPc. This is needed to maintain the art/dx specific + // behaviour that the move-exception pc is associated with the catch-declaration line. + // See debug.ExceptionTest.testStepOnCatch(). + private int lastMoveInstructionPc = NO_PC_INFO; + + // Emitted events. private final List<DexDebugEvent> events = new ArrayList<>(); + // Initial known line for the method. private int startLine = NO_LINE_INFO; - public DexDebugEventBuilder(DexMethod method, DexItemFactory dexItemFactory) { + public DexDebugEventBuilder(DexEncodedMethod method, DexItemFactory factory) { this.method = method; - this.dexItemFactory = dexItemFactory; - arguments = new ArrayList<>(method.proto.parameters.values.length); - current = new PositionState(); - previous = new PositionState(); + this.factory = factory; } - public void startArgument(int register, DebugLocalInfo local, boolean isThis) { - // Verify that arguments are started in order. - assert register > lastArgumentRegister; - lastArgumentRegister = register; - // If this is an actual argument record it for header information. - if (!isThis) { - arguments.add(local); - } - // If the argument does not have a parametrized type, implicitly open it. - if (local != null && local.signature == null) { - openLocals.put(register, local); - lastKnownLocals.put(register, local); - } + // Public method for the DebugStripper. + public void setPosition(int pc, int line) { + emitDebugPosition(pc, line, null); } - /** Emits a positions entry if the position has changed and associates any local changes. */ - public void setPosition(int pc, DebugPosition position) { - setPosition(pc, position.line, position.file, position.getLocals()); - } - - public void setPosition( - int pc, int line, DexString file, ImmutableMap<Integer, DebugLocalInfo> locals) { - // If we have a pending position and the next differs from it flush the pending one. - if (previous.pc != current.pc && positionChanged(current, pc, line, file)) { - flushCurrentPosition(); + /** Add events at pc for instruction. */ + public void add(int pc, Instruction instruction) { + // Initialize locals state on block entry. + if (instruction.getBlock().entry() == instruction) { + updateBlockEntry(instruction); } - current.pc = pc; - current.line = line; - current.file = file; - current.locals = locals; - } + assert pendingLocals != null; - private void flushCurrentPosition() { - // If this is the first emitted possition, initialize previous state: start-line is forced to be - // the first actual line, in-effect, causing the first position to be a zero-delta line change. - if (startLine == NO_LINE_INFO) { - assert events.isEmpty(); - assert previous.pc == NO_PC_INFO; - assert previous.line == NO_LINE_INFO; - startLine = current.line; - previous.line = current.line; - previous.pc = 0; + // If this is a position emit and exit as it always emits events. + if (instruction.isDebugPosition()) { + emitDebugPosition(pc, instruction.asDebugPosition()); + return; } - // Emit position change (which might result in additional advancement events). - emitAdvancementEvents(); - // Emit local changes for new current position (they relate to the already emitted position). - // Locals are either defined on all positions or on none. - assert current.locals != null || previous.locals == null; - if (current.locals != null) { - emitLocalChanges(); + + if (instruction.isArgument()) { + startArgument(instruction.asArgument()); + } else if (instruction.isDebugLocalsChange()) { + updateLocals(instruction.asDebugLocalsChange()); + } else if (instruction.getBlock().exit() == instruction) { + // If this is the end of the block clear out the pending state and exit. + pendingLocals = null; + pendingLocalChanges = false; + return; + } else if (instruction.isMoveException()) { + lastMoveInstructionPc = pc; + } else { + // For non-exit / pc-advancing instructions emit any pending changes. + emitLocalChanges(pc); } } /** Build the resulting DexDebugInfo object. */ public DexDebugInfo build() { - if (previous.pc != current.pc) { - flushCurrentPosition(); - } + assert pendingLocals == null; + assert !pendingLocalChanges; if (startLine == NO_LINE_INFO) { return null; } - DexString[] params = new DexString[method.proto.parameters.values.length]; - assert arguments.isEmpty() || params.length == arguments.size(); - for (int i = 0; i < arguments.size(); i++) { - DebugLocalInfo local = arguments.get(i); - params[i] = (local == null || local.signature != null) ? null : local.name; + DexString[] params = new DexString[method.method.proto.parameters.values.length]; + if (arguments != null) { + assert params.length == arguments.size(); + for (int i = 0; i < arguments.size(); i++) { + DebugLocalInfo local = arguments.get(i); + params[i] = (local == null || local.signature != null) ? null : local.name; + } } return new DexDebugInfo(startLine, params, events.toArray(new DexDebugEvent[events.size()])); } - private static boolean positionChanged( - PositionState current, int nextPc, int nextLine, DexString nextFile) { - return nextPc != current.pc && (nextLine != current.line || nextFile != current.file); + private void updateBlockEntry(Instruction instruction) { + assert pendingLocals == null; + assert !pendingLocalChanges; + Int2ReferenceMap<DebugLocalInfo> locals = instruction.getBlock().getLocalsAtEntry(); + if (locals == null) { + pendingLocals = Int2ReferenceMaps.emptyMap(); + } else { + pendingLocals = new Int2ReferenceOpenHashMap<>(locals); + pendingLocalChanges = true; + } + if (emittedLocals == null) { + initialize(locals); + } } - private void emitAdvancementEvents() { - int pcDelta = current.pc - previous.pc; - int lineDelta = current.line - previous.line; + private void initialize(Int2ReferenceMap<DebugLocalInfo> locals) { + assert arguments == null; + assert emittedLocals == null; + assert lastKnownLocals == null; + assert startLine == NO_LINE_INFO; + if (locals == null) { + emittedLocals = Int2ReferenceMaps.emptyMap(); + lastKnownLocals = Int2ReferenceMaps.emptyMap(); + return; + } + // Implicitly open all unparameterized arguments. + emittedLocals = new Int2ReferenceOpenHashMap<>(); + for (Entry<DebugLocalInfo> entry : locals.int2ReferenceEntrySet()) { + if (entry.getValue().signature == null) { + emittedLocals.put(entry.getIntKey(), entry.getValue()); + } + } + lastKnownLocals = new Int2ReferenceOpenHashMap<>(emittedLocals); + } + + private void startArgument(Argument argument) { + if (arguments == null) { + arguments = new ArrayList<>(method.method.proto.parameters.values.length); + } + if (!argument.outValue().isThis()) { + arguments.add(argument.getLocalInfo()); + } + } + + private void updateLocals(DebugLocalsChange change) { + pendingLocalChanges = true; + for (Entry<DebugLocalInfo> end : change.getEnding().int2ReferenceEntrySet()) { + assert pendingLocals.get(end.getIntKey()) == end.getValue(); + pendingLocals.remove(end.getIntKey()); + } + for (Entry<DebugLocalInfo> start : change.getStarting().int2ReferenceEntrySet()) { + assert !pendingLocals.containsKey(start.getIntKey()); + pendingLocals.put(start.getIntKey(), start.getValue()); + } + } + + private boolean localsChanged() { + if (!pendingLocalChanges) { + return false; + } + pendingLocalChanges = !localsEqual(emittedLocals, pendingLocals); + return pendingLocalChanges; + } + + private void emitDebugPosition(int pc, DebugPosition position) { + emitDebugPosition(pc, position.line, position.file); + } + + private void emitDebugPosition(int pc, int line, DexString file) { + int emitPc = lastMoveInstructionPc != NO_PC_INFO ? lastMoveInstructionPc : pc; + lastMoveInstructionPc = NO_PC_INFO; + // The position requires a pc change event and possible events for line, file and local changes. + // Verify that we do not ever produce two subsequent positions at the same pc. + assert emittedPc != emitPc; + if (startLine == NO_LINE_INFO) { + assert emittedLine == NO_LINE_INFO; + startLine = line; + emittedLine = line; + } + emitAdvancementEvents(emittedPc, emittedLine, emittedFile, emitPc, line, file, events, factory); + emittedPc = emitPc; + emittedLine = line; + emittedFile = file; + if (localsChanged()) { + emitLocalChangeEvents(emittedLocals, pendingLocals, lastKnownLocals, events, factory); + assert localsEqual(emittedLocals, pendingLocals); + } + pendingLocalChanges = false; + } + + private void emitLocalChanges(int pc) { + // If pc advanced since the locals changed and locals indeed have changed, emit the changes. + if (localsChanged()) { + int emitPc = lastMoveInstructionPc != NO_PC_INFO ? lastMoveInstructionPc : pc; + lastMoveInstructionPc = NO_PC_INFO; + emitAdvancementEvents( + emittedPc, emittedLine, emittedFile, emitPc, emittedLine, emittedFile, events, factory); + emittedPc = emitPc; + emitLocalChangeEvents(emittedLocals, pendingLocals, lastKnownLocals, events, factory); + pendingLocalChanges = false; + assert localsEqual(emittedLocals, pendingLocals); + } + } + + private static void emitAdvancementEvents( + int previousPc, + int previousLine, + DexString previousFile, + int nextPc, + int nextLine, + DexString nextFile, + List<DexDebugEvent> events, + DexItemFactory factory) { + int pcDelta = previousPc == NO_PC_INFO ? nextPc : nextPc - previousPc; + int lineDelta = nextLine == NO_LINE_INFO ? 0 : nextLine - previousLine; assert pcDelta >= 0; - if (current.file != previous.file) { - assert current.file == null || !current.file.equals(previous.file); - events.add(dexItemFactory.createSetFile(current.file)); + if (nextFile != previousFile) { + events.add(factory.createSetFile(nextFile)); } if (lineDelta < Constants.DBG_LINE_BASE || lineDelta - Constants.DBG_LINE_BASE >= Constants.DBG_LINE_RANGE) { - events.add(dexItemFactory.createAdvanceLine(lineDelta)); + events.add(factory.createAdvanceLine(lineDelta)); // TODO(herhut): To be super clever, encode only the part that is above limit. lineDelta = 0; } if (pcDelta >= Constants.DBG_ADDRESS_RANGE) { - events.add(dexItemFactory.createAdvancePC(pcDelta)); + events.add(factory.createAdvancePC(pcDelta)); pcDelta = 0; } // TODO(herhut): Maybe only write this one if needed (would differ from DEX). @@ -161,37 +256,65 @@ 0x0a + (lineDelta - Constants.DBG_LINE_BASE) + Constants.DBG_LINE_RANGE * pcDelta; assert specialOpcode >= 0x0a; assert specialOpcode <= 0xff; - events.add(dexItemFactory.createDefault(specialOpcode)); - previous.pc = current.pc; - previous.line = current.line; - previous.file = current.file; + events.add(factory.createDefault(specialOpcode)); } - private void emitLocalChanges() { - if (previous.locals == current.locals) { - return; - } - SortedSet<Integer> currentRegisters = new TreeSet<>(openLocals.keySet()); - SortedSet<Integer> positionRegisters = new TreeSet<>(current.locals.keySet()); - for (Integer register : currentRegisters) { - if (!positionRegisters.contains(register)) { - events.add(dexItemFactory.createEndLocal(register)); - openLocals.remove(register); + public static void emitLocalChangeEvents( + Int2ReferenceMap<DebugLocalInfo> previousLocals, + Int2ReferenceMap<DebugLocalInfo> nextLocals, + Int2ReferenceMap<DebugLocalInfo> lastKnownLocals, + List<DexDebugEvent> events, + DexItemFactory factory) { + Int2ReferenceSortedMap<DebugLocalInfo> ending = new Int2ReferenceAVLTreeMap<>(); + Int2ReferenceSortedMap<DebugLocalInfo> starting = new Int2ReferenceAVLTreeMap<>(); + for (Entry<DebugLocalInfo> entry : previousLocals.int2ReferenceEntrySet()) { + int register = entry.getIntKey(); + DebugLocalInfo local = entry.getValue(); + if (nextLocals.get(register) != local) { + ending.put(register, local); } } - for (Integer register : positionRegisters) { - DebugLocalInfo positionLocal = current.locals.get(register); - DebugLocalInfo currentLocal = openLocals.get(register); - if (currentLocal != positionLocal) { - openLocals.put(register, positionLocal); - if (currentLocal == null && lastKnownLocals.get(register) == positionLocal) { - events.add(dexItemFactory.createRestartLocal(register)); - } else { - events.add(new StartLocal(register, positionLocal)); - lastKnownLocals.put(register, positionLocal); - } + for (Entry<DebugLocalInfo> entry : nextLocals.int2ReferenceEntrySet()) { + int register = entry.getIntKey(); + DebugLocalInfo local = entry.getValue(); + if (previousLocals.get(register) != local) { + starting.put(register, local); } } - previous.locals = current.locals; + assert !ending.isEmpty() || !starting.isEmpty(); + for (Entry<DebugLocalInfo> end : ending.int2ReferenceEntrySet()) { + int register = end.getIntKey(); + if (!starting.containsKey(register)) { + previousLocals.remove(register); + events.add(factory.createEndLocal(register)); + } + } + for (Entry<DebugLocalInfo> start : starting.int2ReferenceEntrySet()) { + int register = start.getIntKey(); + DebugLocalInfo local = start.getValue(); + previousLocals.put(register, local); + if (lastKnownLocals.get(register) == local) { + events.add(factory.createRestartLocal(register)); + } else { + events.add(new StartLocal(register, local)); + lastKnownLocals.put(register, local); + } + } + } + + private static boolean localsEqual( + Int2ReferenceMap<DebugLocalInfo> locals1, Int2ReferenceMap<DebugLocalInfo> locals2) { + if (locals1 == locals2) { + return true; + } + if (locals1.size() != locals2.size()) { + return false; + } + for (Int2ReferenceMap.Entry<DebugLocalInfo> entry : locals1.int2ReferenceEntrySet()) { + if (locals2.get(entry.getIntKey()) != entry.getValue()) { + return false; + } + } + return true; } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java index b9508b5..46149e7 100644 --- a/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java +++ b/src/main/java/com/android/tools/r8/ir/code/BasicBlock.java
@@ -4,13 +4,16 @@ package com.android.tools.r8.ir.code; import com.android.tools.r8.errors.CompilationError; +import com.android.tools.r8.graph.DebugLocalInfo; import com.android.tools.r8.graph.DexType; import com.android.tools.r8.ir.conversion.DexBuilder; import com.android.tools.r8.ir.conversion.IRBuilder; import com.android.tools.r8.utils.CfgPrinter; import com.android.tools.r8.utils.ListUtils; import com.android.tools.r8.utils.StringUtils; +import com.android.tools.r8.utils.StringUtils.BraceType; import com.google.common.collect.ImmutableList; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -31,6 +34,16 @@ */ public class BasicBlock { + private Int2ReferenceMap<DebugLocalInfo> localsAtEntry; + + public void setLocalsAtEntry(Int2ReferenceMap<DebugLocalInfo> localsAtEntry) { + this.localsAtEntry = localsAtEntry; + } + + public Int2ReferenceMap<DebugLocalInfo> getLocalsAtEntry() { + return localsAtEntry; + } + public enum ThrowingInfo { NO_THROW, CAN_THROW } @@ -770,6 +783,11 @@ } else { builder.append("no phis\n"); } + if (localsAtEntry != null) { + builder.append("locals: "); + StringUtils.append(builder, localsAtEntry.int2ReferenceEntrySet(), ", ", BraceType.NONE); + builder.append("\n"); + } for (Instruction instruction : instructions) { StringUtils.appendLeftPadded(builder, Integer.toString(instruction.getNumber()), 6); builder.append(": ");
diff --git a/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java b/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java index 6c5fbb7..0da5b43 100644 --- a/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java +++ b/src/main/java/com/android/tools/r8/ir/code/ConstNumber.java
@@ -90,7 +90,7 @@ @Override public void buildDex(DexBuilder builder) { if (!dest().needsRegister()) { - builder.addFallThrough(this); + builder.addNop(this); return; }
diff --git a/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java new file mode 100644 index 0000000..a08b044 --- /dev/null +++ b/src/main/java/com/android/tools/r8/ir/code/DebugLocalsChange.java
@@ -0,0 +1,84 @@ +// Copyright (c) 2017, the R8 project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +package com.android.tools.r8.ir.code; + +import com.android.tools.r8.errors.Unreachable; +import com.android.tools.r8.graph.DebugLocalInfo; +import com.android.tools.r8.ir.conversion.DexBuilder; +import com.android.tools.r8.utils.InternalOptions; +import com.android.tools.r8.utils.StringUtils; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; + +public class DebugLocalsChange extends Instruction { + + private final Int2ReferenceMap<DebugLocalInfo> ending; + private final Int2ReferenceMap<DebugLocalInfo> starting; + + public DebugLocalsChange( + Int2ReferenceMap<DebugLocalInfo> ending, Int2ReferenceMap<DebugLocalInfo> starting) { + super(null); + this.ending = ending; + this.starting = starting; + } + + public Int2ReferenceMap<DebugLocalInfo> getEnding() { + return ending; + } + + public Int2ReferenceMap<DebugLocalInfo> getStarting() { + return starting; + } + + @Override + public boolean isDebugLocalsChange() { + return true; + } + + @Override + public DebugLocalsChange asDebugLocalsChange() { + return this; + } + + @Override + public void buildDex(DexBuilder builder) { + builder.addNop(this); + } + + @Override + public boolean identicalNonValueParts(Instruction other) { + assert other.isDebugLocalsChange(); + return false; + } + + @Override + public int compareNonValueParts(Instruction other) { + assert other.isDebugLocalsChange(); + return 0; + } + + @Override + public int maxInValueRegister() { + throw new Unreachable(); + } + + @Override + public int maxOutValueRegister() { + throw new Unreachable(); + } + + @Override + public boolean canBeDeadCode(IRCode code, InternalOptions options) { + return false; + } + + @Override + public String toString() { + StringBuilder builder = new StringBuilder(super.toString()); + builder.append("ending: "); + StringUtils.append(builder, ending.int2ReferenceEntrySet()); + builder.append(", starting: "); + StringUtils.append(builder, starting.int2ReferenceEntrySet()); + return builder.toString(); + } +}
diff --git a/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java b/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java index e7917c6..ee6b65f 100644 --- a/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java +++ b/src/main/java/com/android/tools/r8/ir/code/DebugPosition.java
@@ -4,18 +4,14 @@ package com.android.tools.r8.ir.code; import com.android.tools.r8.errors.Unreachable; -import com.android.tools.r8.graph.DebugLocalInfo; import com.android.tools.r8.graph.DexString; import com.android.tools.r8.ir.conversion.DexBuilder; import com.android.tools.r8.utils.InternalOptions; -import com.android.tools.r8.utils.StringUtils; -import com.google.common.collect.ImmutableMap; public class DebugPosition extends Instruction { public final int line; public final DexString file; - private ImmutableMap<Integer, DebugLocalInfo> locals; public DebugPosition(int line, DexString file) { super(null); @@ -72,18 +68,6 @@ builder.append(file).append(":"); } builder.append(line); - if (locals != null && !locals.isEmpty()) { - builder.append(", locals: "); - StringUtils.append(builder, locals.values()); - } return builder.toString(); } - - public void setLocals(ImmutableMap<Integer, DebugLocalInfo> locals) { - this.locals = locals; - } - - public ImmutableMap<Integer, DebugLocalInfo> getLocals() { - return locals; - } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/Instruction.java b/src/main/java/com/android/tools/r8/ir/code/Instruction.java index 0c4697d..083100b 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Instruction.java +++ b/src/main/java/com/android/tools/r8/ir/code/Instruction.java
@@ -713,6 +713,7 @@ public boolean isDebugInstruction() { return isDebugPosition() + || isDebugLocalsChange() || isDebugLocalWrite() || isDebugLocalUninitialized(); } @@ -725,6 +726,14 @@ return null; } + public boolean isDebugLocalsChange() { + return false; + } + + public DebugLocalsChange asDebugLocalsChange() { + return null; + } + public boolean isDebugLocalUninitialized() { return false; }
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java index fc1eab5..de19fdd 100644 --- a/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java +++ b/src/main/java/com/android/tools/r8/ir/conversion/DexBuilder.java
@@ -59,6 +59,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.ListIterator; import java.util.Map; @@ -192,27 +193,16 @@ } while (!ifsNeedingRewrite.isEmpty()); // Build instructions. - DexDebugEventBuilder debugEventBuilder = - new DexDebugEventBuilder(ir.method.method, dexItemFactory); + DexDebugEventBuilder debugEventBuilder = new DexDebugEventBuilder(ir.method, dexItemFactory); List<Instruction> dexInstructions = new ArrayList<>(numberOfInstructions); int instructionOffset = 0; InstructionIterator instructionIterator = ir.instructionIterator(); - int lastMoveExceptionOffset = -1; while (instructionIterator.hasNext()) { com.android.tools.r8.ir.code.Instruction ir = instructionIterator.next(); Info info = getInfo(ir); int previousInstructionCount = dexInstructions.size(); info.addInstructions(this, dexInstructions); - - if (ir.isArgument()) { - int register = registerAllocator.getRegisterForValue(ir.outValue(), ir.getNumber()); - debugEventBuilder.startArgument(register, ir.getLocalInfo(), ir.outValue().isThis()); - } else if (ir.isDebugPosition()) { - int pc = lastMoveExceptionOffset >= 0 ? lastMoveExceptionOffset : instructionOffset; - debugEventBuilder.setPosition(pc, ir.asDebugPosition()); - } - lastMoveExceptionOffset = ir.isMoveException() ? instructionOffset : -1; - + debugEventBuilder.add(instructionOffset, ir); if (previousInstructionCount < dexInstructions.size()) { while (previousInstructionCount < dexInstructions.size()) { Instruction instruction = dexInstructions.get(previousInstructionCount++); @@ -332,13 +322,8 @@ public void addGoto(com.android.tools.r8.ir.code.Goto jump) { if (jump.getTarget() != nextBlock) { add(jump, new GotoInfo(jump)); - return; - } - List<com.android.tools.r8.ir.code.Instruction> instructions = jump.getBlock().getInstructions(); - if (instructions.size() > 1) { - addFallThroughOrNop(jump, instructions.get(instructions.size() - 2), nextBlock.entry()); } else { - addFallThrough(jump); + addNop(jump); } } @@ -351,30 +336,41 @@ add(move, new MoveInfo(move)); } - public void addFallThrough(com.android.tools.r8.ir.code.Instruction instruction) { + public void addNop(com.android.tools.r8.ir.code.Instruction instruction) { add(instruction, new FallThroughInfo(instruction)); } - private void addFallThroughOrNop( - com.android.tools.r8.ir.code.Instruction key, - com.android.tools.r8.ir.code.Instruction instruction, - com.android.tools.r8.ir.code.Instruction nextInstruction) { - if (nextInstruction != null - && instruction.isDebugPosition() - && nextInstruction.isDebugPosition()) { - add(key, new FixedSizeInfo(key, new Nop())); - } else { - addFallThrough(key); - } + private boolean isNopInstruction(com.android.tools.r8.ir.code.Instruction instruction) { + return instruction.isDebugLocalsChange() + || (instruction.isConstNumber() && !instruction.outValue().needsRegister()); } public void addDebugPosition(DebugPosition position) { BasicBlock block = position.getBlock(); - int nextIndex = block.getInstructions().indexOf(position) + 1; - List<com.android.tools.r8.ir.code.Instruction> instructions = block.getInstructions(); - com.android.tools.r8.ir.code.Instruction nextInstruction = - nextIndex < instructions.size() ? instructions.get(nextIndex) : null; - addFallThroughOrNop(position, position, nextInstruction); + int blockIndex = ir.blocks.indexOf(block); + Iterator<com.android.tools.r8.ir.code.Instruction> iterator = + block.listIterator(1 + block.getInstructions().indexOf(position)); + + com.android.tools.r8.ir.code.Instruction next = null; + while (next == null) { + next = iterator.next(); + while (isNopInstruction(next)) { + next = iterator.next(); + } + if (next.isGoto()) { + ++blockIndex; + BasicBlock nextBlock = blockIndex < ir.blocks.size() ? ir.blocks.get(blockIndex) : null; + if (next.asGoto().getTarget() == nextBlock) { + iterator = nextBlock.iterator(); + next = null; + } + } + } + if (next.isDebugPosition()) { + add(position, new FixedSizeInfo(position, new Nop())); + } else { + addNop(position); + } } public void add(com.android.tools.r8.ir.code.Instruction ir, Instruction dex) {
diff --git a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java index eb0aa77..13f2d29 100644 --- a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java +++ b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
@@ -9,7 +9,7 @@ import com.android.tools.r8.ir.code.Add; import com.android.tools.r8.ir.code.And; import com.android.tools.r8.ir.code.BasicBlock; -import com.android.tools.r8.ir.code.DebugPosition; +import com.android.tools.r8.ir.code.DebugLocalsChange; import com.android.tools.r8.ir.code.IRCode; import com.android.tools.r8.ir.code.Instruction; import com.android.tools.r8.ir.code.InstructionListIterator; @@ -32,8 +32,12 @@ import com.google.common.collect.Multiset; import com.google.common.collect.Multiset.Entry; import com.google.common.collect.Multisets; +import it.unimi.dsi.fastutil.ints.Int2ReferenceMap; +import it.unimi.dsi.fastutil.ints.Int2ReferenceOpenHashMap; +import it.unimi.dsi.fastutil.ints.IntArraySet; +import it.unimi.dsi.fastutil.ints.IntIterator; +import it.unimi.dsi.fastutil.ints.IntSet; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.IdentityHashMap; import java.util.Iterator; @@ -268,10 +272,8 @@ ranges.sort(LocalRange::compareTo); // For each debug position compute the set of live locals. - // The previously known locals is used to share locals when unchanged. boolean localsChanged = false; - ImmutableMap<Integer, DebugLocalInfo> previousLocals = ImmutableMap.of(); - Map<Integer, DebugLocalInfo> currentLocals = new HashMap<>(); + Int2ReferenceMap<DebugLocalInfo> currentLocals = new Int2ReferenceOpenHashMap<>(); LinkedList<LocalRange> openRanges = new LinkedList<>(); Iterator<LocalRange> rangeIterator = ranges.iterator(); @@ -282,54 +284,89 @@ currentLocals.put(nextStartingRange.register, nextStartingRange.local); openRanges.add(nextStartingRange); nextStartingRange = rangeIterator.hasNext() ? rangeIterator.next() : null; - localsChanged = true; } for (BasicBlock block : blocks) { - Iterator<Instruction> instructionIterator = block.getInstructions().iterator(); + boolean blockEntry = true; + ListIterator<Instruction> instructionIterator = block.listIterator(); while (instructionIterator.hasNext()) { Instruction instruction = instructionIterator.next(); - if (!instruction.isDebugPosition()) { - continue; - } int index = instruction.getNumber(); ListIterator<LocalRange> it = openRanges.listIterator(0); + Int2ReferenceMap<DebugLocalInfo> ending = new Int2ReferenceOpenHashMap<>(); + Int2ReferenceMap<DebugLocalInfo> starting = new Int2ReferenceOpenHashMap<>(); while (it.hasNext()) { LocalRange openRange = it.next(); if (openRange.end <= index) { it.remove(); assert currentLocals.get(openRange.register) == openRange.local; - currentLocals.put(openRange.register, null); + currentLocals.remove(openRange.register); localsChanged = true; + ending.put(openRange.register, openRange.local); } } while (nextStartingRange != null && nextStartingRange.start <= index) { // If the full range is between the two debug positions ignore it. if (nextStartingRange.end > index) { openRanges.add(nextStartingRange); - assert currentLocals.get(nextStartingRange.register) == null; + assert !currentLocals.containsKey(nextStartingRange.register); currentLocals.put(nextStartingRange.register, nextStartingRange.local); + starting.put(nextStartingRange.register, nextStartingRange.local); localsChanged = true; } nextStartingRange = rangeIterator.hasNext() ? rangeIterator.next() : null; } - DebugPosition position = instruction.asDebugPosition(); - if (localsChanged) { - ImmutableMap.Builder<Integer, DebugLocalInfo> locals = ImmutableMap.builder(); - for (Map.Entry<Integer, DebugLocalInfo> entry : currentLocals.entrySet()) { - if (entry.getValue() != null) { - locals.put(entry); + if (blockEntry) { + blockEntry = false; + block.setLocalsAtEntry(new Int2ReferenceOpenHashMap<>(currentLocals)); + } else if (localsChanged && shouldEmitChangesAtInstruction(instruction)) { + DebugLocalsChange change = createLocalsChange(ending, starting); + if (change != null) { + if (instruction.isDebugPosition() || instruction.isJumpInstruction()) { + instructionIterator.previous(); + instructionIterator.add(new DebugLocalsChange(ending, starting)); + instructionIterator.next(); + } else { + instructionIterator.add(new DebugLocalsChange(ending, starting)); } } - localsChanged = false; - previousLocals = locals.build(); } - assert verifyLocalsEqual(previousLocals, currentLocals); - position.setLocals(previousLocals); + localsChanged = false; } } } + private DebugLocalsChange createLocalsChange( + Int2ReferenceMap<DebugLocalInfo> ending, Int2ReferenceMap<DebugLocalInfo> starting) { + if (ending.isEmpty() || starting.isEmpty()) { + return new DebugLocalsChange(ending, starting); + } + IntSet unneeded = new IntArraySet(Math.min(ending.size(), starting.size())); + for (Int2ReferenceMap.Entry<DebugLocalInfo> entry : ending.int2ReferenceEntrySet()) { + if (starting.get(entry.getIntKey()) == entry.getValue()) { + unneeded.add(entry.getIntKey()); + } + } + if (unneeded.size() == ending.size() && unneeded.size() == starting.size()) { + return null; + } + IntIterator iterator = unneeded.iterator(); + while (iterator.hasNext()) { + int key = iterator.nextInt(); + ending.remove(key); + starting.remove(key); + } + return new DebugLocalsChange(ending, starting); + } + + private boolean shouldEmitChangesAtInstruction(Instruction instruction) { + BasicBlock block = instruction.getBlock(); + // We emit local changes on all non-exit instructions or, since we have only a singe return + // block, any exits directly targeting that. + return instruction != block.exit() + || (instruction.isGoto() && instruction.asGoto().getTarget() == code.getNormalExitBlock()); + } + private boolean verifyLocalsEqual( ImmutableMap<Integer, DebugLocalInfo> a, Map<Integer, DebugLocalInfo> b) { int size = 0;
diff --git a/src/main/java/com/android/tools/r8/optimize/DebugStripper.java b/src/main/java/com/android/tools/r8/optimize/DebugStripper.java index 9d39d29..3f83606 100644 --- a/src/main/java/com/android/tools/r8/optimize/DebugStripper.java +++ b/src/main/java/com/android/tools/r8/optimize/DebugStripper.java
@@ -3,14 +3,12 @@ // BSD-style license that can be found in the LICENSE file. package com.android.tools.r8.optimize; -import com.android.tools.r8.graph.DebugLocalInfo; import com.android.tools.r8.graph.DexCode; import com.android.tools.r8.graph.DexDebugEntry; import com.android.tools.r8.graph.DexDebugEventBuilder; import com.android.tools.r8.graph.DexDebugInfo; import com.android.tools.r8.graph.DexEncodedMethod; import com.android.tools.r8.graph.DexItemFactory; -import com.android.tools.r8.graph.DexMethod; import com.android.tools.r8.graph.DexProgramClass; import com.android.tools.r8.graph.DexString; import com.android.tools.r8.naming.ClassNameMapper; @@ -19,7 +17,6 @@ import com.android.tools.r8.naming.MemberNaming.Range; import com.android.tools.r8.naming.MemberNaming.Signature; import com.android.tools.r8.utils.InternalOptions; -import com.google.common.collect.ImmutableMap; import it.unimi.dsi.fastutil.objects.Reference2IntMap; import it.unimi.dsi.fastutil.objects.Reference2IntOpenHashMap; import java.util.List; @@ -65,8 +62,8 @@ } } - private NumberedDebugInfo processDebugInfo(DexMethod method, DexDebugInfo info, - MemberNaming naming, int startLine) { + private NumberedDebugInfo processDebugInfo( + DexEncodedMethod method, DexDebugInfo info, MemberNaming naming, int startLine) { if (info == null || naming == null) { return new NumberedDebugInfo(0, null); } @@ -75,13 +72,8 @@ // that pertains to a different method body. Range currentRange = naming.topLevelRange; DexDebugEventBuilder builder = new DexDebugEventBuilder(method, dexItemFactory); - // Always start with a no-op bytecode to make sure that the start-line is manifested by - // the Dalvik VM and the event based processing in R8. This also avoids empty bytecode - // sequences. - int entryCount = 1; - DexString file = null; - ImmutableMap<Integer, DebugLocalInfo> locals = null; - builder.setPosition(0, startLine, file, locals); + // Below we insert the start-line at pc-0 except if another entry already defines pc-0. + int entryCount = 0; for (DexDebugEntry entry : info.computeEntries()) { boolean addEntry = false; // We are in a range, check whether we have left it. @@ -98,13 +90,21 @@ } } if (addEntry) { + if (entryCount == 0 && entry.address > 0) { + ++entryCount; + builder.setPosition(0, startLine); + } int line = options.skipDebugLineNumberOpt ? entry.line : startLine + ranges.indexOf(currentRange) + 1; - builder.setPosition(entry.address, line, file, locals); + builder.setPosition(entry.address, line); ++entryCount; } } + if (entryCount == 0) { + ++entryCount; + builder.setPosition(0, startLine); + } return new NumberedDebugInfo(entryCount, builder.build()); } @@ -133,8 +133,8 @@ } } - NumberedDebugInfo numberedInfo = processDebugInfo( - encodedMethod.method, originalInfo, naming, startLine); + NumberedDebugInfo numberedInfo = + processDebugInfo(encodedMethod, originalInfo, naming, startLine); DexDebugInfo newInfo = numberedInfo.info; if (!options.skipDebugLineNumberOpt) { // Fix up the line information.
diff --git a/src/test/java/com/android/tools/r8/debug/DebugTestBase.java b/src/test/java/com/android/tools/r8/debug/DebugTestBase.java index d10357c..a272eca 100644 --- a/src/test/java/com/android/tools/r8/debug/DebugTestBase.java +++ b/src/test/java/com/android/tools/r8/debug/DebugTestBase.java
@@ -73,7 +73,7 @@ ART } - // Set to true to run tests with java + // Set to JAVA to run tests with java private static final RuntimeKind RUNTIME_KIND = RuntimeKind.ART; // Set to true to enable verbose logs @@ -280,6 +280,13 @@ return inspect(t -> t.checkLocal(localName, expectedValue)); } + protected final JUnit3Wrapper.Command checkNoLocal(String localName) { + return inspect(t -> { + List<String> localNames = t.getLocalNames(); + Assert.assertFalse("Unexpected local: " + localName, localNames.contains(localName)); + }); + } + protected final JUnit3Wrapper.Command checkNoLocal() { return inspect(t -> { List<String> localNames = t.getLocalNames();
diff --git a/src/test/java/com/android/tools/r8/debug/ExceptionTest.java b/src/test/java/com/android/tools/r8/debug/ExceptionTest.java index 3d4202b..7ca9eae 100644 --- a/src/test/java/com/android/tools/r8/debug/ExceptionTest.java +++ b/src/test/java/com/android/tools/r8/debug/ExceptionTest.java
@@ -14,22 +14,31 @@ @Test public void testStepOnCatch() throws Throwable { - int catchLine; if (isRunningJava()) { // Java jumps to first instruction of the catch handler, matching the source code. - catchLine = 11; + runDebugTest("Exceptions", + breakpoint("Exceptions", "catchException"), + run(), + checkLine(SOURCE_FILE, 9), // line of the method call throwing the exception + stepOver(), + checkLine(SOURCE_FILE, 11), // first line in the catch handler + checkLocal("e"), + run()); } else { // ART/Dalvik jumps to 'move-exception' which initializes the local variable with the pending // exception. Thus it is "attached" to the line declaring the exception in the catch handler. - catchLine = 10; + runDebugTest("Exceptions", + breakpoint("Exceptions", "catchException"), + run(), + checkLine(SOURCE_FILE, 9), // line of the method call throwing the exception + stepOver(), + checkLine(SOURCE_FILE, 10), // line of the catch declaration + checkNoLocal("e"), + stepOver(), + checkLine(SOURCE_FILE, 11), // first line in the catch handler + checkLocal("e"), + run()); } - runDebugTest("Exceptions", - breakpoint("Exceptions", "catchException"), - run(), - checkLine(SOURCE_FILE, 9), // line of the method call throwing the exception - stepOver(), - checkLine(SOURCE_FILE, catchLine), // line of the catch declaration - run()); } }
diff --git a/src/test/java/com/android/tools/r8/debug/LocalsTest.java b/src/test/java/com/android/tools/r8/debug/LocalsTest.java index 810ec25..6a6a20f 100644 --- a/src/test/java/com/android/tools/r8/debug/LocalsTest.java +++ b/src/test/java/com/android/tools/r8/debug/LocalsTest.java
@@ -285,7 +285,7 @@ run()); } - @Test(expected = Throwable.class) + @Test public void testInvokeRangeLong() throws Throwable { final int initialValueOfX = 21; final long expectedValueOfL = (long) initialValueOfX * 2;
diff --git a/src/test/java/com/android/tools/r8/debuginfo/DebugInfoInspector.java b/src/test/java/com/android/tools/r8/debuginfo/DebugInfoInspector.java index 368a8fc..ad6f925 100644 --- a/src/test/java/com/android/tools/r8/debuginfo/DebugInfoInspector.java +++ b/src/test/java/com/android/tools/r8/debuginfo/DebugInfoInspector.java
@@ -111,8 +111,10 @@ private int checkLines(int line, Consumer<DexDebugEntry> check) { int found = 0; - for (DexDebugEntry entry : entries) { - if (entry.line == line) { + for (int i = 0; i < entries.size(); i++) { + DexDebugEntry entry = entries.get(i); + // Matches each entry at 'line' that is not a zero-line increment. + if (entry.line == line && (i == 0 || entries.get(i - 1).line != line)) { found++; check.accept(entry); }
diff --git a/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java b/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java index d53b6d6..72bbaed 100644 --- a/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java +++ b/src/test/java/com/android/tools/r8/jasmin/DebugLocalTests.java
@@ -161,12 +161,7 @@ info.checkStartLine(1); info.checkLineHasExactLocals(1, "this", "Test"); info.checkLineHasExactLocals(2, "this", "Test", "x", "int"); - // TODO(zerny): Reintroduce this check once we compute exact local changes. b/63243012 - // Without the exact local changes between the last debug position and the return we fail to - // end the local scope of 'x'. - if (false) { - info.checkLineHasExactLocals(3, "this", "Test"); - } + info.checkLineHasExactLocals(3, "this", "Test"); String artResult = runOnArt(d8App, clazz.name); assertEquals(expected, artResult);