Skip to content

Fix for #103 #124

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.

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

Merged
merged 9 commits into from
Dec 16, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import com.ibm.wala.ipa.callgraph.impl.FakeRootMethod;
import com.ibm.wala.ipa.callgraph.impl.FakeWorldClinitMethod;
import com.ibm.wala.ipa.callgraph.propagation.InstanceKey;
import com.ibm.wala.ipa.callgraph.propagation.cfa.CallStringContext;
import com.ibm.wala.ipa.cha.ClassHierarchyException;
import com.ibm.wala.ipa.cha.IClassHierarchy;
import com.ibm.wala.shrikeCT.InvalidClassFileException;
Expand Down Expand Up @@ -636,6 +637,11 @@ private void inferInitialOrdering()
*/
protected CGNode getEnclosingMethodNode() throws IOException, CoreException, NoEnclosingMethodNodeFoundException {
MethodReference methodReference = this.getEnclosingMethodReference();
return getEnclosingMethodNode(methodReference);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per this comment, I advised against altering getEnclosingMethodNode().

}

protected CGNode getEnclosingMethodNode(MethodReference methodReference)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter makes no sense and has not associated documentation.

throws IOException, CoreException, NoEnclosingMethodNodeFoundException {
Set<CGNode> nodes = this.getAnalysisEngine().getCallGraph().getNodes(methodReference);

if (nodes.isEmpty())
Expand All @@ -644,6 +650,19 @@ protected CGNode getEnclosingMethodNode() throws IOException, CoreException, NoE
return nodes.iterator().next(); // just return the first.
}

protected HashSet<CGNode> getEnclosingMethodNodes()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because portions of our call graph are context sensitive, there may be more than one node that represents a single method. But, they are basically the same and are only used methods returning streams. The only difference is the calling context. I don't see any documentation on why all methods representing the enclosing method are needed. Furthermore, there was no internal discussion of this strategy.

throws IOException, CoreException, NoEnclosingMethodNodeFoundException {
HashSet<CGNode> cgNodes = new HashSet<>();
com.ibm.wala.classLoader.IMethod[] methods = ((CallStringContext) (getEnclosingMethodNode().getContext()))
.getCallString().getMethods();

for (int i = 0; i < methods.length - 1; ++i) {
cgNodes.add(getEnclosingMethodNode(methods[i].getReference()));
}
cgNodes.add(this.getEnclosingMethodNode());
return cgNodes;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call graphs can be massive. We don't want an extra step here of adding them to a set if it is not necessary.

}

