Ping.
@fhahn Could you please take a look, if you have some time? Would be great to hear your thoughts.

Hey guys, thank you for the review so far. But I am not sure if we have a full consensus yet.
@nikic Do you think that the arguments above are reasonable for this change?

In D142725#4086632, @nikic wrote:

In D142725#4086159, @martong wrote:

In D142725#4085940, @nikic wrote:

@martong Would the optimization also work if -Xclang -enable-dfa-jump-thread were specified in CFLAGS instead? If "yes", then I'd say this is firmly in the realm of user error and we should not make this change.

Yes, that would work with -mllvm -enable-dfa-jump-thread. And the Coremark score is roughly the same (within measurement error). I agree that it would be better if the user added the DFAJT optimization to CFLAGS rather than to the LDFLAGS, but do we really expect the users to have such a subtle knowledge about the opt passes? In particular, it might be surprising that this opt does not have any effect when it is used with LTO at the moment.

This is an internal LLVM option, it is not intended for end-user consumption. If somebody does want to use it, they had better know what they are doing.

The DFA jump threading pass is intended to be enabled by default in the future, at which point no option will be necessary, and this will just leave behind an (to our knowledge) unnecessary pass run in the LTO pipeline.

In D142725#4085940, @nikic wrote:

@martong Would the optimization also work if -Xclang -enable-dfa-jump-thread were specified in CFLAGS instead? If "yes", then I'd say this is firmly in the realm of user error and we should not make this change.

In D142725#4085822, @alexey.zhikhar wrote:

Thanks for the patch.

Question: why LTO? I'm no longer with the company where I worked on this and my recollections are starting to get fuzzy but AFAIR this optimization doesn't need the whole program. If so, you'll want to place this opt into the regular pipeline, where compilation of different translation units is parallelized.

In D136603#3888065, @steakhal wrote:

Previously, even in the RegionStoreManager::getBinding() even if T was non-null, we replaced it with TVR->getValueType() in case the MR was TypedValueRegion.

Yeah, that means, we actually evaded a cast, am I right?

In D133698#3886329, @aaron.ballman wrote:

FWIW, it looks like this change broke the AIX bot: https://lab.llvm.org/buildbot/#/builders/214/builds/3932

In D101526#3883871, @NoQ wrote:

Looks great to me, thanks!!

In D135360#3885494, @balazske wrote:

If we look only at the C standard we can not tell much about if the functions should set errno. It seems that setting errno is totally implementation-dependent, except for a few functions that mention errno. These are fgetpos, fsetpos, ftell and should set errno to a positive value on error (but there is no guarantee that value of errno is preserved if no error occurs). And errno is always positive. This is different than what the checker currently does (with the new patches). Probably this should be a discourse question?

In D136684#3884983, @balazske wrote:

This is really a NFC-like change but not NFC because it has visible effects of removing some crashes. I could not produce a test that provokes the wrong case (when getParents returns an empty list). Is it enough to have no new test here? If we can get a test for the crash it needs more investigation to check if RecursiveASTVisitor works correct and probably a fix at other place.

• Address ymandel's comments

In D136668#3883241, @ymandel wrote:

Nice!! Just nits.

At a high level, I'm a little concerned about this as a demo, since I wouldn't recommend this implementation in practice (for various reasons, e.g. I would encode with 2 booleans since there are only 3 values). But, I think this approach has the advantage of clarity over optimized approaches. It might be worth pointing this out in comments at places where you made a decision for purposes of clarity/readability, if any come to mind.

• Address ymandel's comments

Thanks for the assiduous review @ymandel !

It is very good that we can get rid of the ParentMapContext because it was sub-optimal to build that up for all declaration contexts of the translation unit. Now we discover the parent-child relations for ourselves and only for the needed declaration contexts. This is the proper way to go.

PS: I'm not sure how/when we should get rid of the LocAsInteger and
represent this by a SymbolCast.
Maybe @ASDenysPetrov or @martong could help me review this.

I am not sure, if the ExprEngine::VisitCast is the proper place to add these new modifications and call SValBuilder's evalCast. I think it might be better positioned in RegionStoreManager::getBinding. Considering, we already do a cast evaluation for certain kind of memregions there before returning with the stored value. (Actually, this is again a legacy hack, which is needed because we do not emit all SymbolCasts and thus we store the SVals with an improper type).

if (const FieldRegion* FR = dyn_cast<FieldRegion>(R))
  return svalBuilder.evalCast(getBindingForField(B, FR), T, QualType{});

