I suspect that the exploded-graph-rewriter should be updated as well so that the HTML dumps also carry this information.

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.

In D103511#2793291, @aaron.ballman wrote:

I think the C++ Core Guideline wording is... confusing. The rule title is C.21: If you define or =delete any copy, move, or destructor function, define or =delete them all, which I take literally. Using = default defines the function, so:

Okay, then I'm going to abandon this in a few days if nothing interesting shows up.
Thanks for the quick response!

I've decided to land this without the test.
Thank you for your contribution.

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.

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.

In D102835#2773818, @tomasz-kaminski-sonarsource wrote:

I do not have commit rights. If there are no additional changes needed, would it be possible to you to commit the changes on my behalf?

Nothing changed, I'm just retriggering the pre-merge bots as my parent revision landed.

Looks good to me.

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.

In D102640#2764759, @shafik wrote:

This is a good catch, thank you for fixing this!

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

In D102136#2761438, @supergrecko wrote:

[NewPM] Remove documentation comments for parameters that were refactored into LLVMPassBuilderOptionsRef

I think it doesn't build. Could you check if this is the case? @aeubanks

I think it's good to go. Thank you!

In D102280#2760402, @AbbasSabra wrote:

Is it a requirement for you to build them on Linux? Maybe you can try to build them on windows with clang-cl?

It is a hard requirement. I'm gonna think more about this in the far future I guess. Thanks for the insight though.

In D102280#2759480, @AbbasSabra wrote:

In D102280#2759167, @steakhal wrote:

Do you have any good (mature, big enough) open-source projects for these msvc constructs?

https://github.com/microsoft/winfile and https://github.com/chakra-core/ChakraCore seem to be using SEH but not heavily.

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?

In D102240#2756967, @vsavchenko wrote:

I couldn't transform c == b into a condition, so it crashes.

Well, that's interesting. It might worth investigating. @NoQ ?
Regardless, it's an improvement, let's land it :D

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.

In D99260#2704102, @NoQ wrote:

In https://bugs.llvm.org/show_bug.cgi?id=45786 the godbolt link shows that there are still problems with addressof (yes, their "trunk" clang is fresh enough). They seem to have __addressof instead of addressof so maybe we should cover that case real quick. Or maybe outright suppress reference invalidation on double-underscore functions because the checker generally relies on the number of standard functions that don't invalidate references while taking a non-const reference being very limited but this limit definitely doesn't take double-underscore functions into account.

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.

In D101763#2745802, @OikawaKirie wrote:

I do not know why the test case always fails on the build server, it runs perfectly on my computer. : (

I've locally run your patch and seemed good to me, I regret not having a look at the actual build bot.

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

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.

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.

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.

This is an improvement! Good job.
I had no time reviewing this, but I think it's already in a pretty good shape.

In D83660#2715097, @OikawaKirie wrote:

In D83660#2675064, @mikhail.ramalho wrote:

Indeed it looks like a copy & paste error, I'm surprised no one found it earlier.

Regarding the tests, we used to have make check-clang-analysis-z3 (or something similar) that would run only the analyzer's tests, but using Z3 as the constraint solver. It looks like this change broke it: https://reviews.llvm.org/D62445

It might worth investigating this build 'target'.

In D97183#2700810, @vsavchenko wrote:

In D97183#2699336, @steakhal wrote:

In D97183#2699080, @RedDocMD wrote:

For the following function:

void foo(std::unique_ptr<A> P) {
  A* praw = P.get();
  A* other = praw;
  if (other) {}
  P->foo();
}

Where do we expect a note? Where praw is initialized, where other is initialized or both?

I would expect no notes at all, since there is no bug.

According to the existing analyzer logic, there is a bug. If you check other for null, we can conclude that there are circumstances when it is null indeed.

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.

In D97183#2699080, @RedDocMD wrote:

For the following function:

void foo(std::unique_ptr<A> P) {
  A* praw = P.get();
  A* other = praw;
  if (other) {}
  P->foo();
}

Where do we expect a note? Where praw is initialized, where other is initialized or both?

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.

I'm still not satisfied with the addressof, but I won't block this either.

Looks good. Thank you.

In D99658#2671747, @NoQ wrote:

I mean, the extent of an ElementRegion is the size of a single element. The reason why our intrinsic isn't doing what you expect is because we represent the pointer with offset as ElementRegion regardless of whether operator [] was used. On the other hand, an SVal doesn't ever represent a region at all, it only points to its first byte, regardless of the structure of MemRegion inside it; for that reason clang_analyzer_getExtent() is impossible to implement correctly in our current model.

So i think both behaviors are incorrect but if you think it makes it easier to write tests then absolutely go for it!

Oh, I got it. Hm, yes, both are wrong xD

Obsoleted by D69726.

In D99658#2665730, @NoQ wrote:

What about clang_analyzer_getExtent(&x[2]) then?

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.

Add a FIXME about placing a NoteTag describing why the extent was getting tainted.

In D83660#2661646, @OikawaKirie wrote:

Can we automatically enable all test cases requiring z3 if clang is built with z3? I do not think the patch D83677 really make the problem fixed.

Z3 constraint manager is unsupported, thus no test runs those parts. And yea, crashes more often than not.

Fix comments.
I could not manage to create an unknown extent, where the behavior would diverge.

In D83660#2661369, @martong wrote:

I went through the change and it looks good, seems like this is indeed a copy paste error from line 132.
I checked the related conversation, and thanks for all the effort spent with the test.

BTW, I was obstructed by the z3 requirement in the regression test case when I tried to understand your test case. Even though I set the variables LLVM_Z3_INSTALL_DIR and LLVM_ENABLE_Z3_SOLVER during CMake, and the CSA can be correctly compiled with Z3, I still cannot make the test case run during make check-all. Therefore, this case was manually executed with llvm-lit -DUSE_Z3_SOLVER=1. Could you please tell me how to enable Z3 during llvm-lit.

I am a bit unhappy that we cannot run the test automatically, but maybe in the future.
@steakhal, https://reviews.llvm.org/D83677 seems to be related, should we push that?

I wouldn't mind fixing that yes, but right now I'm busy with other things.
Let's come back to it later.

In D99658#2661452, @martong wrote:

Ah, I see you need nonloc::SymbolVal in your next patch, and getDynamicSizeWithOffset returns an Unknown if the extend is symbolic.

Anyway, I still feel misleading that clang_analyzer_getExtent does not handle the offset. Could we change getDynamicSizeWithOffset to return with a symbolic offset instead unknown without regression?

I'm gonna investigate. Thank you all for the snappy review!

spam reviewers

I'm adding a couple of reviewers to get this going.

Everything looks fine.
I like that regexp matcher so much. <3

land it

Lets see what others think about this.
Im fine with it on my part.

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.

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

In D97183#2640559, @RedDocMD wrote:

@NoQ, why does the following trigger a null-dereference warning? (https://godbolt.org/z/Kxox8qd16)

void g(std::unique_ptr<A> a) {
  A *aptr = a.get();
  if (!aptr) {}
  a->foo();
}

When a->foo() is called, the constraint !aptr is no longer valid and so InnerPointerVal corresponding to a is no longer constrained to be null.
Am I missing something?

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.

In D99262#2648533, @vsavchenko wrote:

In D99262#2648512, @steakhal wrote:

I see your point.

Would it report this issue?

int test() {
  struct Foo foo = {0, 0}; // I would expect a warning here, that 'foo' was initialized but never read.
  (void)foo;
 return 0;
}

Only nits besides this.

We got one big fat complaint about that, and I can see the point.

We should at least document it as testcase.