Dataflow: Refactor pruning stages. by aschackmull · Pull Request #4668 · github/codeql

aschackmull · 2020-11-13T14:25:36Z

This is a rather large refactor of the pruning stages of the data-flow library. The end result is that stages 2, 3, and 4 are almost bitwise identical (except for 2 instances of unbindBool in stage 3, and 2 missing lines in fwdFlow0/fwdFlow in stage 2). The idea is that this is now the desugared version of an imagined use of parameterised modules. It would be nice to also make stage 1 identical to the other stages, but I've postponed that for now.

Commit-by-commit review is strongly encouraged.

I've tested for bad join-orders and general performance issues between each commit for Java, but the end result should definitely be checked for bad join-orders in all languages.

There are also some pruning improvements, in particular since filtering on potential parameter-flow-through wasn't done consistently. Making the stages share this logic ensures that this additional pruning is applied in all stages.

This column is functionally determined from the access path, and was merely included to help with some join-orders that no longer appear problematic.

This commit is a little bit risky, as it allows for some potentially bad join-orders. The best order starts with the delta and proceeds with the then functional `mid.getEnclosingCallable()` and `getLocalCallContext`. In this order `localFlowEntry` becomes superfluous. The standard order is however somewhat unwilling to choose this. If it picks `getLocalCallContext` and `getEnclosingCallable` as the first join, the result is really bad, but it appears that the existence of `localFlowEntry` at least means that it'll do `localFlowEntry`, `getEnclosingCallable`, `getLocalCallContext` in that order, which appears to be acceptable, although it isn't optimal. Without the `localFlowEntry` conjunct we end up with the worst case. We'll need to watch this particular join-ordering until we get better join-ordering directives.

This constructs a few more tuples in Stage3::fwdFlow0, which are then filtered in Stage3::fwdFlow. This is cleaner and appears faster.

Post-recursion we can filter the forward cons-candidates to only include those that met a read step, and similarly restrict the reverse flow cons-candidates to those that met a store step.

This actually provides a decent pruning improvement in stages 3 and 4.

aschackmull · 2020-11-16T10:33:35Z

aschackmull · 2020-11-17T09:13:29Z

Retrying Java: https://jenkins.internal.semmle.com/job/Changes/job/Java-Differences/998/
C++ and Python look good, but it looks like C# has hit a bad join-order in a standard order (at least that's the most likely conclusion given that the perf problem only hits one project).

aschackmull · 2020-11-17T13:31:27Z

Retrying C# with join-order fix:
~~https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/627/~~
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/632/

aschackmull · 2020-11-19T12:03:54Z

All jobs done and looking good.

hvitved

This is really awesome, thanks for taking on this task ✨