In D135375#3882491, @Szelethus wrote:

Seems like the issues mentioned above are real, but orthogonal to this patch. Would it be okay to address them in followup patches? @martong @NoQ

Ping

Thanks for the update. Nice Work!

• Rebase to latest llvm/main

• Add comments
• Assumption -> ConditionValue
• Use CRTP
• branchTransfer -> transferBranch
• Make just one simple test case for transferBranch

In D135247#3867445, @balazske wrote:

If StdCLibraryFunctionsChecker is added as dependency to StreamChecker and no other thing is changed these tests fail, and I could not find out the reason:
Errors in test stream.c:

error: 'warning' diagnostics expected but not seen: 
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 7: Stream pointer might be NULL
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 13: Stream pointer might be NULL
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 76: Stream pointer might be NULL

Errors in test stream-error.c:

error: 'warning' diagnostics expected but not seen: 
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 97: might be 'indeterminate'
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 100: Stream might be already closed
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 113: Read function called when stream is in EOF state
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 114: Read function called when stream is in EOF state
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 123: FALSE
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 124: FALSE
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 128: FALSE
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 129: TRUE
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 133: TRUE
  File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 134: FALSE

Probably the StdLibraryFunctionsChecker runs before the StreamChecker, and runs always after it if no dependency is there? But this does not explain all test errors and should cause no errors at all. Adding the dependency itself is not enough, ModelPOSIX option should be true too. If the option is set to true in the test file even more test errors appear, and it does not work even when the (StdLibraryFunctionsChecker) checker is added (to the RUN command). Without the dependency the tests do not fail if the order of checkers in the enabled checkers list is changed.

A similar situation could happen if we reinterpret cast pointers, etc. so the situation is not limited to conflicting function prototypes.

Hmm, seems like the conflicting prototype (i.e. the obsolescent use of zero parameters) is needed to reproduce the assertion failure. That makes me wonder, how does the redecl chain of b looks like? Is void b() chained with void b(int*), or are they represented independently from each other? I guess they form the same redecl chain. Which drives us to the next questions.
When the analyzer reaches the CallExpr b(&buffer) which FunctionDecl does it see? Is it b() or b(int*)? My bet, it sees and works with b(). Could we detect if the arguments of the CallExpr does not match the parameters of the FunctionDecl? And if that is the case, could we iterate through the redecl chain to find an appropriate matching FunctionDecl? That would be b(int*) in this case ... and the original bindArray() should work then.

LGTM

In D135360#3839890, @balazske wrote:

I found some anomalies during development:

• If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.

Could you please elaborate? I don't see how to help you with it without seeing more details.

Thanks for patch and the many test cases.

I am happy with the change and with the result. However, this colorSpace related report is a bit concerning. Could you please check if colorSpace at step 1 is evaluated as Undefined? Could that happen? Or that is (should be) Unknown since that is a parameter of a top-level analyzed function? Did you have this report before your change? I think this would worth to be further analyzed, maybe just directing the analyzer to analyze only the QImage::convertedToColorSpace top-level function.

In D135375#3849261, @Szelethus wrote:

Some early results:

https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acxxnew_baseline&newcheck=%2acxxnew_patched&diff-type=New

I'm still waiting on a few projects, but this is something. A few reports from alpha.security.ArrayBound, I still need to look into that.

This report from core.uninitialized.Assign is interesting. First off, I'd appreciate if the "storing uninitialized value" was placed inside the notes about the call to QScopedArrayPointer's constructor. Otherwise, the report looks correct.

Not too happy about this report by core.CallAndMessage. Message 9 is placed on the line

applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));

Messag 10 talks about what happens in applyColorTransform, but the important thing happens at the evaluation of the argument, which is not described, so this isn't a very good bug report, can't tell whether its false.

Yes, I agree, this bug-report is hard to follow. Seems like the colorSpace argument from step 1 is flowing through step 10. I wonder if colorSpace at step 1 is evaluated as Undefined? Could that happen? Did you have this report before your change?

In D101526#3804623, @NoQ wrote:

Hi, looks great! I found a couple of typos and the amount of changes in tests is suspiciously low. And I want to make sure that the promise to change "arg" -> "argument" isn't lost (but I'll be happy if it's addressed in a follow-up patch).

• Changed to be more verbose: "arg" -> "argument"
• Fixed "less than" to "greater than"
• Added new tests
• Fixed typos

• Rebase

