This looks fantastic. Let's enable by it default!

@ddcc, sure, np!

In D69825#1827826, @xazax.hun wrote:

This patch breaks scan-build-py which parses the output of "-###" to get -cc1 command.

@mikhail.ramalho brought this up in pr41809 so let's revisit this.

I'm excited about this effort!~

Uh-oh, what happened here?

In D71612#1814410, @martong wrote:

Ups, I am sorry. Now I am creating another commit which moves it to alpha. (So then, If I understand it correctly then the cplusplus is enabled by default, but alpha is not.)

In D71612#1813937, @martong wrote:

I am going to execute the analysis on LLVM/Clang as you suggested, so then we'll see what the experiment will bring us. And then we could enable it by default I think. BTW how can we do that? I could not find any default enablement related config in Checkers.td ...

In D71612#1812036, @martong wrote:

In D71612#1790045, @NoQ wrote:

I wonder if this checker will find any misuses of placement into llvm::TrailingObjects.

I've evaluated this checker on LLVM/Clang source and it did not find any results, though there are many placement new calls there. I think this is good, because there are no false positives.

In D72018#1803144, @xazax.hun wrote:

I expect not to have too many functions that need such exclusions. It was more about making it easier to do some exclusions without touching the analyzer. I will ask around what are the preferences of the team, maybe Artem's proposal will work for us.

Would changing the literal in the attribute have the same effect? I.e., acquire_handle("Fuchsia_But_Please_Ignore_Me").

Woohoo tests!~~

Wait, i didn't real @Szelethus's reply :)

I am not sure what would be the best way to test this change here.

LGTM! Yeah, please update the ASCII-art, it's great and every checker should have it.

Yay, thanks! It does seem to work on python2 after the fix.

Looks straightforward and useful, i don't see any problems, LGTM!

In D70836#1789059, @xazax.hun wrote:

Will this also work as intended with sugared types? (e.g. typedefs)
I believe this might be one of the main reason why the original author used canonical types in the first place.

I wonder if this checker will find any misuses of placement into llvm::TrailingObjects.

