<div class="gmail_quote"><div class="gmail_quote">Sorry, I still forget to choose "Replay all" option...</div><div class="gmail_quote"><br></div><div class="gmail_quote">W dniu 20 sierpnia 2010 05:35 użytkownik Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> napisał:<div class="im">
<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"><br><div><div><div>On Aug 18, 2010, at 12:11 AM, Marcin Świderski wrote:</div><br><blockquote type="cite">Hi Jordy, Ted<div><br></div><div>Thanks for the appreciation and for comments. Some comments inline:</div>
<div><br><div class="gmail_quote">W dniu 18 sierpnia 2010 07:47 użytkownik Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> napisał:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Wow, good work.<br>
<br>
I agree with most of what Ted said. A few additional comments:<br>
<br>
- I like the single enum rather than kind and subkind, but there are 7<br>
elements, not 6. So it won't fit in a PointerIntPair anyway, though having<br>
a Stmt* field (or void*) and an enum field would be preferable to two<br>
PointerIntPairs.<br>
I'm still not totally convinced we need scope markers in the CFG, though.<br>
<br></blockquote><div>For both destructor kinds I will need space for two pointers. For automatic object I need it's declaration and statement that cause destructor to be called. For temporary object I need expression bound to the temporary and the full expression that contains the temporary.</div>
</div></div></blockquote><div><br></div></div><div>Repeating what I said in my reply to Jordy, I think it's fine to burn an extra pointer if it gives us flexibility to track more information that would be useful for diagnostics.</div>
<div><br></div></div></div></blockquote></div><div>So I have three acceptable solutions.</div><div>1. Two pointers for each element.</div><div>2. One pointer for each element, but in case of destructor it points to allocated pair of pointers.</div>
<div>3. One pointer for each element, and location of call to destructor is calculated on demand.</div><div><br></div><div>1 is wasteful, because only destructors need two pointers. 3 will give as possibly unclean API for clients. 2 seams to be the best and I will implement it.</div>
<div class="im">
<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><blockquote type="cite"><div><div class="gmail_quote">
<div><br></div><div>I could allocate separate struct for those and point to it from CFGElement. If we drop the scopes and move information about kind of destructor to this struct, then CFGElement will need only one PointerIntPair.</div>
</div></div></blockquote><div><br></div></div><div>That is also a possibility. It could be allocated from the BumpPtrAllocator.</div><div><br></div></div></div></blockquote></div><div>When will be the memory allocated with BumpPtrAllocator freed? If I will allocate memory for use in one instance of CFG, can I forget about it (not call Deallocate)? </div>
<div class="im">
<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><blockquote type="cite"><div><div class="gmail_quote">
<div><br></div><div>If we would want scopes modeled we could use same struct as for destructors. So we would end up with something like this:</div><div><br></div><div>- Statement and LValStatement with pointer to Stmt*,</div>
<div>- Initializer with pointer to CXXBaseOrMemberInitializer*,</div><div>- Extendend CFGElement with pointer to struct with information about destructor or scope.</div><div><br></div><div>Our kind enum would look like this:</div>
<div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse">enum Kind {<br> Statement,<br> LvalStatment,<br> BEGIN_STATEMENT = Statement,<br> END_STATEMENT = LvalStatement,<br>
Initializer,</span></div><div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"><br></span></div><div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"> ExtendedElement, // This would be internal, client would never get this</span></div>
<div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"> // with getKind() method.</span></div><div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"><br>
AutomaticObjectDtor,<br> TemporaryObjectDtor,<br> BEGIN_DTOR = AutomaticObjectDtor,<br> END_DTOR = TemporaryObjectDtor,<br> StartScope,<br> EndScope,<br> BEGIN_SCOPE = StartScope,<br> END_SCOPE = EndScope<br>};</span></div>
</div></div></blockquote><div><br></div></div><div>What is the purpose of the 'ExtendedElement' kind?</div><div><br></div></div></div></blockquote></div><div>Int part of PointerIntPair where pointer part points to pointer can have only 2 bits. ExtendedElement kind marks that the kind (- int(ExtendedElement)) is stored in int part of first PointerIntPair in pointed struct. It is an implementation detail realy.</div>
<div class="im">
<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><blockquote type="cite"><div><div class="gmail_quote">
<div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse"><br></span></div><div><span style="font-family:arial, sans-serif;font-size:13px;border-collapse:collapse">This way we will have space efficient CFG for Obj-C and C languages. For C++ CFG will occupy less memory if there will be more statement then destructor CFGElements.</span></div>
<div><br></div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
- Why don't we make this a static type hierarchy like SVal or MemRegion?<br>
We'd have CFGStmt, CFGInitializer, CFGDestructor, and CFGScope(Marker)<br>
inheriting from CFGElement. That way we can have, among other things, a<br>
CFGStmt that simplifies to a Stmt. Instead of asserts, we can just have the<br>
type-specific methods on the subclasses.<br>
<br></blockquote><div>Yes, this will be much better approach. I will change this.</div></div></div></blockquote><div><br></div></div>Sounds great.</div><div><div> </div></div></div></blockquote></div><div>The hierarchy is almost ready. I'll send a patch with this and both destructors implemented after weekend.</div>
<div class="im">
<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></div></blockquote><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><blockquote type="cite"><div><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">
- In UnreachableCodeChecker::getUnreachableStmt(), isn't the block<br>
guaranteed to have an initial statement for the same reason as in<br>
ReachableCode.cpp?<br>
<br></blockquote><div>Do you mean that if block is unreachable it will start with a statement and not with initializer, destructor or scope?</div></div></div></blockquote><div><br></div></div><div>Yes, an unreachable block could certainly start with an initializer if it had no initializer argument and the previous initializer called a no-return function. Gross, but I guess possible.</div>
</div><br><div>Moreover, a call to builtin_unreachable is not guaranteed to be the first CFGElement in a CFGBlock. I think the checker's logic is likely wrong here. It should be iterating over the block, looking for this specific CallExpr.</div>
</div></blockquote></div></div>
</div><br>