/**
* Returns true iff all of the predecessors of all of the given {@link CGNode}s
* in the {@link CallGraph} are {@link FakeRootMethod}s.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.jdt.core.dom.MethodInvocation;

import com.google.common.collect.HashBasedTable;
import com.google.common.collect.Table;
Expand Down Expand Up @@ -281,114 +280,116 @@ public void start() throws IOException, CoreException, CallGraphBuilderCancelExc
// the node of where the stream was declared: TODO: Can this be
// somehow rewritten to get blocks corresponding to terminal
// operations?
CGNode cgNode = this.getStream().getEnclosingMethodNode();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add a FOR statement. It changes the format of the block in the loop, so red block is large.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop is from line 283 to line 396.

for (Iterator<CallSiteReference> callSites = cgNode.iterateCallSites(); callSites.hasNext();) {
CallSiteReference callSiteReference = callSites.next();
MethodReference calledMethod = callSiteReference.getDeclaredTarget();

// is it a terminal operation? TODO: Should this be
// cached somehow? Collection of all terminal operation
// invocations?
if (isTerminalOperation(calledMethod)) {
// get the basic block for the call.
ISSABasicBlock[] blocksForCall = cgNode.getIR().getBasicBlocksForCall(callSiteReference);

assert blocksForCall.length == 1 : "Expecting only a single basic block for the call: "
+ callSiteReference;

for (int i = 0; i < blocksForCall.length; i++) {
ISSABasicBlock block = blocksForCall[i];

BasicBlockInContext<IExplodedBasicBlock> blockInContext = getBasicBlockInContextForBlock(
block, cgNode, supergraph)
.orElseThrow(() -> new IllegalStateException(
"No basic block in context for block: " + block));

if (!terminalBlockToPossibleReceivers.containsKey(blockInContext)) {
// associate possible receivers with the
// blockInContext.
// search through each instruction in the
// block.
int processedInstructions = 0;
for (Iterator<SSAInstruction> it = block.iterator(); it.hasNext();) {
SSAInstruction instruction = it.next();

// if it's a phi instruction.
if (instruction instanceof SSAPhiInstruction)
// skip it. The pointer analysis
// below will handle it.
continue;

// Get the possible receivers. This
// number corresponds to the value
// number of the receiver of the method.
int valueNumberForReceiver = instruction.getUse(0);

// it should be represented by a pointer
// key.
PointerKey pointerKey = engine.getHeapGraph().getHeapModel()
.getPointerKeyForLocal(cgNode, valueNumberForReceiver);

// get the points to set for the
// receiver. This will give us all
// object instances that the receiver
// reference points to.
OrdinalSet<InstanceKey> pointsToSet = engine.getPointerAnalysis()
.getPointsToSet(pointerKey);
assert pointsToSet != null : "The points-to set (I think) should not be null for pointer: "
+ pointerKey;

OrdinalSet<InstanceKey> previousReceivers = terminalBlockToPossibleReceivers
.put(blockInContext, pointsToSet);
assert previousReceivers == null : "Reassociating a blockInContext: "
+ blockInContext + " with a new points-to set: " + pointsToSet
+ " that was originally: " + previousReceivers;

++processedInstructions;
// CGNode cgNode = this.getStream().getEnclosingMethodNode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not accept commented code.

for (CGNode cgNode : this.getStream().getEnclosingMethodNodes()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these diff blocks so large? I cannot see what has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just move the block into a loop and change its format by shift + ctrl + f。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure of:

  1. What the collection of enclosing method nodes represents in this context, and
  2. why there are needed.

In other words, there is a lot of documentation missing here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not, for starters, iterate over all nodes in the graph? If that can cover all cases (i.e., returning streams, passing streams, and storing streams into fields), and we can create proper unit tests for that functionality, we can then think about how to reduce the search space while still maintaining correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the last weekend, I tried to iterate over all nodes, but got a new exception, so I tried the current strategy to see whether changing call sites can work. However, you are right. Although the current strategy may be smarter (it just iterates over the necessary node), it needs much more time to check whether the strategy are absolutely right. Hence, I will follow your advice.

for (Iterator<CallSiteReference> callSites = cgNode.iterateCallSites(); callSites.hasNext();) {
CallSiteReference callSiteReference = callSites.next();
MethodReference calledMethod = callSiteReference.getDeclaredTarget();

// is it a terminal operation? TODO: Should this be
// cached somehow? Collection of all terminal operation
// invocations?
if (isTerminalOperation(calledMethod)) {
// get the basic block for the call.

ISSABasicBlock[] blocksForCall = cgNode.getIR().getBasicBlocksForCall(callSiteReference);

assert blocksForCall.length == 1 : "Expecting only a single basic block for the call: "
+ callSiteReference;

for (int i = 0; i < blocksForCall.length; i++) {
ISSABasicBlock block = blocksForCall[i];

BasicBlockInContext<IExplodedBasicBlock> blockInContext = getBasicBlockInContextForBlock(
block, cgNode, supergraph)
.orElseThrow(() -> new IllegalStateException(
"No basic block in context for block: " + block));

if (!terminalBlockToPossibleReceivers.containsKey(blockInContext)) {
// associate possible receivers with the
// blockInContext.
// search through each instruction in the
// block.
int processedInstructions = 0;
for (Iterator<SSAInstruction> it = block.iterator(); it.hasNext();) {
SSAInstruction instruction = it.next();

// if it's a phi instruction.
if (instruction instanceof SSAPhiInstruction)
// skip it. The pointer analysis
// below will handle it.
continue;

// Get the possible receivers. This
// number corresponds to the value
// number of the receiver of the method.
int valueNumberForReceiver = instruction.getUse(0);

// it should be represented by a pointer
// key.
PointerKey pointerKey = engine.getHeapGraph().getHeapModel()
.getPointerKeyForLocal(cgNode, valueNumberForReceiver);

// get the points to set for the
// receiver. This will give us all
// object instances that the receiver
// reference points to.
OrdinalSet<InstanceKey> pointsToSet = engine.getPointerAnalysis()
.getPointsToSet(pointerKey);
assert pointsToSet != null : "The points-to set (I think) should not be null for pointer: "
+ pointerKey;

OrdinalSet<InstanceKey> previousReceivers = terminalBlockToPossibleReceivers
.put(blockInContext, pointsToSet);
assert previousReceivers == null : "Reassociating a blockInContext: "
+ blockInContext + " with a new points-to set: " + pointsToSet
+ " that was originally: " + previousReceivers;

++processedInstructions;
}

assert processedInstructions == 1 : "Expecting to process one and only one instruction here.";
}

assert processedInstructions == 1 : "Expecting to process one and only one instruction here.";
}

IntSet intSet = instanceResult.getResult().getResult(blockInContext);
for (IntIterator it = intSet.intIterator(); it.hasNext();) {
int nextInt = it.next();

// retrieve the state set for this instance
// and block.
Map<TypestateRule, Set<IDFAState>> ruleToStates = instanceBlockStateTable
.get(instanceKey, blockInContext);

// if it doesn't yet exist.
if (ruleToStates == null) {
// allocate a new rule map.
ruleToStates = new HashMap<>();

// place it in the table.
instanceBlockStateTable.put(instanceKey, blockInContext, ruleToStates);
}

Set<IDFAState> stateSet = ruleToStates.get(rule);

// if it does not yet exist.
if (stateSet == null) {
// allocate a new set.
stateSet = new HashSet<>();

// place it in the map.
ruleToStates.put(rule, stateSet);
}

// get the facts.
Factoid factoid = instanceResult.getDomain().getMappedObject(nextInt);
if (factoid != DUMMY_ZERO) {
BaseFactoid baseFactoid = (BaseFactoid) factoid;
assert baseFactoid.instance.equals(
instanceKey) : "Sanity check that the fact instance should be the same as the instance being examined.";

// add the encountered state to the set.
stateSet.add(baseFactoid.state);
IntSet intSet = instanceResult.getResult().getResult(blockInContext);
for (IntIterator it = intSet.intIterator(); it.hasNext();) {
int nextInt = it.next();

// retrieve the state set for this instance
// and block.
Map<TypestateRule, Set<IDFAState>> ruleToStates = instanceBlockStateTable
.get(instanceKey, blockInContext);

// if it doesn't yet exist.
if (ruleToStates == null) {
// allocate a new rule map.
ruleToStates = new HashMap<>();

// place it in the table.
instanceBlockStateTable.put(instanceKey, blockInContext, ruleToStates);
}

Set<IDFAState> stateSet = ruleToStates.get(rule);

// if it does not yet exist.
if (stateSet == null) {
// allocate a new set.
stateSet = new HashSet<>();

// place it in the map.
ruleToStates.put(rule, stateSet);
}

// get the facts.
Factoid factoid = instanceResult.getDomain().getMappedObject(nextInt);
if (factoid != DUMMY_ZERO) {
BaseFactoid baseFactoid = (BaseFactoid) factoid;
assert baseFactoid.instance.equals(
instanceKey) : "Sanity check that the fact instance should be the same as the instance being examined.";

// add the encountered state to the set.
stateSet.add(baseFactoid.state);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package p;

import java.util.HashSet;
import java.util.stream.*;

import edu.cuny.hunter.streamrefactoring.annotations.*;

class A {

Stream<Object> m() {
Stream<Object> stream = new HashSet<>().stream().distinct();
return stream;
}

@EntryPoint
void n() {
Stream<Object> s = m();
s.count();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package p;

import java.util.HashSet;
import java.util.stream.*;

import edu.cuny.hunter.streamrefactoring.annotations.*;

class A {

Stream<Object> m() {
Stream<Object> stream = new HashSet<>().stream();
return stream;
}

@EntryPoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package p;

import java.util.HashSet;
import java.util.stream.*;

import edu.cuny.hunter.streamrefactoring.annotations.*;

class A {

Stream<Object> m() {
Stream<Object> stream = new HashSet<>().stream().sorted();
return stream;
}

@EntryPoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package p;

import java.util.HashSet;
import java.util.stream.*;

import edu.cuny.hunter.streamrefactoring.annotations.*;

class A {

Stream<Object> m() {
Stream<Object> stream = new HashSet<>().stream().parallel();
return stream;
}

@EntryPoint
void n() {
Stream<Object> s = m();
s.distinct().count();
}
}
Loading