[analyzer] Re-enables trustnonnullchecker_test and removes redundant code from TrustNonNullChecker

[steakhal]

Why do you need to remove the code doing the garbage collection?
What code was redundant, according to your title?

[NoQ]

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.

When I suggested this change in D118657 my thought process was that given that we now handle operators == and != a lot better, these extra traits are now redundant as we can replace them with solver constraints. But now that I had a closer look, it looks like these constraints are much more sophisticated, i.e. not just $a == $b but more like (bool)$a => (bool)$b, so we might still need them. Unless, of course, somebody wants to move this code to the constraint solver, which probably might work.

@t-rasmud can you confirm some of these tests still fail if you remove the traits entirely? If so we probably can't do much here.

Separate question, if the code isn't dead, why can we suddenly reenable tests? It might be interesting to git-bisect to find out which commit caused them to no longer fail, maybe we'd get more pointers from there and still find a way to simplify something.

[t-rasmud]

Reverting changes to TrustNonNullChecker

[t-rasmud]

In D119270#3305768, @NoQ wrote:

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.

I reverted these changes in my latest revision. I was under the wrong impression that this code was moved into the constraint solver.

When I suggested this change in D118657 my thought process was that given that we now handle operators == and != a lot better, these extra traits are now redundant as we can replace them with solver constraints. But now that I had a closer look, it looks like these constraints are much more sophisticated, i.e. not just $a == $b but more like (bool)$a => (bool)$b, so we might still need them. Unless, of course, somebody wants to move this code to the constraint solver, which probably might work.

@t-rasmud can you confirm some of these tests still fail if you remove the traits entirely? If so we probably can't do much here.

Removing these changes causes trustnonnullchecker_test.m to fail at lines 78 , 87, 96, etc (return s in retImpliesIndex... methods).

[NoQ]

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.

[uabelho]

Hello,

WIth EXPENSIVE_CHECKS this testcase now fails all the time.
So

build-all-expensive/bin/clang -cc1 -internal-isystem build-all-expensive/lib/clang/15.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -fblocks -analyze -analyzer-checker=core,nullability,apiModeling,debug.ExprInspection  -verify ../clang/test/Analysis/trustnonnullchecker_test.m

fails with

clang: ../../clang/include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:100: clang::ento::ConstraintManager::ProgramStatePair clang::ento::ConstraintManager::assumeDual(clang::ento::ProgramStateRef, clang::ento::DefinedSVal): Assertion `assume(State, Cond, false) && "System is over constrained."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: build-all-expensive/bin/clang -cc1 -internal-isystem build-all-expensive/lib/clang/15.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -fblocks -analyze -analyzer-checker=core,nullability,apiModeling,debug.ExprInspection -verify ../clang/test/Analysis/trustnonnullchecker_test.m
1.	<eof> parser at end of file
2.	While analyzing stack: 
	#0 Calling -[DictionarySubclass coder]
3.	../clang/test/Analysis/trustnonnullchecker_test.m:190:9: Error evaluating branch
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
build-all-expensive/bin/clang(_ZN4llvm3sys15PrintStackTraceERNS_11raw_ostreamEi+0x23)[0x2e66ae3]
build-all-expensive/bin/clang(_ZN4llvm3sys17RunSignalHandlersEv+0xee)[0x2e6475e]
build-all-expensive/bin/clang[0x2e66e66]
/lib64/libpthread.so.0(+0xf630)[0x7f5af3a04630]
/lib64/libc.so.6(gsignal+0x37)[0x7f5af1137387]
/lib64/libc.so.6(abort+0x148)[0x7f5af1138a78]
/lib64/libc.so.6(+0x2f1a6)[0x7f5af11301a6]
/lib64/libc.so.6(+0x2f252)[0x7f5af1130252]
build-all-expensive/bin/clang[0x4143927]
build-all-expensive/bin/clang(_ZN5clang4ento10ExprEngine13processBranchEPKNS_4StmtERNS0_18NodeBuilderContextEPNS0_12ExplodedNodeERNS0_15ExplodedNodeSetEPKNS_8CFGBlockESD_+0xaae)[0x44fd8de]
build-all-expensive/bin/clang(_ZN5clang4ento10CoreEngine12HandleBranchEPKNS_4StmtES4_PKNS_8CFGBlockEPNS0_12ExplodedNodeE+0xa1)[0x44dafd1]
build-all-expensive/bin/clang(_ZN5clang4ento10CoreEngine15ExecuteWorkListEPKNS_15LocationContextEjN4llvm18IntrusiveRefCntPtrIKNS0_12ProgramStateEEE+0x4a4)[0x44d9624]
build-all-expensive/bin/clang[0x4118d2c]
build-all-expensive/bin/clang[0x40fc6aa]
build-all-expensive/bin/clang(_ZN5clang8ParseASTERNS_4SemaEbb+0x273)[0x45f2633]
build-all-expensive/bin/clang(_ZN5clang14FrontendAction7ExecuteEv+0x106)[0x38464a6]
build-all-expensive/bin/clang(_ZN5clang16CompilerInstance13ExecuteActionERNS_14FrontendActionE+0x144)[0x37c19e4]
build-all-expensive/bin/clang(_ZN5clang25ExecuteCompilerInvocationEPNS_16CompilerInstanceE+0x2a2)[0x3901152]
build-all-expensive/bin/clang(_Z8cc1_mainN4llvm8ArrayRefIPKcEES2_Pv+0x5fa)[0xa2980a]
build-all-expensive/bin/clang[0xa27c2f]
build-all-expensive/bin/clang(main+0x2c73)[0xa277c3]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7f5af1123555]
build-all-expensive/bin/clang[0xa24849]
Abort

[t-rasmud]

Reverts previous changes; We are keeping trustnonnullchecker_test.m disabled because it still fails expensive checks.

[steakhal]

We'll get rid of that expensive check in the close future since it's no longer that meaningful.
After that, we can give it another shot.

[martong]

In D119270#3322747, @steakhal wrote:

We'll get rid of that expensive check in the close future since it's no longer that meaningful.
After that, we can give it another shot.

That moment is imminent.
Keep an eye on D124674 .

clang/test/Analysis/trustnonnullchecker_test.m

+// Temporarily disabling the test, it failes the "system is over-constrained" (part of expensive checks)
+// assertion in *non* optimized builds.
+// REQUIRES: rdar44992170
 // RUN: %clang_analyze_cc1 -fblocks -analyze -analyzer-checker=core,nullability,apiModeling,debug.ExprInspection  -verify %s
 
 #include "Inputs/system-header-simulator-for-nullability.h"
 
 void clang_analyzer_warnIfReached();
 
 NSString* _Nonnull trust_nonnull_framework_annotation() {
   NSString* out = [NSString generateString];