[clang][deps] Ensure module invocation can be serialized

[benlangmuir]

Summary

When reseting modular options, propagate the values from certain options
that have ImpliedBy relations instead of setting to the default. Also,
verify in clang-scan-deps that the command line produced round trips
exactly.

Ideally we would automatically derive the set of options that need this
kind of propagation, but for now there aren't very many impacted.

[jansvoboda11]

Having this check is great! Left some nits in-line.

clang/lib/Frontend/CompilerInvocation.cpp

643–661

The logic is getting a bit complicated here. I think it'd be good to break down what the CheckAgainstOriginalInvocation flag does. Feel free to come up with your own description.

756–757

This name is now not accurate (along with the comment below). Can you make sure these still make sense with the new logic?

clang/tools/clang-scan-deps/ClangScanDeps.cpp

668

The call to getNumOccurrences() is a bit odd. Can you drop that if you add llvm::cl::init(false) to the option definition?

[benlangmuir]

• Improved doc comment for RoundTrip (mostly followed the suggested text; also converted from to /).
• Renamed variables
• Moved default value for RoundTripArgs

[benlangmuir]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

668

I did this so that you can do -round-trip-args=false even if the default is true. But I simplified it by doing lvm::cl::init(DoRoundTripDefault) instead.

[jansvoboda11]

LGTM, thanks!

clang/include/clang/Frontend/CompilerInvocation.h

void generateCC1CommandLine(llvm::SmallVectorImpl<const char *> &Args,
                               StringAllocator SA) const;
 
   /// Generate cc1-compatible command line arguments from this instance,
   /// wrapping the result as a std::vector<std::string>.
   ///
   /// This is a (less-efficient) wrapper over generateCC1CommandLine().
   std::vector<std::string> getCC1CommandLine() const;
 
+  /// Check that \p Args can be parsed and re-serialized without change,
+  /// emiting diagnostics for any differences.
+  ///
+  /// This check is only suitable for command-lines that are expected to already
+  /// be canonical.
+  ///
+  /// \return false if there are any errors.
+  static bool checkCC1RoundTrip(ArrayRef<const char *> Args,
+                                DiagnosticsEngine &Diags,
+                                const char *Argv0 = nullptr);
+
   /// Reset all of the options that are not considered when building a
   /// module.
   void resetNonModularOptions();
 
   /// Disable implicit modules and canonicalize options that are only used by
   /// implicit modules.
   void clearImplicitModuleBuildOptions();
 

clang/lib/Basic/LangOptions.cpp

 
 void LangOptions::resetNonModularOptions() {
 #define LANGOPT(Name, Bits, Default, Description)
 #define BENIGN_LANGOPT(Name, Bits, Default, Description) Name = Default;
 #define BENIGN_ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
   Name = static_cast<unsigned>(Default);
 #include "clang/Basic/LangOptions.def"
 
+  // Reset "benign" options with implied values (Options.td ImpliedBy relations)
+  // rather than their defaults. This avoids unexpected combinations and
+  // invocations that cannot be round-tripped to arguments.
+  // FIXME: we should derive this automatically from ImpliedBy in tablegen.
+  AllowFPReassoc = UnsafeFPMath;
+  NoHonorNaNs = FiniteMathOnly;
+  NoHonorInfs = FiniteMathOnly;
+
   // These options do not affect AST generation.
   NoSanitizeFiles.clear();
   XRayAlwaysInstrumentFiles.clear();
   XRayNeverInstrumentFiles.clear();
 
   CurrentModule.clear();
   IsHeaderFile = false;
 }

clang/lib/Frontend/CompilerInvocation.cpp

 using ParseFn =
     llvm::function_ref<bool(CompilerInvocation &, ArrayRef<const char *>,
                             DiagnosticsEngine &, const char *)>;
 
 // Generate command line arguments from CompilerInvocation.
 using GenerateFn = llvm::function_ref<void(
     CompilerInvocation &, SmallVectorImpl<const char *> &,
     CompilerInvocation::StringAllocator)>;
 
