Skip to content

[GR-61590] [GR-61408] [GR-60880] Add support for handling unsigned comparisons in points-to analysis. #10628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[GR-61590] [GR-61408] [GR-60880] Add support for handling unsigned comparisons in points-to analysis. #10628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ...acle.graal.pointsto/src/com/oracle/graal/pointsto/flow/BooleanPrimitiveCheckTypeFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,22 @@ public class BooleanPrimitiveCheckTypeFlow extends BooleanCheckTypeFlow {
private final TypeFlow<?> left;
private final TypeFlow<?> right;
private final PrimitiveComparison comparison;
private final boolean isUnsigned;

public BooleanPrimitiveCheckTypeFlow(BytecodePosition position, AnalysisType declaredType, TypeFlow<?> left, TypeFlow<?> right, PrimitiveComparison comparison) {
public BooleanPrimitiveCheckTypeFlow(BytecodePosition position, AnalysisType declaredType, TypeFlow<?> left, TypeFlow<?> right, PrimitiveComparison comparison, boolean isUnsigned) {
super(position, declaredType);
this.left = left;
this.right = right;
this.comparison = comparison;
this.isUnsigned = isUnsigned;
}

private BooleanPrimitiveCheckTypeFlow(PointsToAnalysis bb, MethodFlowsGraph methodFlows, BooleanPrimitiveCheckTypeFlow original) {
super(original, methodFlows);
this.left = methodFlows.lookupCloneOf(bb, original.left);
this.right = methodFlows.lookupCloneOf(bb, original.right);
this.comparison = original.comparison;
this.isUnsigned = original.isUnsigned;
}

@Override
Expand Down Expand Up @@ -86,6 +89,6 @@ public TypeState eval(PointsToAnalysis bb) {
}
assert leftState.isPrimitive() : left;
assert rightState.isPrimitive() : right;
return convertToBoolean(bb, TypeState.filter(leftState, comparison, rightState), TypeState.filter(leftState, comparison.negate(), rightState));
return convertToBoolean(bb, TypeState.filter(leftState, comparison, rightState, isUnsigned), TypeState.filter(leftState, comparison.negate(), rightState, isUnsigned));
}
}
17 changes: 10 additions & 7 deletions ...c/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
import jdk.graal.compiler.nodes.calc.ConditionalNode;
import jdk.graal.compiler.nodes.calc.FloatEqualsNode;
import jdk.graal.compiler.nodes.calc.FloatLessThanNode;
import jdk.graal.compiler.nodes.calc.IntegerBelowNode;
import jdk.graal.compiler.nodes.calc.IntegerEqualsNode;
import jdk.graal.compiler.nodes.calc.IntegerLowerThanNode;
import jdk.graal.compiler.nodes.calc.IntegerTestNode;
Expand Down Expand Up @@ -1028,7 +1029,7 @@ public TypeFlowBuilder<?> lookup(ValueNode n) {
var y = lookup(equalsNode.getY());
var type = getNodeType(equalsNode);
result = TypeFlowBuilder.create(bb, method, getPredicate(), node, BooleanPrimitiveCheckTypeFlow.class, () -> {
var flow = new BooleanPrimitiveCheckTypeFlow(AbstractAnalysisEngine.sourcePosition(node), type, x.get(), y.get(), PrimitiveComparison.EQ);
var flow = new BooleanPrimitiveCheckTypeFlow(AbstractAnalysisEngine.sourcePosition(node), type, x.get(), y.get(), PrimitiveComparison.EQ, false);
flowsGraph.addMiscEntryFlow(flow);
return flow;
});
Expand All @@ -1037,9 +1038,10 @@ public TypeFlowBuilder<?> lookup(ValueNode n) {
} else if (node instanceof IntegerLowerThanNode lowerThan) {
var x = lookup(lowerThan.getX());
var y = lookup(lowerThan.getY());
var isUnsigned = lowerThan instanceof IntegerBelowNode;
var type = getNodeType(lowerThan);
result = TypeFlowBuilder.create(bb, method, getPredicate(), node, BooleanPrimitiveCheckTypeFlow.class, () -> {
var flow = new BooleanPrimitiveCheckTypeFlow(AbstractAnalysisEngine.sourcePosition(node), type, x.get(), y.get(), PrimitiveComparison.LT);
var flow = new BooleanPrimitiveCheckTypeFlow(AbstractAnalysisEngine.sourcePosition(node), type, x.get(), y.get(), PrimitiveComparison.LT, isUnsigned);
flowsGraph.addMiscEntryFlow(flow);
return flow;
});
Expand Down Expand Up @@ -1311,7 +1313,7 @@ private TypeFlowBuilder<?> uniqueReturnFlowBuilder(ReturnNode node) {
return returnBuilder;
}

private void handleCompareNode(ValueNode source, CompareNode condition, PrimitiveComparison comparison) {
private void handleCompareNode(ValueNode source, CompareNode condition, PrimitiveComparison comparison, boolean isUnsigned) {
var xNode = typeFlowUnproxify(condition.getX());
var yNode = typeFlowUnproxify(condition.getY());
var xConstant = xNode.isConstant();
Expand All @@ -1320,7 +1322,7 @@ private void handleCompareNode(ValueNode source, CompareNode condition, Primitiv
var yFlow = state.lookup(yNode);
if (!xConstant) {
var leftFlowBuilder = TypeFlowBuilder.create(bb, method, state.getPredicate(), source, PrimitiveFilterTypeFlow.class, () -> {
var flow = new PrimitiveFilterTypeFlow(AbstractAnalysisEngine.sourcePosition(source), xFlow.get().declaredType, xFlow.get(), yFlow.get(), comparison);
var flow = new PrimitiveFilterTypeFlow(AbstractAnalysisEngine.sourcePosition(source), xFlow.get().declaredType, xFlow.get(), yFlow.get(), comparison, isUnsigned);
if (yConstant) {
flowsGraph.addNodeFlow(source, flow);
}
Expand All @@ -1334,7 +1336,7 @@ private void handleCompareNode(ValueNode source, CompareNode condition, Primitiv
}
if (!yConstant) {
var rightFlowBuilder = TypeFlowBuilder.create(bb, method, state.getPredicate(), source, PrimitiveFilterTypeFlow.class, () -> {
var flow = new PrimitiveFilterTypeFlow(AbstractAnalysisEngine.sourcePosition(source), yFlow.get().declaredType, yFlow.get(), xFlow.get(), comparison.flip());
var flow = new PrimitiveFilterTypeFlow(AbstractAnalysisEngine.sourcePosition(source), yFlow.get().declaredType, yFlow.get(), xFlow.get(), comparison.flip(), isUnsigned);
flowsGraph.addNodeFlow(source, flow);
return flow;

Expand All @@ -1359,9 +1361,10 @@ private void handleUnaryOpLogicNode(UnaryOpLogicNode condition, TypeFlowBuilder<
private void handleCondition(ValueNode source, LogicNode condition, boolean isTrue) {
if (state.usePredicates()) {
if (condition instanceof IntegerLowerThanNode lowerThan) {
handleCompareNode(source, lowerThan, isTrue ? PrimitiveComparison.LT : PrimitiveComparison.GE);
var isUnsigned = lowerThan instanceof IntegerBelowNode;
handleCompareNode(source, lowerThan, isTrue ? PrimitiveComparison.LT : PrimitiveComparison.GE, isUnsigned);
} else if (condition instanceof IntegerEqualsNode equalsNode) {
handleCompareNode(source, equalsNode, isTrue ? PrimitiveComparison.EQ : PrimitiveComparison.NEQ);
handleCompareNode(source, equalsNode, isTrue ? PrimitiveComparison.EQ : PrimitiveComparison.NEQ, false);
}
}
if (condition instanceof IsNullNode nullCheck) {
Expand Down
7 changes: 5 additions & 2 deletions ...com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/PrimitiveFilterTypeFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,22 @@ public class PrimitiveFilterTypeFlow extends TypeFlow<BytecodePosition> {
private final TypeFlow<?> left;
private final TypeFlow<?> right;
private final PrimitiveComparison comparison;
private final boolean isUnsigned;

public PrimitiveFilterTypeFlow(BytecodePosition position, AnalysisType declaredType, TypeFlow<?> left, TypeFlow<?> right, PrimitiveComparison comparison) {
public PrimitiveFilterTypeFlow(BytecodePosition position, AnalysisType declaredType, TypeFlow<?> left, TypeFlow<?> right, PrimitiveComparison comparison, boolean isUnsigned) {
super(position, declaredType);
this.left = left;
this.right = right;
this.comparison = comparison;
this.isUnsigned = isUnsigned;
}

private PrimitiveFilterTypeFlow(PointsToAnalysis bb, MethodFlowsGraph methodFlows, PrimitiveFilterTypeFlow original) {
super(original, methodFlows);
this.left = methodFlows.lookupCloneOf(bb, original.left);
this.right = methodFlows.lookupCloneOf(bb, original.right);
this.comparison = original.comparison;
this.isUnsigned = original.isUnsigned;
}

@Override
Expand Down Expand Up @@ -85,7 +88,7 @@ private TypeState eval(PointsToAnalysis bb) {
var rightState = right.getOutputState(bb);
assert leftState.isPrimitive() || leftState.isEmpty() : left;
assert rightState.isPrimitive() || rightState.isEmpty() : right;
return TypeState.filter(leftState, comparison, rightState);
return TypeState.filter(leftState, comparison, rightState, isUnsigned);
}

public PrimitiveComparison getComparison() {
Expand Down
68 changes: 25 additions & 43 deletions ...com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/typestate/PrimitiveTypeState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import com.oracle.graal.pointsto.meta.AnalysisType;
import com.oracle.graal.pointsto.util.AnalysisError;

import jdk.graal.compiler.core.common.calc.UnsignedMath;

/**
* Represents type state for primitive values. These type states can be either concrete constants
* represented by {@link PrimitiveConstantTypeState} or {@link AnyPrimitiveTypeState} that represent
Expand Down Expand Up @@ -72,18 +74,6 @@ public TypeState forUnion(PrimitiveTypeState right) {
return AnyPrimitiveTypeState.SINGLETON;
}

/** Returns a type state filtered with respect to the comparison and right. */
public TypeState filter(PrimitiveComparison comparison, PrimitiveTypeState right) {
return switch (comparison) {
case EQ -> forEquals(right);
case NEQ -> forNotEquals(right);
case LT -> forLessThan(right);
case GE -> forGreaterOrEqual(right);
case GT -> forGreaterThan(right);
case LE -> forLessOrEqual(right);
};
}

public TypeState forEquals(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant) {
if (right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() != rightConstant.getValue()) {
Expand All @@ -100,39 +90,31 @@ public TypeState forEquals(PrimitiveTypeState right) {
throw AnalysisError.shouldNotReachHere("Combination not covered, this=" + this + ". right=" + right);
}

public TypeState forNotEquals(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant && right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() == rightConstant.getValue()) {
return forEmpty();
}
return this;
}

public TypeState forLessThan(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant && right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() >= rightConstant.getValue()) {
return forEmpty();
}
return this;
}

public TypeState forGreaterOrEqual(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant && right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() < rightConstant.getValue()) {
return forEmpty();
}
return this;
}

public TypeState forLessOrEqual(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant && right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() > rightConstant.getValue()) {
return forEmpty();
/** Returns a type state filtered with respect to the comparison and right. */
public TypeState filter(PrimitiveComparison comparison, boolean isUnsigned, PrimitiveTypeState right) {
if (comparison == PrimitiveComparison.EQ) {
/*
* This is the only case where we can improve even if one of the operands is Any, so we
* handle it first.
*/
return forEquals(right);
}
return this;
}

public TypeState forGreaterThan(PrimitiveTypeState right) {
if (this instanceof PrimitiveConstantTypeState thisConstant && right instanceof PrimitiveConstantTypeState rightConstant && thisConstant.getValue() <= rightConstant.getValue()) {
return forEmpty();
if (!(this instanceof PrimitiveConstantTypeState leftConstant && right instanceof PrimitiveConstantTypeState rightConstant)) {
/*
* Apart from EQ, we cannot handle non-constant case, so always return the original
* value.
*/
return this;
}
return this;
boolean filterToEmpty = switch (comparison) {
case NEQ -> leftConstant.getValue() == rightConstant.getValue();
case LT -> isUnsigned ? UnsignedMath.aboveOrEqual(leftConstant.getValue(), rightConstant.getValue()) : leftConstant.getValue() >= rightConstant.getValue();
case GE -> isUnsigned ? UnsignedMath.belowThan(leftConstant.getValue(), rightConstant.getValue()) : leftConstant.getValue() < rightConstant.getValue();
case GT -> isUnsigned ? UnsignedMath.belowOrEqual(leftConstant.getValue(), rightConstant.getValue()) : leftConstant.getValue() <= rightConstant.getValue();
case LE -> isUnsigned ? UnsignedMath.aboveThan(leftConstant.getValue(), rightConstant.getValue()) : leftConstant.getValue() > rightConstant.getValue();
default -> throw AnalysisError.shouldNotReachHere("Unreachable case in switch.");
};
return filterToEmpty ? forEmpty() : this;
}

public abstract boolean canBeTrue();
Expand Down
4 changes: 2 additions & 2 deletions ...tevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/typestate/TypeState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,13 +274,13 @@ public static TypeState forSubtraction(PointsToAnalysis bb, TypeState s1, TypeSt
}

/** Returns a type state representing left filtered with respect to the comparison and right. */
public static TypeState filter(TypeState left, PrimitiveComparison comparison, TypeState right) {
public static TypeState filter(TypeState left, PrimitiveComparison comparison, TypeState right, boolean isUnsigned) {
assert left.isPrimitive() || left.isEmpty() : left;
assert right.isPrimitive() || right.isEmpty() : right;
if (left.isEmpty() || right.isEmpty()) {
return forEmpty();
}
return ((PrimitiveTypeState) left).filter(comparison, (PrimitiveTypeState) right);
return ((PrimitiveTypeState) left).filter(comparison, isUnsigned, (PrimitiveTypeState) right);
}
}

Expand Down