LGTM.

These all look good to me.

Applied in r247510.

LGTM as well. Thanks Sean.

Committed in r246978.

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

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.

In D12358#238303, @seaneveson wrote:

In 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 &MRMgr = 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?

In D12358#235142, @seaneveson wrote:

If I refactor this patch in its current state into a separate file and put it behind a flag, would it then be acceptable? I would then like to take some time to look at the invalidation improvements as discussed in this thread.

In D12358#234975, @krememek wrote:

Hi Sean,

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.

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

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.

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.

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

Looks fine in UninitializedValues.

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

Committed r244400

In D10356#217564, @honggyu.kim wrote:

In D10356#186078, @honggyu.kim wrote:

Hi all,

I wrote this patch because I had some problem generating a clang static analysis report for ARM Linux kernel code. Since it uses
some target specific flags for ARM, I got the following error message during analysis.

error: unknown target CPU 'armv7-a'

I have checked scan-build script, but there was no way to pass target triple information to clang, so clang thinks that the source
code can be compiled for host machine, x86_64. This patch adds --triple option to fix this problem.

With this patch, I was able to generate clang static analysis report for ARM Linux kernel code with the following command:
$ scan-build --use-cc=arm-linux-gcc --triple=arm-linux-gnueabi make ARCH=arm CROSS_COMPILE=arm-linux-

Please let me know your opinion.
I appreciate all your comment.

Thanks,
Honggyu

In D10356#216741, @honggyu.kim wrote:

I have updated the new patchset with more information in help message and changed the option name to —analyzer-target.
If you think the "extra flags to pass to the compiler” is still needed, I will write another patch for that.
I appreciate your comment and suggestion.

In general I'm not a fan of the taint methods being directly on ProgramState, but this extends the current pattern. We could move the APIs elsewhere later.

I don't remember why the 'tainted' methods were added to ProgramState in the first place, but it doesn't seem quite right. Taint can easily be modeled as a set of APIs that modify and produce new ProgramStates, but I don't see why it should be part of ProgramState itself.

This feels like a big hammer to wield just to prevent the conservative invalidation done by the static analyzer for calls to functions whose bodies are not available.

Thanks Aleksei. That explanation helps quite a bit. I've responded in Phabricator with some comments.

Can you please provide a test case that shows what problem this fixed?

Looks good to me.

Hi Aleksei,