The two checkers work together and 'apiModeling.StdCLibraryFunctions'
and its 'ModelPOSIX=true' option should be now a dependency of
checker 'alpha.unix.Stream'.

I don't see this case different to the unary expressions. Consider the unary minus for example. Let's say, the symbol a is constrained to [1, 1] and then -a is constrained to [-2, -2]. This is clearly an infeasible state and the analyzer will terminate the path. So, no further call of SValBuilder should happen from that point. That is, it is meaningless to evaluate -(-a) if there is a contradiction in the constraints that are assigned to the sub-symbols of the the symbol tree of -(-a).
Here is a test case that demonstrates this (this passes with latest llvm/main):

// RUN: %clang_analyze_cc1 %s \
// RUN:   -analyzer-checker=core \
// RUN:   -analyzer-checker=debug.ExprInspection \
// RUN:   -analyzer-config eagerly-assume=false \
// RUN:   -verify

Thanks for the updates. I am okay with it now. LGTM. But please wait for NoQ's approval. So, this is a gentle ping for you @NoQ :)

Thanks for the update! LGTM.

I like the approach of this patch and I think this is somewhat aligned with @NoQ's ideas about

Thank you! Increasing coverage in tests is always great.

In D133119#3827432, @balazske wrote:

I added a simple detection of create a copy of p before p = realloc(p, ...). This can remove the warning at very obvious cases when a copy of p is created (but even if the copy is made inside an if branch for example).

Actually, you already have a logic for checking if there is a contradiction in your D103096 patch, in ConstraintAssignor::updateExistingConstraints:

// Update constraints in the map which bitwidth is equal or lower then
// `MinBitWidth`.
if (CM) {
  for (auto &Item : *CM) {
    // Stop after reaching a bigger bitwidth.
    if (Item.first > MinBitWidth)
      break;
    RangeSet RS = RangeFactory.intersect(Item.second, CastTo(Item.first));
    // If the intersection is empty, then the branch is infisible.
    if (RS.isEmpty()) {
      State = nullptr;
      return false;
    }
    NewCM = CMF.add(NewCM, Item.first, RS);
  }
}

Suppose we have found the way to handle it. But what if we find a contradiction while simplifying, like (short)(int x) = 0 and (int x) = 1. So that we would already know that it is impossible. What SVal should we return in such case? Undef? Unknown? Original one?

LGTM, thanks!

Yeah okay. I get it now. Thank you for your patience and your time on elaborating the issue.

Assume we have (int)(short)(int x). VisitSymbolCast will try to get the constant recursively in the next order:

• (short)(int x)
• (int x)

And here is my concern. Whether it is correct to get the range for int x and consider it as a correct simplification of (int)(short)(int x). IMO it can't be simplified like that. It should go through the set of conventions and intersections.

We had a discussion about this with @dkrupp . We think that the p = realloc(p, var) construct in itself is an error-prone style of using realloc. This style does not necessarily give birth to erroneous code (see the error-free escaping example in foo above from @steakhal). However, the form p = realloc(p, var) is an indication that the programmer might missed the non-trivial error handling case, and chances are high. Thus, one should use the p2 = realloc(p1, var) form as a best practice. So, this looks good to me, but please consider this a weak approve and wait for someone else's approve who has more confidence in clang-tidy (@whisperity could you please take a look?).

Aligned with the RFC, I am going to dissect this patch into two patches:

LGTM, Thanks!

Looks good to me!

Gentle ping @steakhal @NoQ
Trying to revive this after a year :) I am sorry it took so long to get back to this.

• Rebase
• move Msg into the lambda

Okay, I don't see any problem with this change. LGTM.

LGTM, with nits.

Thanks, you have nice tests with good coverage! LGTM!

LGTM

I am okay with this change, it does give a proper canonical form, which is good. On the other hand, I agree that (on the long term) base regions and offsets would be a better memory model than what we have now with field and element regions.

LGTM!

So I think the most valuable optimizations are low-level optimizations to ImmutableMap. There were a few suggestions on the mailing list to use something more modern than the AVL trees under the hood but I don't think authors found much success with those.

@martong I kept your idea in mind for this warning message and implemented it as a new checker if you don't mind it. If the size of an array is Undefined when it's allocated, this patch emits a warning, and ends the analysis.

Thank you for taking care of this!

Thank you for the update! LGTM!

LGTM

Thank you, nice and assiduous work!

Thanks, looks good with some nits.

I think there's only some very minor style nits which are remained, but that should not block this any further. Let's land it.