Skip to content

[clang][modules] Only modulemaps of included textual headers are affecting #89729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[clang][modules] Only modulemaps of included textual headers are affecting #89729

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9165d60
[clang][modules] Allow module map files with textual header be non-af…
jansvoboda11 Apr 19, 2024
0bf2b15
Track textually included files, and consider their modulemaps affecting.
sam-mccall Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/include/clang/Lex/HeaderSearch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,10 @@ class HeaderSearch {
bool isImport, bool ModulesEnabled, Module *M,
bool &IsFirstIncludeOfFile);

/// Record that we're parsing this file as part of the current module, if any.
/// The header's owning modulemap is considered to affect the current module.
void EnteredTextualFile(FileEntryRef File);

/// Return whether the specified file is a normal header,
/// a system header, or a C++ friendly system header.
SrcMgr::CharacteristicKind getFileDirFlavor(FileEntryRef File) {
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Lex/HeaderSearch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,10 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
HFI.isCompilingModuleHeader |= isCompilingModuleHeader;
}

void HeaderSearch::EnteredTextualFile(FileEntryRef File) {
getFileInfo(File).isCompilingModuleHeader = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for isCompilingModuleHeader says:

  /// Whether this header is part of the module that we are building, even if it
  /// doesn't build with the module. i.e. this will include `excluded` and
  /// `textual` headers as well as normal headers.

To me this means this should set only for headers that semantically belong to the current module, not headers that happen to be compiled with it due to being textual.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I meant to alter that description in this patch, it seems that change got lost.

At this point, isCompilingModuleHeader is used only for this "is affecting" check, and for a heuristic related to translating #include into #import. The latter only cares about modular headers, so it seems reasonable to change the semantics for textual headers to something more useful. (But I do need to be careful we're only setting this for textual modular headers).

}

bool HeaderSearch::ShouldEnterIncludeFile(Preprocessor &PP,
FileEntryRef File, bool isImport,
bool ModulesEnabled, Module *M,
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Lex/PPDirectives.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2609,6 +2609,7 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
}

assert(!IsImportDecl && "failed to diagnose missing module for import decl");
HeaderInfo.EnteredTextualFile(*File);
return {ImportAction::None};
}

Expand Down
10 changes: 6 additions & 4 deletions clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
continue;

const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
!HFI->isCompilingModuleHeader))
continue;

for (const auto &KH : HS.findResolvedModulesForHeader(*File)) {
Expand Down Expand Up @@ -2055,11 +2056,12 @@ void ASTWriter::WriteHeaderSearch(const HeaderSearch &HS) {

// Get the file info. Skip emitting this file if we have no information on
// it as a header file (in which case HFI will be null) or if it hasn't
// changed since it was loaded. Also skip it if it's for a modular header
// from a different module; in that case, we rely on the module(s)
// changed since it was loaded. Also skip it if it's for a non-excluded
// header from a different module; in that case, we rely on the module(s)
// containing the header to provide this information.
const HeaderFileInfo *HFI = HS.getExistingLocalFileInfo(*File);
if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader))
if (!HFI || ((HFI->isModuleHeader || HFI->isTextualModuleHeader) &&
!HFI->isCompilingModuleHeader))
continue;

// Massage the file path into an appropriate form.
Expand Down
47 changes: 47 additions & 0 deletions clang/test/Modules/prune-non-affecting-module-map-files-textual.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// This test checks that a module map with a textual header can be marked as
// non-affecting.

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

//--- X.modulemap
module X { textual header "X.h" }
//--- X.h
typedef int X_int;

//--- Y.modulemap
module Y { textual header "Y.h" }
//--- Y.h
typedef int Y_int;

//--- A.modulemap
module A { header "A.h" export * }
//--- A.h
#include "X.h"

// RUN: %clang_cc1 -fmodules -emit-module %t/A.modulemap -fmodule-name=A -o %t/A0.pcm \
// RUN: -fmodule-map-file=%t/X.modulemap
// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/A0.pcm | FileCheck %s --check-prefix=A0 --implicit-check-not=Y.modulemap
// A0: Input file: {{.*}}/X.modulemap

// RUN: %clang_cc1 -fmodules -emit-module %t/A.modulemap -fmodule-name=A -o %t/A1.pcm \
// RUN: -fmodule-map-file=%t/X.modulemap -fmodule-map-file=%t/Y.modulemap
// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/A0.pcm | FileCheck %s --check-prefix=A1 \
// RUN: --implicit-check-not=Y.modulemap
// A1: Input file: {{.*}}/X.modulemap

// RUN: diff %t/A0.pcm %t/A1.pcm

//--- B.modulemap
module B { header "B.h" export * }
//--- B.h
#include "A.h"
typedef X_int B_int;

// RUN: %clang_cc1 -fmodules -emit-module %t/B.modulemap -fmodule-name=B -o %t/B.pcm \
// RUN: -fmodule-file=A=%t/A0.pcm \
// RUN: -fmodule-map-file=%t/A.modulemap -fmodule-map-file=%t/X.modulemap -fmodule-map-file=%t/Y.modulemap
// RUN: %clang_cc1 -fsyntax-only -module-file-info %t/B.pcm | FileCheck %s --check-prefix=B \
// RUN: --implicit-check-not=X.modulemap --implicit-check-not=Y.modulemap
// B: Input file: {{.*}}/B.modulemap