- User Since
- Mar 7 2019, 1:49 AM (119 w, 15 h)
I suspect that the exploded-graph-rewriter should be updated as well so that the HTML dumps also carry this information.
Fri, Jun 4
Overall I think it's a useful checker not only for checking the getenv() but a bunch of other functions as well, which might return a pointer to a statically allocated buffer.
The implementation could be polished a bit but it's ok I think.
There is so much fancy stuff going on upstream. Awesome to see.
I'm trying to catch up ASAP, I'm finally done with my master's thesis.
Wed, Jun 2
Okay, then I'm going to abandon this in a few days if nothing interesting shows up.
Thanks for the quick response!
Tue, May 25
I've decided to land this without the test.
Thank you for your contribution.
Mon, May 24
It seems like everything passes. Yeey, good job!
Shall I commit this tomorrow on your behalf?
Could you please reupload your diff to retrigger the bots?
I'd like to make sure that everything passes, even the tests that do not belong to you.
Unfortunately, I don't know any better way of retriggering the pre-merge checks - I don't have permission to manually retrigger builds.
Fri, May 21
I think It would be also beneficial to document the semantics. Whenever we say $x + $y it's not entirely clear whether we talk about the addition operation in a mathematical sense or we follow C/C++ language semantics. Right now it's mostly mixed within the analyzer. It would be really nice to see for example how and when wrapping is considered.
What if the types of the operands don't match in bitwidth or signess. This is sort of the reason why ArrayBoundV2 has false positives in some scenarios.
Nothing changed, I'm just retriggering the pre-merge bots as my parent revision landed.
Thu, May 20
Looks good to me.
Wed, May 19
There are quite a few places where the extdef mappings should be updated.
For discovering them I suggest you asserting the new file format (only for detecting them!). This way if you miss one, it wouldn't silently 'work' somehow, but raise your attention.
Introduce the test fixture, and use its setUp() method for implementing this GTEST_SKIP stuff.
May 18 2021
I don't really like having multiple files with the same name.
And the importer TU should be simple to be simply cat-ed into a temporal file.
At that point, you could put the importee's content into this file. It would result in a single, self-contained test case.
now it should build
May 17 2021
I think it doesn't build. Could you check if this is the case? @aeubanks
I think it's good to go. Thank you!
May 15 2021
It is a hard requirement. I'm gonna think more about this in the far future I guess. Thanks for the insight though.
May 14 2021
Well, and how can I build them on Linux xD
I tried mingw64, but failed :D
Do you have any good (mature, big enough) open-source projects for these msvc constructs?
May 13 2021
Well, that's interesting. It might worth investigating. @NoQ ?
Regardless, it's an improvement, let's land it :D
May 12 2021
By checking the line coverage of the LoopUnrolling.cpp test file, looks like all lines are covered you touched.
Please, add these reviewers for your upcoming [analyzer] patches.
Inline a couple of nits.
Nice to see some fixes for visual c++ stuff.
May 11 2021
I'm gonna have a look at this tomorrow.
Dam, we will be always haunted by these.
Looks good btw.
The test looks somewhat artificial - like a raw creduce result.
May 8 2021
I've locally run your patch and seemed good to me, I regret not having a look at the actual build bot.
May 7 2021
On second thought the current behavior is reasonable.
We call clang from a command line, so I think it's fair to expect that relative paths are resolved using the CWD.
AFAIK if one does not supply the ctu-invocation-list, then it would be substituted by ctu-dir/invocations.yaml anyway.
I would love to see PreviousParsingResult combined with InvocationList using a llvm::Error. I'm pretty sure it can be done.
Regardless, I think it's already better than it was.
I would rather match on the complete line (except the file descriptor ofc).
May 6 2021
Okay, so you 'just' want an indication for the given open call. What about using strace?
strace -e trace=openat %clang_cc1 ... 2>&1 | grep '"invocations.yaml"' | FileCheck %s
Overall, it looks promising. But I don't quite get this test.
There is no invocation yaml in the temp directory. So, you are probably not testing the right thing.
You wanted to test if the invocation yaml exists, and could be opened but the parsing fails.
You should demonstrate that when a parsing error happens, the error code has recoded and it won't try to reparse the invocation yaml again and again.
May 4 2021
May 3 2021
Awesome! Seems good to me. Though I've got limited experience on CTU stuff.
It would be nice to have tests, but it seems pretty hard to come up with one for this. Given that this is just a 'performance' issue, I'm fine with it.
Somehow try to check if this resolved your original concern.
May 1 2021
I don't know how did we miss this. I run your patch on several projects and it seemed good. Does anyone have an idea how to prevent such a silly mistake from happening again? I was thinking of coverage data, but that wouldn't be enough for this example.
Apr 29 2021
Apr 26 2021
This is an improvement! Good job.
I had no time reviewing this, but I think it's already in a pretty good shape.
It might worth investigating this build 'target'.
Apr 20 2021
I think we can conclude that P must be non-null (since it was unconditionally dereferenced), thus the previous check on the inner pointer and the branch it guards must be dead! This fact deserves a report, you are right. My bad.
Add 'Limitations and bugs' section with a false-positive example.
It would also help users classifying certain types of false-positive reports.
Apr 19 2021
I would expect no notes at all, since there is no bug.
I really want to move this forward so I made a further evaluation on this, on the MongoDB project.
The analysis took approx. 22 and half hours on 24 cores for the baseline and for this revision as well.
Apr 8 2021
I'm still not satisfied with the addressof, but I won't block this either.
Apr 7 2021
Looks good. Thank you.
Oh, I got it. Hm, yes, both are wrong xD
Apr 6 2021
Obsoleted by D69726.
I guess (extent of heap segment that starts at symbol of type 'int *' conjured at statement 'new int [ext]') - 8 is the value I expect - which is the size of the remaining part of the memory region from x+2 in bytes.
We can argue about the semantics of this debug intrinsic or the name of it though.
Apr 1 2021
Mar 31 2021
Add a FIXME about placing a NoteTag describing why the extent was getting tainted.
Z3 constraint manager is unsupported, thus no test runs those parts. And yea, crashes more often than not.
I could not manage to create an unknown extent, where the behavior would diverge.
I wouldn't mind fixing that yes, but right now I'm busy with other things.
Let's come back to it later.
I'm gonna investigate. Thank you all for the snappy review!
I'm adding a couple of reviewers to get this going.
Mar 30 2021
Everything looks fine.
I like that regexp matcher so much. <3
Mar 28 2021
Lets see what others think about this.
Im fine with it on my part.
Mar 26 2021
I don't know. I think it's already way better than it was.
I think we can reiterate this later.
Some minor logical issues inline.
Mar 25 2021
I really like it. Looks good.
I'm letting someone else accept this as I've not really touched the trackExpression parts.
I recommend splitting this into two. I would happily accept the part about std::data().
When the if's condition is evaluated, it probably triggered a state split. On one path the aptr (aka. the inner pointer) will be constrained to null.
The only way to be sure is by checking the exploded graph and see where it goes.
Mar 24 2021
We should at least document it as testcase.