[analyzer] Use %clang_cc1 instead of %clang_analyze_cc1 if the test requires a specific constraint manager

[steakhal]

Summary

AFAIK the only difference between %clang_cc1 and %clang_analyze_cc1 that the latter inserts a placeholder which is going to be replaced by the appropriate constraint manager.
This placeholder was placed by the llvm/utils/lit/lit/llvm/config.py:

ToolSubst('%clang_analyze_cc1', command='%clang_cc1', extra_args=['-analyze', '%analyze', '-setup-static-analyzer']+additional_flags),
ToolSubst('%clang_cc1', command=self.config.clang, extra_args=['-cc1', '-internal-isystem', builtin_include_dir, '-nostdsysteminc']+additional_flags),

The constraint manager could either the ranged based CM or the Z3 based CM, depending on the test environment and configuration.

Relevant code, that substitutes the %analyzer placeholder first to the range based one, then to the z3 based one:
https://github.com/llvm/llvm-project/blob/2997a3042e41dfc1d842fbe7be8a7777454ad4b5/clang/test/Analysis/analyzer_test.py#L18-L29

if 'z3' not in test.requires:
    results.append(self.executeWithAnalyzeSubstitution(
        saved_test, litConfig, '-analyzer-constraints=range'))

    if results[-1].code == lit.Test.FAIL:
        return results[-1]

# If z3 backend available, add an additional run line for it
if self.use_z3_solver == '1':
    assert(test.config.clang_staticanalyzer_z3 == '1')
    results.append(self.executeWithAnalyzeSubstitution(
        saved_test, litConfig, '-analyzer-constraints=z3 -DANALYZER_CM_Z3'))

After seeing all of the relevant code segments, I assume that %clang_analyze_cc1 should be ONLY USED if the test author wants to run ALL available constraint managers on each RUN command.

So, if the code depends on a specific constraint manager, it should use the %clang_cc1 placeholder instead, and explicitly state which CM it depends on.

We could also introduce a new joker command (like %clang_analyze_cc1_ranged or something similar) in the llvm/utils/lit/lit/llvm/config.py if it's more appropriate.

On the cfe-dev mailing list we had a related discussion, dealing with this subject in more depth:
ClangSA tests that 'REQUIRES: z3' won't run even if you have z3 installed

[ddcc]

The original intention was to have tests run with both constraint managers, unless Z3 was required, in which case it would be Z3 only, but AFAIK that testing infrastructure was never set up. This was before Z3 got merged into LLVM proper, so the additional USE_Z3_SOLVER was to prevent bots from failing if Z3 wasn't installed. Given that all of this should be unused, if there's interest in setting up test bots for the SMT constraint manager, it should be fine to change any of this as needed.

[NoQ]

AFAIK the only difference between %clang_cc1 and %clang_analyze_cc1 that the latter inserts a placeholder which is going to be replaced by the appropriate constraint manager.

ToolSubst('%clang_analyze_cc1', command='%clang_cc1', extra_args=['-analyze', '%analyze', '-setup-static-analyzer']+additional_flags),

Well, obviously it also inserts -analyze and -setup-static-analyzer. So it already serves two purposes: ensures that the analyzer is correctly set up for testing *and* produces multiple run-lines for different constraint managers. It's not unimaginable that we insert more common setup into that run-line, and then all the code duplication will bite us.

I'm too bad with LIT to come up with a better solution but usually when %clang_analyze_cc1 doesn't work for me i insert a FIXME to find a better solution :)

[steakhal]

In D83558#2144716, @NoQ wrote:

AFAIK the only difference between %clang_cc1 and %clang_analyze_cc1 that the latter inserts a placeholder which is going to be replaced by the appropriate constraint manager.

ToolSubst('%clang_analyze_cc1', command='%clang_cc1', extra_args=['-analyze', '%analyze', '-setup-static-analyzer']+additional_flags),

Well, obviously it also inserts -analyze and -setup-static-analyzer. So it already serves two purposes: ensures that the analyzer is correctly set up for testing *and* produces multiple run-lines for different constraint managers. It's not unimaginable that we insert more common setup into that run-line, and then all the code duplication will bite us.

You are right, I simplified a bit to focus on the behavior that I wanted to highlight.

I'm too bad with LIT to come up with a better solution but usually, when %clang_analyze_cc1 doesn't work for me I insert a FIXME to find a better solution :)

I'm also not an expert, but I would gladly spend some time to consolidate our testing infra.

I think we should be more clear about that %clang_analyze_cc1 has this strange behavior. I even think that renaming that to something more meaningful would be appreciated by the community.
Eg. to something like clang_analyze_cc1_also_with_z3_solver. TBH I lack the imagination of naming things, so you can help me out with this one.
Maybe: introduce the %clang_analyze_cc1_multi_cm to substitute the current %clang_analyze_cc1 and then modify the %clang_analyze_cc1 to only set up the -analyze and -setup-static-analyzer flags letting the test author add the rest of the configuration like the CM.

I'm also reluctant to keep this feature as-is since the generated exploded graph could be significantly different using different CMs.
But removing this would lower our coverage with the Z3-CM significantly - although I'm not entirely sure about this since I don't know if there were build bots using this USE_Z3_SOLVER option.
If there were none of such build bots, I would recommend removing this, otherwise, we should come up with some other solution.

What do you think @NoQ?

[steakhal]

Abandoned in favor of D83677.

clang/test/Analysis/PR24184.cpp

-// UNSUPPORTED: z3
-// RUN: %clang_analyze_cc1 -w -fcxx-exceptions -analyzer-checker=core -analyzer-checker=alpha.core.PointerArithm,alpha.core.CastToStruct -analyzer-max-loop 64 -verify %s
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -analyzer-checker=cplusplus -fcxx-exceptions -analyzer-checker alpha.core.PointerArithm,alpha.core.CastToStruct -analyzer-max-loop 63 -verify %s
+// RUN: %clang_cc1 -w -fcxx-exceptions -analyzer-max-loop 64 -verify %s \
+// RUN:   -analyze -analyzer-constraints=range -setup-static-analyzer \
+// RUN:   -analyzer-checker=core,alpha.core.PointerArithm,alpha.core.CastToStruct
+//
+// RUN: %clang_cc1 -w -fcxx-exceptions -analyzer-max-loop 63 -verify %s \
+// RUN:   -analyze -analyzer-constraints=range -setup-static-analyzer \
+// RUN:   -analyzer-checker=core,alpha.core.PointerArithm,alpha.core.CastToStruct,cplusplus
 
 // These tests used to hit an assertion in the bug report. Test case from http://llvm.org/PR24184.
 typedef struct {
   int cbData;
   unsigned pbData;
 } CRYPT_DATA_BLOB;
 
 typedef enum { DT_NONCE_FIXED } DATA_TYPE;