Even though this is probably not the right solution for the static analyzer use-case (because we may end up having duplicate expressions in the CFG), it might actually make the static analyzer perform better than before (because it's still better than not having these expressions at all in the CFG). I guess we could experiment.

In D71433#1784920, @zukatsinadze wrote:

@NoQ I like the idea, but I am not really sure how to do that. I started working on Static Analyzer just lask week.

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Thanks!!

Looks great but i keep worrying that you're re-inventing CallDescription which already supports this feature (and many more) and we really need a single implementation of this logic because it has been historically very annoying and bugprone. Like, if you can convert your configs to a CallDescriptionMap<> while loading that'd be awesome.

I doubt we'll ever need these callbacks. I can't imagine any kind of analysis that would need them. Like, these statements have absolutely no effect on the runtime program state.

Hmm, it was supposed to get a green tick mark. Let me re-accept.

One way to criticize the current solution would be to put an InitListExpr in a loop and immediately discard it and see how states after different numbers of iterations no longer join together only because they have different NoCachingOutForConsts values.

So the AST is like

|   |-DeclStmt 0x7f9f3b8932e0 <line:6:3, col:43>
|   | `-VarDecl 0x7f9f3b892f50 <col:3, col:42> col:7 used a 'int [5]' cinit
|   |   `-InitListExpr 0x7f9f3b893268 <col:14, col:42> 'int [5]'
|   |     |-array_filler: ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int'
|   |     |-IntegerLiteral 0x7f9f3b893118 <col:41> 'int' 4
|   |     |-ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int'
|   |     |-IntegerLiteral 0x7f9f3b893078 <col:31> 'int' 15
|   |     |-ImplicitValueInitExpr 0x7f9f3b8932d0 <<invalid sloc>> 'int'
|   |     `-IntegerLiteral 0x7f9f3b892fd8 <col:21> 'int' 29

Instead, i conservatively assume that they don't overlap, unless they're already known to certainly overlap on the current execution path. This loses a bit of coverage but the lost path is where they do actually overlap. This path is in my opinion not only rare but also fairly useless, as it immediately introduces an aliasing problem that we aren't quite ready to deal with.

Fair point. Yeah, i wonder what the real difference is :/ Added tests.

Thanks!! Nothing controversial remains here, right? :)

What about __attribute__((acquire_handle("fuchsia")))?

I plan some refactoring but this first patch is meant to be a single separation of code.

Fantastic, thanks!!

Thanks!!~

That's a fair point, these days we always have to account for unknown values.

In D71224#1778332, @xazax.hun wrote:

So basically what I am wonder/worrying about is the following:
The analyzer core will decide that the stack region is escaped and the checkers has no word about this.

In D71224#1778284, @xazax.hun wrote:

So I was wondering if we got the default right. Maybe a checker should do more work to get the escaping rather than more work preventing it?

In D71224#1778204, @xazax.hun wrote:

I don't think this is a good enough model currently. The problem is that, it does not play well with annotations. E.g. the checker can see a symbol escaping, but it does not have a whole lot of information how. For example, currently, there is no way to check if the output parameter through which the escape happened was annotated somehow.

In any case, every checker is allowed to make their own decisions about escaping. Escape on its own is not material, it's all about how the checker reacts to escapes. Say, it's up to MallocChecker to decide whether the function may or may not release memory that escapes on call.

Looks great, thanks!~

Usually this kind of code is hard to re-use because all use cases require different definitions of "has descendant". We already have at least 3 such definitions floating around and we can't re-use them.

It still mildly worries me that the attributes aren't truly reusable, as the exact meaning of the attribute depends on the domain. In particular, every domain is expected to have different approaches to error handling, i.e. a function that creates or destroys a handle may fail to, respectively, create or destroy the handle, but instead indicate the failure in a domain-specific manner, eg. through magical return values or null handle or errno or whatever.

Looks great, thanks! I'd love to see what effect it immediately causes and whether there will be any true or false negatives because of this change.

Ok, so, what should the checker warn on? What should be the intended state machine for this checker?

I think you don't need to smuggle WasConservative all the way up. It's implied that if the evaluation was not conservative, then the respective ExplodedNodeSet is going to be empty, as all nodes will be put directly into the worklist instead. Eg., checkPostCall isn't going to be invoked immediately after inlineCall, but only after enqueueEndOfFunction.

In D71224#1776052, @xazax.hun wrote:

Also, it looks like for example CStringChecker calls ProgramState::InvalidateRegions directly when modeling function calls. So it looks like if checkers are using evalCall, pointerEscape will not be triggered automatically. I am not sure if that would be a bug or feature :D

Hiding the trait in ExprEngine has the additional benefit of disallowing *direct* access to the trait - it can only be accessed through getters. It also doesn't prevent us from exposing the trait to checkers as a method on ProgramState in the future, because ProgramStateManager has access to SubEngine.

I've been hiding these internal traits in ExprEngine lately but i don't have a strong preference. I don't see any immediate need to expose this info to the checkers, nor do i see a need to actively hide it.

Thanks!! This definitely doesn't sort out all the problems of this kind, but that's a strict improvement.

In D70411#1769628, @Charusso wrote:

str31-c.tar.gz868 KBDownload

A while back I had a look into implementing some of the CERT checkers and my vague recollection from these attempts was that it's a bad idea to take CERT examples literally. Most of the CERT rules are explained in an informal natural language and it makes CERT great for demonstrating common mistakes that programmers make when they write in C/C++. Humans can easily understand the problem by reading CERT pages. But it doesn't necessarily mean that static analysis tools can be built by generalizing over the examples from the CERT wiki.

In D70693#1762137, @xazax.hun wrote:

So after investigating the effects of this patch, the result is the following:

Verbose mode (-v) that also prints findings to the command line will no longer be colored after the patch.

Hey, thanks for waiting on me! I'm slow these days, just 50 more mails to go >.<

Mmm, right, i guess you should also skip adding the tag if the notes array is empty.

Yay, it actually works!

Of course i mean something like this:

std::string text = note();
if (!text.empty())
  return text;

(and return something like "" on the other return path in the inner lambdas).

In D70725#1765981, @xazax.hun wrote:

the same call might both acquire and release handles (there are such calls in Fuchsia), so we might end up adding more than one note for a call for which we would need to add more than one transitions

I feel like the 2. is a better solution. Of course, that change might have a performance impact as well.

Sooooo... note tags?

Fair enough!

In D69746#1756448, @gribozavr2 wrote:

This patch introduces a way to apply the fix-its by the Analyzer:

I'm not sure this option is very useful... I don't know of anyone who uses the same option in Clang or ClangTidy. The only way I know people apply fixits is with the help of IDEs. I am also skeptical that people want to apply *all* fixits. Usually people want to pick a few specific ones, or all fixits of a certain kind; but not everything.

What workflow are you thinking of for this option?

Ah, the good old "stringly typed" APIs.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/16031
MSan doesn't catch it >.<

I'll commit in order to get rid of the crash, but i'm open for discussions :)

I think it would really help if you draw a state machine for the checker, like the ASCII-art thing in D70470; you don't need to spend a lot of time turning it into ASCII-art, a photo of a quick hand-drawn picture would be totally fine, because it's, first and foremost, for discussion :)