[PATCH] check for Incorrect logic in operator

Richard Trieu rtrieu at google.com
Tue Oct 29 19:14:29 PDT 2013

On Tue, Oct 29, 2013 at 5:02 PM, Ted Kremenek <kremenek at apple.com> wrote:

> 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.
> I agree, we don't necessarily need warnings on malformed code.

> 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.

Also reasonable.

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

Is this because we chose not to process them or is this a limitation of the
CFG?  Yes, many warnings special case template code.

> 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?
> I think that globals and global initializers are not represented in the
CFG.  That's my main concern about using only the CFG at the moment.

>   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.

I wasn't complaining about -Wuninitialized.  That warning works pretty
well.  I was trying to point out that we need a different checker for
globals.  At least for self-reference checking, there's already an existing
Sema-based version to piggy-back on for the globals.  Can the CFG cover
this case or will we need to run a separate pass over them?

> 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.
> This is a good point.  Many warnings are interwoven within the
construction of the AST itself.  It makes sense to separate the AST
construction from the warning diagnostics where possible.  Certain warnings
that are merely pattern matching would have little difference between being
AST or CFG based, but others would benefit from ignoring dead code.
 Certain cases of -Wconstant-conversion and -Wtautological-compare come to

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

More information about the cfe-commits mailing list