[PATCH] check for Incorrect logic in operator

[Ted Kremenek]

Hi Richard,

Great feedback.  Comments inline.

On Oct 28, 2013, at 3:16 PM, Richard Trieu <rtrieu at google.com> wrote:

> On Mon, Oct 28, 2013 at 11:47 AM, Ted Kremenek <kremenek at apple.com> wrote:
> On Oct 28, 2013, at 11:41 AM, Ted Kremenek <kremenek at apple.com> wrote:
> 
>> On Oct 25, 2013, at 4:42 PM, Richard Trieu <rtrieu at google.com> wrote:
>> 
>>> If you check out -Wlogical-not-parentheses and -Wtautological-compare-out-of-range, they are in lib/Sema/SemaChecking.cpp.  Why did you use a CFG approach instead?
>> 
>> Hi Richard,
>> 
>> I told Anders that modifying the CFG builder would be an interesting approach for two reasons:
>> 
>> (1) The CFG is used for a variety of dataflow analyses, thus folding this “smarts” into the CFG builder seems to have dividends for other uses.
>> 
>> (2) The CFG builder already does some branch pruning by doing some simply analysis of expressions at branches.  This seemed like a natural enhancement.
>> 
>> I did consider this to take a similar approach to -Wlogical-not-parentheses, but thought this would be a more promising direction to explore.
>> 
>> Flipping the question around, do you think it would be interesting to consider the opposite approach and move the logic to -Wlogical-not-parentheses, etc., into the CFG builder, for the same reasons I have given above?
>> 
>> Ted
> 
> Further, there is a very natural relationship between *all* of these warnings and -Wunreachable-code (a warning we’d one day like to fully productize), which is built directly on top of the CFG.  Bringing the underlying pieces closer together seems like a promising direction to explore.
> 
> My worry is that using the CFG won't check all the code a Sema based warning would (ignoring intentionally skipped dead/unreachable code).  This would then require code in both the CFG and Sema for a warning to cover all cases.

This is an interesting point.  Let’s first explore the “CFG won’t check all the code a Sema based warning would”.  I think there is some truth to that, but let’s see if we are on the same page.

First, the CFG doesn’t get constructed for malformed code.  If it is important to issue these warnings for malformed code, then it should go in Sema.  One could argue that this is a feature.  Malformed code in a function body is, well, malformed, and we are going to issue an error for it.  Mentioning additional semantic warnings on malformed code doesn’t necessarily add value.  We already have this user model for some warnings, such as -Wuninitialized.  The end-user doesn’t know that -Wuniniitalized is implemented on top of the CFG.  They just know they won’t get this warning if the function body isn’t well-formed.  That’s clearly the right user model for -Wuninitialized, regardless of the underlying implementation.  For these other warnings, I think it could be debated either way whether or not they should be tied to the CFG in this way.

Second, we don’t construct the CFG for code in system headers, unless -Wsystem-headers is uttered (although that might not be working as expected right now, need to check).  This is consistent with how other warnings are suppressed, so this isn’t an issue.

Third, CFGs are constructed for template instantiations, but not uninstantiated templates.  I suspect the Sema warnings also have similar kind of refinements.

Finally, there are cases where a CFG is not being constructed because the CFG builder has incomplete language coverage.  This is a problem that just needs to be fixed.  Very important warnings, such as -Wuninitialized and -Wreturn-type, are based on the CFG.  Getting full CFG coverage seems like in our best interests anyway.

With these points in mind, are there particular concerns about cases where the “CFG won’t check all the code a Sema based warning would”.  If you addressed the last point, I think you’d pretty much get the coverage that you want.  What do you think?


>  Clang already has this for the self-reference on initialization warning.  If you have "int x = x + x;" in a function scope, the CFG-based uninitialized checker will produce the warning.  If that same code is in a global scope, the AST-based check is performed in Sema.  I think it's better to have the warning in one place, but that may differ by warning.

Interesting point, and I agree with your last point that this may differ (significantly) by warning.  The uninitialized value warnings are a bit more complicated because they are flow-sensitive, so they are a bit hairier to refactor to serve both masters.  But for the warnings we are talking about, I could see some fairly intuitive refactoring.

For example, with the warnings Anders is proposing, or -Wtautological-compare, essentially we have some logic that recognizes a particular pattern.  Anders put his logic in CFG.cpp, but it could be extracted out elsewhere.  That logic doesn’t need to live in the CFG builder, and could be used in multiple places if it makes sense to do so.  In this case, Anders warning is riding off the CFGBuilder doing a walk of the AST.

> 
> Some warnings might benefit having its results be fed back into CFG, such as using -Wtautological-compare for branch analysis, although the enum part might need to be changed.  Other warnings, such as warnings based on size would be good to silence on unreachable code.  On the other hand, -Wlogical-not-parentheses doesn't provide any information to the CFG, and is almost always a true positive so warning on unreachable code is desirable.

After thinking about this some more, maybe the guidance here is that we should factor some of this “smarts” into separate logic in libAnalysis.  This logic could be used both by Sema to issue warnings (e.g., do the check right after type checking an IfStmt), but also could be fed into the CFG builder.  At least then we would be reusing this logic, allowing more parts of the compiler to use it, and allow the dataflow-based warnings to benefit from this additional intelligence.  The one downside I see here is that this logic will get redundantly called, both by the CFG builder and by Sema.  That was part of my motivation to explore riding this off the CFG construction.

Richard/Jordan/Anders: what do you think?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131029/5b59709e/attachment.html>