[WIP][clang][cli] Test manual header search argument generation with round-trip

[jansvoboda11]

Summary

This patch tests the manual header search argument generation by performing a "parse, generate, parse" round-trip on each compiler invocation. This way, the manually written C++ code gets exercised by the suite of Clang's end-to-end tests. Moreover, this ensures people adding new command line options are forced to implement the generation as well, either via the TableGen marshalling infrastructure or manually in CompilerInvocation.

It makes sense to enable this only for assert builds, so that we don't perform useless work in release builds.

Depending on our roll-out strategy, we might create more patches like this for other groups of options and upstream the work one-by-one, or put all option groups into this patch and enable round-tripping for all of -cc1 in a single commit.

Please note this is a work-in-progress. Ideally, we'll be able to get rid of the duplicate methods (parseSimpleArgs, generateCC1CommandLine, AddAllArgsExcept) and the HeaderSearchOptSpecs array.

Depends on D94472.

[jansvoboda11]

Marking this as "Changes Planned" to not occupy reviewers' Ready to Review queue.

[dexonsmith]

clang/include/clang/Frontend/CompilerInvocation.h

199–203

You can avoid the duplication at least for the WIP with:

void generateCC1CommandLine(llvm::SmallVectorImpl<const char *> &Args,
                              StringAllocator SA,
                              ArrayRef<llvm::opt::OptSpecifier> Included = None) const;

and then in the body:

auto IsIncluded = Included.empty()
    ? [](OptSpecifier) { return true; }
    : [&Included](OptSpecifier Opt) { return Included.count(Opt); };

