[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 closed 
https://github.com/llvm/llvm-project/pull/70144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-25 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir approved this pull request.

> Yeah. I did try to fix up all calls to LookupFile to perform module map 
> lookup, but a bunch of tests started failing (mostly standard C++ modules 
> tests IIRC), so there's probably more nuance required there.

Okay, I do think this is worth fixing long term, but I don't want to block on 
it.  Your change LGTM in the meantime.

https://github.com/llvm/llvm-project/pull/70144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-25 Thread Jan Svoboda via cfe-commits 

jansvoboda11 wrote:

> Can you clarify how this bug occurs in terms of what information about the 
> header is stored that causes us to get the wrong module?

I updated the test case to be more in line with how the bug manifested in the 
wild. Let me annotate the test case here to hopefully clarify things:

```c
//--- tu.c
#include "poison.h" // This imports Poison.pcm. In its HEADER_SEARCH_TABLE 
record, this PCM stores
// information that "FW/B.h" is textual. This happens 
because the compiler never
// looked for the module map when it encountered 
`__has_include()`, and
// therefore set 
`HeaderFileInfo::{isModuleHeader,isCompilingModuleHeader}` to
// `false` for that header. `ASTWriter` then decided to 
serialize info on that header:
// 
https://github.com/llvm/llvm-project/blob/740582fa4c9512b34128dc97b2eff56820984421/clang/lib/Serialization/ASTWriter.cpp#L2023

#if __has_include() // This looks into Poison.pcm, finds out that 
"FW/B.h" is textual,
// and caches that in HeaderSearch::FileInfo.
#endif

#include "import.h" // This transitively imports FW_Private.pcm.
// This does not parse FW_Private module map, that would 
actually tell us that "FW/B.h"
// is most likely part of FW_Private (via the 
PrivateHeaders umbrella header).
// FW_Private.pcm does contain the information that 
"FW/B.h" is part of FW_Private, but...

#include  // ... this does not deserialize the info on "FW/B.h" from 
FW_Private.pcm
  // due to the unimplemented FIXME here:
  // 
https://github.com/llvm/llvm-project/blob/740582fa4c9512b34128dc97b2eff56820984421/clang/lib/Lex/HeaderSearch.cpp#L1320
  // This header is therefore considered textual.
```

So an alternative solution would be to implement that fixme and ensure we do 
deserialize `HeaderFileInfo` from newly loaded PCM files. That would tell us 
the FW_Private.pcm considers "FW/B.h" modular. I'd rather fix what we actually 
serialize into PCM files first.

> It seems bad that passing `nullptr` to `LookupFile` could cause incorrect 
> behavior and makes me wonder if we need to always trigger module lookup or if 
> there is another way to fix this that would make it safe to not do module 
> lookup here.

Yeah. I did try to fix up all calls to `LookupFile` to perform module map 
lookup, but a bunch of tests started failing (mostly standard C++ modules tests 
IIRC), so there's probably more nuance required there.

https://github.com/llvm/llvm-project/pull/70144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-25 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 updated 
https://github.com/llvm/llvm-project/pull/70144

>From 4199b80e5cb0a8873f63c356e4c4304833d6fffa Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 24 Oct 2023 16:30:22 -0700
Subject: [PATCH 1/2] [clang][deps] Fix `__has_include` behavior with umbrella
 headers

Previously, Clang wouldn't try to resolve the module for the header being 
checked via `__has_include`. This meant that such header was considered textual 
(a.k.a. part of the module Clang is currently compiling).
---
 clang/lib/Lex/PPMacroExpansion.cpp|  7 +-
 .../modules-has-include-umbrella-header.c | 99 +++
 2 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 
clang/test/ClangScanDeps/modules-has-include-umbrella-header.c

diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index b371f8cf7a9c072..30c4abdbad8aa44 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1248,10 +1248,15 @@ static bool EvaluateHasIncludeCommon(Token , 
IdentifierInfo *II,
   if (Filename.empty())
 return false;
 
+  // Passing this to LookupFile forces header search to check whether the found
+  // file belongs to a module. Skipping that check could incorrectly mark
+  // modular header as textual, causing issues down the line.
+  ModuleMap::KnownHeader KH;
+
   // Search include directories.
   OptionalFileEntryRef File =
   PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, 
LookupFromFile,
-nullptr, nullptr, nullptr, nullptr, nullptr, nullptr);
+nullptr, nullptr, nullptr, , nullptr, nullptr);
 
   if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
 SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
