Skip to content

Commit

Permalink
[clang][modules][deps] System module maps might not be affecting
Browse files Browse the repository at this point in the history
The dependency scanner relies on the module map filtering logic in `ASTWriter`. The algorithm currently considers all system module maps affecting, which is not only sub-optimal, but can also cause failures when building a module explicitly (see attached test case).

This patch applies the same filtering logic to system module maps.

Reviewed By: Bigcheese

Differential Revision: https://reviews.llvm.org/D136007

(cherry picked from commit f33173a)
  • Loading branch information
jansvoboda11 committed Nov 5, 2022
1 parent 961f337 commit d759352
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 13 deletions.
2 changes: 1 addition & 1 deletion clang/lib/Serialization/ASTWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4548,7 +4548,7 @@ void ASTWriter::collectNonAffectingInputFiles() {
continue;

if (!isModuleMap(File.getFileCharacteristic()) ||
isSystem(File.getFileCharacteristic()) || AffectingModuleMaps.empty() ||
AffectingModuleMaps.empty() ||
AffectingModuleMaps.find(Cache->OrigEntry) != AffectingModuleMaps.end())
continue;

Expand Down
6 changes: 2 additions & 4 deletions clang/test/ClangScanDeps/modules-implicit-dot-private.m
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
[{
"file": "DIR/tu.m",
"directory": "DIR",
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -iframework DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -F DIR/frameworks -c DIR/tu.m -o DIR/tu.o"
}]
//--- tu.m
@import FW.Private;
Expand All @@ -33,8 +33,7 @@
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h",
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap"
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "name": "FW"
// CHECK: },
Expand All @@ -45,7 +44,6 @@
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK: "file-deps": [
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h"
// CHECK-NEXT: ],
Expand Down
12 changes: 4 additions & 8 deletions clang/test/ClangScanDeps/modules-incomplete-umbrella.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ framework module FW_Private {
[{
"file": "DIR/from_tu.m",
"directory": "DIR",
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -iframework DIR/frameworks -c DIR/from_tu.m -o DIR/from_tu.o"
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -F DIR/frameworks -c DIR/from_tu.m -o DIR/from_tu.o"
}]
//--- from_tu.m
#include "FW/FW.h"
Expand All @@ -45,8 +45,7 @@ framework module FW_Private {
// CHECK_TU-NEXT: "context-hash": "{{.*}}",
// CHECK_TU-NEXT: "file-deps": [
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h",
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap"
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap"
// CHECK_TU-NEXT: ],
// CHECK_TU-NEXT: "name": "FW"
// CHECK_TU-NEXT: },
Expand All @@ -57,7 +56,6 @@ framework module FW_Private {
// CHECK_TU: ],
// CHECK_TU-NEXT: "context-hash": "{{.*}}",
// CHECK_TU-NEXT: "file-deps": [
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h",
// CHECK_TU-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/One.h"
Expand Down Expand Up @@ -102,7 +100,7 @@ framework module FW_Private {
[{
"file": "DIR/from_module.m",
"directory": "DIR",
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -iframework DIR/frameworks -c DIR/from_module.m -o DIR/from_module.o"
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -F DIR/frameworks -c DIR/from_module.m -o DIR/from_module.o"
}]
//--- module.modulemap
module Mod { header "Mod.h" }
Expand All @@ -125,8 +123,7 @@ module Mod { header "Mod.h" }
// CHECK_MODULE-NEXT: "context-hash": "{{.*}}",
// CHECK_MODULE-NEXT: "file-deps": [
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Headers/FW.h",
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap"
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap"
// CHECK_MODULE-NEXT: ],
// CHECK_MODULE-NEXT: "name": "FW"
// CHECK_MODULE-NEXT: },
Expand All @@ -137,7 +134,6 @@ module Mod { header "Mod.h" }
// CHECK_MODULE: ],
// CHECK_MODULE-NEXT: "context-hash": "{{.*}}",
// CHECK_MODULE-NEXT: "file-deps": [
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.modulemap",
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/FW_Private.h",
// CHECK_MODULE-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/One.h"
Expand Down
60 changes: 60 additions & 0 deletions clang/test/ClangScanDeps/modules-redefinition.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// This test checks that we don't report non-affecting system module maps.
// More specifically, we check that explicitly-specified module map file is not
// being shadowed in explicit build by a module map file found during implicit
// module map search.

// RUN: rm -rf %t
// RUN: split-file %s %t

//--- tu.m
@import first;

//--- zeroth/module.modulemap
module X {}

//--- first/module.modulemap
module first { header "first.h" }
//--- first/first.h
@import third;
//--- second/module.modulemap
module X {}
//--- third/module.modulemap
module third {}

//--- cdb.json.template
[{
"file": "DIR/tu.m",
"directory": "DIR",
"command": "clang -fmodules -fmodules-cache-path=DIR/cache -fmodule-name=X -fmodule-map-file=DIR/zeroth/module.modulemap -isystem DIR/first -isystem DIR/second -isystem DIR/third -c DIR/tu.m -o DIR/tu.o"
}]

// 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 > %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: {
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "module-name": "third"
// CHECK-NEXT: }
// CHECK-NEXT: ],
// CHECK-NEXT: "clang-modulemap-file": "[[PREFIX]]/first/module.modulemap",
// CHECK-NEXT: "command-line": [
// CHECK: ],
// CHECK-NEXT: "context-hash": "{{.*}}",
// CHECK-NEXT: "file-deps": [
// CHECK-NEXT: [[PREFIX]]/first/first.h",
// CHECK-NEXT: [[PREFIX]]/first/module.modulemap",
// CHECK-NEXT: [[PREFIX]]/third/module.modulemap"
// CHECK-NEXT: ],
// CHECK-NEXT: "name": "first"
// CHECK-NEXT: }
// CHECK: ]
// CHECK: }

// RUN: %deps-to-rsp %t/result.json --module-name=third > %t/third.cc1.rsp
// RUN: %deps-to-rsp %t/result.json --module-name=first > %t/first.cc1.rsp
// RUN: %clang @%t/third.cc1.rsp
// RUN: %clang @%t/first.cc1.rsp

0 comments on commit d759352

Please sign in to comment.