// ...
if (... && IsIncluded(Opt)) {

[dexonsmith]

I'm wondering if we can get this in incrementally without needing to list in code which options are correctly handled. Here's one way to do it:

• Add a tool, clang-cc1-args, that shows the diff between round-tripping when given a set of args. (This could potentially grow other functionality vs. diffing. So maybe clang-cc1-args diff [args] would be the usage?)
• Add RUN lines to tests that test the args we think should round-trip correctly and confirm there's no diff. This can be done incrementally as options get handled.
• Only once we think we have all the options covered, add the assertion from this patch.

[jansvoboda11]

Hmm, that would still require us to keep a list of options we've converted. It's better than hard-coding it like this patch does, but I'd like it to be more automatic.

I've updated D94472 with my initial implementation of more hands-off solution. The gist is that the original ArgList instance constructs set of options the CompilerInvocation queries during parsing. This will tell us which options we should be able to generate and which we shouldn't need to copy from the original argument list to the new one via AddAllArgsXxx.

Abandoning this patch as per your suggestion to merge round-tripping with the actual command line generation.

clang/include/clang/Frontend/CompilerInvocation.h

 //===- CompilerInvocation.h - Compiler Invocation Helper Data ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_FRONTEND_COMPILERINVOCATION_H
 #define LLVM_CLANG_FRONTEND_COMPILERINVOCATION_H
 
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/LangStandard.h"
 #include "clang/Frontend/DependencyOutputOptions.h"
 #include "clang/Frontend/FrontendOptions.h"
 #include "clang/Frontend/MigratorOptions.h"
 #include "clang/Frontend/PreprocessorOutputOptions.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Option/OptSpecifier.h"
 #include <memory>
 #include <string>
 
 namespace llvm {
 
 class Triple;
 
 namespace opt {
public:
   /// Generate a cc1-compatible command line arguments from this instance.
   ///
   /// \param [out] Args - The generated arguments. Note that the caller is
   /// responsible for inserting the path to the clang executable and "-cc1" if
   /// desired.
   /// \param SA - A function that given a Twine can allocate storage for a given
   /// command line argument and return a pointer to the newly allocated string.
   /// The returned pointer is what gets appended to Args.
   void generateCC1CommandLine(llvm::SmallVectorImpl<const char *> &Args,
                               StringAllocator SA) const;
+  void generateCC1CommandLine(llvm::SmallVectorImpl<const char *> &Args,
+                              StringAllocator SA,
+                              ArrayRef<llvm::opt::OptSpecifier> Included) const;
 
   /// @}
   /// @name Option Subgroups
   /// @{
 
   AnalyzerOptionsRef getAnalyzerOpts() const { return AnalyzerOpts; }
 
   MigratorOptions &getMigratorOpts() { return MigratorOpts; }
private:
   /// Parse options for flags that expose marshalling information in their
   /// table-gen definition
   ///
   /// \param Args - The argument list containing the arguments to parse
   /// \param Diags - The DiagnosticsEngine associated with CreateFromArgs
   /// \returns - True if parsing was successful, false otherwise
   bool parseSimpleArgs(const llvm::opt::ArgList &Args,
                        DiagnosticsEngine &Diags);
+  bool parseSimpleArgs(const llvm::opt::ArgList &Args, DiagnosticsEngine &Diags,
+                       ArrayRef<llvm::opt::OptSpecifier> Included);
 };
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 createVFSFromCompilerInvocation(const CompilerInvocation &CI,
                                 DiagnosticsEngine &Diags);
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem> createVFSFromCompilerInvocation(
     const CompilerInvocation &CI, DiagnosticsEngine &Diags,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_COMPILERINVOCATION_H

clang/lib/Frontend/CompilerInvocation.cpp

 //===- CompilerInvocation.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
PARSE_OPTION_WITH_MARSHALLING(Args, Diags, Success, ID, FLAGS, PARAM,        \
                                 IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER,      \
                                 MERGER, TABLE_INDEX)
 #include "clang/Driver/Options.inc"
 #undef OPTION_WITH_MARSHALLING
 
   return Success;
 }
 
+bool CompilerInvocation::parseSimpleArgs(
+    const ArgList &Args, DiagnosticsEngine &Diags,
+    ArrayRef<llvm::opt::OptSpecifier> Included) {
+  bool Success = true;
+
+#define OPTION_WITH_MARSHALLING(                                               \
+    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
+    HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
+    DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
+    MERGER, EXTRACTOR, TABLE_INDEX)                                            \
+  if ((FLAGS)&options::CC1Option &&                                            \
+      find(Included, OPT_##ID) != Included.end()) {                            \
+    this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE);                      \
+    if (IMPLIED_CHECK)                                                         \
+      this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE);                    \
+    if (SHOULD_PARSE)                                                          \
+      if (auto MaybeValue =                                                    \
+              NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags, Success))         \
+        this->KEYPATH = MERGER(                                                \
+            this->KEYPATH, static_cast<decltype(this->KEYPATH)>(*MaybeValue)); \
+  }
+#include "clang/Driver/Options.inc"
+#undef OPTION_WITH_MARSHALLING
+
+  return Success;
+}
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions &Opts, ArgList &Args,
                                 DiagnosticsEngine *Diags,
                                 bool DefaultDiagColor) {
   Optional<DiagnosticsEngine> IgnoringDiags;
   if (!Diags) {
     IgnoringDiags.emplace(new DiagnosticIDs(), new DiagnosticOptions(),
                           new IgnoringDiagConsumer());
     Diags = &*IgnoringDiags;
Success &= ParseDiagnosticArgs(Res.getDiagnosticOpts(), Args, &Diags,
                                  /*DefaultDiagColor=*/false);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
                                       LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
                               Res.getTargetOpts(), Res.getFrontendOpts());
+
+  // Specify all header search options.
+  OptSpecifier HeaderSearchOptSpecs[] = {
+      // Marshalled via TableGen.
+      OPT_fmodules_user_build_path,
+      OPT_fprebuilt_implicit_modules,
+      OPT_fno_prebuilt_implicit_modules,
+      OPT_fmodules_prune_interval,
+      OPT_fmodules_prune_after,
+      OPT_fbuild_session_timestamp,
+      OPT_fmodules_validate_once_per_build_session,
+      OPT_fmodules_disable_diagnostic_validation,
+      OPT_fmodules_validate_system_headers,
+      OPT_fno_modules_validate_system_headers,
+      OPT_fvalidate_ast_input_files_content,
+      OPT_fimplicit_module_maps,
+      OPT_isysroot,
+      OPT_nobuiltininc,
+      OPT_nostdincxx,
+      OPT_resource_dir,
+      OPT_v,
+      OPT_fmodule_map_file_home_is_cwd,
+      OPT_fmodule_format_EQ,
+      OPT_nostdsysteminc,
+      OPT_fdisable_module_hash,
+      OPT_fmodules_hash_content,
+      OPT_fmodules_strict_context_hash,
+
+      // Marshalled manually.
+      OPT_stdlib_EQ,
+      OPT_fmodules_cache_path,
+      OPT_fmodule_file,
+      OPT_fprebuilt_module_path,
+      OPT_fmodules_ignore_macro,
+      OPT__sysroot_EQ,
+      OPT_I,
+      OPT_F,
+      OPT_index_header_map,
+      OPT_iprefix,
+      OPT_iwithprefix,
+      OPT_iwithprefixbefore,
+      OPT_idirafter,
+      OPT_iquote,
+      OPT_isystem,
+      OPT_iwithsysroot,
+      OPT_iframework,
+      OPT_iframeworkwithsysroot,
+      OPT_c_isystem,
+      OPT_cxx_isystem,
+      OPT_objc_isystem,
+      OPT_objcxx_isystem,
+      OPT_internal_isystem,
+      OPT_internal_externc_isystem,
+      OPT_system_header_prefix,
+      OPT_no_system_header_prefix,
+      OPT_ivfsoverlay,
+  };
+
+  // Prepare string allocator.
+  SmallVector<std::string, 32> GeneratedArgsStorage;
+  auto SA = [&GeneratedArgsStorage](const Twine &Arg) -> const char * {
+    return GeneratedArgsStorage.emplace_back(Arg.str()).c_str();
+  };
+
+  // Parse header search options from original arguments.
   ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args,
                         Res.getFileSystemOpts().WorkingDir);