diff --git a/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c 
b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
new file mode 100644
index 000..45ff2bd3b9cd24e
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
@@ -0,0 +1,99 @@
+// This test checks that __has_include() in a module does
+// not clobber #include  in importers of said module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -I 
DIR/modules -F DIR/frameworks -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private {
+  umbrella header "A.h"
+  module * { export * }
+}
+//--- frameworks/FW.framework/PrivateHeaders/A.h
+#include 
+//--- frameworks/FW.framework/PrivateHeaders/B.h
+struct B {};
+
+//--- modules/module.modulemap
+module Foo { header "foo.h" }
+//--- modules/foo.h
+#if __has_include()
+#define HAS_B 1
+#else
+#define HAS_B 0
+#endif
+
+//--- tu.c
+#include "foo.h"
+#include 
+
+// 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/deps.json
+// RUN: cat %t/deps.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/A.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/B.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: },
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": 
"[[PREFIX]]/modules/module.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:  
"-fmodule-map-file=[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// FIXME: The frameworks/FW.framework/PrivateHeaders/B.h header never makes it 
into SourceManager,
+//so we don't track it as a file dependency (even though we should).
+// CHECK-NEXT: 
"[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/modules/foo.h",
+// CHECK-NEXT: "[[PREFIX]]/modules/module.modulemap"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "Foo"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "commands": [
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-context-hash":

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-24 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir commented:

Can you clarify how this bug occurs in terms of what information about the 
header is stored that causes us to get the wrong module? It seems bad that 
passing `nullptr` to `LookupFile` could cause incorrect behaviour and makes me 
wonder if we need to always trigger module lookup or if there is another way to 
fix this that would make it safe to not do module lookup here.

https://github.com/llvm/llvm-project/pull/70144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-24 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/70144
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-24 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)


Changes

Previously, Clang wouldn't try to resolve the module for the header being 
checked via `__has_include`. This meant that such header was considered textual 
(a.k.a. part of the module Clang is currently compiling).


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


2 Files Affected:

- (modified) clang/lib/Lex/PPMacroExpansion.cpp (+6-1) 
- (added) clang/test/ClangScanDeps/modules-has-include-umbrella-header.c (+99) 


``diff
diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index b371f8cf7a9c072..30c4abdbad8aa44 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1248,10 +1248,15 @@ static bool EvaluateHasIncludeCommon(Token , 
IdentifierInfo *II,
   if (Filename.empty())
 return false;
 
+  // Passing this to LookupFile forces header search to check whether the found
+  // file belongs to a module. Skipping that check could incorrectly mark
+  // modular header as textual, causing issues down the line.
+  ModuleMap::KnownHeader KH;
+
   // Search include directories.
   OptionalFileEntryRef File =
   PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, 
LookupFromFile,
-nullptr, nullptr, nullptr, nullptr, nullptr, nullptr);
+nullptr, nullptr, nullptr, , nullptr, nullptr);
 
   if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
 SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
diff --git a/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c 
b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
new file mode 100644
index 000..45ff2bd3b9cd24e
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
@@ -0,0 +1,99 @@
+// This test checks that __has_include() in a module does
+// not clobber #include  in importers of said module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -I 
DIR/modules -F DIR/frameworks -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private {
+  umbrella header "A.h"
+  module * { export * }
+}
+//--- frameworks/FW.framework/PrivateHeaders/A.h
+#include 
+//--- frameworks/FW.framework/PrivateHeaders/B.h
+struct B {};
+
+//--- modules/module.modulemap
+module Foo { header "foo.h" }
+//--- modules/foo.h
+#if __has_include()
+#define HAS_B 1
+#else
+#define HAS_B 0
+#endif
+
+//--- tu.c
+#include "foo.h"
+#include 
+
+// 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/deps.json
+// RUN: cat %t/deps.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/A.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/B.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: },
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": 
"[[PREFIX]]/modules/module.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:  
"-fmodule-map-file=[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// FIXME: The frameworks/FW.framework/PrivateHeaders/B.h header never makes it 
into SourceManager,
+//so we don't track it as a file dependency (even though we should).
+// CHECK-NEXT: 
"[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/modules/foo.h",
+// CHECK-NEXT: "[[PREFIX]]/modules/module.modulemap"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "Foo"
+// 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: },
+//