-// May perform round-trip of command line arguments. By default, the round-trip
-// is enabled in assert builds. This can be overwritten at run-time via the
-// "-round-trip-args" and "-no-round-trip-args" command line flags.
-// During round-trip, the command line arguments are parsed into a dummy
-// instance of CompilerInvocation which is used to generate the command line
-// arguments again. The real CompilerInvocation instance is then created by
-// parsing the generated arguments, not the original ones.
+/// May perform round-trip of command line arguments. By default, the round-trip
+/// is enabled in assert builds. This can be overwritten at run-time via the
+/// "-round-trip-args" and "-no-round-trip-args" command line flags, or via the
+/// ForceRoundTrip parameter.
+///
+/// During round-trip, the command line arguments are parsed into a dummy
+/// CompilerInvocation, which is used to generate the command line arguments
+/// again. The real CompilerInvocation is then created by parsing the generated
+/// arguments, not the original ones. This (in combination with tests covering
+/// argument behavior) ensures the generated command line is complete (doesn't
+/// drop/mangle any arguments).
+///
+/// Finally, we check the command line that was used to create the real
+/// CompilerInvocation instance. By default, we compare it to the command line
+/// the real CompilerInvocation generates. This checks whether the generator is
+/// deterministic. If \p CheckAgainstOriginalInvocation is enabled, we instead
+/// compare it to the original command line to verify the original command-line
+/// was canonical and can round-trip exactly.
 static bool RoundTrip(ParseFn Parse, GenerateFn Generate,
                       CompilerInvocation &RealInvocation,
                       CompilerInvocation &DummyInvocation,
                       ArrayRef<const char *> CommandLineArgs,
-                      DiagnosticsEngine &Diags, const char *Argv0) {
+                      DiagnosticsEngine &Diags, const char *Argv0,
+                      bool CheckAgainstOriginalInvocation = false,
+                      bool ForceRoundTrip = false) {
 #ifndef NDEBUG
   bool DoRoundTripDefault = true;
 #else
   bool DoRoundTripDefault = false;
 #endif
 
   bool DoRoundTrip = DoRoundTripDefault;
+  if (ForceRoundTrip) {
+    DoRoundTrip = true;
+  } else {
     for (const auto *Arg : CommandLineArgs) {
       if (Arg == StringRef("-round-trip-args"))
         DoRoundTrip = true;
       if (Arg == StringRef("-no-round-trip-args"))
         DoRoundTrip = false;
     }
+  }
 
   // If round-trip was not requested, simply run the parser with the real
   // invocation diagnostics.
   if (!DoRoundTrip)
     return Parse(RealInvocation, CommandLineArgs, Diags, Argv0);
 
   // Serializes quoted (and potentially escaped) arguments.
   auto SerializeArgs = [](ArrayRef<const char *> Args) {
#endif
   llvm::StringSaver StringPool(Alloc);
   auto SA = [&StringPool](const Twine &Arg) {
     return StringPool.save(Arg).data();
   };
 
   // Generate arguments from the dummy invocation. If Generate is the
   // inverse of Parse, the newly generated arguments must have the same
   // semantics as the original.
-  SmallVector<const char *> GeneratedArgs1;
-  Generate(DummyInvocation, GeneratedArgs1, SA);
+  SmallVector<const char *> GeneratedArgs;
+  Generate(DummyInvocation, GeneratedArgs, SA);
 
   // Run the second parse, now on the generated arguments, and with the real
   // invocation and diagnostics. The result is what we will end up using for the
   // rest of compilation, so if Generate is not inverse of Parse, something down
   // the line will break.
-  bool Success2 = Parse(RealInvocation, GeneratedArgs1, Diags, Argv0);
+  bool Success2 = Parse(RealInvocation, GeneratedArgs, Diags, Argv0);
 
   // The first parse on original arguments succeeded, but second parse of
   // generated arguments failed. Something must be wrong with the generator.
   if (!Success2) {
     Diags.Report(diag::err_cc1_round_trip_ok_then_fail);
     Diags.Report(diag::note_cc1_round_trip_generated)
-        << 1 << SerializeArgs(GeneratedArgs1);
+        << 1 << SerializeArgs(GeneratedArgs);
     return false;
   }
 
+  SmallVector<const char *> ComparisonArgs;
+  if (CheckAgainstOriginalInvocation)
+    // Compare against original arguments.
+    ComparisonArgs.assign(CommandLineArgs.begin(), CommandLineArgs.end());
+  else
     // Generate arguments again, this time from the options we will end up using
     // for the rest of the compilation.
-  SmallVector<const char *> GeneratedArgs2;
-  Generate(RealInvocation, GeneratedArgs2, SA);
+    Generate(RealInvocation, ComparisonArgs, SA);
 
-  // Compares two lists of generated arguments.
+  // Compares two lists of arguments.
   auto Equal = [](const ArrayRef<const char *> A,
                   const ArrayRef<const char *> B) {
     return std::equal(A.begin(), A.end(), B.begin(), B.end(),
                       [](const char *AElem, const char *BElem) {
                         return StringRef(AElem) == StringRef(BElem);
                       });
   };
 
   // If we generated different arguments from what we assume are two
   // semantically equivalent CompilerInvocations, the Generate function may
   // be non-deterministic.
-  if (!Equal(GeneratedArgs1, GeneratedArgs2)) {
+  if (!Equal(GeneratedArgs, ComparisonArgs)) {
     Diags.Report(diag::err_cc1_round_trip_mismatch);
     Diags.Report(diag::note_cc1_round_trip_generated)
-        << 1 << SerializeArgs(GeneratedArgs1);
+        << 1 << SerializeArgs(GeneratedArgs);
     Diags.Report(diag::note_cc1_round_trip_generated)
-        << 2 << SerializeArgs(GeneratedArgs2);
+        << 2 << SerializeArgs(ComparisonArgs);
     return false;
   }
 
   Diags.Report(diag::remark_cc1_round_trip_generated)
-      << 1 << SerializeArgs(GeneratedArgs1);
+      << 1 << SerializeArgs(GeneratedArgs);
   Diags.Report(diag::remark_cc1_round_trip_generated)
-      << 2 << SerializeArgs(GeneratedArgs2);
+      << 2 << SerializeArgs(ComparisonArgs);
 
   return Success2;
 }
 
+bool CompilerInvocation::checkCC1RoundTrip(ArrayRef<const char *> Args,
+                                           DiagnosticsEngine &Diags,
+                                           const char *Argv0) {
+  CompilerInvocation DummyInvocation1, DummyInvocation2;
+  return RoundTrip(
+      [](CompilerInvocation &Invocation, ArrayRef<const char *> CommandLineArgs,
+         DiagnosticsEngine &Diags, const char *Argv0) {
+        return CreateFromArgsImpl(Invocation, CommandLineArgs, Diags, Argv0);
+      },
+      [](CompilerInvocation &Invocation, SmallVectorImpl<const char *> &Args,
+         StringAllocator SA) {
+        Args.push_back("-cc1");
+        Invocation.generateCC1CommandLine(Args, SA);
+      },
+      DummyInvocation1, DummyInvocation2, Args, Diags, Argv0,
+      /*CheckAgainstOriginalInvocation=*/true, /*ForceRoundTrip=*/true);
+}
+
 static void addDiagnosticArgs(ArgList &Args, OptSpecifier Group,
                               OptSpecifier GroupWithValue,
                               std::vector<std::string> &Diagnostics) {
   for (auto *A : Args.filtered(Group)) {
     if (A->getOption().getKind() == Option::FlagClass) {
       // The argument is a pure flag (such as OPT_Wall or OPT_Wdeprecated). Add
       // its name (minus the "W" or "R" at the beginning) to the diagnostics.
       Diagnostics.push_back(

clang/test/ClangScanDeps/modules-implied-args.c

+// Check that we get canonical round-trippable command-lines, in particular
+// for the options modified for modules.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full -round-trip-args > %t/result.json
+// RUN: cat %t/result.json  | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefixes=CHECK,NEGATIVE
+
+// -ffast-math implies -menable-no-infs, -menable-no-nans, and -mreassociate;
+// those options are modified by resetNonModularOptions.
+
+// NEGATIVE-NOT: "-menable-no-infs"
+// NEGATIVE-NOT: "-menable-no-nans"
+// NEGATIVE-NOT: "-mreassociate"
+
+// CHECK:      "modules": [
+// CHECK-NEXT:   {
+// CHECK:          "clang-module-deps": []
+// CHECK:          "command-line": [
+// CHECK:            "-ffast-math"
+// CHECK:          ]
+// CHECK:          "name": "Mod"
+// CHECK:        }
+// CHECK-NEXT: ]
+// CHECK:      "translation-units": [
+// CHECK-NEXT:   {
+// CHECK-NEXT:     "commands": [
+// CHECK:            {
+// CHECK:              "command-line": [
+// CHECK:                "-ffast-math"
+// CHECK:              ]
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang -fsyntax-only DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -ffast-math"
+}]
+
+//--- module.modulemap
+module Mod { header "mod.h" }
+//--- mod.h
+//--- tu.c
+#include "mod.h"

clang/tools/clang-scan-deps/ClangScanDeps.cpp

 //===- ClangScanDeps.cpp - Implementation of clang-scan-deps --------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Driver/Driver.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
llvm::cl::values(
                    "the compilation database. This assumes it's part of the "
                    "same toolchain as this clang-scan-deps. (default)"),
         clEnumValN(RDRK_InvokeCompiler, "invoke-compiler",
                    "Invoke the compiler with '-print-resource-dir' and use the "
                    "reported path as the resource directory. (deprecated)")),
     llvm::cl::init(RDRK_ModifyCompilerPath),
     llvm::cl::cat(DependencyScannerCategory));
 
+#ifndef NDEBUG
+static constexpr bool DoRoundTripDefault = true;
+#else
+static constexpr bool DoRoundTripDefault = false;
+#endif
+
+llvm::cl::opt<bool>
+    RoundTripArgs("round-trip-args", llvm::cl::Optional,
+                  llvm::cl::desc("verify that command-line arguments are "
+                                 "canonical by parsing and re-serializing"),
+                  llvm::cl::init(DoRoundTripDefault),
+                  llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt<bool> Verbose("v", llvm::cl::Optional,
                             llvm::cl::desc("Use verbose output."),
                             llvm::cl::init(false),
                             llvm::cl::cat(DependencyScannerCategory));
 
 } // end anonymous namespace
 
 /// Takes the result of a dependency scan and prints error / dependency files
for (const ModuleDeps &MD : Graph) {
       if (I != Modules.end()) {
         I->first.InputIndex = std::min(I->first.InputIndex, InputIndex);
         continue;
       }
       Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
     }
   }
 
+  bool roundTripCommand(ArrayRef<std::string> ArgStrs,
+                        DiagnosticsEngine &Diags) {
+    if (ArgStrs.empty() || ArgStrs[0] != "-cc1")
+      return false;
+    SmallVector<const char *> Args;
+    for (const std::string &Arg : ArgStrs)
+      Args.push_back(Arg.c_str());
+    return !CompilerInvocation::checkCC1RoundTrip(Args, Diags);
+  }
+
+  // Returns \c true if any command lines fail to round-trip. We expect
+  // commands already be canonical when output by the scanner.
+  bool roundTripCommands(raw_ostream &ErrOS) {
+    IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions{};
+    TextDiagnosticPrinter DiagConsumer(ErrOS, &*DiagOpts);
+    IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+        CompilerInstance::createDiagnostics(&*DiagOpts, &DiagConsumer,
+                                            /*ShouldOwnClient=*/false);
+
+    for (auto &&M : Modules)
+      if (roundTripCommand(M.second.BuildArguments, *Diags))
+        return true;
+
+    for (auto &&I : Inputs)
+      for (const auto &Cmd : I.Commands)
+        if (roundTripCommand(Cmd.Arguments, *Diags))
+          return true;
+
+    return false;
+  }
+
   void printFullOutput(raw_ostream &OS) {
     // Sort the modules by name to get a deterministic order.
     std::vector<IndexedModuleID> ModuleIDs;
     for (auto &&M : Modules)
       ModuleIDs.push_back(M.first);
     llvm::sort(ModuleIDs,
                [](const IndexedModuleID &A, const IndexedModuleID &B) {
                  return std::tie(A.ID.ModuleName, A.InputIndex) <
Pool.async([I, &Lock, &Index, &Inputs, &HadErrors, &FD, &WorkerTools,
                                           DependencyOS, Errs))
             HadErrors = true;
         }
       }
     });
   }
   Pool.wait();
 
+  if (RoundTripArgs)
+    if (FD.roundTripCommands(llvm::errs()))
+      HadErrors = true;
+
   if (Format == ScanningOutputFormat::Full)
     FD.printFullOutput(llvm::outs());
 
   return HadErrors;
 }