[clang][Dependency Scanning] Canonicalize Defines of a Compiler Invocation As Early As Possible

[qiongsiwu]

Before this patch, we only perform -D canonicalization on the deep copy of the CompilerInvocation instance, since the canonicalization should have no impact on scanning.

However, in the presence of CAS, the content of the builtin macros are included in the context hash. This patch makes sure that we canonicalize the scanning CompilerInvocation's -Ds.

Part of work for rdar://136303612.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

Before this patch, we only perform -D canonicalization on the deep copy of the CompilerInvocation instance, since the canonicalization should have no impact on scanning.

However, in the presence of CAS, the content of the builtin macros are hashed and used as context hash. This patch moves the canonicalization to an earlier point so the canonicalized macros are used consistently for the command line argument for explicit PCM builds, and during scanning.

Part of work for rdar://136303612.

---

Full diff: https://github.com/llvm/llvm-project/pull/159620.diff

1 Files Affected:

• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+12-6)

diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 0855e6dec6158..8c4a3684a10a7 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -393,8 +393,6 @@ class DependencyScanningAction {
                      DiagnosticConsumer *DiagConsumer) {
     // Make a deep copy of the original Clang invocation.
     CompilerInvocation OriginalInvocation(*Invocation);
-    if (any(Service.getOptimizeArgs() & ScanningOptimizations::Macros))
-      canonicalizeDefines(OriginalInvocation.getPreprocessorOpts());
 
     if (Scanned) {
       // Scanning runs once for the first -cc1 invocation in a chain of driver
@@ -708,7 +706,8 @@ static bool createAndRunToolInvocation(
     std::vector<std::string> CommandLine, DependencyScanningAction &Action,
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
     std::shared_ptr<clang::PCHContainerOperations> &PCHContainerOps,
-    DiagnosticsEngine &Diags, DependencyConsumer &Consumer) {
+    DiagnosticsEngine &Diags, DependencyConsumer &Consumer,
+    bool CanonicalizeDefs) {
 
   // Save executable path before providing CommandLine to ToolInvocation
   std::string Executable = CommandLine[0];
@@ -723,6 +722,9 @@ static bool createAndRunToolInvocation(
     return false;
   }
 
+  if (CanonicalizeDefs)
+    canonicalizeDefines(Invocation->getPreprocessorOpts());
+
   if (!Action.runInvocation(std::move(Invocation), std::move(FS),
                             PCHContainerOps, Diags.getClient()))
     return false;
@@ -744,14 +746,17 @@ bool DependencyScanningWorker::scanDependencies(
   sanitizeDiagOpts(*DiagOpts);
   auto Diags = CompilerInstance::createDiagnostics(*FS, *DiagOpts, &DC,
                                                    /*ShouldOwnClient=*/false);
+  bool canonicalizeDefs =
+      any(Service.getOptimizeArgs() & ScanningOptimizations::Macros);
 
   DependencyScanningAction Action(Service, WorkingDirectory, Consumer,
                                   Controller, DepFS, ModuleName);
 
   bool Success = false;
   if (CommandLine[1] == "-cc1") {
-    Success = createAndRunToolInvocation(CommandLine, Action, FS,
-                                         PCHContainerOps, *Diags, Consumer);
+    Success =
+        createAndRunToolInvocation(CommandLine, Action, FS, PCHContainerOps,
+                                   *Diags, Consumer, canonicalizeDefs);
   } else {
     Success = forEachDriverJob(
         CommandLine, *Diags, FS, [&](const driver::Command &Cmd) {
@@ -774,7 +779,8 @@ bool DependencyScanningWorker::scanDependencies(
           // are made by the driver do not go through the
           // dependency scanning filesystem.
           return createAndRunToolInvocation(std::move(Argv), Action, FS,
-                                            PCHContainerOps, *Diags, Consumer);
+                                            PCHContainerOps, *Diags, Consumer,
+                                            canonicalizeDefs);
         });
   }
 

[qiongsiwu]

Note to reviewers:

I did not add any tests here since I don't think there are visible behavior change observable without CAS. I am testing this work using https://github.com/swiftlang/swift and its llvm fork. Please let me know if there are things I can test against and I will add a test.

Thanks!

[cachemeifyoucan]

Note to reviewers:

I did not add any tests here since I don't think there are visible behavior change observable without CAS. I am testing this work using https://github.com/swiftlang/swift and its llvm fork. Please let me know if there are things I can test against and I will add a test.

Thanks!

Do we have test downstream? Can you link a PR with test added?

[qiongsiwu]

Note to reviewers:
I did not add any tests here since I don't think there are visible behavior change observable without CAS. I am testing this work using https://github.com/swiftlang/swift and its llvm fork. Please let me know if there are things I can test against and I will add a test.
Thanks!

Do we have test downstream? Can you link a PR with test added?

Here's a downstream test swiftlang#11454.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jansvoboda11]

LGTM, thanks! FWIW I think this is easy to test even upstream by scanning two TUs with different defines that canonicalize to the same thing, and then checking the number of entries in the module cache. Might be worth doing here.

[qiongsiwu]

LGTM, thanks! FWIW I think this is easy to test even upstream by scanning two TUs with different defines that canonicalize to the same thing, and then checking the number of entries in the module cache. Might be worth doing here.

Ah great suggestion! Thanks! A test is now added here as well.