|
9 | 9 | import com.ibm.cldk.schema.JParameter; |
10 | 10 | import com.ibm.cldk.schema.JType; |
11 | 11 | import com.ibm.cldk.schema.Span; |
| 12 | +import com.ibm.cldk.syntactic_analysis.controlflow.ControlFlowGraph; |
12 | 13 | import java.nio.charset.StandardCharsets; |
13 | 14 | import java.util.ArrayDeque; |
14 | 15 | import java.util.ArrayList; |
|
36 | 37 | * <p>The relation is derived <em>syntactically</em>, in the weak-update (may-flow, never-drop) |
37 | 38 | * posture the L4 design takes: a parameter reaches the return if the callable's {@code ddg} carries |
38 | 39 | * it there, if a call site passes it on and that callee's own summary returns it, or if the |
39 | | - * parameter is simply <em>named</em> in a body node's source text. That last rule is what makes the |
40 | | - * pass useful at all on ordinary code, because {@link DdgBuilder} gives a parameter no defining node |
41 | | - * — nothing in the {@code ddg} is ever rooted at one. Without it {@code int c(int z) { return z - 3; |
42 | | - * }} has no def-use to follow at all, and {@code int m(int q) { int t = q; return t; }} has only |
43 | | - * {@code (decl → ret, var "t")}, which no seed reaches. The cost is counting a parameter merely |
| 40 | + * parameter is simply <em>named</em> in a body node's source text. That last rule is what made the |
| 41 | + * pass useful at all on ordinary code back when {@link DdgBuilder} gave a parameter no defining node, |
| 42 | + * so nothing in the {@code ddg} could be rooted at one: {@code int c(int z) { return z - 3; }} had no |
| 43 | + * def-use to follow at all, and {@code int m(int q) { int t = q; return t; }} had only |
| 44 | + * {@code (decl → ret, var "t")}, which no seed reached. The cost is counting a parameter merely |
44 | 45 | * <em>mentioned</em> in a statement as flowing out of it. Over-approximating there is the accepted |
45 | | - * trade. |
| 46 | + * trade. {@code DdgBuilder} now defines the formals at {@code @entry}, so the ddg rule reaches these |
| 47 | + * shapes on its own and the two text rules have been measured redundant on both corpora; retiring |
| 48 | + * them is a separate change. |
46 | 49 | */ |
47 | 50 | public final class SummaryPass { |
48 | 51 |
|
@@ -204,11 +207,12 @@ private static Fn facts(JCallable c, byte[] source) { |
204 | 207 | } |
205 | 208 |
|
206 | 209 | /** |
207 | | - * Where a parameter's value is visible, syntactically: the def end of any {@code ddg} edge whose |
208 | | - * access path is rooted at it, any call-site {@code actual_in} vertex whose argument text names |
209 | | - * it, and <em>every</em> body node whose source text names it (see the class javadoc — a parameter |
210 | | - * has no defining node in the {@code ddg}, so text is the only thing that can put it on the map at |
211 | | - * all). |
| 210 | + * Where a parameter's value is visible, syntactically: an end of any {@code ddg} edge whose |
| 211 | + * access path is rooted at it — the def end for an ordinary edge, the <em>use</em> end for one |
| 212 | + * rooted at {@code @entry} (see the comment on that rule) — any call-site {@code actual_in} |
| 213 | + * vertex whose argument text names it, and <em>every</em> body node whose source text names it |
| 214 | + * (see the class javadoc for why the two text rules exist and why they now have nothing left to |
| 215 | + * add). |
212 | 216 | * |
213 | 217 | * <p>Every kind with a span is seedable, containers included. A {@code branch}/{@code loop}/ |
214 | 218 | * {@code switch} span swallows the statements nested inside it, so seeding one on a name mentioned |
@@ -241,7 +245,37 @@ private static Set<String> seedsFor(Fn fn, String name, Map<String, String> span |
241 | 245 | if (ddg != null) { |
242 | 246 | for (JDdgEdge e : ddg) { |
243 | 247 | if (name.equals(base(e.getVar()))) { |
244 | | - seeds.add(e.getSrc()); |
| 248 | + // For an entry-rooted edge, the *use* end — the obvious `e.getSrc()` is wrong |
| 249 | + // there. A seed is a node id, and the graph `facts` builds is keyed on node |
| 250 | + // identity alone (it discards `var`), so seeding a def node also hands this |
| 251 | + // parameter the out-edges of every *other* variable defined at that node. |
| 252 | + // `@entry` is where that bites hardest: `DdgBuilder` defines all of a callable's |
| 253 | + // formals at that one synthetic node, so `getSrc()` collapses every parameter's |
| 254 | + // seed set to `{@entry}` and lets one parameter's reach credit them all — 43 |
| 255 | + // spurious summary edges on daytrader8, every one of them on a callable of arity |
| 256 | + // greater than 1. The dst is where *this* parameter's value is observed, which |
| 257 | + // is exactly what the `var`-filtered edge licenses and no more. |
| 258 | + // |
| 259 | + // A real def site keeps its `src`. The same conflation happens there in |
| 260 | + // miniature, but it is the bounded may-flow over-approximation this pass already |
| 261 | + // accepts. Doing this unconditionally was measured against the pre-change |
| 262 | + // baseline and found to cost three daytrader8 edges at |
| 263 | + // `TradeDirect.completeOrder(Connection, Integer)`, all under the `var` label |
| 264 | + // `orderID` — but that label is nominal, not semantic: those edges are rooted at |
| 265 | + // the node defining `orderData` and reach a `getOrderID()` call on it, so `var` |
| 266 | + // names the *field* `OrderDataBean.orderID`, not this callable's own parameter of |
| 267 | + // the same spelling. The parameter reaches the return, if at all, only through |
| 268 | + // JDBC that neither this engine nor WALA models, so the three are a conflation of |
| 269 | + // the same class this fix removes, not a real flow it would drop. |
| 270 | + // |
| 271 | + // The scoping still stops at `@entry`, though — for a narrower reason than "it |
| 272 | + // would cost something real". This branch is a regression fix: its job is |
| 273 | + // restoring the baseline it perturbed, not auditing every other instance of this |
| 274 | + // same over-seeding. And the L4 posture forbids under-approximation on evidence |
| 275 | + // this thin — widening `src` to `dst` for every rule-1 edge, not just entry-rooted |
| 276 | + // ones, is measured safe on only two corpora, not enough to rule out dropping a |
| 277 | + // real flow somewhere neither has been run. That burden belongs to a follow-on. |
| 278 | + seeds.add(ControlFlowGraph.ENTRY.equals(e.getSrc()) ? e.getDst() : e.getSrc()); |
245 | 279 | } |
246 | 280 | } |
247 | 281 | } |
|
0 commit comments