hvitved · 2020-11-26T09:36:46Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

 }

 private module Stage2 {
+  class ApApprox = Stage1::Ap;


It looks like this is always PrevStage::Ap, so I would either call it class PrevStageAp = PrevStage::Ap or just use PrevStage::Ap directly.

It ends up as class ApApprox = PrevStage::Ap; in a later commit (I tried minimising commits with changes that were obsoleted by later commits, but it was hard to avoid completely).

Right, but what I meant was I am not a fan of the ApApprox name. I would rather have PrevStageAp or just PrevStage::Ap.

hvitved · 2020-11-26T09:58:11Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll


  class Cc = boolean;

+  /* Begin: Stage 1 logic. */


I think I would prefer a module instead of begin/end comments.

It would just be a marker-module to be immediately imported, so I prefer the comments.

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

hvitved · 2020-11-26T10:06:40Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

  pragma[nomagic]
  private predicate fwdFlowOutFromArg(
-    DataFlowCall call, Node out, boolean argAp, Ap ap, Configuration config
+    DataFlowCall call, Node node, Ap argAp, Ap ap, Configuration config


What was wrong with calling it out?

Nothing, I guess. IIRC it was inconsistently named so I just chose one - do you prefer out over node here?

I would prefer out because that is the name used by fwdFlowOut.

hvitved · 2020-11-26T10:19:09Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

+  private Ap apCons(TypedContent tc, Ap tail) { result = true and exists(tc) and exists(tail) }
+
+  pragma[inline]
+  private Content getHeadContent(Ap ap) { exists(result) and ap = true }


What happens if you remove these predicates, and instead replace their single use with

ap = apCons(any(TypedContent tc | c = tc.getContent()), _)

Stage2::apCons will have to be changed to

pragma[inline] private Ap apCons(TypedContent tc, Ap tail) { result = true and exists(tc) and tail in [false, true] }

That would correspond to a pop of an Ap just to get the head, so we'd introduce a needless fan-out, which we would then hope that the compiler could eliminate in all stages. Besides this risk I think it would be more confusing, so I don't think it's worth it just to eliminate one predicate-parameter.

hvitved · 2020-11-26T12:48:26Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

+    not fullBarrier(node, config) and
+    not inBarrier(node, config) and
+    not outBarrier(node, config)


Aren't these redundant?

Not quite. But we don't have a lot of use for in- and out-barriers, so they are certainly very corner-case. (We'd need something like a parameter that's simultaneously part of through-flow and a source node and an in-barrier). Arguably these barriers could have been checked a little earlier, but they aren't.

The fullBarrier check is done in fwdFlow.

Ah, right. Good catch.

hvitved · 2020-11-26T12:54:45Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

+      // we don't expect a parameter to return stored in itself
+      not exists(int pos |
+        kind.(ParamUpdateReturnKind).getPosition() = pos and p.isParameterOf(_, pos)
+      )


[Not related to your PR] I am not sure that this restriction makes sense anymore, now that flow summaries are content-sensitive. It is not unrealistic for a function to transfer data from one component of a parameter to another.

That's potentially annoying to deal with as this restriction actually cuts off significant amounts of nonsense flow IIRC.

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

hvitved · 2020-11-26T13:03:52Z

java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll

+  bindingset[node, ap]
+  private predicate filter(Node node, Ap ap) { any() }
+
+  bindingset[ap, contentType]
+  private predicate typecheckStore(Ap ap, DataFlowType contentType) { any() }


Perhaps add comments that these are trivial because filtering/type checking happens in previous stage.

hvitved · 2020-11-26T13:05:50Z

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowPrivate.qll

+/** The unit type. */
+private newtype TUnit = TMkUnit()
+
+/** The trivial type with a single element. */
+class Unit extends TUnit {
+  /** Gets a textual representation of this element. */
+  string toString() { result = "unit" }
+}
+


This should be in DataFlowImplCommon.qll instead.

It can't, because for Java the Unit type is in the common library.

I don't see why that is a problem, as DataFlowImplCommon.qll is not exposed.

Right, that ought to work if no instance of import java leaks through. Currently there is such a leak (otherwise I couldn't refer to the Java version of Unit), so it wouldn't work without fixing that (but that should probably be separate from this PR as it could break things if someone has referenced a non-data-flow Java QL class through a DataFlow:: prefix). Another angle on this is that we should possibly push towards all the languages adopting a common Unit.qll, and then use that instead of introducing such a fundamental class in DataFlowImplCommon.qll - it has other uses. In summary, I think the definition should still come from DataFlowPrivate.qll, but it should be in the form of import semmle.code.Unit.

Currently there is such a leak

:-(

aschackmull added 30 commits November 13, 2020 15:09

Dataflow: Add modules.

aab5263

Dataflow: Rename predicates.

f3f968c

Dataflow: Rename some types and variables.

3f25df9

Dataflow: More renaming.

586d52f

Dataflow: Stage comments and some formatting.

1fe4235

Dataflow: Remove AccessPathFront column.

7eeae49

This column is functionally determined from the access path, and was merely included to help with some join-orders that no longer appear problematic.

Dataflow: Rename option type branches.

c054295

Dataflow: Minor refactor of Stage2::revFlow.

60b5101

Dataflow: Align on ApNil.

0a60a3a

Dataflow: Rename stage 2 cons-cand predicates.

bfd8a3d

Dataflow: A few variable renamings.

628e0a7

Dataflow: Refactor forward stores and remove some useless conjuncts.

261ef0f

Dataflow: Refactor stage 3 conscand predicates.

b0e5925

Dataflow: Refactor stage 2 read and stores.

2afc572

Dataflow: Refactor stage 4 read and stores.

00d726d

Dataflow: Minor reorderings and renamings.

d037909

Dataflow: Some renamings.

dc2b2cc

Dataflow: Refactor call contexts.

03ef9d0

Dataflow: Remove superfluous conjuncts.

7a95466

Dataflow: Reshuffle a few conjuncts.

19a9285

Dataflow: Align on localStep.

0a4c680

Dataflow: Align some qldoc.

aa28fdb

Dataflow: Reorder, rename, and add columns to store-flow.

12fe38b

Dataflow: Refactor step relation in revFlowStore.

b6f1ab6

Dataflow: Refactor forward store step relation.

c5a2c26

Dataflow: Refactor step predicate in fwdFlowRead.

3e18e02

Dataflow: Get rid of early filter.

5a1c0e9

This constructs a few more tuples in Stage3::fwdFlow0, which are then filtered in Stage3::fwdFlow. This is cleaner and appears faster.

Dataflow: Reshuffle some predicates.

e4fb415

Dataflow: Improve cons-cand relation.

8b5e452

Post-recursion we can filter the forward cons-candidates to only include those that met a read step, and similarly restrict the reverse flow cons-candidates to those that met a store step.

aschackmull added 8 commits November 13, 2020 15:09

Dataflow: Align on parameterMayFlowThrough.

786edbf

This actually provides a decent pruning improvement in stages 3 and 4.

Dataflow: Renamings.

6e6e5d6

Dataflow: Align more predicates.

aa66b9b

Dataflow: Change some headUsesContent to getHead.

d028e6b

Dataflow: Make a bunch of the interface predicates private.

293429f

Dataflow: Some qldoc.

d324cd1

Dataflow: Sync.

e0a6a48

Dataflow: Remove headUsesContent.

9e45f10

aschackmull requested a review from hvitved November 13, 2020 14:25

aschackmull requested review from a team as code owners November 13, 2020 14:25

github-actions bot added C# C++ Java Python labels Nov 13, 2020

Dataflow: Add Unit type for all languages.

3dbd480

Dataflow: Fix bad join-orders.

f74fc0f

jbj assigned hvitved Nov 19, 2020

hvitved requested changes Nov 26, 2020

View reviewed changes

aschackmull added 2 commits November 27, 2020 12:16

Dataflow: Review fixes.

8f4fce1

Dataflow: Sync.

fec9758

hvitved approved these changes Nov 27, 2020

View reviewed changes

aschackmull merged commit 931322e into github:main Nov 30, 2020

aschackmull deleted the dataflow/refactor-pruning branch November 30, 2020 08:37

Conversation

aschackmull commented Nov 13, 2020

Uh oh!

aschackmull commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented Nov 17, 2020

Uh oh!

aschackmull commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented Nov 19, 2020

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

aschackmull commented Nov 16, 2020 •

edited

Loading

aschackmull commented Nov 17, 2020 •

edited

Loading