r348038 - [analyzer] Emit an error for invalid -analyzer-config inputs

Ilya Biryukov via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 3 03:38:47 PST 2018


Hi Kristóf,

There is clearly a typo in checking for -ctu-dir and -model-path being a
directory. Instead of checking -model-path, the code was checking for
-ctu-dir twice.
Landed a fix in r348125.

On Fri, Nov 30, 2018 at 10:27 PM Kristof Umann via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: szelethus
> Date: Fri Nov 30 13:24:31 2018
> New Revision: 348038
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348038&view=rev
> Log:
> [analyzer] Emit an error for invalid -analyzer-config inputs
>
> Differential Revision: https://reviews.llvm.org/D53280
>
> Added:
>     cfe/trunk/test/Analysis/invalid-analyzer-config-value.c
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
>     cfe/trunk/include/clang/Driver/CC1Options.td
>     cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
>     cfe/trunk/lib/Driver/ToolChains/Clang.cpp
>     cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td?rev=348038&r1=348037&r2=348038&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td Fri Nov 30
> 13:24:31 2018
> @@ -299,6 +299,9 @@ def err_analyzer_config_no_value : Error
>    "analyzer-config option '%0' has a key but no value">;
>  def err_analyzer_config_multiple_values : Error<
>    "analyzer-config option '%0' should contain only one '='">;
> +def err_analyzer_config_invalid_input : Error<
> +  "invalid input for analyzer-config option '%0', that expects %1 value">;
> +def err_analyzer_config_unknown : Error<"unknown analyzer-config '%0'">;
>
>  def err_drv_invalid_hvx_length : Error<
>    "-mhvx-length is not supported without a -mhvx/-mhvx= flag">;
>
> Modified: cfe/trunk/include/clang/Driver/CC1Options.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CC1Options.td?rev=348038&r1=348037&r2=348038&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Driver/CC1Options.td (original)
> +++ cfe/trunk/include/clang/Driver/CC1Options.td Fri Nov 30 13:24:31 2018
> @@ -138,6 +138,12 @@ def analyzer_list_enabled_checkers : Fla
>  def analyzer_config : Separate<["-"], "analyzer-config">,
>    HelpText<"Choose analyzer options to enable">;
>
> +def analyzer_config_compatibility_mode : Separate<["-"],
> "analyzer-config-compatibility-mode">,
> +  HelpText<"Don't emit errors on invalid analyzer-config inputs">;
> +
> +def analyzer_config_compatibility_mode_EQ : Joined<["-"],
> "analyzer-config-compatibility-mode=">,
> +  Alias<analyzer_config_compatibility_mode>;
> +
>
>  //===----------------------------------------------------------------------===//
>  // Migrator Options
>
>  //===----------------------------------------------------------------------===//
>
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h?rev=348038&r1=348037&r2=348038&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
> (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Fri Nov
> 30 13:24:31 2018
> @@ -200,6 +200,7 @@ public:
>    unsigned ShowCheckerHelp : 1;
>    unsigned ShowEnabledCheckerList : 1;
>    unsigned ShowConfigOptionsList : 1;
> +  unsigned ShouldEmitErrorsOnInvalidConfigValue : 1;
>    unsigned AnalyzeAll : 1;
>    unsigned AnalyzerDisplayProgress : 1;
>    unsigned AnalyzeNestedBlocks : 1;
> @@ -222,6 +223,7 @@ public:
>    /// The mode of function selection used during inlining.
>    AnalysisInliningMode InliningMode = NoRedundancy;
>
> +  // Create a field for each -analyzer-config option.
>  #define ANALYZER_OPTION_DEPENDS_ON_USER_MODE(TYPE, NAME, CMDFLAG, DESC,
>       \
>                                               SHALLOW_VAL, DEEP_VAL)
>       \
>    ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, SHALLOW_VAL)
> @@ -233,13 +235,39 @@ public:
>  #undef ANALYZER_OPTION
>  #undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
>
> +  // Create an array of all -analyzer-config command line options. Sort
> it in
> +  // the constructor.
> +  std::vector<StringRef> AnalyzerConfigCmdFlags = {
> +#define ANALYZER_OPTION_DEPENDS_ON_USER_MODE(TYPE, NAME, CMDFLAG, DESC,
>       \
> +                                             SHALLOW_VAL, DEEP_VAL)
>       \
> +  ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, SHALLOW_VAL)
> +
> +#define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL)
>       \
> +    CMDFLAG,
> +
> +#include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
> +#undef ANALYZER_OPTION
> +#undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
> +  };
> +
> +  bool isUnknownAnalyzerConfig(StringRef Name) const {
> +
> +    assert(std::is_sorted(AnalyzerConfigCmdFlags.begin(),
> +                          AnalyzerConfigCmdFlags.end()));
> +
> +    return !std::binary_search(AnalyzerConfigCmdFlags.begin(),
> +                               AnalyzerConfigCmdFlags.end(), Name);
> +  }
> +
>    AnalyzerOptions()
>        : DisableAllChecks(false), ShowCheckerHelp(false),
>          ShowEnabledCheckerList(false), ShowConfigOptionsList(false),
>          AnalyzeAll(false), AnalyzerDisplayProgress(false),
>          AnalyzeNestedBlocks(false), eagerlyAssumeBinOpBifurcation(false),
>          TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
> -        UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false)
> {}
> +        UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false)
> {
> +    llvm::sort(AnalyzerConfigCmdFlags);
> +  }
>
>    /// Interprets an option's string value as a boolean. The "true" string
> is
>    /// interpreted as true and the "false" string is interpreted as false.
>
> Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=348038&r1=348037&r2=348038&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
> +++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Fri Nov 30 13:24:31 2018
> @@ -2368,6 +2368,9 @@ static void RenderAnalyzerOptions(const
>    // 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");
>
> Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=348038&r1=348037&r2=348038&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
> +++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Fri Nov 30 13:24:31 2018
> @@ -181,8 +181,10 @@ static void addDiagnosticArgs(ArgList &A
>    }
>  }
>
> +// Parse the Static Analyzer configuration. If \p Diags is set to nullptr,
> +// it won't verify the input.
>  static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
> -                                 DiagnosticsEngine &Diags);
> +                                 DiagnosticsEngine *Diags);
>
>  static void getAllNoBuiltinFuncValues(ArgList &Args,
>                                        std::vector<std::string> &Funcs) {
> @@ -284,6 +286,12 @@ static bool ParseAnalyzerArgs(AnalyzerOp
>    Opts.ShowCheckerHelp = Args.hasArg(OPT_analyzer_checker_help);
>    Opts.ShowConfigOptionsList = Args.hasArg(OPT_analyzer_config_help);
>    Opts.ShowEnabledCheckerList =
> Args.hasArg(OPT_analyzer_list_enabled_checkers);
> +  Opts.ShouldEmitErrorsOnInvalidConfigValue =
> +      /* negated */!llvm::StringSwitch<bool>(
> +
>  Args.getLastArgValue(OPT_analyzer_config_compatibility_mode))
> +        .Case("true", true)
> +        .Case("false", false)
> +        .Default(false);
>    Opts.DisableAllChecks = Args.hasArg(OPT_analyzer_disable_all_checks);
>
>    Opts.visualizeExplodedGraphWithGraphViz =
> @@ -320,7 +328,7 @@ static bool ParseAnalyzerArgs(AnalyzerOp
>
>    // Go through the analyzer configuration options.
>    for (const auto *A : Args.filtered(OPT_analyzer_config)) {
> -    A->claim();
> +
>      // We can have a list of comma separated config names, e.g:
>      // '-analyzer-config key1=val1,key2=val2'
>      StringRef configList = A->getValue();
> @@ -342,11 +350,24 @@ static bool ParseAnalyzerArgs(AnalyzerOp
>          Success = false;
>          break;
>        }
> +
> +      // TODO: Check checker options too, possibly in CheckerRegistry.
> +      // Leave unknown non-checker configs unclaimed.
> +      if (!key.contains(":") && Opts.isUnknownAnalyzerConfig(key)) {
> +        if (Opts.ShouldEmitErrorsOnInvalidConfigValue)
> +          Diags.Report(diag::err_analyzer_config_unknown) << key;
> +        continue;
> +      }
> +
> +      A->claim();
>        Opts.Config[key] = val;
>      }
>    }
>
> -  parseAnalyzerConfigs(Opts, Diags);
> +  if (Opts.ShouldEmitErrorsOnInvalidConfigValue)
> +    parseAnalyzerConfigs(Opts, &Diags);
> +  else
> +    parseAnalyzerConfigs(Opts, nullptr);
>
>    llvm::raw_string_ostream os(Opts.FullCompilerInvocation);
>    for (unsigned i = 0; i < Args.getNumInputArgStrings(); ++i) {
> @@ -365,56 +386,82 @@ static StringRef getStringOption(Analyze
>  }
>
>  static void initOption(AnalyzerOptions::ConfigTable &Config,
> +                       DiagnosticsEngine *Diags,
>                         StringRef &OptionField, StringRef Name,
>                         StringRef DefaultVal) {
> +  // String options may be known to invalid (e.g. if the expected string
> is a
> +  // file name, but the file does not exist), those will have to be
> checked in
> +  // parseConfigs.
>    OptionField = getStringOption(Config, Name, DefaultVal);
>  }
>
>  static void initOption(AnalyzerOptions::ConfigTable &Config,
> +                       DiagnosticsEngine *Diags,
>                         bool &OptionField, StringRef Name, bool
> DefaultVal) {
> -  // FIXME: We should emit a warning here if the value is something other
> than
> -  // "true", "false", or the empty string (meaning the default value),
> -  // but the AnalyzerOptions doesn't have access to a diagnostic engine.
> -  OptionField = llvm::StringSwitch<bool>(getStringOption(Config, Name,
> -                                               (DefaultVal ? "true" :
> "false")))
> +  auto PossiblyInvalidVal = llvm::StringSwitch<Optional<bool>>(
> +                 getStringOption(Config, Name, (DefaultVal ? "true" :
> "false")))
>        .Case("true", true)
>        .Case("false", false)
> -      .Default(DefaultVal);
> +      .Default(None);
> +
> +  if (!PossiblyInvalidVal) {
> +    if (Diags)
> +      Diags->Report(diag::err_analyzer_config_invalid_input)
> +        << Name << "a boolean";
> +    else
> +      OptionField = DefaultVal;
> +  } else
> +    OptionField = PossiblyInvalidVal.getValue();
>  }
>
>  static void initOption(AnalyzerOptions::ConfigTable &Config,
> +                       DiagnosticsEngine *Diags,
>                         unsigned &OptionField, StringRef Name,
>                         unsigned DefaultVal) {
> +
>    OptionField = DefaultVal;
>    bool HasFailed = getStringOption(Config, Name,
> std::to_string(DefaultVal))
>                       .getAsInteger(10, OptionField);
> -  assert(!HasFailed && "analyzer-config option should be numeric");
> -  (void)HasFailed;
> +  if (Diags && HasFailed)
> +    Diags->Report(diag::err_analyzer_config_invalid_input)
> +      << Name << "an unsigned";
>  }
>
>  static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
> -                                 DiagnosticsEngine &Diags) {
> -  // TODO: Emit warnings for incorrect options.
> +                                 DiagnosticsEngine *Diags) {
>    // TODO: There's no need to store the entire configtable, it'd be plenty
>    // enough tostore checker options.
>
>  #define ANALYZER_OPTION(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL)
>       \
> -  initOption(AnOpts.Config, AnOpts.NAME, CMDFLAG, DEFAULT_VAL);
>       \
> +  initOption(AnOpts.Config, Diags, AnOpts.NAME, CMDFLAG, DEFAULT_VAL);
>
>  #define ANALYZER_OPTION_DEPENDS_ON_USER_MODE(TYPE, NAME, CMDFLAG, DESC,
>       \
>                                             SHALLOW_VAL, DEEP_VAL)
>       \
>    switch (AnOpts.getUserMode()) {
>       \
>    case UMK_Shallow:
>       \
> -    initOption(AnOpts.Config, AnOpts.NAME, CMDFLAG, SHALLOW_VAL);
>       \
> +    initOption(AnOpts.Config, Diags, AnOpts.NAME, CMDFLAG, SHALLOW_VAL);
>      \
>      break;
>      \
>    case UMK_Deep:
>      \
> -    initOption(AnOpts.Config, AnOpts.NAME, CMDFLAG, DEEP_VAL);
>      \
> +    initOption(AnOpts.Config, Diags, AnOpts.NAME, CMDFLAG, DEEP_VAL);
>       \
>      break;
>      \
>    }
>       \
>
>  #include "clang/StaticAnalyzer/Core/AnalyzerOptions.def"
>  #undef ANALYZER_OPTION
>  #undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
> +
> +  // At this point, AnalyzerOptions is configured. Let's validate some
> options.
> +
> +  if (!Diags)
> +    return;
> +
> +  if (!AnOpts.CTUDir.empty() &&
> !llvm::sys::fs::is_directory(AnOpts.CTUDir))
> +    Diags->Report(diag::err_analyzer_config_invalid_input)
> +      << "ctu-dir" << "a filename";
> +
> +  if (!AnOpts.CTUDir.empty() &&
> !llvm::sys::fs::is_directory(AnOpts.CTUDir))
> +    Diags->Report(diag::err_analyzer_config_invalid_input)
> +      << "model-path" << "a filename";
>  }
>
>  static bool ParseMigratorArgs(MigratorOptions &Opts, ArgList &Args) {
>
> Added: cfe/trunk/test/Analysis/invalid-analyzer-config-value.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/invalid-analyzer-config-value.c?rev=348038&view=auto
>
> ==============================================================================
> --- cfe/trunk/test/Analysis/invalid-analyzer-config-value.c (added)
> +++ cfe/trunk/test/Analysis/invalid-analyzer-config-value.c Fri Nov 30
> 13:24:31 2018
> @@ -0,0 +1,71 @@
> +// 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'
> +
> +// expected-no-diagnostics
> +
> +int main() {}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


-- 
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181203/fbd6fc23/attachment-0001.html>


More information about the cfe-commits mailing list