[clang][include-tree] Load even spurious modular dependencies #8011

jansvoboda11 · 2024-01-22T02:14:01Z

With implicit modules, some modular dependencies can be "spurious". By that I mean that #include "Header.h" results in a PCM file load, but the header ends up being included textually anyways. (This can happen in situations that would trigger the -Wincomplete-umbrella warning.)

This patch ensures this behavior is preserved with include-tree. Previously, include-tree would simply textually include the header file and skip the PCM file load. This now ensures consistent behavior in edge-cases like the one in the attached test case (with -fmodule-name=X).

rdar://121220983

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c

jansvoboda11 · 2024-01-30T05:50:03Z

Pushed new commit that approaches the fix from different angle. It introduces new include-tree node that attempts to mimic what happens in implicit builds and non-caching explicit builds: Spurious.pcm is loaded, the Missing submodule is found to be missing, and the header is included textually instead. Loading Spurious.pcm also loads Mod.pcm, so importing Mod for visibility only has the desired effect of making its symbols visible.

benlangmuir

I suggest we upstream the PPCallbacks change. Relatedly, should anywhere you have if (SuggestedModule && ModuleImported) be simplified to if (ModuleImported) ? It seems like SuggestedModule should never be null if we're importing it, right?

clang/include/clang/CAS/IncludeTree.h

clang/lib/CAS/IncludeTree.cpp

clang/lib/Frontend/FrontendAction.cpp

clang/lib/Frontend/IncludeTreePPActions.cpp

jansvoboda11 · 2024-01-30T23:11:33Z

I suggest we upstream the PPCallbacks change.

Yes, if this approach gets a green light, I'll upstream this change.

Relatedly, should anywhere you have if (SuggestedModule && ModuleImported) be simplified to if (ModuleImported) ? It seems like SuggestedModule should never be null if we're importing it, right?

That's correct. I'll update any current uses of SuggestedModule && ModuleImported.

jansvoboda11 · 2024-01-30T23:14:21Z

Sorry, I should've mentioned this is more of a proof-of-concept that I wanted to get some high-level feedback on. I'm happy to implement the TODO and deal with errors properly if we agree this approach makes sense.

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c

benlangmuir

LGTM, just a couple of small suggestions

benlangmuir · 2024-02-07T23:54:04Z

clang/lib/Lex/PPDirectives.cpp

