It looks like this class is used for more than dumping. History digging points to @steveire who introduced this class in order to re-use it for ASTMatcher traversal. Stephen, does this look like the right thing to do? Holding vars aren't visible in the source, should they be skipped in matcher traversal when they are set up to skip invisible nodes?

Thx Dmitry!

Looks awesome!

Thanks! I'll commit.

Looks great!

Yes, we've discussed this before, and I'm very much in favor of this change. This is assertion removal, and the assertion has been really useful back in the day, but the assertion doesn't seem to be realistic to maintain with all the new logic in the constraint solver coming in in recent years.

Thanks!

I'm worried that even if the warning is correct, the suggested fix is not necessarily the right solution.

I looked up the history. I believe this refers to https://clang.llvm.org/docs/LanguageExtensions.html#memory-references-to-specified-segments:

I think interestingness definitely requires more work. In particular, in null dereference from getenv(), the note should be unprunable. But we can't control prunability dynamically yet, so it requires a bit more work. So I think this patch is ok to go and I'll hopefully follow up with more interestingness logic.

• Eliminate notes on known paths.
• Add some documentation for the new class.
  • Mostly moved from existing documentation for the entire summary class.
  • Fix incorrect assessment that Exploded Graph is a tree. It's not even acyclic!
• Fix clang-format warnings.

Looks good. I never intended taint to be inaccessible from plugins. Probably there are other headers in lib/StaticAnalyzer/Checkers that deserve the same treatment.

Other than that, looks great now!

getSVal is probably not at fault, it simply retrieves the value it was previously told to put there, it doesn't care what the value is. You probably want to look at ExprEngine::VisitCast().

Great! I'll commit.

I mean, regardless of whether we need to report a warning or an error against this code, the AST says that the RHS type and the LHS type are the same. This means that the same should have applied to our SVals that represent LHS and RHS, rendering std::max(APSIntType(LHSValue), APSIntType(RHSValue)) redundant. So I suspect that the problem is not that we're not std::maxing hard enough, the problem is that our values weren't correct in the first place, so I think you need to take a look at the transfer function corresponding to statement

ImplicitCastExpr 0x118f2040 <col:18, col:26> '__attribute__((address_space(3))) int *' <AddressSpaceConversion>

...and see why didn't it cause the type of the SVal to be updated with the address space attribute.

Why doesn't the AST handle this for us through implicit casts? Do we really need to duplicate type promotion logic in the static analyzer?

Ok there's actually a huge bug in this patch, namely we can't say "Assuming..." if there's no state split (i.e., when we know from the start which branch is taken so we don't have to assume). I'll fix.

Looks great, thanks! I'll wait a bit to see if other folks have something to say about this and then commit the patch for you.

Looks great! It might make sense to add tests to test/Sema/attr-enforce-tcb-errors.cpp to ensure that attribute conflicts are resolved correctly on Objective-C methods but also it's not like we've changed anything there. I have only one tiny nitpick.

Thanks! I agree that this assertion doesn't make much sense. Judging by the name, the function should be able to handle all symbols.

Looks like the API was used incorrectly 50% of the time anyway :)

All clear now. Amazing, thank you for your detailed response, I've corrected my understanding of address spaces so I'll be able to understand future patches better.

I guess there's the usual direction that I occasionally suggest: develop a way to verify that all possible paths were explored during symbolic execution (CoreEngine::hasWorkRemaining() on steroids), then do most of the work in checkEndAnalysis.

In D120992#3359864, @steakhal wrote:

BTW what is the semantics of [p retain] in ObjC? Can p be null in that context? Or does it count as a dereferences, hence it constraints the pointer?

This check checks must-properties/all-paths properties. This has to be a data flow / CFG-based warning. I don't think there's a way around.

Oh wait, should we accept this given this serious limitation?

Also I think we might be able to enable this checker for the situation when the buffer is *completely* uninitialized. It is relatively easy for RegionStore to answer whether there are any bindings in a top-level region.

I removed the [NFCi] marker because this commit does introduce functional change (a new checker).

This looks like a good check to ultimately have but we probably won't be able to move it out of alpha until the extents issue is fixed (which is going to be a fairly intrusive fix).

The patch looks great to me now. As soon as you address comments about isErrnoAvailable() (at least, the parameter can now be removed), I think you can commit.

Looks great, thanks!

steakhal performed systems testing across a large set of open source projects.

