[clang][modules] Correctly set module map systemness

[Bigcheese]

This uses the systemness of the module map instead of of the Module instance, as doing otherwise could incorrectly parse the other modules in that module map as system.

This is still correct as the only ways to get a system module are by the module map being in a system path, or the module having the [system] attribute, both of which are handled here.

This makes it so that the systemness of a module is deterministic instead of depending on the path taken to build it.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Michael Spencer (Bigcheese)

Changes

This uses the systemness of the module map instead of of the Module instance, as doing otherwise could incorrectly parse the other modules in that module map as system.

This is still correct as the only ways to get a system module are by the module map being in a system path, or the module having the [system] attribute, both of which are handled here.

This makes it so that the systemness of a module is deterministic instead of depending on the path taken to build it.

---

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

2 Files Affected:

• (modified) clang/lib/Frontend/CompilerInstance.cpp (+8-1)
• (added) clang/test/Modules/transitive-system.test (+27)

diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 5709e7afa3658..02994bce8b335 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1360,10 +1360,17 @@ static bool compileModule(CompilerInstance &ImportingInstance,
 
     StringRef ModuleMapFilePath = ModuleMapFile->getNameAsRequested();
 
+    // Use the systemness of the module map as parsed instead of using the
+    // IsSystem attribute of the module. If the module has [system] but the
+    // module map is not in a system path, then this would incorrectly parse
+    // any other modules in that module map as system too.
+    const SrcMgr::SLocEntry &SLoc = SourceMgr.getSLocEntry(ModuleMapFID);
+    bool IsSystem = isSystem(SLoc.getFile().getFileCharacteristic());
+
     // Use the module map where this module resides.
     Result = compileModuleImpl(
         ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
-        FrontendInputFile(ModuleMapFilePath, IK, +Module->IsSystem),
+        FrontendInputFile(ModuleMapFilePath, IK, IsSystem),
         ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
   } else {
     // FIXME: We only need to fake up an input file here as a way of
diff --git a/clang/test/Modules/transitive-system.test b/clang/test/Modules/transitive-system.test
new file mode 100644
index 0000000000000..b1f1558b31742
--- /dev/null
+++ b/clang/test/Modules/transitive-system.test
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// 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 -module-name=direct > %t/result1.json
+// RUN: rm -rf %t/cache
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full -module-name=transitive > %t/result2.json
+// RUN: %deps-to-rsp %t/result1.json --module-name transitive > %t/1.rsp
+// RUN: %deps-to-rsp %t/result2.json --module-name transitive > %t/2.rsp
+// RUN: diff %t/1.rsp %t/2.rsp
+
+//--- module.modulemap
+module direct [system] { header "direct.h" }
+module transitive { header "transitive.h" }
+
+//--- direct.h
+#include "transitive.h"
+
+//--- transitive.h
+// empty
+
+//--- cdb.json.template
+[{
+  "file": "",
+  "directory": "DIR",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR -x c"
+}]

[llvmbot]

@llvm/pr-subscribers-clang-modules

Author: Michael Spencer (Bigcheese)

Changes

This uses the systemness of the module map instead of of the Module instance, as doing otherwise could incorrectly parse the other modules in that module map as system.

This is still correct as the only ways to get a system module are by the module map being in a system path, or the module having the [system] attribute, both of which are handled here.

This makes it so that the systemness of a module is deterministic instead of depending on the path taken to build it.

---

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

2 Files Affected:

• (modified) clang/lib/Frontend/CompilerInstance.cpp (+8-1)
• (added) clang/test/Modules/transitive-system.test (+27)

diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 5709e7afa3658..02994bce8b335 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1360,10 +1360,17 @@ static bool compileModule(CompilerInstance &ImportingInstance,
 
     StringRef ModuleMapFilePath = ModuleMapFile->getNameAsRequested();
 
+    // Use the systemness of the module map as parsed instead of using the
+    // IsSystem attribute of the module. If the module has [system] but the
+    // module map is not in a system path, then this would incorrectly parse
+    // any other modules in that module map as system too.
+    const SrcMgr::SLocEntry &SLoc = SourceMgr.getSLocEntry(ModuleMapFID);
+    bool IsSystem = isSystem(SLoc.getFile().getFileCharacteristic());
+
     // Use the module map where this module resides.
     Result = compileModuleImpl(
         ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
-        FrontendInputFile(ModuleMapFilePath, IK, +Module->IsSystem),
+        FrontendInputFile(ModuleMapFilePath, IK, IsSystem),
         ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
   } else {
     // FIXME: We only need to fake up an input file here as a way of
diff --git a/clang/test/Modules/transitive-system.test b/clang/test/Modules/transitive-system.test
new file mode 100644
index 0000000000000..b1f1558b31742
--- /dev/null
+++ b/clang/test/Modules/transitive-system.test
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// 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 -module-name=direct > %t/result1.json
+// RUN: rm -rf %t/cache
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full -module-name=transitive > %t/result2.json
+// RUN: %deps-to-rsp %t/result1.json --module-name transitive > %t/1.rsp
+// RUN: %deps-to-rsp %t/result2.json --module-name transitive > %t/2.rsp
+// RUN: diff %t/1.rsp %t/2.rsp
+
+//--- module.modulemap
+module direct [system] { header "direct.h" }
+module transitive { header "transitive.h" }
+
+//--- direct.h
+#include "transitive.h"
+
+//--- transitive.h
+// empty
+
+//--- cdb.json.template
+[{
+  "file": "",
+  "directory": "DIR",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -I DIR -x c"
+}]

[jansvoboda11]

LGTM