[Bash-autocompletion] Add support for static analyzer flags

[yamaguchi]

Summary

This is a patch for clang autocomplete feature.

It will collect values which -analyzer-checker takes, which is defined in
clang/StaticAnalyzer/Checkers/Checkers.inc, dynamically.
First, from ValuesCode class in Options.td, TableGen will generate C++
code in Options.inc. Options.inc will be included in DriverOptions.cpp, and
calls OptTable's addValues function. addValues function will add second
argument to Option's Values class. Values contains string like "foo,bar,.."
which is handed to Values class
in OptTable.

[teemperor]

That's a really nice approach to this problem, good job Yuka! See my inline comments for some minor remarks.

clang/include/clang/Driver/CC1Options.td

104 ↗

(On Diff #111312)

Please also add the PACKAGE macros from Checkers.inc here. There are also valid macros:

#define GET_PACKAGES
#define PACKAGE(FULLNAME, G, D)  FULLNAME ","
#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
#undef GET_PACKAGES

107 ↗

(On Diff #111312)

Maybe we should treat the indentation of 2 here as the minimum indentation. Otherwise this file becomes hard to read. I.e. this instead:

const char* Values =
#define GET_CHECKERS
#define CHECKER(FULLNAME, CLASS, DESCFILE, HT, G, H)  FULLNAME ","
#include "clang/StaticAnalyzer/Checkers/Checkers.inc"
#undef GET_CHECKERS
;

clang/test/Driver/autocomplete.c

97 ↗

(On Diff #111312)

Can you test for unix.Malloc instead which is more mature? alpha.clone.CloneChecker will probably change soon-ish :)

llvm/include/llvm/Option/OptTable.h

60 ↗

(On Diff #111312)

This is no longer static :)

149 ↗

(On Diff #111312)

/// instead of // (same below).

Also maybe add an example here like ... which values will be changed. For example "-fstd="/.

153 ↗

(On Diff #111312)

Right now this function just silently fails when we specify an invalid option. I think returning a bool indicating success would make this more reliable.

We maybe should check if we can change the OptTable value array type that we don't have to do a , separated list of flags here but a proper vector, but that's out of the scope of this patch.

llvm/utils/TableGen/OptParserEmitter.cpp

309 ↗

(On Diff #111312)

We should wrap this in a scope. E..g emit { and } around so that if you reuse the same variable name we don't get redefinition errors.

312 ↗

(On Diff #111312)

If addValues returns true, we should also assert here if it fails (which should never happen unless something seriously breaks).

[yamaguchi]

Update diff according to Raphael's comments.

[teemperor]

clang/lib/Driver/DriverOptions.cpp

14 ↗

(On Diff #111318)

I think the C++ version of the assert header is more consistent: #include <cassert>

llvm/utils/TableGen/OptParserEmitter.cpp

313 ↗

(On Diff #111318)

Don't call this inside an assert itself. The call would be removed on release builds (where assert is a no-op) and we wouldn't do the addValues anymore which breaks everything silently in release builds. Try something like this:

bool ValuesWereAdded = Opt.addValues(...);
(void)ValuesWereAdded; // Prevents unused variable warnings
assert(ValuesWereAdded && "Couldn't add values to OptTable!");

[ruiu]

This patch allows us to embed a piece of C++ code to each command line option to construct a list of argument candidates at runtime. With this patch, .inc files generated by OptParserEmitter contain C macros that in turn include other .inc files. That is a flexible mechanism but can be too flexible and fragile, as you can write any code in .td file, and pieces of C++ code you write in .td files are not verified even for syntax validation until that nested text inclusions are processed by a C++ compiler. It does two-stages of code generation, one is done by OptTable and the other is done by C macros. I can imagine that a single typo in a .td file can cause a mysterious error that is hard to debug. I feel like, even though command line processing is not that easy, I wonder if it is that complicated to require two-stage code generation.

That being said, I don't oppose to this patch, as people who know more about this code seem to like this approach. I'm just expressing my concern.

llvm/include/llvm/Option/OptTable.h

60 ↗

(On Diff #111312)

Just remove "non-static" instead of saying this is non-static, as being non-const is not special.

[yamaguchi]

Update diff.

[yamaguchi]

@ruiu
I understand your concern. However by doing this (rather than building functions for each flag), we will be able to get all information in OptTable itself. It is true that this is quite fragile, but adding ValuesCode to new flag is not complicated, so I believe overall it's fine.

[yamaguchi]

Update diff.

[yamaguchi]

const char* Values -> const char *Values

[ruiu]

clang/include/clang/Driver/CC1Options.td

104 ↗

(On Diff #111547)

const char* Values -> const char *Values

llvm/utils/TableGen/OptParserEmitter.cpp

308 ↗

(On Diff #111547)

Maybe it is better to do early continue to reduce indentation depth.

if (isa<UnsetInit>(R.getValueInit("ValuesCode")))
  continue;

313–314 ↗

(On Diff #111547)

You can combine these two lines:

OS << "bool ValuesWereAdded = Opt.addValues(";

318–319 ↗

(On Diff #111547)

It is more readable if you split it at a natural boundary:

OS << "(void)ValuesWereAdded;\n";
OS << "assert(ValuesWereAdded && \"Couldn't add values to OptTable!\");\n";

[yamaguchi]

@ruiu
I addressed your comments in D36820, so please take a look at it. Thank you!

[teemperor]

Found one more minor comment typo. And could you do your changes to OptParserEmitter.cpp all in this patch? Because Rui/Me pointed out those things on this review, so this patch should also fix them IMHO :)

Otherwise this is good to go, nice work!

llvm/include/llvm/Option/OptTable.h

152 ↗

(On Diff #111548)

/// instead of //.

cfe/trunk/include/clang/Driver/CC1Options.td

def analyzer_disable_retry_exhausted : Flag<["-"], "analyzer-disable-retry-exhausted">,
   HelpText<"Do not re-analyze paths leading to exhausted nodes with a different strategy (may decrease code coverage)">;
   
 def analyzer_max_loop : Separate<["-"], "analyzer-max-loop">,
   HelpText<"The maximum number of times the analyzer will go through a loop">;
 def analyzer_stats : Flag<["-"], "analyzer-stats">,
   HelpText<"Print internal analyzer statistics.">;
 
 def analyzer_checker : Separate<["-"], "analyzer-checker">,
-  HelpText<"Choose analyzer checkers to enable">;
+  HelpText<"Choose analyzer checkers to enable">,
+  ValuesCode<[{
+    const char *Values =
+    #define GET_CHECKERS
+    #define CHECKER(FULLNAME, CLASS, DESCFILE, HT, G, H)  FULLNAME ","
+    #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+    #undef GET_CHECKERS
+    #define GET_PACKAGES
+    #define PACKAGE(FULLNAME, G, D)  FULLNAME ","
+    #include "clang/StaticAnalyzer/Checkers/Checkers.inc"
+    #undef GET_PACKAGES
+    ;
+  }]>;
 def analyzer_checker_EQ : Joined<["-"], "analyzer-checker=">,
   Alias<analyzer_checker>;
 
 def analyzer_disable_checker : Separate<["-"], "analyzer-disable-checker">,
   HelpText<"Choose analyzer checkers to disable">;
 def analyzer_disable_checker_EQ : Joined<["-"], "analyzer-disable-checker=">,
   Alias<analyzer_disable_checker>;
 

cfe/trunk/lib/Driver/DriverOptions.cpp

 //===--- DriverOptions.cpp - Driver Options Table -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Driver/Options.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
+#include <cassert>
 
 using namespace clang::driver;
 using namespace clang::driver::options;
 using namespace llvm::opt;
 
 #define PREFIX(NAME, VALUE) static const char *const NAME[] = VALUE;
 #include "clang/Driver/Options.inc"
 #undef PREFIX

 public:
   DriverOptTable()
     : OptTable(InfoTable) {}
 };
 
 }
 
 std::unique_ptr<OptTable> clang::driver::createDriverOptTable() {
-  return llvm::make_unique<DriverOptTable>();
+  auto Result = llvm::make_unique<DriverOptTable>();
+  // Options.inc is included in DriverOptions.cpp, and calls OptTable's
+  // addValues function.
+  // Opt is a variable used in the code fragment in Options.inc.
+  OptTable &Opt = *Result;
+#define OPTTABLE_ARG_INIT
+#include "clang/Driver/Options.inc"
+#undef OPTTABLE_ARG_INIT
+  return std::move(Result);
 }

cfe/trunk/test/Driver/autocomplete.c

 // WARNING: -Wmacro-redefined
 // WARNING-NEXT: -Wmain
 // WARNING-NEXT: -Wmain-return-type
 // WARNING-NEXT: -Wmalformed-warning-check
 // WARNING-NEXT: -Wmany-braces-around-scalar-init
 // WARNING-NEXT: -Wmax-unsigned-zero
 // RUN: %clang --autocomplete=-Wno-invalid-pp- | FileCheck %s -check-prefix=NOWARNING
 // NOWARNING: -Wno-invalid-pp-token
+// RUN: %clang --autocomplete=-analyzer-checker, | FileCheck %s -check-prefix=ANALYZER
+// ANALYZER: unix.Malloc

llvm/trunk/include/llvm/Option/OptParser.td

class Option<list<string> prefixes, string name, OptionKind kind> {
   list<string> Prefixes = prefixes;
   string Name = name;
   OptionKind Kind = kind;
   // Used by MultiArg option kind.
   int NumArgs = 0;
   string HelpText = ?;
   string MetaVarName = ?;
   string Values = ?;
+  code ValuesCode = ?;
   list<OptionFlag> Flags = [];
   OptionGroup Group = ?;
   Option Alias = ?;
   list<string> AliasArgs = [];
 }
 
 // Helpers for defining options.
 

 class Alias<Option alias> { Option Alias = alias; }
 class AliasArgs<list<string> aliasargs> { list<string> AliasArgs = aliasargs; }
 class EnumName<string name> { string EnumName = name; }
 class Flags<list<OptionFlag> flags> { list<OptionFlag> Flags = flags; }
 class Group<OptionGroup group> { OptionGroup Group = group; }
 class HelpText<string text> { string HelpText = text; }
 class MetaVarName<string name> { string MetaVarName = name; }
 class Values<string value> { string Values = value; }
+class ValuesCode<code valuecode> { code ValuesCode = valuecode; }
 
 // Predefined options.
 
 // FIXME: Have generator validate that these appear in correct position (and
 // aren't duplicated).
 def INPUT : Option<[], "<input>", KIND_INPUT>;
 def UNKNOWN : Option<[], "<unknown>", KIND_UNKNOWN>;

llvm/trunk/include/llvm/Option/OptTable.h

struct Info {
     unsigned short Flags;
     unsigned short GroupID;
     unsigned short AliasID;
     const char *AliasArgs;
     const char *Values;
   };
 
 private:
-  /// \brief The static option information table.
-  ArrayRef<Info> OptionInfos;
+  /// \brief The option information table.
+  std::vector<Info> OptionInfos;
   bool IgnoreCase;
 
   unsigned TheInputOptionID = 0;
   unsigned TheUnknownOptionID = 0;
 
   /// The index of the first option which can be parsed (i.e., is not a
   /// special option like 'input' or 'unknown', and is not an option group).
   unsigned FirstSearchableIndex = 0;
public:
   ///
   /// \param [in] Cur - String prefix that all returned flags need
   //  to start with.
   ///
   /// \return The vector of flags which start with Cur.
   std::vector<std::string> findByPrefix(StringRef Cur,
                                         unsigned short DisableFlags) const;
 
+  /// Add Values to Option's Values class
+  ///
+  /// \param [in] Option - Prefix + Name of the flag which Values will be
+  ///  changed. For example, "-analyzer-checker".
+  /// \param [in] Values - String of Values seperated by ",", such as
+  ///  "foo, bar..", where foo and bar is the argument which the Option flag
+  ///  takes
+  ///
+  /// \return true in success, and false in fail.
+  bool addValues(const char *Option, const char *Values);
+
   /// \brief Parse a single argument; returning the new argument and
   /// updating Index.
   ///
   /// \param [in,out] Index - The current parsing position in the argument
   /// string list; on return this will be the index of the next argument
   /// string to parse.
   /// \param [in] FlagsToInclude - Only parse options with any of these flags.
   /// Zero is the default which includes all flags.

llvm/trunk/lib/Option/OptTable.cpp

if (Str.startswith(Prefix)) {
         return Prefix.size() + StringRef(I->Name).size();
     }
   }
   return 0;
 }
 
 // Returns true if one of the Prefixes + In.Names matches Option
 static bool optionMatches(const OptTable::Info &In, StringRef Option) {
-  if (In.Values && In.Prefixes)
+  if (In.Prefixes)
     for (size_t I = 0; In.Prefixes[I]; I++)
       if (Option == std::string(In.Prefixes[I]) + In.Name)
         return true;
   return false;
 }
 
 // This function is for flag value completion.
 // Eg. When "-stdlib=" and "l" was passed to this function, it will return
 // appropiriate values for stdlib, which starts with l.
 std::vector<std::string>
 OptTable::suggestValueCompletions(StringRef Option, StringRef Arg) const {
   // Search all options and return possible values.
-  for (const Info &In : OptionInfos.slice(FirstSearchableIndex)) {
-    if (!optionMatches(In, Option))
+  for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
+    const Info &In = OptionInfos[I];
+    if (!In.Values || !optionMatches(In, Option))
       continue;
 
     SmallVector<StringRef, 8> Candidates;
     StringRef(In.Values).split(Candidates, ",", -1, false);
 
     std::vector<std::string> Result;
     for (StringRef Val : Candidates)
       if (Val.startswith(Arg))
         Result.push_back(Val);
     return Result;
   }
   return {};
 }
 
 std::vector<std::string>
 OptTable::findByPrefix(StringRef Cur, unsigned short DisableFlags) const {
   std::vector<std::string> Ret;
-  for (const Info &In : OptionInfos.slice(FirstSearchableIndex)) {
+  for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
+    const Info &In = OptionInfos[I];
     if (!In.Prefixes || (!In.HelpText && !In.GroupID))
       continue;
     if (In.Flags & DisableFlags)
       continue;
 
     for (int I = 0; In.Prefixes[I]; I++) {
       std::string S = std::string(In.Prefixes[I]) + std::string(In.Name) + "\t";
       if (In.HelpText)
         S += In.HelpText;
       if (StringRef(S).startswith(Cur))
         Ret.push_back(S);
     }
   }
   return Ret;
 }
 
+bool OptTable::addValues(const char *Option, const char *Values) {
+  for (size_t I = FirstSearchableIndex, E = OptionInfos.size(); I < E; I++) {
+    Info &In = OptionInfos[I];
+    if (optionMatches(In, Option)) {
+      In.Values = Values;
+      return true;
+    }
+  }
+  return false;
+}
+
 Arg *OptTable::ParseOneArg(const ArgList &Args, unsigned &Index,
                            unsigned FlagsToInclude,
                            unsigned FlagsToExclude) const {
   unsigned Prev = Index;
   const char *Str = Args.getArgString(Index);
 
   // Anything that doesn't start with PrefixesUnion is an input, as is '-'
   // itself.
   if (isInput(PrefixesUnion, Str))
     return new Arg(getOption(TheInputOptionID), Str, Index++, Str);
 
-  const Info *Start = OptionInfos.begin() + FirstSearchableIndex;
-  const Info *End = OptionInfos.end();
+  const Info *Start = OptionInfos.data() + FirstSearchableIndex;
+  const Info *End = OptionInfos.data() + OptionInfos.size();
   StringRef Name = StringRef(Str).ltrim(PrefixChars);
 
   // Search for the first next option which could be a prefix.
   Start = std::lower_bound(Start, End, Name.data());
 
   // Options are stored in sorted order, with '\0' at the end of the
   // alphabet. Since the only options which can accept a string must
   // prefix it, we iteratively search for the next option which could

llvm/trunk/utils/TableGen/OptParserEmitter.cpp

for (unsigned i = 0, e = Opts.size(); i != e; ++i) {
     if (!isa<UnsetInit>(R.getValueInit("Values")))
       write_cstring(OS, R.getValueAsString("Values"));
     else
       OS << "nullptr";
 
     OS << ")\n";
   }
   OS << "#endif // OPTION\n";
+
+  OS << "\n";
+  OS << "#ifdef OPTTABLE_ARG_INIT\n";
+  OS << "//////////\n";
+  OS << "// Option Values\n\n";
+  for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
+    const Record &R = *Opts[I];
+    if (isa<UnsetInit>(R.getValueInit("ValuesCode")))
+      continue;
+    OS << "{\n";
+    OS << R.getValueAsString("ValuesCode");
+    OS << "\n";
+    for (const std::string &Pref : R.getValueAsListOfStrings("Prefixes")) {
+      OS << "bool ValuesWereAdded = ";
+      OS << "Opt.addValues(";
+      std::string S = (Pref + R.getValueAsString("Name")).str();
+      write_cstring(OS, S);
+      OS << ", Values);\n";
+      OS << "(void)ValuesWereAdded;\n";
+      OS << "assert(ValuesWereAdded && \"Couldn't add values to "
+            "OptTable!\");\n";
+    }
+    OS << "}\n";
+  }
+  OS << "\n";
+  OS << "#endif // OPTTABLE_ARG_INIT\n";
 }
 } // end namespace llvm