[PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[Ted Kremenek via cfe-commits]

krememek added a comment.

LGTM.


Repository:
  rL LLVM

https://reviews.llvm.org/D24010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D25503: [analyzer] Remove superquadratic behaviour from DataflowWorklist

[Ted Kremenek via cfe-commits]

krememek added a comment.

Looks great to me too.  Thanks for doing this!


Repository:
  rL LLVM

https://reviews.llvm.org/D25503



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10021: Refactor: Simplify boolean conditional return statements in lib/StaticAnalyzer/Checkers

[Ted Kremenek via cfe-commits]

krememek added a subscriber: krememek.
krememek accepted this revision.
krememek added a reviewer: krememek.
krememek added a comment.
This revision is now accepted and ready to land.

These all look good to me.


http://reviews.llvm.org/D10021



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [libcxx] r249738 - Split out of .

[Ted Kremenek via cfe-commits]

> On Oct 14, 2015, at 3:49 PM, Duncan P. N. Exon Smith  
> wrote:
> 
> My understanding is that we don't expect clang to be used with *newer* libc++ 
> headers, just older or equal.  This kind of break is a little unfortunate, 
> but probably fine.  Ted, can you confirm?

On Apple platforms, we will always be shipping new clangs whenever the libc++ 
headers are updated, so I see no concerns there.  That’s pretty much in line 
with how we handle our headers + compiler changes in general at Apple.  For 
example, we rolled out new Objective-C changes in the last update that were 
widely and unconditionally adopted in our headers.  Even the Clang we released 
the previous year can no longer parse those headers because of those language 
changes.


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r247510 - Use -f instead of -d flag for testing existing of clang executable (http://reviews.llvm.org/D12827).

[Ted Kremenek via cfe-commits]

Author: kremenek
Date: Sat Sep 12 11:01:34 2015
New Revision: 247510

URL: http://llvm.org/viewvc/llvm-project?rev=247510=rev
Log:
Use -f instead of -d flag for testing existing of clang executable 
(http://reviews.llvm.org/D12827).

Modified:
cfe/trunk/tools/scan-build/scan-build

Modified: cfe/trunk/tools/scan-build/scan-build
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/scan-build?rev=247510=247509=247510=diff
==
--- cfe/trunk/tools/scan-build/scan-build (original)
+++ cfe/trunk/tools/scan-build/scan-build Sat Sep 12 11:01:34 2015
@@ -1644,9 +1644,9 @@ if (!@ARGV and !$RequestDisplayHelp) {
 
 # Find 'clang'
 if (!defined $Options{AnalyzerDiscoveryMethod}) {
-  $Clang = Cwd::realpath("$RealBin/bin/clang") if (-d "$RealBin/bin/clang");
+  $Clang = Cwd::realpath("$RealBin/bin/clang") if (-f "$RealBin/bin/clang");
   if (!defined $Clang || ! -x $Clang) {
-$Clang = Cwd::realpath("$RealBin/clang") if (-d "$RealBin/clang");
+$Clang = Cwd::realpath("$RealBin/clang") if (-f "$RealBin/clang");
   }
   if (!defined $Clang || ! -x $Clang) {
 if (!$RequestDisplayHelp && !$ForceDisplayHelp) {


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12827: [analyzer] fix an error finding clang path

[Ted Kremenek via cfe-commits]

krememek accepted this revision.
krememek added a comment.
This revision is now accepted and ready to land.

Applied in r247510.


http://reviews.llvm.org/D12827



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Preventing several replacements on a macro call.

[Ted Kremenek via cfe-commits]

+ Argyrios

Hi Angel,

I believe Argyrios is the original author of the code in question, as this 
looks related to the Objective-C “modernizer” migrator he wrote a while back.  
This code started life on our internal development branch at Apple related to 
an Xcode feature we were doing, and I did the work on pushing it back to open 
source trunk.  Argyrios is the best one to answer your technical question here.

Ted

> On Sep 9, 2015, at 6:05 AM, Angel Garcia  wrote:
> 
> +cfe-commits
> 
> On Tue, Sep 8, 2015 at 6:56 PM, Angel Garcia  > wrote:
> Hi Ted,
> 
> I was working on a clang-tidy check, and today I discovered that it was 
> unable to do several replacements in different arguments of the same macro 
> call. At first, I thought it was a bug, and trying to figure out why this was 
> happening, I found that the replacements were rejected in 
> lib/Edit/EditedSource.cpp:46, where there is a comment that says "Trying to 
> write in a macro argument input that has already been written for another 
> argument of the same macro". This comment means that this changes are 
> rejected on purpose.
> 
> At the commit history, I saw that you had commited 
>  this code (that's why I am asking you). Do 
> you think that there is a way around this? I don't really understand why 
> there is a particular case for the macros here, and understanding it would 
> help me to decide whether I should give up on trying to make this work, or 
> try to find a "better" solution.
> 
> Thanks and sorry for the inconvenience,
> Angel
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12673: [analyzer] Remove whitespace in source code

[Ted Kremenek via cfe-commits]

krememek added a comment.

I am OK with taking these changes.  For those using git, git blame -w should 
suffice to show the real blame history.


http://reviews.llvm.org/D12673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12406: [Analyzer] Add -analyzer-config option for function size the inliner considers as large

[Ted Kremenek via cfe-commits]

krememek added a subscriber: krememek.
krememek added a comment.

This looks fine, but I'm not a fan of the actual name chosen here.  It's not 
very clear what it means by just looking at the name, and as we grow the number 
of configuration options that will become more important.

A few suggestions:

- `min-block-size-treat-functions-as-large`
- `min-cfg-size-treat-functions-as-large` (since "blocks" is a bit ambiguous)
- `min-cfg-size-categorize-functions-as-large`

Unrelated to this patch, several of our other options suffer from my same 
concerns, e.g.: `max-times-inline-large`, `max-inclinable-size`, but changing 
those are not the focus of this patch.

Other than the name, this looks good to me.  Whatever name we choose, I think 
the method name should also be appropriately named, e.g.:

- `getMinCFGSizeToTreatFunctionsAsLarge()`




http://reviews.llvm.org/D12406



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

In http://reviews.llvm.org/D12358#238303, @seaneveson wrote:

> In http://reviews.llvm.org/D12358#237099, @krememek wrote:
>
> > To get the conservative invalidation that Anna suggests (actually, a little 
> > less conservative), I think you can just pass in a two MemRegions as the 
> > input to that method and get a new ProgramState with the appropriate 
> > regions invalidated.
>
>
> Thank you for the suggestion. Unfortunately nothing seems to get invalidated 
> when I call invalidateRegions, in the following code:
>
>   const StackFrameContext *STC = LCtx->getCurrentStackFrame();
>   MemRegionManager  = svalBuilder.getRegionManager();
>   const MemRegion *Regions[] = {
> MRMgr.getStackLocalsRegion(STC),
> MRMgr.getStackArgumentsRegion(STC),
> MRMgr.getGlobalsRegion()
>   };
>   ProgramStateRef State;
>   State = PrevState->invalidateRegions(Regions, Cond, BlockCount, LCtx, 
> false);
>
>
> Do you have any suggestions on what I have done wrong?


I suspect this has to do with `invalidateRegions` itself.  I will take a look.  
In the meantime, can you provide an updated patch that I can try out so I can 
step through the algorithm (if necessary) in the debugger and reproduce what 
you are seeing?


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

In http://reviews.llvm.org/D12358#234949, @zaks.anna wrote:

  I accept that my current patch is not a comprehensive solution to the 
  problem and that it may introduce  false positives, however I do think it 
  is an improvement, where it is preferable to have false positives 

   over doing no analysis after the loop.


 We try to avoid false positives as much as possible. They are very painful 
 for users to deal with.

  In my experience, constant bound loops are normally used to make simple 
  modifications to fixed 

   length collections of data, I think the behaviour of the majority of these 
  loops will be represented by 

   the first and last iteration.


 The main issue with the patch is that it produces a false path on which value 
 of only one of the variables is reset to the last iteration of the loop and 
 the rest of them are set as if it is the 3d iteration. A way to solve this is 
 to compute what can be invalidated by the loop and set those to unknown 
 values (a widening operation).

 You should develop this feature behind a flag. This would allow for 
 incremental development and simplify evaluation.


I agree this should be under a flag, or more specifically, an -analyzer-config 
option.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

I think the functionality started here by doing something clever with loops is 
complex enough to warrant putting this into a separate .cpp file.  We can keep 
this here for now, but this seems like complexity that is going to naturally 
creep up and make this file even more monolithic than it already is.

This change also unconditionally never evaluates the body of a loop if the loop 
bound is constant.  That seems like a **major** degradation in precision, as it 
is worth analyzing the loop body at least once to get full precision of the 
paths explored within the loop body.  That's why I suggest unrolling at least a 
couple times.  The first unrolling will get good coverage of the code within 
the loop, and the second unrolling will capture some correlated conditions 
across loop iterations that can diagnose some interesting behavior.  After the 
second unrolling (or 'nth' for some 'n'), this widening seems applicable.  I've 
also seen some static analyzers that focused on buffer overflow detection 
unroll looks like this by simply extending 'n' to be the constant loop bounds, 
opting for maximum precision at the cost of analyzing all those iterations 
(which isn't necessarily the best option either, but it is an option).

It seems the patch needs two things:

1. A general scheme for widening, which can just be invalidation of anything 
touched within a loop.  I think that can be done by having an algorithm that 
does an AST walk, and looks at all variables whose lvalues are taken (used to 
store values or otherwise get their address using '') or anything 
passed-by-reference to a function.  That set of VarDecls can be cached in a 
side table, mapping ForStmt*'s (other loops as well) to the set of VarDecls 
that are invalidated.  Those could then be passed to something that does the 
invalidation using the currently invalidation logic we have in place.  The 
invalidation logic should also handle checker state, which also needs to get 
invalidated alongside variable values.
2. We need to consult the number of times a loop has been executed to determine 
when to apply this widening.  We can look at the basic block counts, which are 
already used to cull off too many loop iterations.

This patch cannot go in as is, as it will seriously degrade the quality of the 
analyzer without further refinements.  It is a great start, and I would be 
supportive of the patch landing more-or-less as is if it is behind a flag, and 
not on by default until the improvements suggested are made.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

In http://reviews.llvm.org/D12358#234975, @krememek wrote:






 

 

 1. We need to consult the number of times a loop has been executed to 
 determine when to apply this widening.  We can look at the basic block 
 counts, which are already used to cull off too many loop iterations.


On closer inspection, this is actually already in the patch:

  BldCtx.blockCount() == AMgr.options.maxBlockVisitOnPath - 1

I do wonder if nested loops will cause a problem here, however.  If a loop is 
nested, `blockCount()` could already be the same as `maxBlockVisitOnPath() ` 
because of the outer loop.  For example:

  for (int i = 0; i  3; i++) {
for (int j = 0; j  2; j++) { ... }
  }

On the second iteration of the outer loop, but the first iteration of the inner 
loop within that second outer loop iteration, the block count for the body of 
the nested loop will already be 2.  The analyzer will prune paths, regardless 
of this loop widening heuristic.  I don't think the suggested patch actually 
helps with this case because the block count gets exhausted.  We may need a way 
to enhance the accounting of block counts when taking into account heuristics 
for loops.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

Hi Sean,

I responded with some more feedback.  Conceptually the patch is quite simple, 
but I think Anna’s points are all spot on.  I think we’d all be quite amendable 
for this work to go in under a flag, with further improvements going in on top 
of that.  That way we can all collectively start hashing this out in trunk 
instead of feature creeping a patch.

Ted

 On Aug 27, 2015, at 10:15 AM, Ted Kremenek kreme...@apple.com wrote:
 
 
 
 
 
 On Aug 27, 2015, at 8:57 AM, Sean Eveson eveson.s...@gmail.com wrote:
 
 I accept that my current patch is not a comprehensive solution to the 
 problem and that it may introduce false positives, however I do think it is 
 an improvement, where it is preferable to have false positives over doing no 
 analysis after the loop.
 
 Hi Sean,
 
 I'll take another closer look at your patch tonight and come back with more 
 feedback. I completely understand your eagerness to push this forward.  I 
 don't think that anybody disagrees with you that we want to do analysis of 
 more code, especially the code that's currently not being analyzed all 
 because of our current handling of loops. That said, if the solution is not 
 precise enough it may cause a flurry of false positives, which then renders 
 the efficacy of the tool impotent.  It doesn't matter if the tool has more 
 coverage if it produces a high-volume of false positives.  I'm not saying 
 that your patch will result in that happening all the time, but we should not 
 just land the patch without understanding its implications.  I also think 
 that with some relatively small enhancements most of our concerns can be 
 addressed.
 
 Because of the way the analyzer works, false positives often happen because 
 of correlated conditions that are improperly truck by the analyzer.  
 Essentially, if an event that happened earlier in a path would have had an 
 anti-correlation with something happening later in the path then reporting an 
 issue along that path related to those two conditions would be a false 
 positive.  We go to great lengths in the analyzer to handle correlated 
 conditions; that is why it is a path sensitive analysis, which is very 
 expensive.  When we drop information on the floor, we lose precision and for 
 some kinds of code can greatly increase the chance of producing false 
 positives.
 
 We do drop information on the floor all the time, but when we do we try to 
 make those decisions quite deliberate and understand their implications.  
 Some checkers also are written with the mindset that if there is insufficient 
 information to report an issue with high confidence than no issue is reported 
 at all.  But the problem here by not invalidating values possibly touched by 
 the loop is that there is a incorrect perception that the information is 
 precise when indeed it is not.
 
 Thanks again for pushing on this; handling loops better is something I have 
 wanted us to tackle for a very long time but never found the time to do it.  
 I'll circle back with you tonight with more feedback.
 
 Cheers,
 Ted

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

One thing about the tests passing: that's great, but that's obviously 
insufficient.  We probably have fewer tests showing that the analyzer can't 
handle something than tests that show it does handle something.

When a patch like this lands, which expands the analysis scope of the core 
analyzer, a few things are worth measuring:

(1) The impacted analysis time on a typical project.

(2) The impacted memory usage of the analyzer on a  typical project.

(3) Increased analysis coverage.  Are we covering more statements in practice?  
Etc.  Since that is the motivation of this patch, it would be good to benchmark 
it on some real code.

We should, as a community, start keeping track of such things for a few 
projects so we know whether or not experiments like these are objectively a 
real win.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

In http://reviews.llvm.org/D12358#233983, @zaks.anna wrote:

 A way this could be improved is by invalidating all the values that the loops 
 effects, both the values on the stack and on the heap. (We could even start 
 overly conservative and invalidate all the values in the state; setting the 
 known values to unknown values.)


I worry that this would be far too conservative, and would introduce new false 
positives because of lost precision.

One unsound hack would be to analyze a loop once by unrolling, and then 
invalidate anything that was touched in the ProgramState during that loop 
iteration.  That invalidation could also be transitive, e.g. any subregions of 
touched state, etc.

Another, more principled hack, would be to look at all DeclRefExprs within a 
loop and invalidate all memory in the cone-of-influence of those variables 
(i.e., values they point to, etc.), but that's it.

Then there is the problem of called functions within the loop, as they won't be 
analyzed.  Those could interfere with the ability of a checker to do its job.

My recommendation is that we still unroll loops a couple times, getting full 
precision, and then employ a widening technique like the ones being discussed 
to allow the last loop iteration to act as the last one, but as a conservative 
over-approximation.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

 On Aug 26, 2015, at 3:59 AM, Sean Eveson via cfe-commits 
 cfe-commits@lists.llvm.org wrote:
 
 We have been looking at the following problem, where any code after the 
 constant bound loop is not analyzed because of the limit on how many times 
 the same block is visited, as described in bugzillas #7638 and #23438. This 
 problem is of interest to us because we have identified significant bugs that 
 the checkers are not locating. We have been discussing a solution involving 
 ranges as a longer term project, but I would like to propose a patch to 
 improve the current implementation.

FWIW, I do think this is a great problem to work on.  It is easy to come up 
with solutions that work for specific examples but fall over on general code.  
I completely agree that failing to analyzing code after the loop is a major 
hole and lost opportunity to find bugs, but fixing that should not be at a 
tradeoff of a huge influx in false positives.  Some basic invalidation of 
values touched by the loop, which includes possibly invalidating checker state, 
will likely be necessary.  I think this is what Anna was getting to in her 
comment.___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Ted Kremenek via cfe-commits]

krememek added a comment.

FWIW, I do think this is a great problem to work on.  It is easy to come up 
with solutions that work for specific examples but fall over on general code.  
I completely agree that failing to analyzing code after the loop is a major 
hole and lost opportunity to find bugs, but fixing that should not be at a 
tradeoff of a huge influx in false positives.  Some basic invalidation of 
values touched by the loop, which includes possibly invalidating checker state, 
will likely be necessary.  I think this is what Anna was getting to in her 
comment.


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r246003 - Add missing newline.

[Ted Kremenek via cfe-commits]

Author: kremenek
Date: Tue Aug 25 22:11:31 2015
New Revision: 246003

URL: http://llvm.org/viewvc/llvm-project?rev=246003view=rev
Log:
Add missing newline.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp?rev=246003r1=246002r2=246003view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp Tue Aug 25 
22:11:31 2015
@@ -592,4 +592,4 @@ void ento::registerNonLocalizedStringChe
 
 void ento::registerEmptyLocalizationContextChecker(CheckerManager mgr) {
   mgr.registerCheckerEmptyLocalizationContextChecker();
-}
\ No newline at end of file
+}


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11700: Added remove taint support to ProgramState.

[Ted Kremenek via cfe-commits]

krememek added inline comments.


Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:444-448
@@ +443,7 @@
+
+  SymbolRef
+  getSymbolFromStmt(const Stmt *S, const LocationContext *LCtx) const;
+
+  const MemRegion*
+  getRegionFromStmt(const Stmt *S, const LocationContext *LCtx) const;
+

Can we add documentation comments for these?  Seems like generally useful 
utility methods.  We could also probably just make these public.


Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:653-654
@@ -654,3 +652,4 @@
+  *LCtx) const {
   if (const Expr *E = dyn_cast_or_nullExpr(S))
 S = E-IgnoreParens();
 

Is this even needed?  I think Environment::getSVal() already handles 
parenthesis and other ignored expressions.  This looks like dead code.

This can probably just be an inline method in ProgramState.h, that just 
forwards to getSVal(S, LCtx).getAsSymbol().

Alternatively, if this is only called once, we don't need to add a method at 
all, since it is just a one liner.


Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:660-663
@@ +659,6 @@
+
+const MemRegion* ProgramState::getRegionFromStmt(const Stmt *S, const 
LocationContext
+ *LCtx) const {
+  return getSVal(S, LCtx).getAsRegion();
+}
+

This is just a one-liner.  Do we really need this method at all?  It is only 
called once.


Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:672-676
@@ -660,7 +671,7 @@
 
-  const MemRegion *R = getSVal(S, LCtx).getAsRegion();
+  const MemRegion *R = getRegionFromStmt(S, LCtx);
   addTaint(R, Kind);
 
   // Cannot add taint, so just return the state.
   return this;
 }

This looks fishy.  'addTaint' returns a new state, but then the return value is 
ignored, and 'this' is returned.


Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:704-708
@@ +703,7 @@
+
+  const MemRegion *R = getRegionFromStmt(S, LCtx);
+  removeTaint(R, Kind);
+
+  // Cannot remove taint, so just return the state.
+  return this;
+}

This looks fishy.  'removeTaint' returns a new state, but then the return value 
is ignored.  'ProgramState' values are immutable, so this method appears to do 
nothing.


http://reviews.llvm.org/D11700



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12119: Analyzer: Fix a crasher in UbigraphViz

[Ted Kremenek via cfe-commits]

krememek added inline comments.


Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:749
@@ -748,3 @@
-
-  assert (Src != Dst  Self-edges are not allowed.);
-

Did you look at the test case that causes this assertion to fail?  I think it 
would be good to know if this assertion is actually safe to remove.  I'm a bit 
skeptical that it is safe to remove, and that (per my last review) that this 
may be detecting that an invariant is getting violated.  If you are not certain 
how to investigate that part, please report which test is triggering the 
problem and myself or someone else familiar with the engine core can take a 
look.  Thanks.


http://reviews.llvm.org/D12119



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12119: Analyzer: Fix a crasher in UbigraphViz

[Ted Kremenek via cfe-commits]

krememek added inline comments.


Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:778
@@ -780,3 +777,3 @@
 
-UbigraphViz::UbigraphViz(std::unique_ptrraw_ostream Out, StringRef Filename)
-: Out(std::move(Out)), Filename(Filename), Cntr(0) {
+UbigraphViz::UbigraphViz(std::unique_ptrraw_ostream Stm, StringRef Filename)
+: Out(std::move(Stm)), Filename(Filename), Cntr(0) {

While succinct, I think 'Stm' is an actively harmful name as it conveys no 
meaning.  There's no need to hyper optimize here.

How about 'outStream', or something that clearly indicates what it is.

You're only going to use it twice.  No harm in spelling it out.  'Stm' could 
mean a bunch of things.


http://reviews.llvm.org/D12119



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12119: Analyzer: Fix a crasher in UbigraphViz

[Ted Kremenek via cfe-commits]

krememek added inline comments.


Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:784
@@ -783,3 +783,3 @@
 
-  *Out  ('vertex_style_attribute', 0, ('shape', 'icosahedron'))\n;
-  *Out  ('vertex_style', 1, 0, ('shape', 'sphere'), ('color', '#ffcc66'),
+  *this-Out  ('vertex_style_attribute', 0, ('shape', 'icosahedron'))\n;
+  *this-Out  ('vertex_style', 1, 0, ('shape', 'sphere'), ('color', 
'#ffcc66'),

This seems really brittle.

Can we just rename the parameter 'Out' to something else, and then just use 
'Out'?  Seems more reasonable to rename the parameter than to change every 
single functional line of code that uses 'Out'.


http://reviews.llvm.org/D12119



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12123: [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

[Ted Kremenek via cfe-commits]

krememek added a subscriber: krememek.
krememek added a comment.

I think this is a great refinement overall, with a few minor nits.  It also 
isn't clear what the test does.



Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:577
@@ -559,1 +576,3 @@
 
+  const std::vectorCheckObjCMessageFunc 
+  getObjCMessageCheckers(ObjCCheckerKind Kind);

Can we break with tradition here and add a documentation comment?


Comment at: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp:64
@@ -62,2 +63,3 @@
   void checkPreObjCMessage(const ObjCMethodCall msg, CheckerContext C) const;
+  void checkObjCMessageNil(const ObjCMethodCall msg, CheckerContext C) const;
   void checkPreCall(const CallEvent Call, CheckerContext C) const;

Can we break with tradition here and actually add a small documentation comment?


Comment at: test/Analysis/NSContainers.m:297
@@ -294,1 +296,3 @@
 
+@interface MyView : NSObject
+-(NSArray *)subviews;

Can we add a comment above this test that makes it clear what this test is 
doing?  It is not obvious at all from basic inspection.

There's also no matching 'no-warning' or 'expected-warning', so it is not clear 
at all what it is testing.  Having the comment clearly say what the test is 
testing will make it more resilient to somebody accidentally deleting it.


http://reviews.llvm.org/D12123



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r244400 - [Static Analyzer] Add --analyzer-target option to scan-build.

[Ted Kremenek via cfe-commits]

Author: kremenek
Date: Sat Aug  8 12:58:47 2015
New Revision: 244400

URL: http://llvm.org/viewvc/llvm-project?rev=244400view=rev
Log:
[Static Analyzer] Add --analyzer-target option to scan-build.

When interposing on a compiler doing cross-compilation, scan-build
does not infer the target triple needed to pass to clang for
doing static analysis.  The --analyzer-target option allows one
to manually specify the target triple used during static analysis
(and only static analysis) for such cases.

Patch by Honggyu Kim!

Reviewed in http://reviews.llvm.org/D10356.

Modified:
cfe/trunk/tools/scan-build/ccc-analyzer
cfe/trunk/tools/scan-build/scan-build

Modified: cfe/trunk/tools/scan-build/ccc-analyzer
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/ccc-analyzer?rev=244400r1=244399r2=244400view=diff
==
--- cfe/trunk/tools/scan-build/ccc-analyzer (original)
+++ cfe/trunk/tools/scan-build/ccc-analyzer Sat Aug  8 12:58:47 2015
@@ -68,6 +68,7 @@ my $Clang;
 my $DefaultCCompiler;
 my $DefaultCXXCompiler;
 my $IsCXX;
+my $AnalyzerTarget;
 
 # If on OSX, use xcrun to determine the SDK root.
 my $UseXCRUN = 0;
@@ -104,6 +105,8 @@ else {
   $IsCXX = 0
 }
 
+$AnalyzerTarget = $ENV{'CLANG_ANALYZER_TARGET'};
+
 
##===--===##
 # Cleanup.
 
##===--===##
@@ -245,6 +248,10 @@ sub Analyze {
   push @Args, -Xclang, -analyzer-viz-egraph-ubigraph;
 }
 
+if (defined $AnalyzerTarget) {
+  push @Args, -target, $AnalyzerTarget;
+}
+
 my $AnalysisArgs = GetCCArgs($HtmlDir, --analyze, \@Args);
 @CmdArgs = @$AnalysisArgs;
   }

Modified: cfe/trunk/tools/scan-build/scan-build
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/scan-build?rev=244400r1=244399r2=244400view=diff
==
--- cfe/trunk/tools/scan-build/scan-build (original)
+++ cfe/trunk/tools/scan-build/scan-build Sat Aug  8 12:58:47 2015
@@ -1145,10 +1145,21 @@ OPTIONS:
scan-build to use a specific compiler for *compilation* then you can use
this option to specify a path to that compiler.
 
+   If the given compiler is a cross compiler, you may also need to provide
+   --analyzer-target option to properly analyze the source code because static
+   analyzer runs as if the code is compiled for the host machine by default.
+
  --use-c++ [compiler path]
  --use-c++=[compiler path]
 
-   This is the same as -use-cc but for C++ code.
+   This is the same as --use-cc but for C++ code.
+
+ --analyzer-target [target triple name for analysis]
+ --analyzer-target=[target triple name for analysis]
+
+   This provides target triple information to clang static analyzer.
+   It only changes the target for analysis but doesn't change the target of a
+   real compiler given by --use-cc and --use-c++ options.
 
  -v
 
@@ -1462,6 +1473,24 @@ while (@ARGV) {
 next;
   }
 
+  if ($arg =~ /^--analyzer-target(=(.+))?$/) {
+shift @ARGV;
+my $AnalyzerTarget;
+
+if (!defined $2 || $2 eq ) {
+  if (!@ARGV) {
+DieDiag('--analyzer-target' option requires a target triple name.\n);
+  }
+  $AnalyzerTarget = shift @ARGV;
+}
+else {
+  $AnalyzerTarget = $2;
+}
+
+$ENV{CLANG_ANALYZER_TARGET} = $AnalyzerTarget;
+next;
+  }
+
   if ($arg eq -v) {
 shift @ARGV;
 $Verbose++;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits