[analyzer] Fix backward compatibility issue after D53280 'Emit an error for invalid -analyzer-config inputs'

[NoQ]

Summary

Fix according to the discussion here: D53280

[NoQ]

Ugh. No, it doesn't work. It seems that stuff after -Xclang is not included in Args (even at the beginning of the function, when no arguments are consumed), and it doesn't make it into CmdArgs (yet?).

Test passes because -analyzer-config is a sub-string of the file name. If you rename the test, it stops working (:

I'll be debugging this a bit more on my end, but help is welcome. And in any case, thanks a lot for looking into this!~ I'm even more surprised how hard it is to come up with a sensible solution to our weird problem.

test/Analysis/invalid-analyzer-config-value.c

72 ↗

(On Diff #178661)

This run-line *does* have -analyze specified, because that's what --analyze expands to. I guess the test that we need would be along the lines of clang -Xclang -analyzer-config ... without --analyze.

[NoQ]

These arguments seem to be available during the top-level call, Driver::BuildCompilation. I guess we need to stick this somewhere in between.

[Szelethus]

test/Analysis/invalid-analyzer-config-value.c

72 ↗

(On Diff #178661)

Uhhhh sorry about that.

[Szelethus]

Yea, should've used hasArg anyways. I'll look into this tonight.

[Szelethus]

lib/Driver/ToolChains/Clang.cpp

3694 ↗

(On Diff #178661)

When I dump Args for the following invocation, I get this:

clang -c dummie.cpp -Xclang -analyzer-config -Xclang blahblah=haha

* < Opt:<FlagClass Prefixes:["-"] Name:"c" Group:<GroupClass Name:"<action group>">
>
 Index:0 Values: []>
* < Opt:<InputClass Prefixes:[] Name:"<input>">
 Index:1 Values: ['test/Analysis/cxx-uninitialized-object.cpp']>
* < Opt:<SeparateClass Prefixes:["-"] Name:"Xclang" Group:<GroupClass Name:"<CompileOnly group>">
>
 Index:2 Values: ['-analyzer-config']>
* < Opt:<SeparateClass Prefixes:["-"] Name:"Xclang" Group:<GroupClass Name:"<CompileOnly group>">
>
 Index:4 Values: ['blahblah=haha']>

so there should be a way to retrieve it. For Args.getArgString(/*index*/3) it also prints "-analyzer-config" -- but I'm still looking for a "proper" way.

[Szelethus]

I'm still looking for a sensible solution, but I'll at least share a patch that actually works.

[Szelethus]

test/Analysis/invalid-analyzer-config-value.c

72 ↗

(On Diff #178661)

I checked this against a completely different file, and it works.

[NoQ]

It works! :D

I guess, please commit this if you don't find anything better.

test/Analysis/invalid-analyzer-config-value.c

72 ↗

(On Diff #178661)

Given that we stepped on this, i guess it's still worth it to rename the test file.

[NoQ]

lib/Driver/ToolChains/Clang.cpp

3700 ↗

(On Diff #178927)

Needs an LLVM-style loop!~ :)

[george.karpenkov]

lib/Driver/ToolChains/Clang.cpp

3700 ↗

(On Diff #178927)

Shouldn't this be under else- for the previous branch? Otherwise it seems that the option would be added twice.

[NoQ]

@Szelethus: Ok if we commit this real quick and then see if there's a better solution?

[Szelethus]

Sure! I'm on my phone, and will be for a little while, can you commit on my behalf?

lib/Driver/ToolChains/Clang.cpp

3700 ↗

(On Diff #178927)

I originally deleted that line, but I lost in somewhere :/

[NoQ]

Re-remove the first argument append.

Add a test in which file name doesn't match our search.

[Szelethus]

Thanks! Sorry about being a little slow with this, I'm sadly busier than I expected, but I'll definitely think about a nicer solution.

[NoQ]

Np! We really appreciate your work!

[NoQ]

Hmm, this http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/14563/steps/test-stage2-compiler/logs/stdio also looks like ours. I guess i'll revert cause i've no idea what causes that.

[arphaman]

I think Args.getArgString might return nullptr so that's why you could crash.

[NoQ]

In D55823#1338337, @arphaman wrote:

I think Args.getArgString might return nullptr so that's why you could crash.

Nope, we crash on index lookup. It seems that Args.size() is larger than the small vector that we're allowed to access via .getArgStrings(). I tried Args.getNumInputArgStrings() and it seems better.

[NoQ]

Though yeah, thanks, we might be crashing for that reason as well in the future!

[NoQ]

Remove debug prints >.<

[NoQ]

George committed this as rC349863. Before that happened, we kinda figured out how it works: -analyzer-config is not a flag, it's an argument of the -Xclang flag, so we should look for -Xclang and see what value does the argument have.

Now, there's also -Xanalyzer. But -Xanalyzer is automatically not consumed during compilation, so any -analyzer-config flags passed through it would be ignored during compilation.

However, --analyze -Xanalyzer -analyzer-config ... is now broken :/

[NoQ]

Should be sorted our in rC349866.

Ugh, what a mess :) Thanks everyone :)

[Szelethus]

Missed out on the developments. Thank you so much for handling this!

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

static void RenderAnalyzerOptions(const ArgList &Args, ArgStringList &CmdArgs,
                                   const llvm::Triple &Triple,
                                   const InputInfo &Input) {
   // Enable region store model by default.
   CmdArgs.push_back("-analyzer-store=region");
 
   // Treat blocks as analysis entry points.
   CmdArgs.push_back("-analyzer-opt-analyze-nested-blocks");
 
-  // Enable compatilibily mode to avoid analyzer-config related errors.
-  CmdArgs.push_back("-analyzer-config-compatibility-mode=true");
-
   // Add default argument set.
   if (!Args.hasArg(options::OPT__analyzer_no_default_checks)) {
     CmdArgs.push_back("-analyzer-checker=core");
     CmdArgs.push_back("-analyzer-checker=apiModeling");
 
     if (!Triple.isWindowsMSVCEnvironment()) {
       CmdArgs.push_back("-analyzer-checker=unix");
     } else {
if (Args.hasArg(options::OPT_static))
     CmdArgs.push_back("-static-define");
 
   if (Args.hasArg(options::OPT_municode))
     CmdArgs.push_back("-DUNICODE");
 
   if (isa<AnalyzeJobAction>(JA))
     RenderAnalyzerOptions(Args, CmdArgs, Triple, Input);
 
+  // Enable compatilibily mode to avoid analyzer-config related errors.
+  // Since we can't access frontend flags through hasArg, let's manually iterate
+  // through them.
+  for (size_t Index = 0; Index < Args.size(); ++Index) {
+    if (StringRef(Args.getArgString(Index)).contains("-analyzer-config")) {
+      CmdArgs.push_back("-analyzer-config-compatibility-mode=true");
+      break;
+    }
+  }
+
   CheckCodeGenerationOptions(D, Args);
 
   unsigned FunctionAlignment = ParseFunctionAlignment(TC, Args);
   assert(FunctionAlignment <= 31 && "function alignment will be truncated!");
   if (FunctionAlignment) {
     CmdArgs.push_back("-function-alignment");
     CmdArgs.push_back(Args.MakeArgString(std::to_string(FunctionAlignment)));
   }

cfe/trunk/test/Analysis/invalid-a-na-ly-zer-con-fig-value.c

+// Same as invalid-analyzer-config-value.c but without -analyzer-config
+// in the file name, so that argument string pattern matching
+// didn't accidentally match it.
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config notes-as-events=yesplease \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-BOOL-INPUT
+
+// CHECK-BOOL-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-BOOL-INPUT-SAME:        'notes-as-events', that expects a boolean value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config notes-as-events=yesplease
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config max-inlinable-size=400km/h \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-UINT-INPUT
+
+// CHECK-UINT-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-UINT-INPUT-SAME:        'max-inlinable-size', that expects an unsigned
+// CHECK-UINT-INPUT-SAME:        value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config max-inlinable-size=400km/h
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config ctu-dir=0123012301230123 \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-FILENAME-INPUT
+
+// CHECK-FILENAME-INPUT: (frontend): invalid input for analyzer-config option
+// CHECK-FILENAME-INPUT-SAME:        'ctu-dir', that expects a filename
+// CHECK-FILENAME-INPUT-SAME:        value
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config ctu-dir=0123012301230123
+
+
+// RUN: not %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config no-false-positives=true \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-UNKNOWN-CFG
+
+// CHECK-UNKNOWN-CFG: (frontend): unknown analyzer-config 'no-false-positives'
+
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config-compatibility-mode=true \
+// RUN:   -analyzer-config no-false-positives=true
+
+
+// Test the driver properly using "analyzer-config-compatibility-mode=true",
+// no longer causing an error on input error.
+// RUN: %clang --analyze %s
+
+// RUN: not %clang --analyze %s \
+// RUN:   -Xclang -analyzer-config -Xclang no-false-positives=true \
+// RUN:   -Xclang -analyzer-config-compatibility-mode=false \
+// RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NO-COMPAT
+
+// CHECK-NO-COMPAT: error: unknown analyzer-config 'no-false-positives'
+
+// Test the driver properly using "analyzer-config-compatibility-mode=true",
+// even if -analyze isn't specified.
+// RUN: %clang -Xclang -analyzer-config -Xclang remember=TheVasa %s
+
+// expected-no-diagnostics
+
+int main() {}

cfe/trunk/test/Analysis/invalid-analyzer-config-value.c

 
 // RUN: not %clang --analyze %s \
 // RUN:   -Xclang -analyzer-config -Xclang no-false-positives=true \
 // RUN:   -Xclang -analyzer-config-compatibility-mode=false \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-NO-COMPAT
 
 // CHECK-NO-COMPAT: error: unknown analyzer-config 'no-false-positives'
 
+// Test the driver properly using "analyzer-config-compatibility-mode=true",
+// even if -analyze isn't specified.
+// RUN: %clang -Xclang -analyzer-config -Xclang remember=TheVasa %s
+
 // expected-no-diagnostics
 
 int main() {}