+
+  // Create arg string without header search options.
+  ArgStringList MarshalledArgStr;
+  Args.AddAllArgsExcept(MarshalledArgStr, HeaderSearchOptSpecs);
+  // Fill arg string with header search options.
+  Res.generateCC1CommandLine(MarshalledArgStr, SA, HeaderSearchOptSpecs);
+  GenerateHeaderSearchArgs(Res.getHeaderSearchOpts(), MarshalledArgStr, SA);
+
+  InputArgList MarshalledArgs = Opts.ParseArgs(
+      MarshalledArgStr, MissingArgIndex, MissingArgCount, IncludedFlagsBitmask);
+
+  // Empty the header search options parsed from original arguments.
+  auto OriginalHeaderSearchOpts = std::make_shared<HeaderSearchOptions>();
+  Res.HeaderSearchOpts.swap(OriginalHeaderSearchOpts);
+  // Parse header search options from generated arguments.
+  Success &= Res.parseSimpleArgs(MarshalledArgs, Diags, HeaderSearchOptSpecs);
+  ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), MarshalledArgs,
+                        Res.getFileSystemOpts().WorkingDir);
+
   llvm::Triple T(Res.getTargetOpts().Triple);
   if (DashX.getFormat() == InputKind::Precompiled ||
       DashX.getLanguage() == Language::LLVM_IR) {
     // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
     // PassManager in BackendUtil.cpp. They need to be initializd no matter
     // what the input type is.
     if (Args.hasArg(OPT_fobjc_arc))
       LangOpts.ObjCAutoRefCount = 1;

 
 #undef DIAG_OPTION_WITH_MARSHALLING
 #undef OPTION_WITH_MARSHALLING
 #undef GENERATE_OPTION_WITH_MARSHALLING
 
   GenerateHeaderSearchArgs(getHeaderSearchOpts(), Args, SA);
 }
 
