subject:"\[PATCH\] D142165\: \[clang\]\[deps\] Account for transitive spurious dependencies"

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

2023-01-24 Thread Jan Svoboda via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG96a54b2258cd: [clang][deps] Account for transitive spurious 
dependencies (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142165/new/

https://reviews.llvm.org/D142165

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-implementation-private.m

Index: clang/test/ClangScanDeps/modules-implementation-private.m
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-implementation-private.m
@@ -0,0 +1,70 @@
+// This test checks that we don't crash or report spurious dependencies on
+// FW_Private when compiling the implementation of framework module FW.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "file": "DIR/tu.m",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=FW -F DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "FW_Private.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+//--- frameworks/FW.framework/PrivateHeaders/FW_Private.h
+//--- frameworks/FW.framework/PrivateHeaders/Missed.h
+#import  // When included from tu.m, this ends up adding (spurious) dependency on FW for FW_Private.
+
+//--- tu.m
+@import FW_Private; // This is a direct dependency.
+#import 
+
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "commands": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-context-hash": "{{.*}}",
+// CHECK-NEXT:   "clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "module-name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "executable": "clang",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/tu.m",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/Missed.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "input-file": "[[PREFIX]]/tu.m"
+// CHECK-NEXT: }
+// CHECK:]
+// CHECK:  }
+// CHECK:]
+// CHECK:  }
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -377,15 +377,8 @@
 if (!MDC.isPrebuiltModule(M))
   DirectModularDeps.insert(M);
 
-  for (const Module *M : DirectModularDeps) {
-// A top-level module might not be actually imported as a module when
-// -fmodule-name is used to compile a translation unit that imports this
-// module. In that case it can be skipped. The appropriate header
-// dependencies will still be reported as expected.
-if (!M->getASTFile())
-  continue;
+  for (const Module *M : DirectModularDeps)
 handleTopLevelModule(M);
-  }
 
   MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
 
@@ -399,9 +392,17 @@
 MDC.Consumer.handlePrebuiltModuleDependency(I.second);
 }
 
-ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
+std::optional
+ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
   assert(M == M->getTopLevelModule() && "Expected top level module!");
 
+  // A top-level module might not be actually imported as a module when
+  // -fmodule-name is

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

2023-01-20 Thread Jan Svoboda via Phabricator via cfe-commits

jansvoboda11 updated this revision to Diff 490962.
jansvoboda11 added a comment.

Fix test on Windows


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142165/new/

https://reviews.llvm.org/D142165

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-implementation-private.m

Index: clang/test/ClangScanDeps/modules-implementation-private.m
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-implementation-private.m
@@ -0,0 +1,70 @@
+// This test checks that we don't crash or report spurious dependencies on
+// FW_Private when compiling the implementation of framework module FW.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "file": "DIR/tu.m",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=FW -F DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "FW_Private.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+//--- frameworks/FW.framework/PrivateHeaders/FW_Private.h
+//--- frameworks/FW.framework/PrivateHeaders/Missed.h
+#import  // When included from tu.m, this ends up adding (spurious) dependency on FW for FW_Private.
+
+//--- tu.m
+@import FW_Private; // This is a direct dependency.
+#import 
+
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "commands": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-context-hash": "{{.*}}",
+// CHECK-NEXT:   "clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "module-name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "executable": "clang",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/tu.m",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/Missed.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "input-file": "[[PREFIX]]/tu.m"
+// CHECK-NEXT: }
+// CHECK:]
+// CHECK:  }
+// CHECK:]
+// CHECK:  }
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -379,15 +379,8 @@
 if (!MDC.isPrebuiltModule(M))
   DirectModularDeps.insert(M);
 
-  for (const Module *M : DirectModularDeps) {
-// A top-level module might not be actually imported as a module when
-// -fmodule-name is used to compile a translation unit that imports this
-// module. In that case it can be skipped. The appropriate header
-// dependencies will still be reported as expected.
-if (!M->getASTFile())
-  continue;
+  for (const Module *M : DirectModularDeps)
 handleTopLevelModule(M);
-  }
 
   MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
 
@@ -401,9 +394,17 @@
 MDC.Consumer.handlePrebuiltModuleDependency(I.second);
 }
 
-ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
+std::optional
+ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
   assert(M == M->getTopLevelModule() && "Expected top level module!");
 
+  // A top-level module might not be actually imported as a module when
+  // -fmodule-name is used to compile a translation unit that imports this
+  // module. In that case it can be skipped. The appropriate header
+  // dependencies will

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

2023-01-19 Thread Jan Svoboda via Phabricator via cfe-commits

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, benlangmuir.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In D106100 , we started guarding against 
spurious dependencies on modules that ended up being textual includes and thus 
didn't have any AST file associated. That patch accounted only for direct 
dependencies. There's a way how to get spurious dependencies for modules that 
are transitive. This patch guards against that scenario and adds a test case.

rdar://104324602


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142165

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-implementation-private.c

Index: clang/test/ClangScanDeps/modules-implementation-private.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/modules-implementation-private.c
@@ -0,0 +1,70 @@
+// This test checks that we don't crash or report spurious dependencies on
+// FW_Private when compiling the implementation of framework module FW.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "file": "DIR/tu.c",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=FW -F DIR/frameworks -c DIR/tu.c -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "FW_Private.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+//--- frameworks/FW.framework/PrivateHeaders/FW_Private.h
+//--- frameworks/FW.framework/PrivateHeaders/Missed.h
+#import  // When included from tu.c, this ends up adding (spurious) dependency on FW for FW_Private.
+
+//--- tu.c
+@import FW_Private; // This is a direct dependency.
+#import 
+
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:  {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "commands": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-context-hash": "{{.*}}",
+// CHECK-NEXT:   "clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "module-name": "FW_Private"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "command-line": [
+// CHECK:],
+// CHECK-NEXT:   "executable": "clang",
+// CHECK-NEXT:   "file-deps": [
+// CHECK-NEXT: "[[PREFIX]]/tu.c",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/Missed.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "input-file": "[[PREFIX]]/tu.c"
+// CHECK-NEXT: }
+// CHECK:]
+// CHECK:  }
+// CHECK:]
+// CHECK:  }
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -379,15 +379,8 @@
 if (!MDC.isPrebuiltModule(M))
   DirectModularDeps.insert(M);
 
-  for (const Module *M : DirectModularDeps) {
-// A top-level module might not be actually imported as a module when
-// -fmodule-name is used to compile a translation unit that imports this
-// module. In that case it can be skipped. The appropriate header
-// dependencies will still be reported as expected.
-if (!M->getASTFile())
-  continue;
+  for (const Module *M : DirectModularDeps)
 handleTopLevelModule(M);
-  }
 
   MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
 
@@ -401,9 +394,17 @@

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

[PATCH] D142165: [clang][deps] Account for transitive spurious dependencies

3 matches

Site Navigation

Mail list logo

Footer information