-    if (auto *Import = std::get_if<PPCachedActions::IncludeModule>(&Include)) {
+    };
+
+    auto LoadModule = [&](const PPCachedActions::IncludeModule *Import) {


Maybe factor HandleIncludeMod to call LoadModule and make the diagnostic part conditional? The error-handling logic is sufficiently interesting to not want it repeated.

benlangmuir · 2024-02-07T23:56:02Z

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c

+// RUN: FileCheck %s -input-file %t/tu-include-tree.txt -DPREFIX=%/t
+// CHECK:      [[PREFIX]]/tu.m llvmcas://
+// CHECK-NEXT: 1:1 <built-in> llvmcas://
+// CHECK-NEXT: 2:1 (Module) Spurious.Missing [[PREFIX]]/frameworks/Spurious.framework/Headers/Missing.h llvmcas://


It seems like we should mark this as a spurious import explicitly in the output for readability. Either inline or you could split it across lines and indent the import + tree.

…allback (#81061) This patch provides more information to the `PPCallbacks::InclusionDirective()` hook. We now always pass the suggested module, regardless of whether it was actually imported or not. The extra `bool ModuleImported` parameter then denotes whether the header `#include` will be automatically translated into import the the module. The main change is in `clang/lib/Lex/PPDirectives.cpp`, where we take care to not modify `SuggestedModule` after it's been populated by `LookupHeaderIncludeOrImport()`. We now exclusively use the `SM` (`ModuleToImport`) variable instead, which has been equivalent to `SuggestedModule` until now. This allows us to use the original non-modified `SuggestedModule` for the callback itself. (This patch turns out to be necessary for swiftlang#8011).

…allback (llvm#81061) This patch provides more information to the `PPCallbacks::InclusionDirective()` hook. We now always pass the suggested module, regardless of whether it was actually imported or not. The extra `bool ModuleImported` parameter then denotes whether the header `#include` will be automatically translated into import the the module. The main change is in `clang/lib/Lex/PPDirectives.cpp`, where we take care to not modify `SuggestedModule` after it's been populated by `LookupHeaderIncludeOrImport()`. We now exclusively use the `SM` (`ModuleToImport`) variable instead, which has been equivalent to `SuggestedModule` until now. This allows us to use the original non-modified `SuggestedModule` for the callback itself. (This patch turns out to be necessary for #8011). (cherry picked from commit da95d92)

With implicit modules, some modular dependencies can be "spurious". By that I mean that `#include "Header.h"` results in a PCM file load, but the header ends up being included textually anyways. (This can happen in situations that would trigger the `-Wincomplete-umbrella` warning.) This patch ensures this behavior is preserved with include-tree. Previously, include-tree would simply textually include the header file and skip the PCM file load. This now ensures consistent behavior in edge-cases like the one in the attached test case (with `-fmodule-name=X`). rdar://121220983 (cherry picked from commit b230339)

…allback (llvm#81061) This patch provides more information to the `PPCallbacks::InclusionDirective()` hook. We now always pass the suggested module, regardless of whether it was actually imported or not. The extra `bool ModuleImported` parameter then denotes whether the header `#include` will be automatically translated into import the the module. The main change is in `clang/lib/Lex/PPDirectives.cpp`, where we take care to not modify `SuggestedModule` after it's been populated by `LookupHeaderIncludeOrImport()`. We now exclusively use the `SM` (`ModuleToImport`) variable instead, which has been equivalent to `SuggestedModule` until now. This allows us to use the original non-modified `SuggestedModule` for the callback itself. (This patch turns out to be necessary for swiftlang#8011). Change-Id: I6f64614c4468ae4737b20609e065d69390d2e443

jansvoboda11 requested a review from benlangmuir January 22, 2024 02:14

jansvoboda11 commented Jan 22, 2024

View reviewed changes

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c Show resolved Hide resolved

jansvoboda11 commented Jan 22, 2024

View reviewed changes

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c Show resolved Hide resolved

benlangmuir reviewed Jan 30, 2024

View reviewed changes

clang/include/clang/CAS/IncludeTree.h Outdated Show resolved Hide resolved

clang/lib/CAS/IncludeTree.cpp Outdated Show resolved Hide resolved

clang/lib/Frontend/FrontendAction.cpp Outdated Show resolved Hide resolved

clang/lib/Frontend/IncludeTreePPActions.cpp Outdated Show resolved Hide resolved

benlangmuir reviewed Feb 7, 2024

View reviewed changes

clang/test/ClangScanDeps/modules-include-tree-implementation-transitive.c Show resolved Hide resolved

jansvoboda11 force-pushed the jan_svoboda/module-implementation-caching branch from 3b2e935 to ed3e1e7 Compare February 7, 2024 22:40

jansvoboda11 changed the title ~~[clang][deps] Ensure include-tree loads implementation module when necessary~~ [clang][include-tree] Load even spurious modular dependencies Feb 7, 2024

jansvoboda11 requested a review from benlangmuir February 7, 2024 23:50

benlangmuir approved these changes Feb 7, 2024

View reviewed changes

jansvoboda11 mentioned this pull request Feb 8, 2024

[clang][lex] Always pass suggested module to InclusionDirective() callback llvm/llvm-project#81061

Merged

jansvoboda11 added 3 commits February 8, 2024 12:54

[clang][deps] Fix include-tree with module implementation

dce0136

[clang][include-tree] Make spurious import explicit in print output

a6ad865

[clang][include-tree] Extract error handling

a3fdbdc

jansvoboda11 force-pushed the jan_svoboda/module-implementation-caching branch from cd70936 to a3fdbdc Compare February 8, 2024 21:01

jansvoboda11 merged commit b230339 into next Feb 8, 2024

jansvoboda11 deleted the jan_svoboda/module-implementation-caching branch February 8, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][include-tree] Load even spurious modular dependencies #8011

[clang][include-tree] Load even spurious modular dependencies #8011

Uh oh!

jansvoboda11 commented Jan 22, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

benlangmuir left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

Uh oh!

benlangmuir left a comment

Uh oh!

benlangmuir Feb 7, 2024

Uh oh!

benlangmuir Feb 7, 2024

Uh oh!

Uh oh!

[clang][include-tree] Load even spurious modular dependencies #8011

[clang][include-tree] Load even spurious modular dependencies #8011

Uh oh!

Conversation

jansvoboda11 commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

jansvoboda11 commented Jan 30, 2024

Uh oh!

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

benlangmuir Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

benlangmuir Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jansvoboda11 commented Jan 22, 2024 •

edited

Loading