<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 27, 2014 at 1:10 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div class="">On Feb 27, 2014, at 11:22 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
</div><div><div class=""><br><blockquote type="cite"><span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">I'm not sure I'm entirely following you here (sorry, maybe just</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">haven't got enough CFG-related code-state in my head at this point,</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">it's been a while since I thought about this). I did notice your</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">AdjacentBlock has support for this "alternative" situation you've</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">described. When does this come up?</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
</blockquote><div><br></div></div><div>Yes, and we have code in there now that uses it.</div><div><br></div><div>Here is a specific example:</div><div><br></div><div>    noreturn_function();</div><div>    break;</div><div>
<br></div><div>In the pruned CFG, 'noreturn_function()' will be in its own basic block and ‘break;’ in another, and there will not be an edge between them.  Indeed, the block with 'noreturn_function()' will have a special block as its successor for noreturn function calls.  In ReachableCode.cpp, we look to see what would have been the alternate predecessor to ‘break;’ in the CFG, which allows us to determine that it was immediately preceded in the original unprunned CFG by a call to this noreturn function.  This is a special case we wish to elide as a warning because it adds no value.</div>
</div></div></blockquote><div><br></div><div>OK. So the "new" thing here that I hadn't anticipated was the extra edge for noreturn functions - my original implementation was just that "unreachable code needs a superset of edges of other analyses", these edges are an example of that not being true (where the other analyses have edges that unreachable code doesn't have). Good to know. Thanks for the example.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>We could recover the same kind of information in other ways (e.g., groveling the AST), but they would be very complicated and error prone.  Looking at the pure control-flow gives us the information we need.<br>
</div></div></div></blockquote><div><br></div><div>Sure - I totally agreed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div></div><div class=""><br><blockquote type="cite"><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">Do any analyses need to know about both the reachable and the</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">alternative - or is it just the case that any given analysis wants one</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">or the other?</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
</blockquote><div><br></div></div><div>Most will just care about the reachable blocks.  That said, for successors we generally expect them to be laid out in a certain way for some clients of the CFG, such as the core static analyzer engine.  I thus see 3 kinds of uses:</div>
<div><br></div><div>(1) Clients that just want to *iterate* the reachable successor/predecessors.</div><div><br></div><div>(2) Clients that want to possibly iterate the successor/predecessors, but also want to know *which* successor they are looking at.  For example, if we have a block terminated an ‘if’, we can expect the ‘then’ block to be the first successor and ‘else’ block to be the second successor.  Perhaps we should encode this in a more natural way in the CFG, but that is how it is represented now.  Such clients will also be able to see if a given branch is infeasible by construction by seeing that the successor/predecessor is NULL.  Both -Wuninitialized and ExprEngine rely on such details.</div>
<div><br></div><div>(3) Clients that care about the blocks that *would* have been reachable.  Currently -Wunreachable-code is the only client of this interface.  For such clients, the probably will want to know in some cases which successor/predecessor a block was, and not just want to iterate over them (which loses this detail).</div>
</div></div></blockquote><div><br></div><div>Sorry, I'm not sure I understand your last sentence there - you mean an unreachable-code (optimistic/unpruned/whatever we want to call it) -like navigation of the graph, but with the ordering/null gap guarantees of (2) above? OK.<br>
<br>(hmm - would an unpruned graph ever have those null gaps in the successors? Or are they only ever used to indicate impossible branches that we've discovered due to constant folding - well I suppose some constant folding will still be used in the unpruned graph, just not /certain/ constant folding (sizeof, dependent expressions, etc))</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class=""><br><blockquote type="cite"><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">So long as no analysis needs to actually correlate the alternatives -</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">ie: so long as they only care about being entirely optimistic or</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">entirely pessimistic about edges/reachability, the filtering API still</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">seems like a rather clean(er) approach. Though admittedly my initial</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">implementation assumed that the set of optimistic edges was a superset</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">of the pessimistic edges - if there are alternatives then that's not</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">true and I'd need 2 bits (as you've used in AdjacentBlock) rather than</span><br style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<span style="font-family:Helvetica;font-size:14px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">1 in the edge to filter on, but that's easily do-able.</span></blockquote>
<br></div></div><div>I think a better (new) starting place would be to identify the problems that a revised iterator API would solve.  I don’t doubt they exist, but I’d rather start from there rather than prescribing it as a solution because that’s what we came up with before.  The observation that we need 2 bits is a revelation I made last night when implementing extra heuristics in -Wunreachable-code.  In some ways, I’d rather push the current design a little further, flush out more of the design considerations, and then clean it up.</div>
</div></blockquote><div><br>Sure - if you like. My only concern with doing it this way is that the incumbent solution tends to look a bit more appealing over time when looking at how invasive a redesign will be (even now, the extra null checks that have been inserted are now already there (and won't necessarily be immediately discoverable if changing to an filtering iterator (they'd just become always-true tests on pointers)) so the cleanup doesn't look as significant).<br>
<br>But to list a few of my thoughts in comparison between the two solutions:<br><br>* asymmetry between successor and predecessor sets seems confusing/problematic (but I don't work with these structures often, so I don't know how problematic that really is - you mentioned in (3) above that successor unpruned edges might be needed eventually?)<br>
<br>* the explicit querying of alternative edges stored in the same entry seems like a needless burden on callers (since none seem to care about the fact that /this/ edge is an alternative to /this/ edge - what they care about is the optimistic or pessimistic edges) - essentially an "alternate" edge could be encoded as one unreachable edge and one reachable edge with no loss of functionality. And that's all the consumers really care about (& have to care about unreachable, but not alternative, edges - so by having both kinds it just burdens the unreachable code analysis to consider both kinds when they're the same thing).<br>
<br>Does that seem reasonable/make sense?<br><br></div><div>(side thought: actually those null edges in the successor list could totally use the extra bits to determine if they're null or not (PointerIntPair?) that way they'd come at no cost - if you navigate for "optimistic reachability" they're non-null and otherwise they're null - but that'd just be a minor space optimization compared to putting two edges in, one optimistic one pessimistic... - hmm, since I didn't have pessimistic-only edges, I wonder how my filtering iterator solution even worked for this case... *ponder*)</div>
</div></div></div>