[clang] [clang][deps] Fix `__has_include` behavior with umbrella headers (PR #70144)

2023-10-24 Thread Jan Svoboda via cfe-commits 

https://github.com/jansvoboda11 created 
https://github.com/llvm/llvm-project/pull/70144

Previously, Clang wouldn't try to resolve the module for the header being 
checked via `__has_include`. This meant that such header was considered textual 
(a.k.a. part of the module Clang is currently compiling).


>From 4199b80e5cb0a8873f63c356e4c4304833d6fffa Mon Sep 17 00:00:00 2001
From: Jan Svoboda 
Date: Tue, 24 Oct 2023 16:30:22 -0700
Subject: [PATCH] [clang][deps] Fix `__has_include` behavior with umbrella
 headers

Previously, Clang wouldn't try to resolve the module for the header being 
checked via `__has_include`. This meant that such header was considered textual 
(a.k.a. part of the module Clang is currently compiling).
---
 clang/lib/Lex/PPMacroExpansion.cpp|  7 +-
 .../modules-has-include-umbrella-header.c | 99 +++
 2 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 
clang/test/ClangScanDeps/modules-has-include-umbrella-header.c

diff --git a/clang/lib/Lex/PPMacroExpansion.cpp 
b/clang/lib/Lex/PPMacroExpansion.cpp
index b371f8cf7a9c072..30c4abdbad8aa44 100644
--- a/clang/lib/Lex/PPMacroExpansion.cpp
+++ b/clang/lib/Lex/PPMacroExpansion.cpp
@@ -1248,10 +1248,15 @@ static bool EvaluateHasIncludeCommon(Token , 
IdentifierInfo *II,
   if (Filename.empty())
 return false;
 
+  // Passing this to LookupFile forces header search to check whether the found
+  // file belongs to a module. Skipping that check could incorrectly mark
+  // modular header as textual, causing issues down the line.
+  ModuleMap::KnownHeader KH;
+
   // Search include directories.
   OptionalFileEntryRef File =
   PP.LookupFile(FilenameLoc, Filename, isAngled, LookupFrom, 
LookupFromFile,
-nullptr, nullptr, nullptr, nullptr, nullptr, nullptr);
+nullptr, nullptr, nullptr, , nullptr, nullptr);
 
   if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
 SrcMgr::CharacteristicKind FileType = SrcMgr::C_User;
diff --git a/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c 
b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
new file mode 100644
index 000..45ff2bd3b9cd24e
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-has-include-umbrella-header.c
@@ -0,0 +1,99 @@
+// This test checks that __has_include() in a module does
+// not clobber #include  in importers of said module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- cdb.json.template
+[{
+  "file": "DIR/tu.c",
+  "directory": "DIR",
+  "command": "clang DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -I 
DIR/modules -F DIR/frameworks -o DIR/tu.o"
+}]
+
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private {
+  umbrella header "A.h"
+  module * { export * }
+}
+//--- frameworks/FW.framework/PrivateHeaders/A.h
+#include 
+//--- frameworks/FW.framework/PrivateHeaders/B.h
+struct B {};
+
+//--- modules/module.modulemap
+module Foo { header "foo.h" }
+//--- modules/foo.h
+#if __has_include()
+#define HAS_B 1
+#else
+#define HAS_B 0
+#endif
+
+//--- tu.c
+#include "foo.h"
+#include 
+
+// 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/deps.json
+// RUN: cat %t/deps.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/A.h",
+// CHECK-NEXT: "[[PREFIX]]/frameworks/FW.framework/PrivateHeaders/B.h"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "FW_Private"
+// CHECK-NEXT: },
+// CHECK-NEXT: {
+// CHECK-NEXT:   "clang-module-deps": [],
+// CHECK-NEXT:   "clang-modulemap-file": 
"[[PREFIX]]/modules/module.modulemap",
+// CHECK-NEXT:   "command-line": [
+// CHECK:  
"-fmodule-map-file=[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK:],
+// CHECK-NEXT:   "context-hash": "{{.*}}",
+// CHECK-NEXT:   "file-deps": [
+// FIXME: The frameworks/FW.framework/PrivateHeaders/B.h header never makes it 
into SourceManager,
+//so we don't track it as a file dependency (even though we should).
+// CHECK-NEXT: 
"[[PREFIX]]/frameworks/FW.framework/Modules/module.private.modulemap",
+// CHECK-NEXT: "[[PREFIX]]/modules/foo.h",
+// CHECK-NEXT: "[[PREFIX]]/modules/module.modulemap"
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "name": "Foo"
+//