Skip to content
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

Improve interface of reachable method analysis and related states to prevent misuse #220

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Improve interface of reachable method analysis and related states to prevent misuse #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f8735c
Improve misuse handling of reachable method analysis
maximilianruesch Sep 16, 2024
30bec19
Refactor reachable method analyses and their state / TAC handling
maximilianruesch Sep 18, 2024
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
4 changes: 2 additions & 2 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/SystemPropertiesAnalysisScheduler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ import org.opalj.fpcf.PropertyStore
import org.opalj.fpcf.Results
import org.opalj.fpcf.UBP
import org.opalj.tac.cg.TypeIteratorKey
import org.opalj.tac.fpcf.analyses.cg.ReachableMethodAnalysis
import org.opalj.tac.fpcf.analyses.cg.DefinedBodyReachableMethodAnalysis
import org.opalj.tac.fpcf.properties.TACAI
import org.opalj.value.ValueInformation

class SystemPropertiesAnalysisScheduler private[analyses] (
final val project: SomeProject
) extends ReachableMethodAnalysis {
) extends DefinedBodyReachableMethodAnalysis {

def processMethod(
callContext: ContextType,
Expand Down
19 changes: 6 additions & 13 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/TACAIBasedAnalysisState.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,18 @@ trait TACAIBasedAnalysisState[TheContextType <: Context]
def callContext: ContextType

protected[this] var _tacDependee: EOptionP[Method, TACAI]
assert((_tacDependee eq null) || (_tacDependee.hasUBP && _tacDependee.ub.tac.isDefined))
assert(_tacDependee.hasUBP && _tacDependee.ub.tac.isDefined)

abstract override def hasOpenDependencies: Boolean = {
(_tacDependee ne null) && _tacDependee.isRefinable || super.hasOpenDependencies
}
final def hasTACDependee: Boolean = _tacDependee.isRefinable
abstract override def hasOpenDependencies: Boolean = _tacDependee.isRefinable || super.hasOpenDependencies
final def isTACDependeeRefinable: Boolean = _tacDependee.isRefinable

/**
* Inherited classes that introduce new dependencies must override this method and call add a
* call to super!
*/
abstract override def dependees: Set[SomeEOptionP] = {
val otherDependees = super.dependees
if ((_tacDependee ne null) && _tacDependee.isRefinable)
if (_tacDependee.isRefinable)
otherDependees + _tacDependee
else
otherDependees
Expand All @@ -49,12 +47,7 @@ trait TACAIBasedAnalysisState[TheContextType <: Context]
_tacDependee = tacDependee
}

final def tac: TACode[TACMethodParameter, DUVar[ValueInformation]] = {
_tacDependee.ub.tac.get
}

final def tacDependee: EOptionP[Method, TACAI] = {
_tacDependee
}
final def tac: TACode[TACMethodParameter, DUVar[ValueInformation]] = _tacDependee.ub.tac.get

final def tacDependee: EOptionP[Method, TACAI] = _tacDependee
}
17 changes: 13 additions & 4 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CGState.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ import org.opalj.tac.fpcf.properties.TACAI
/**
* @author Florian Kuebler
*/
class CGState[ContextType <: Context](
override val callContext: ContextType,
override protected[this] var _tacDependee: EOptionP[Method, TACAI]
) extends BaseAnalysisState with TypeIteratorState with TACAIBasedAnalysisState[ContextType] {
class CGState[TheContextType <: Context](
val callContext: TheContextType
) extends BaseAnalysisState with TypeIteratorState with ContextualAnalysis {

override type ContextType = TheContextType

// maps a definition site to the receiver var
private[this] val _virtualCallSites: mutable.Map[CallSite, (V, Set[ReferenceType])] =
Expand All @@ -38,3 +39,11 @@ class CGState[ContextType <: Context](

def hasNonFinalCallSite: Boolean = _virtualCallSites.nonEmpty
}

/**
* @author Maximilian Rüsch
*/
class TACAIBasedCGState[ContextType <: Context](
callContext: ContextType,
override protected[this] var _tacDependee: EOptionP[Method, TACAI]
) extends CGState[ContextType](callContext) with TACAIBasedAnalysisState[ContextType]
31 changes: 15 additions & 16 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import org.opalj.br.fpcf.properties.cg.Callees
import org.opalj.br.fpcf.properties.cg.Callers
import org.opalj.br.fpcf.properties.cg.OnlyCallersWithUnknownContext
import org.opalj.fpcf.Entity
import org.opalj.fpcf.EOptionP
import org.opalj.fpcf.EPK
import org.opalj.fpcf.EPS
import org.opalj.fpcf.InterimEUBP
Expand Down Expand Up @@ -56,14 +57,13 @@ import org.opalj.value.IsSObjectValue
class CallGraphAnalysis private[cg] (
override val project: SomeProject
) extends ReachableMethodAnalysis with TypeConsumerAnalysis {
type LocalTypeInformation

private[this] val isMethodOverridable: Method => Answer = project.get(IsOverridableMethodKey)
private[this] lazy val getCBSTargets = project.get(CallBySignatureKey)
private[this] val resovleCallBySignature =
project.config.getBoolean("org.opalj.br.analyses.cg.callBySignatureResolution")

def c(state: CGState[ContextType])(eps: SomeEPS): ProperPropertyComputationResult = {
def c(state: TACAIBasedCGState[ContextType])(eps: SomeEPS): ProperPropertyComputationResult = {
eps match {
case UBP(tacai: TACAI) if tacai.tac.isDefined =>
state.updateTACDependee(eps.asInstanceOf[EPS[Method, TACAI]])
Expand All @@ -74,7 +74,7 @@ class CallGraphAnalysis private[cg] (
case UBP(_: TACAI) =>
throw new IllegalStateException("there was already a tac defined")

case EPS(e) =>
case EPS(_) =>
val relevantCallSites = state.dependersOf(eps.toEPK).asInstanceOf[Set[CallSite]]

// ensures, that we only add new calls
Expand Down Expand Up @@ -121,25 +121,24 @@ class CallGraphAnalysis private[cg] (

override final def processMethod(
callContext: ContextType,
tacEP: EPS[Method, TACAI]
tacEPOpt: EOptionP[Method, TACAI]
): ProperPropertyComputationResult = {
val state = new CGState[ContextType](callContext, tacEP)
if (tacEP ne null)
if (tacEPOpt.hasUBP) {
val state = new TACAIBasedCGState[ContextType](callContext, tacEPOpt)
processMethod(state, new DirectCalls())
else
returnResult(new DirectCalls(), enforceCalleesResult = true)(state)
} else {
Results(new DirectCalls().partialResults(callContext, enforceCalleesResult = true))
}
}

override final val processesMethodsWithoutBody = true

protected[this] def doHandleVirtualCall(
callContext: ContextType,
call: Call[V] with VirtualCall[V],
pc: Int,
specializedDeclaringClassType: ReferenceType,
isPrecise: Boolean,
calleesAndCallers: DirectCalls
)(implicit state: CGState[ContextType]): Unit = {
)(implicit state: TACAIBasedCGState[ContextType]): Unit = {
val callerType = callContext.method.declaringClassType
val callSite = CallSite(pc, call.name, call.descriptor, call.declaringClass)

Expand Down Expand Up @@ -209,7 +208,7 @@ class CallGraphAnalysis private[cg] (
}

protected final def processMethod(
state: CGState[ContextType],
state: TACAIBasedCGState[ContextType],
calls: DirectCalls
): ProperPropertyComputationResult = {
val tac = state.tac
Expand Down Expand Up @@ -294,13 +293,13 @@ class CallGraphAnalysis private[cg] (
case _ => // nothing to do
}

returnResult(calls, true)(state)
returnResult(calls, enforceCalleesResult = true)(state)
}

protected[this] def returnResult(
calleesAndCallers: DirectCalls,
enforceCalleesResult: Boolean = false
)(implicit state: CGState[ContextType]): ProperPropertyComputationResult = {
)(implicit state: TACAIBasedCGState[ContextType]): ProperPropertyComputationResult = {
val results = calleesAndCallers.partialResults(state.callContext, enforceCalleesResult)

// FIXME: This won't work for refinable TACs as state.hasNonFinalCallSite may return false
Expand Down Expand Up @@ -388,7 +387,7 @@ class CallGraphAnalysis private[cg] (
call: Call[V] with VirtualCall[V],
pc: Int,
calleesAndCallers: DirectCalls
)(implicit state: CGState[ContextType]): Unit = {
)(implicit state: TACAIBasedCGState[ContextType]): Unit = {
val rvs = call.receiver.asVar.value.asReferenceValue.allValues
for (rv <- rvs) rv match {
case _: IsSArrayValue =>
Expand Down Expand Up @@ -422,7 +421,7 @@ class CallGraphAnalysis private[cg] (
call: Call[V] with VirtualCall[V],
pc: Int,
calleesAndCallers: DirectCalls
)(implicit state: CGState[ContextType]): Unit = {
)(implicit state: TACAIBasedCGState[ContextType]): Unit = {
doHandleVirtualCall(callContext, call, pc, calleeType, isPrecise = true, calleesAndCallers)
}
}
Expand Down
11 changes: 6 additions & 5 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/DoPrivilegedCGAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,10 @@ class DoPrivilegedMethodAnalysis private[cg] (
if (params.nonEmpty && params.head.isDefined) {
val param = params.head.get.asVar

implicit val state: CGState[ContextType] =
new CGState[ContextType](callerContext, FinalEP(callerContext.method.definedMethod, TheTACAI(tac)))
implicit val state: TACAIBasedCGState[ContextType] = new TACAIBasedCGState[ContextType](
callerContext,
FinalEP(callerContext.method.definedMethod, TheTACAI(tac))
)

val thisActual = persistentUVar(param)(state.tac.stmts)

Expand All @@ -93,8 +95,7 @@ class DoPrivilegedMethodAnalysis private[cg] (
thisVar: V,
thisActual: Some[(ValueInformation, IntTrieSet)],
calls: IndirectCalls
)(implicit state: CGState[ContextType]): ProperPropertyComputationResult = {

)(implicit state: TACAIBasedCGState[ContextType]): ProperPropertyComputationResult = {
val partialResults = calls.partialResults(state.callContext)
if (state.hasOpenDependencies)
Results(
Expand Down Expand Up @@ -127,7 +128,7 @@ class DoPrivilegedMethodAnalysis private[cg] (
}

def c(
state: CGState[ContextType],
state: TACAIBasedCGState[ContextType],
thisVar: V,
thisActual: Some[(ValueInformation, IntTrieSet)]
)(eps: SomeEPS): ProperPropertyComputationResult = {
Expand Down
67 changes: 34 additions & 33 deletions OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import org.opalj.br.fpcf.FPCFAnalysis
import org.opalj.br.fpcf.properties.cg.Callers
import org.opalj.br.fpcf.properties.cg.NoCallers
import org.opalj.fpcf.EOptionP
import org.opalj.fpcf.EPK
import org.opalj.fpcf.EPS
import org.opalj.fpcf.FinalP
import org.opalj.fpcf.InterimPartialResult
import org.opalj.fpcf.NoResult
import org.opalj.fpcf.ProperPropertyComputationResult
Expand All @@ -36,18 +36,9 @@ trait ReachableMethodAnalysis extends FPCFAnalysis with TypeConsumerAnalysis {

final def analyze(declaredMethod: DeclaredMethod): PropertyComputationResult = {
val callersEOptP = propertyStore(declaredMethod, Callers.key)
(callersEOptP: @unchecked) match {
case FinalP(NoCallers) =>
// nothing to do, since there is no caller
return NoResult;

case eps: EPS[_, _] =>
if (eps.ub eq NoCallers) {
// we can not create a dependency here, so the analysis is not allowed to create
// such a result
throw new IllegalStateException("illegal immediate result for callers")
}
// the method is reachable, so we analyze it!

if (callersEOptP.isFinal && callersEOptP.ub == NoCallers) {
return NoResult;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we keep the check for an illegal NoCallers interim state at least as an assertion? Right now it is not clearly visible that the method is reachable and should be analyzed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The NoCallers interim state was illegal since the code at the top did not know the class could use the continuation without a depending caller. Since the class can invoke and handle continuations just fine without using a depender constructed from a caller, the NoCallers interim state is not illegal anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it's an illegal state in general, isn't it? I don't think call graph analyses are supposed to ever create such an interim state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it is an illegal state in general, then it should be asserted by the call graph generating framework and not the framework for analysis that use the call graph (even if they at least partially overlap). This class seems like the wrong area of code to make such an assertion (e.g. it would only be triggered when using such an analysis even though the result should be illegal regardless of using this class or not).

}

// we only allow defined methods
Expand All @@ -65,47 +56,41 @@ trait ReachableMethodAnalysis extends FPCFAnalysis with TypeConsumerAnalysis {
return processMethodWithoutBody(callersEOptP);

val tacEP = propertyStore(method, TACAI.key)

if (tacEP.hasUBP && tacEP.ub.tac.isDefined) {
processMethod(callersEOptP, null, tacEP.asEPS)
processMethod(callersEOptP, NoCallers, tacEP)
} else {
InterimPartialResult(Set(tacEP), continuationForTAC(declaredMethod))
}
}

protected val processesMethodsWithoutBody = false

protected def processMethodWithoutBody(
eOptP: EOptionP[DeclaredMethod, Callers]
): PropertyComputationResult = {
if (processesMethodsWithoutBody) {
processMethod(eOptP, null, null)
} else
NoResult
}
): PropertyComputationResult = processMethod(eOptP, NoCallers, EPK(eOptP.e.definedMethod, TACAI.key))

private[this] def processMethod(
eOptP: EOptionP[DeclaredMethod, Callers],
seen: Callers,
tacEP: EPS[Method, TACAI]
eOptP: EOptionP[DeclaredMethod, Callers],
oldCallers: Callers,
tacEOptP: EOptionP[Method, TACAI]
): ProperPropertyComputationResult = {
val newCallers = if (eOptP.hasUBP) eOptP.ub else NoCallers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be more sensible to turn this into an if, only executing the forNewCalleeContexts if there is a UBP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we need the newCallers for constructing a continuation and forNewCalleeContexts does not do anything when NoCallers is given, I view this as reasonable and less verbose as well.

var results: List[ProperPropertyComputationResult] = Nil
eOptP.ub.forNewCalleeContexts(seen, eOptP.e) { calleeContext =>

newCallers.forNewCalleeContexts(oldCallers, eOptP.e) { calleeContext =>
val theCalleeContext =
if (calleeContext.hasContext) calleeContext.asInstanceOf[ContextType]
else typeIterator.newContext(eOptP.e)
results ::= processMethod(theCalleeContext, tacEP)
results ::= processMethod(theCalleeContext, tacEOptP)
}

Results(
InterimPartialResult(Set(eOptP), continuationForCallers(eOptP.ub, tacEP)),
InterimPartialResult(Set(eOptP), continuationForCallers(newCallers, tacEOptP)),
results
)
}

def processMethod(
callContext: ContextType,
tacEP: EPS[Method, TACAI]
tacEOptP: EOptionP[Method, TACAI]
): ProperPropertyComputationResult

protected def continuationForTAC(
Expand All @@ -115,7 +100,7 @@ trait ReachableMethodAnalysis extends FPCFAnalysis with TypeConsumerAnalysis {
case UBP(tac: TACAI) if tac.tac.isDefined =>
processMethod(
propertyStore(declaredMethod, Callers.key),
null,
NoCallers,
someEPS.asInstanceOf[EPS[Method, TACAI]]
)
case _ =>
Expand All @@ -125,12 +110,28 @@ trait ReachableMethodAnalysis extends FPCFAnalysis with TypeConsumerAnalysis {

private[this] def continuationForCallers(
oldCallers: Callers,
tacEP: EPS[Method, TACAI]
tacEOptP: EOptionP[Method, TACAI]
)(
update: SomeEPS
): ProperPropertyComputationResult = {
val newCallers = update.asInstanceOf[EPS[DeclaredMethod, Callers]]
processMethod(newCallers, oldCallers, tacEP)
processMethod(newCallers, oldCallers, tacEOptP)
}
}

trait DefinedBodyReachableMethodAnalysis extends ReachableMethodAnalysis {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there seems to be only one ReachableMethodAnalysis that does process methods without body, might it be more sensible to switch the logic around, with the base trait not doing that and the specialized trait doing it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pondered this and did could not answer this question. On one hand, if the specialized trait is used far more than the base trait, something might be wrong. Additionally, the specialized trait right now restricts the abilities of the base trait (if you want to view "does not process but just returns no result" as a restriction) which feels a bit against the core principles of inheritance.

On the other hand, even if the base trait does not handle unknown bodies, it still needs to define extension points for

  1. The unknown body specialised trait to handle such unknown bodies
  2. A processMethod or similar that has an EOptionP as a parameter.

In short (using the flipped version): If you only extend the ReachableMethodAnalysis you should only need to implement the EPS version of processMethod since you know you have a body. If you extend the UnknownBodyReachableMethodAnalysis, you should only need to implement the EOptionP version of processMethod which can handle both scenarios.

All in all, I feel more comfortable with the current solution, but I am happy to replace it with a better one if you have some ideas.


override protected final def processMethodWithoutBody(
eOptP: EOptionP[DeclaredMethod, Callers]
): PropertyComputationResult = NoResult

override final def processMethod(
callContext: ContextType,
tacEPOpt: EOptionP[Method, TACAI]
): ProperPropertyComputationResult = processMethod(callContext, tacEPOpt.asEPS)

def processMethod(
callContext: ContextType,
tacEP: EPS[Method, TACAI]
): ProperPropertyComputationResult
}
8 changes: 4 additions & 4 deletions ...tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/SerializationRelatedCallsAnalysis.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ class OOSWriteObjectAnalysis private[analyses] (
val receiver = persistentUVar(param)
val parameters = Seq(receiverOption.flatMap(os => persistentUVar(os.asVar)))

implicit val state: CGState[ContextType] = new CGState[ContextType](
implicit val state: TACAIBasedCGState[ContextType] = new TACAIBasedCGState[ContextType](
callerContext,
FinalEP(callerContext.method.definedMethod, TheTACAI(tac))
)
Expand All @@ -101,7 +101,7 @@ class OOSWriteObjectAnalysis private[analyses] (
receiverVar: V,
receiver: Option[(ValueInformation, IntTrieSet)],
parameters: Seq[Option[(ValueInformation, IntTrieSet)]],
state: CGState[ContextType]
state: TACAIBasedCGState[ContextType]
)(eps: SomeEPS): ProperPropertyComputationResult = {
val pc = state.dependersOf(eps.toEPK).head.asInstanceOf[Int]

Expand All @@ -126,7 +126,7 @@ class OOSWriteObjectAnalysis private[analyses] (
receiver: Option[(ValueInformation, IntTrieSet)],
parameters: Seq[Option[(ValueInformation, IntTrieSet)]],
indirectCalls: IndirectCalls
)(implicit state: CGState[ContextType]): ProperPropertyComputationResult = {
)(implicit state: TACAIBasedCGState[ContextType]): ProperPropertyComputationResult = {
val results = indirectCalls.partialResults(state.callContext)
if (state.hasOpenDependencies)
Results(
Expand All @@ -144,7 +144,7 @@ class OOSWriteObjectAnalysis private[analyses] (
receiver: Option[(ValueInformation, IntTrieSet)],
parameters: Seq[Option[(ValueInformation, IntTrieSet)]],
indirectCalls: IndirectCalls
)(implicit state: CGState[ContextType]): Unit = {
)(implicit state: TACAIBasedCGState[ContextType]): Unit = {
typeIterator.foreachType(
param,
typeIterator.typesProperty(param, callContext, callPC.asInstanceOf[Entity], state.tac.stmts)
Expand Down
Loading
Loading