See also https://xkcd.com/1638/

I guess it makes sense to add a test into test/Analysis/exploded-graph-rewriter/escapes.c as well, so that to learn if we can actually parse it later.

Yay thx!

I think i'd rather remove it entirely.

In normal dumps i'd rather have it the old way because it's similar to how stack frames are displayed in the debugger.

Great, thanks!

Nice!~ I'm glad this is getting sorted out.

In D62883#1554514, @Szelethus wrote:

In D62883#1554494, @NoQ wrote:

It should be pretty easy to implement, just add your new visitor to the list of default visitors in findValidReport().

The thinking was to preserve this patch (roughly) in the state as the analyses were run.

I guess let's add a test for the unicode problem that you're seeing.

Pls commit? ^.^

Aha, yep, nice debug flag!

It should be pretty easy to implement, just add your new visitor to the list of default visitors in findValidReport().

Aha, ok, got it. I guess the official term is "error node" (where "error" means "warning").

In D62883#1553248, @Szelethus wrote:

• Add a TODO: in trackExpressionValue about maybe tracking conditions to all bug locations, rather than only for tracked variables.

Oh, wait, no, loops.

In D62883#1552969, @xazax.hun wrote:

In D62883#1552870, @Szelethus wrote:

• Uniqueing already tracked conditions as an (Expr, ExplodedNode) pair instead of on Expr

I would be surprised to see the same ExploderNode multiple time a lot. Also, ExploderNode will have a program point, so having both an ExploderNode and an Expr feels redundant to me.

Ok, thanks!

I think this kinda makes sense, but also it looks like a risky change, because who knows how many times we have took the old behavior for granted. In particular, i'm slightly worried about this breaking the wonky logic of ExprEngine::VisitLogicalExpr (cf. D59857). But i've no specific concerns, so i think it's worth a try, given that it's a massive simplification.

In D62899#1549715, @Szelethus wrote:

Added a proper testfile. The only downside of it is that it doesn't test anything.

Hey, thanks! That's a nice catch.

Oh, right, thanks!

My problem is demonstrated (and solved) by D63519. If i revert my changes but apply this patch instead, my test keeps failing.

Don't try to sneak in an unrelated change.

I mean, i'm removing backslashes but you're adding more backslashes. Therefore i think we're talking about different issues.

Hmm, this doesn't seem to solve my problem in D62761. Let me write some actual test case so that you knew it's fixed when it's fixed.

Great, thanks!

Add tests, rebase.

Yes, this would be helpful! Tests?

Great, thanks!!

I don't remember what exactly does markInteresting() do and these tests don't really convince me that it does anything at all. Halp? >.<

Fair enough, they aren't really needed here.

(then, again, not sure what happens if more nested stack frames are around)

How about we track the condition value past its collapse point only if it involves an overwrite of a variable (or other region) from which the value is loaded? Like, if there are no overwrites, stop at the collapse point. If there are overwrites, stop at the oldest overwrite point (i.e., the value was loaded from 'x' which was previously overwritten by the value of value 'y' which was previously overwritten by the initial value of variable 'z' => stop at the overwrite point of 'y').

In D62883#1542802, @Szelethus wrote:

Some conclusions:

• Cases where the condition is also a variable initialization tend to expand the bug path greatly. This isn't always bad, but should be noted. In general, unless we have a good heuristic to figure out whether there is meaningful information on the right hand side of the initialization, I think we just shouldn't track these. A good example for this is this one. The 37th event contains pretty much every information we need, it's obvious that the optional could either be None or non-None, since it's in a condition. dyn_cast is a prime example too: in this case, note 9 is all we really need.
• We should probably believe that operator bool() is implemented correctly, and shouldn't track the value all the way there (at least, when we're only tracking the condition). Example
• We shouldn't ever track assert-like conditions. Example (note 38-41)
• When the report didn't suffer from any of the above issues, I found the extra notes to be helpful! :D

I am not sure about assuming operator bool being correct. I think we first could think about other tricks to limit the tracking (see my idea above) and if we fail I would only add such rules as a last resort.

*sloowly catches up >.<*

Great, thanks!! Let's commit this.

Yup, this makes sense now! I'll do some nit-picking for a little longer.

This problem is fairly complicated. We clearly need both behaviors (sometimes track until the definition, sometimes track until the collapse-to-null), and it's not clear to me right now when exactly do we need each of them. This is also not a high priority for GSoC because neither there are a lot of warnings of this kind (~15 or so) nor they're actually that false. I suggest taking this more slowly and delay this patch until we actually understand what is the right thing to do here.

All right, it seems that i'm completely misunderstanding this problem and we've been talking past each other this whole time.

In D62761#1539137, @Charusso wrote:

I asked for the new behavior, but I think it should be cool.

In D62761#1525923, @Charusso wrote:

In D62761#1525917, @NoQ wrote:

Remove "-" from program point dumps because it resembles a removal marker in diffs.

Could you add an image? I have not seen any problematic stuff, just that.

In such cases i recommend starting with writing down a test. Like in TDD: first test, then code.

Whoa, this looks like a much needed improvement, i'm glad that you found it!

In D62883#1536894, @Szelethus wrote:

I'm not sure how long it'll take for me to figure out what's wrong, but these numbers are so ridiculous, I suspect a programming error rather then an algorithmic issue.

edit: I seem to have found a solution, I'll share more later when I get to evaluate this :)

Thx for the cleanup!

Hey, i'm seeing a crash in this checker, would you like to look at it? It looks as if you're not being careful about dereferences/lvalue-to-rvalue-casts so it tries to compare &e to e1.

Thanks!

Ok! I'll be happy to have this addressed incrementally.

I wouldn't be surprised if some of these are entirely unused.

Fair enough!

In D59555#1514602, @NoQ wrote:

I'm still in doubts on how to connect your work with the CallDescription effort. I'll think more about that.

I think we should:

• Pre-normalize our expected outputs so that we didn't have to normalize them in every run-line.
• Treat the lack of newline in plist output as a bug and try to fix it.

Looks great! Feel free to add a Driver flag as well (i.e., --analyzer-werror or something like that) so that not to type -Xclang every time.

Aha, nice, that's much cleaner! Indeed, we have to skip the construct-expression, which is going to be on our way to the program point of new-expression which corresponds to calling the allocator, despite being nested within the new-expression. An annoying corner-case.

Ah, that positive!

Aha, that's something! And nice to see we've already had this bug covered with tests. Because, of course, i added these tests a year ago without even thinking about what the correct behavior should look like :/

Yay, this thing really works!

In D62638#1529170, @krytarowski wrote:

This commit breaks the NetBSD buildbot node.

Thanks!! Will fix as soon as i get to my desktop.

I disabled tests on Windows for now (rC362343 -> rC362347), but other than that the tests are working well, there was nothing to worry about.

Remove "-" from program point dumps because it resembles a removal marker in diffs.

I'll add some tests as soon as i'm sure tests for this thing actually work (by landing D62638).

Rebase.

In D62638#1525843, @Charusso wrote:

In D62638#1525823, @NoQ wrote:

Also switched to python2 because it seems to be the default. The differences are minimal.

What about the state of the patch itself? Planned stuff WIP, could I take it to SVGify, or?

Also switched to python2 because it seems to be the default. The differences are minimal.