Oh, great point. Added tests that were crashing the other way round so that to demonstrate that this fix is still justified. But I agree that the visitor can totally terminate early so that to make the original tests pass for two reasons!

Aha ok. I guess let's try to re-enable the test. Tests are good. Even if these tests weren't fulfilled automatically by changes in the constraint manager, that assertion failure may have been avoided.

Yes, as long as the maps are there, checkDeadSymbols has to be there. It doesn't have any visible effects (because the symbols are dead, there's in fact a guarantee that these map keys will never be queried anymore) but it keeps the maps small which is essential for performance.

Can you post a link to a specific buildbot failure so that we could look at it together?

Do you happen to know how stable are these doxygen hashes?

It's definitely a bug-prone scenario. I can totally see people stuffing whatever happens to be more readily available in the code into the API without thinking too much about the pros and cons. The difference between CallExpr and CallEvent is large in general but with respect to this API it's very subtle. I can easily imagine people missing it even when they know the difference between CallExpr and CallEvent in general. So I think it's worth attracting attention to.

Looks correct, thanks for fixing this!

Thanks!!

The original lookup() isn't exactly precise either, it's just slightly more precise as it has better access to path-sensitive information such as current values of function pointers, but this doesn't necessarily help given that these pointers can still be unknown. And when the information is not available the lookup silently fails in both cases.

I agree with @martong, LocationContext is the correct key here. The pair (Call expression, Location context) uniquely identifies the call as that call is being evaluated. This is exactly how expression evaluations are identified in the Environment.

Awesome! I'll commit.

The phabricator expects complete patches reuploaded (i.e., diffs against main branch rather than diffs against the previous upload); but it knows how to produce patches-on-patches from complete patches through the History tab. Another reason to upload complete patches is to unconfuse pre-merge buildbots (the "Build Status: patch application failed" part).

Lovely! I have some nits.

Looks great overall!

Fair enough!

Oh interesting, so it's the same problem that causes ConstructionContexts to be necessary: evaluation of a function depends on its AST parents that we didn't yet encounter in the CFG.

Assuming that the code compiles, bugreports coming from constexpr variable initializer expressions are by definition false-positives.

This looks great with the option flag. Landing this patch will enable more people to test the new mode and produce feedback on whether the constraint solver keeps working well enough in presence of the new symbols.

I haven't read this whole patch with full scrutiny but it sure looks lovely. I now also see what the problem is with non-static strings in call descriptions.

Interesting. Might it be that in this scenario in order to be of interest to the user the condition value has to be trackable back to the current stack frame?

Ooof you fixed a lot of static analyzer tests. If you ever get tired of this, feel free to -w them entirely because the driver's --analyze flag implies -w anyway. Hmm, maybe we should even change %clang_analyze_cc1 to include -w.

I'm really glad it actually works!

P.S. bugprone-branch-clone seems to have attempted to use CloneDetector in the past but now it's no more than a dead #include. I wonder what happened there.

Oh nice, it's great to see these things moving.

These checks are almost 2000 lines of code each and it looks like all they do is figure out if two statements are the same and we have a very flexible reusable solution for this sort of stuff - CloneDetector, but none of them use it. Your patch demonstrates that they all have the same bugs that need to be fixed in each of them separately, so reusability seems really valuable. If I was to work on these checks, the first thing I'd try would be to throw out their machines and plug in a reusable solution.

In D113622#3194580, @steakhal wrote:

Could you please share the results to have look? How can I reproduce and evaluate the effect of this change?

Like, that's the whole reason why nonloc::LocAsInteger exists: so that we could cast a pointer to an integer and actually have a way to represent the resulting value as NonLoc.

There is the reinterpret-cast operation which is capable of crossing these two domains, producing an expression that can participate in arithmetic operations, but on the abstract domain side, we still stick to Locs

It can happen if the Loc was perfectly constrained to a concrete
value (nonloc::ConcreteInt)

Awesome, I love it!

Why though? Are you actually putting anything in there that isn't a static string literal?

In theory, we should try hard to reduce the matching complexity to O(1), since this operation is heavily used by the checkers.

Taking advantage of strict aliasing is good as long as it produces strictly smaller analysis space (less paths, more constrained states). I.e., we can use it for eliminating possibilities, but not for discovering possibilities.

Some -analyzer-config flags indeed deserve better treatment. I appreciate the effort.