+void CompilerInvocation::generateCC1CommandLine(
+    SmallVectorImpl<const char *> &Args, StringAllocator SA,
+    ArrayRef<OptSpecifier> Included) const {
+  // Capture the extracted value as a lambda argument to avoid potential issues
+  // with lifetime extension of the reference.
+#define OPTION_WITH_MARSHALLING(                                               \
+    PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,        \
+    HELPTEXT, METAVAR, VALUES, SPELLING, SHOULD_PARSE, ALWAYS_EMIT, KEYPATH,   \
+    DEFAULT_VALUE, IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER,     \
+    MERGER, EXTRACTOR, TABLE_INDEX)                                            \
+  if ((FLAGS)&options::CC1Option &&                                            \
+      find(Included, OPT_##ID) != Included.end()) {                            \
+    [&](const auto &Extracted) {                                               \
+      if (ALWAYS_EMIT ||                                                       \
+          (Extracted !=                                                        \
+           static_cast<decltype(this->KEYPATH)>(                               \
+               (IMPLIED_CHECK) ? (IMPLIED_VALUE) : (DEFAULT_VALUE))))          \
+        DENORMALIZER(Args, SPELLING, SA, Option::KIND##Class, TABLE_INDEX,     \
+                     Extracted);                                               \
+    }(EXTRACTOR(this->KEYPATH));                                               \
+  }
+
+#include "clang/Driver/Options.inc"
+#undef OPTION_WITH_MARSHALLING
+}
+
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 clang::createVFSFromCompilerInvocation(const CompilerInvocation &CI,
                                        DiagnosticsEngine &Diags) {
   return createVFSFromCompilerInvocation(CI, Diags,
                                          llvm::vfs::getRealFileSystem());
 }
 
 IntrusiveRefCntPtr<llvm::vfs::FileSystem>

llvm/include/llvm/Option/ArgList.h

 //===- ArgList.h - Argument List Management ---------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
public:
 
   /// Render only the last argument match \p Id0, if present.
   template<typename ...OptSpecifiers>
   void AddLastArg(ArgStringList &Output, OptSpecifiers ...Ids) const {
     if (Arg *A = getLastArg(Ids...)) // Calls claim() on all Ids's Args.
       A->render(*this, Output);
   }
 
+  /// AddAllArgsExcept - Render all arguments not matching any of the excluded
+  /// ids.
+  void AddAllArgsExcept(ArgStringList &Output,
+                        ArrayRef<OptSpecifier> ExcludeIds = {}) const;
   /// AddAllArgsExcept - Render all arguments matching any of the given ids
   /// and not matching any of the excluded ids.
   void AddAllArgsExcept(ArgStringList &Output, ArrayRef<OptSpecifier> Ids,
                         ArrayRef<OptSpecifier> ExcludeIds) const;
   /// AddAllArgs - Render all arguments matching any of the given ids.
   void AddAllArgs(ArgStringList &Output, ArrayRef<OptSpecifier> Ids) const;
 
   /// AddAllArgs - Render all arguments matching the given ids.

llvm/lib/Option/ArgList.cpp

 //===- ArgList.cpp - Argument List Management -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 }
 
 std::vector<std::string> ArgList::getAllArgValues(OptSpecifier Id) const {
   SmallVector<const char *, 16> Values;
   AddAllArgValues(Values, Id);
   return std::vector<std::string>(Values.begin(), Values.end());
 }
 
 void ArgList::AddAllArgsExcept(ArgStringList &Output,
+                      ArrayRef<OptSpecifier> ExcludeIds) const {
+  for (const Arg *Arg : *this) {
+    bool Excluded = false;
+    for (OptSpecifier Id : ExcludeIds) {
+      if (Arg->getOption().matches(Id)) {
+        Excluded = true;
+        break;
+      }
+    }
+    if (!Excluded) {
+      Arg->claim();
+      Arg->render(*this, Output);
+    }
+  }
+}
+
+void ArgList::AddAllArgsExcept(ArgStringList &Output,
                                ArrayRef<OptSpecifier> Ids,
                                ArrayRef<OptSpecifier> ExcludeIds) const {
   for (const Arg *Arg : *this) {
     bool Excluded = false;
     for (OptSpecifier Id : ExcludeIds) {
       if (Arg->getOption().matches(Id)) {
         Excluded = true;
         break;