Rebase. Fix behavior when the return code is not constrained enough. Test the C++11 attribute syntax (just in case). Update comments.

Rebase.

Rebase.

Whoops - make sure that the Objective-C test does actually test something.

Rebase due to an update in D58365 - namely, add support for Objective-C messages and blocks that are now allowed to wear the attribute.

Fxd, thx!

Add a test that demonstrates that we correctly handle unknown or under-constrained return values. I.e., it would fail if we replace

!State->isNonNull(V).isConstrainedTrue()

with

!State->isNonNull(V).isConstrainedTrue()

I'll take a closer look in a few days, sorry for the delays! The progress you're making is fantastic and i really appreciate your work, but i have an urgent piece of work of my own to deal with this week.

In D58367#1402796, @Szelethus wrote:

2. I guess most of the visitors could be changed to this format, do you have plans to convert them? I'll happily land a hand, because it sounds like a big chore. I guess that would also test this implementation fairly well.

Clean up a bit. Fix kern_return_t accidentally defined as unsigned. Expand one comment.

In D58065#1400615, @Szelethus wrote:

I've also wasted my second weekend trying to make Static Analyzer unit tests run under check-clang-analysis

:) I tried to have a quick look but got confused pretty quickly.

After staring at this for an hour or two, i didn't manage to force myself to understand how our cluster analysis works here, but i totally agree that it's most likely broken; i guess, we should eventually move away from the idea that everything works through base regions, but for now this idea seems to be hardcoded in a lot of places.

Had a look. Great stuff, just as planned :) Old fanboy wisdom: Try to avoid documenting bugs you want to fix! But i don't have many high-level comments here - only appreciation of the effort.

since I couldn't reproduce the error

The analyzer test change maybe indicates we could simplify the analyzer code a little with this fix? Apparently a hack was added to support lvalues in initializers in r315750, but I'm not really familiar with the relevant code.

Sounds reasonable, but it also sounds like something that should be reproducible on the upstream clang. Do you have a code snippet that causes the problematic AST to appear? Even if we don't have the false positive up here in upstream, Is it something we can test via -analyzer-display-progress | FileCheck or with the help of the analysis order checker or something like that?

We should eventually remove operator==() from the SVal class because it doesn't do anything you'd possibly imagine it to do. However we do need to come up with adequate alternatives. And by looking at this patch, it seems that we actually need two alternatives:

1. An attempt to figure out that two SVals are equal. The result of such comparison is state-specific - eg., ($a == $b) may be true in states that follow the true-branch of "if (a == b)" and false otherwise. This is how evalBinOp(BO_EQ) should behave. And this sounds impossible to implement without a perfect constraint solver, so the user should always be prepared to receive an unknown answer in some cases.
2. An attempt to figure out if two SVals are representing the same value, that is, you cannot replace one value with another without an assignment. This is what we need in this patch. Eg., in

int a;
int b;

(already in the works!) Document the frontend of the analyzer, sneak peak here.

I think this is awesome!

Looks cool, thanks!!

That's the one:

There seem to be a few regressions - weird memory leaks of inner objects in C++ destructors. Trying to investigate/reproduce.

Yay, thanks! Looks useful.

This was a good read for me.

Hmm. writes_to_superobject?

Dunno but made a random guess in the inline comment.

Fxd the global variable problem in D57619, thanks! I think you should change back to getInit() once D57619 lands.

These new notes are fantastic, we totally need them :) Changes in edges-new.mm and dtors.cpp look like a regression, i guess one of us would have to investigate it - feel free to start and poke me if it turns out to be hard, because i really appreciate where this is going!

Thx!

P.S. I also support the idea to have a separate page per checker. It encourages people to write as much documentation as necessary and continously expand it. Otherwise there's a perception that all docs need to be roughly of the same size and shape in order to make the page look consistent and not too cluttered.

I think let's get this up and running and slowly redirect links from the front page to the new docs until only the front page is left?

I like AnyCall.

Hmm, the diff is a mixture of D54918 and this patch. I'll apply it by reverting D54918 locally and then applying this diff, but please let us know if you accidentally uploaded a wrong diff!

In D57356#1374662, @george.karpenkov wrote:

I just fail to understand why nobody did that before. As far as i understand, that's a very large improvement.

I think because it required D57346 and all the AnyCall machinery.

Ok! I'll try to evaluate it and see if i an example of improvement shows up - this would probably be easier than to come up with one manually.

In D46421#1354188, @r.stahl wrote:

In my old version I seemed to get away with the tests, but they failed after rebasing. It seems like earlier there was only a single VarDecl for the imported ones with InitExpr. Now after importing there is one without init and a redecl with the init. This is why I changed getInit() in RegionStore to getAnyInititializer. I think these three should be enough, but I'm not sure where else in the analyzer this would have to be changed.

I'm slow, but at least i admit it... Just made myself a fancier phabricator query so that not to forget about patches, so hopefully i won't forget about patches that often anymore :/

Ok! I hope that the C11 check would do the trick, let's see how it goes :)

In D55734#1372972, @boga95 wrote:

Is it ready to land?

I just fail to understand why nobody did that before. As far as i understand, that's a very large improvement.

Hmm. Yeah, this looks unused :)

In D57230#1373721, @xazax.hun wrote:

Do you think I should try to reduce additional files?

Aha, ok, it reduced an interesting positive into a non-interesting positive. So i guess my method only works when you're catching changes that are more unexpected than this one :) Ok, nvm then, thank you for trying this out! I didn't have time to evaluate this change yet, but that definitely shouldn't block you from committing.

In D57230#1372488, @xazax.hun wrote:

In D57230#1372275, @NoQ wrote:

Do you have success reducing false positives using creduce? My problem usually is that we cannot tell if a reduction rendered a false positive into a true positive.

*gets hyped for the upcoming patchlanding party*

I guess we still need to think how exactly do we want to resolve conflicts between dependencies.

Yup, totally.

Thanks! A surprise, to be sure :) I'll try to test this on my set of projects as well :)

Fair enough.

It sounds to me that for now you're only emitting a warning when a *literally* OSObject is being casted, i.e. not a sub-class to an even more specific sub-class. Is this intended?

Hmm, looks correct to me now.

*celebrates*

it's not always suitable, since it's tightly coupled to other analyzer classes (ExplodedNode, ProgramState, etc.) and it's not always possible to construct

I think there are much more problems that we will find this way, starting from

long foo(long x) {
  unsigned y = (unsigned)x;
  if (y <= -1)
    return 0;

I have no objections. George, this was your idea, does it look good to you?

Nope, it doesn't seem to skew results at all. I hope my testing machinery is actually working :/

Yay, visitors.

Thx! We should transition away from using all other constructors (except this constructor from ProgramPoint), at least in our path-sensitive checkers.

Yay thx!

Aha, yeah, cool, there are still uses to be fixed :)