[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
sam-mccall wrote:
Thanks for the pointer to 87848 - reverting that one locally doesn't help
though, even in combination with applying #89005 and #89428. So this change
isn't on the critical path to fixing our builds, but still much appreciated and
will take a look now.
---
Unsurprisingly, the builds that hit this limit are big and slow :-)
Here's a reproducer that seems to capture our case well:
[modulegen.zip](https://github.com/llvm/llvm-project/files/15045686/modulegen.zip)
`gen.sh` in there creates a bunch of modules under `modules_repro`,
`modules_repro/build.sh` builds them with `$CLANG`, and `print.sh` shows the
sloc usage.
Good
```
:2:23: remark: source manager location address space usage:
[-Rsloc-usage]
2 | #pragma clang __debug sloc_usage
| ^
note: 14498B in local locations, 1447248B in locations loaded from AST files,
for a total of 1461746B (0% of available space)
:1:1: note: file entered 202 times using 1451554B of space
1 | # 1 "" 3
| ^
:1:1: note: file entered 1 time using 112B of space
1 | #include
"/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"
| ^
```
Bad
```
:2:23: remark: source manager location address space usage:
[-Rsloc-usage]
2 | #pragma clang __debug sloc_usage
| ^
note: 14537B in local locations, 3956449B in locations loaded from AST files,
for a total of 3970986B (0% of available space)
/usr/local/google/home/sammccall/modulegen/modules_repro/constant.modulemap:1:1:
note: file entered 100 times using 2505300B of space
1 | module "constant" {
| ^
:1:1: note: file entered 202 times using 1455493B of space
1 | # 1 "" 3
| ^
:1:1: note: file entered 1 time using 112B of space
1 | #include
"/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"
```
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
jansvoboda11 wrote: Just so you're aware, it's possible that https://github.com/llvm/llvm-project/pull/87848 introduced some unexpected behavior change. It would be good to check if your issues are caused just by one patch in isolation or by a combination of more patches that landed recently. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
sam-mccall wrote: #89441 fixes our build problems, with or without this patch applied. It's possible this patch makes things better - I haven't checked for actual sloc usage yet, just whether the build fails. So I'll focus on understanding that one and then return here. (I think I'm close to a sharable reproducer too) https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
jansvoboda11 wrote: https://github.com/llvm/llvm-project/pull/89441 might be of interest too. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
sam-mccall wrote: Hmm, I locally reverted https://github.com/llvm/llvm-project/pull/87849 and still see the same issue. I'll patch #89428 instead, but I don't see how it could do better - it's just putting the old and new behavior of #87849 behind a flag, right? In any case I'll try it, and then work on constructing a reproducer (so far I've been mostly treating this as a black box) https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
zygoloid wrote: > Unfortunately with this patch I'm still seeing the same > source-location-exhausted error. Can you try applying #89428 as well? I think we would need both in order to prune the module map from the serialized SLocEntries. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: > Unfortunately with this patch I'm still seeing the same > source-location-exhausted error. I'm going to try to understand why... Damn I really thought that was going to be the one. If you have any way I could reproduce or better write a test that would be great. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
sam-mccall wrote: Unfortunately with this patch I'm still seeing the same source-location-exhausted error. I'm going to try to understand why... https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
sam-mccall wrote: Ilya is out on vacation, I'm able to test this and will get started on that now (apologies for delay & thanks for digging into this) https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: > Ok _this_ one should fix the logic I think. @ilya-biryukov can you give this > a try please? @ilya-biryukov sorry to ping you again, just nobody else knows how to test this. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
https://github.com/ian-twilightcoder edited https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
https://github.com/ian-twilightcoder edited https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
ian-twilightcoder wrote:
I don't think I quite understand. I think the test looked something like this.
```
module A [no_undeclared_includes] {
module one { header "one.h" }
module two { header "two" }
}
module B {
module one { textual header "one.h" }
module three { header "three.h" }
}
```
That allows one.h to include three.h even though A doesn't have a `use B`. But
wouldn't the better setup be to have the `use B`? I might not be quite
understanding what you're saying.
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
zygoloid wrote:
If a file is in multiple modules, we use the logic in [`findModuleForHeader`
and
`isBetterKnownHeader`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/ModuleMap.cpp#L578)
to pick between them when deciding how to handle a `#include`: prefer the
declaration from the current module over anything else, prefer available over
non-available, public over private, non-textual over textual, and (if we're
allowing excluded headers at all) non-excluded over excluded.
If there's still a tie, then yeah, we make an arbitrary decision. (Not
non-deterministic -- we prefer the first-loaded module map -- but also not
likely to be predictable or useful. But it should generally not matter which
one we pick -- the header should have been parsed and interpreted in the same
way regardless -- unless different `.pcm`s have different compiler flags, in
which case we have an ODR violation.)
The point of `textual header` -- at least in our configuration -- is to mark a
header as being intentionally part of a module, so that
`[no_undeclared_includes]` / `-fstrict-modules-decluse` can diagnose a
`#include` of a file that wasn't a declared dependency. This means that it can
be reasonable for a header to be a textual header in one module and a
non-textual header in another module -- in particular, that would mean that
code with a direct dependency on the first module is permitted to include the
header, but that the header is not built as part of building that library. If
that compilation is passed a `.pcm` containing the same header as a non-textual
header, the desired behavior is that we perform an import for that header
(treat it as non-textual for `#include` handling) but use the `textual header`
declaration when determining whether the inclusion is permitted.
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: > Also note that `ASTWriter` uses this logic in couple of places to avoid > serializing unrelated headers: > > ```c++ > if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader)) > continue; > ``` > > Since textual headers from other modules have `isModuleHeader=false` and > `isCompilingModuleHeader=false` after #83660 we always serialize them, even > if we just implicitly found their module map and never entered them. I didn't > check how this patch interacts with that logic, just wanted to surface this. #83660 _shouldn't_ affect that logic. `isModuleHeader` and `isCompilingModuleHeader` _should_ always have the same values after that change. It's supposed to just add an extra `isTextualModuleHeader` without changing any of the other bits. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
jansvoboda11 wrote: Also note that `ASTWriter` uses this logic in couple of places to avoid serializing unrelated headers: ```c++ if (!HFI || (HFI->isModuleHeader && !HFI->isCompilingModuleHeader)) continue; ``` Since textual headers from other modules have `isModuleHeader=false` and `isCompilingModuleHeader=false` after #83660 we always serialize them, even if we just implicitly found their module map and never entered them. I didn't check how this patch interacts with that logic, just wanted to surface this. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: > As a test, maybe you could probe the resulting PCM with `-module-file-info`. What would I be looking for? Presumably the problem is we import the same module twice but fail to find the built pcm in the module cache and so we build it again? I don't know what else would cause runaway growth of pcm files... I actually don't really understand the problem Ilya is hitting... But Richard found (2 now!) good bugs in my code and I'm hoping fixing that will fix Ilya's problem. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
https://github.com/ian-twilightcoder edited https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
@@ -84,7 +84,9 @@ struct HeaderFileInfo {
LLVM_PREFERRED_TYPE(bool)
unsigned isModuleHeader : 1;
- /// Whether this header is a `textual header` in a module.
+ /// Whether this header is a `textual header` in a module. If a header is
+ /// textual in one module and normal in another module, this bit will not be
+ /// set, only `isModuleHeader`.
ian-twilightcoder wrote:
Maybe this behavior is weird? Maybe both bits should be set in this scenario?
`HeaderSearch::ShouldEnterIncludeFile` -> `MaybeReenterImportedFile` could
change its check to `!FileInfo. isModuleHeader &&
FileInfo.isTextualModuleHeader`
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: Ok _this_ one should fix the logic I think. @ilya-biryukov can you give this a try please? https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
https://github.com/ian-twilightcoder updated
https://github.com/llvm/llvm-project/pull/89005
>From 0ea2af8066f2fb307f3bd273cf34da82189af0ab Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Tue, 16 Apr 2024 17:08:28 -0700
Subject: [PATCH] [clang][modules] HeaderSearch::MarkFileModuleHeader creates
extra HeaderFileInfo, breaks PCM reuse
HeaderSearch::MarkFileModuleHeader is no longer properly checking for
no-changes, and so creates a new HeaderFileInfo for every `textual header`,
causes PCM use to go ballistic.
---
clang/include/clang/Lex/HeaderSearch.h | 4 +++-
clang/lib/Lex/HeaderSearch.cpp | 14 +++---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/clang/include/clang/Lex/HeaderSearch.h
b/clang/include/clang/Lex/HeaderSearch.h
index c5f90ef4cb3682..863878986560c4 100644
--- a/clang/include/clang/Lex/HeaderSearch.h
+++ b/clang/include/clang/Lex/HeaderSearch.h
@@ -84,7 +84,9 @@ struct HeaderFileInfo {
LLVM_PREFERRED_TYPE(bool)
unsigned isModuleHeader : 1;
- /// Whether this header is a `textual header` in a module.
+ /// Whether this header is a `textual header` in a module. If a header is
+ /// textual in one module and normal in another module, this bit will not be
+ /// set, only `isModuleHeader`.
LLVM_PREFERRED_TYPE(bool)
unsigned isTextualModuleHeader : 1;
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0632882b296146..fd2333015b61ad 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1313,11 +1313,19 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool moduleMembershipNeedsMerge(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ if (ModuleMap::isModular(Role))
+return !HFI->isModuleHeader || HFI->isTextualModuleHeader;
+ else if (!HFI->isModuleHeader && (Role & ModuleMap::TextualHeader))
+return !HFI->isTextualModuleHeader;
+ else
+return false;
+}
+
static void mergeHeaderFileInfoModuleBits(HeaderFileInfo ,
bool isModuleHeader,
bool isTextualModuleHeader) {
- assert((!isModuleHeader || !isTextualModuleHeader) &&
- "A header can't build with a module and be textual at the same time");
HFI.isModuleHeader |= isModuleHeader;
if (HFI.isModuleHeader)
HFI.isTextualModuleHeader = false;
@@ -1432,7 +1440,7 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
if ((Role & ModuleMap::ExcludedHeader))
return;
auto *HFI = getExistingFileInfo(FE);
-if (HFI && HFI->isModuleHeader)
+if (HFI && !moduleMembershipNeedsMerge(HFI, Role))
return;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
ian-twilightcoder wrote:
Oh I see what you're saying. I must have a HFI that says the header is
normal-modular, and a role that says it's textual-modular. When I merge in the
role it's not going to change the HFI since `isModuleHeader` never gets
downgraded, but my matching check will return false. Sneaky.
> It looks like you're considering "modular header" and "textual header" to be
> mutually exclusive
Yes, I even put an assert in the merging code. But I'm told asserts aren't
usually (ever?) compiled in, so I guess that's probably why it's not getting
hit. I did notice a test listing a header as normal in one module and textual
in another. What does that actually mean though? The test uses it to exploit a
bug in `[no_undeclared_includes]` where the compiler will use one module to
resolve the include and a different module to check for uses and bypass what
the used module says is allowed. Really what does it mean for a header to be in
multiple modules at all? Doesn't that just cause non-deterministic behavior in
header->module lookup?
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
zygoloid wrote:
Should this be:
```suggestion
return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
(HFI->isModuleHeader ||
HFI->isTextualModuleHeader ==
((Role & ModuleMap::TextualHeader) != 0));
```
It looks like you're considering "modular header" and "textual header" to be
mutually exclusive in `HeaderFileInfo` when merging, so presumably we should do
the same here. I don't think this would make a difference for our use case,
though.
Incidentally, this choice of meaning for `isTextualModuleHeader` seems a little
confusing to me from a modeling perspective, given that a header can absolutely
be modular in one module and textual in another, but I think it's fine from a
correctness standpoint. I think it'd be clearer to either use a three-value
enum here, or to independently track whether the header is textual or modular
and change the check below to `isTextualModuleHeader && !isModuleHeader`. But
that's not relevant for this PR.
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: > > @ilya-biryukov can you check that this fixes your running out of source > > location space problem please? > > Just tried it. The patch as is did not help. I've also tried changing the > previous line to `getExistingFileInfo(, /*WantExternal=*/false)` and it > didn't help either. > > Changing from `if ((Role & ModuleMap::ExcludedHeader))` back to `if > (!ModuleMap::isModular(Role))` does help, though, but that clearly leads to > an incorrect behavior as far as the code is concerned. Does it not help because `headerFileInfoModuleBitsMatchRole` is returning `false`? The previous code was doing `WantExternal=true` so I don't think we want `getExistingLocalFileInfo()` here? I think if we used `getExistingLocalFileInfo()`, we'd get `nullptr` back more often and fall down into the `getFileInfo()` case more often wouldn't we? https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
jansvoboda11 wrote: > getExistingFileInfo(, /*WantExternal=*/false) Until [recently](https://github.com/llvm/llvm-project/pull/87848) that function still consulted `ExternalSource` for `HeaderFileInfo` that `IsValid && !External && !Resolved`. Maybe you want to try the new `getExistingLocalFileInfo()`? https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ilya-biryukov wrote: > @ilya-biryukov can you check that this fixes your running out of source > location space problem please? Just tried it. The patch as is did not help. I've also tried changing the previous line to `getExistingFileInfo(, /*WantExternal=*/false)` and it didn't help either. Changing from `if ((Role & ModuleMap::ExcludedHeader))` back to `if (!ModuleMap::isModular(Role))` does help, though, but that clearly leads to an incorrect behavior as far as the code is concerned. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
jansvoboda11 wrote: As a test, maybe you could probe the resulting PCM with `-module-file-info`. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: @ilya-biryukov can you check that this fixes your running out of source location space problem please? https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
ian-twilightcoder wrote: I don't really know a great way to write a test for this. If someone could point me at a similar existing test or has an idea how to write a new test that would be much appreciated. https://github.com/llvm/llvm-project/pull/89005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Ian Anderson (ian-twilightcoder)
Changes
HeaderSearch::MarkFileModuleHeader is no longer properly checking for
no-changes, and so creates a new HeaderFileInfo for every `textual header`,
causes PCM use to go ballistic.
---
Full diff: https://github.com/llvm/llvm-project/pull/89005.diff
1 Files Affected:
- (modified) clang/lib/Lex/HeaderSearch.cpp (+9-1)
``diff
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0632882b296146..9409b97ba0e64a 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
+}
+
static void mergeHeaderFileInfoModuleBits(HeaderFileInfo ,
bool isModuleHeader,
bool isTextualModuleHeader) {
@@ -1432,7 +1440,7 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
if ((Role & ModuleMap::ExcludedHeader))
return;
auto *HFI = getExistingFileInfo(FE);
-if (HFI && HFI->isModuleHeader)
+if (HFI && headerFileInfoModuleBitsMatchRole(HFI, Role))
return;
}
``
https://github.com/llvm/llvm-project/pull/89005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse (PR #89005)
https://github.com/ian-twilightcoder created
https://github.com/llvm/llvm-project/pull/89005
HeaderSearch::MarkFileModuleHeader is no longer properly checking for
no-changes, and so creates a new HeaderFileInfo for every `textual header`,
causes PCM use to go ballistic.
>From f45cb72cb9c70d714bdc071ac51dff1a5e11502b Mon Sep 17 00:00:00 2001
From: Ian Anderson
Date: Tue, 16 Apr 2024 17:08:28 -0700
Subject: [PATCH] [clang][modules] HeaderSearch::MarkFileModuleHeader creates
extra HeaderFileInfo, breaks PCM reuse
HeaderSearch::MarkFileModuleHeader is no longer properly checking for
no-changes, and so creates a new HeaderFileInfo for every `textual header`,
causes PCM use to go ballistic.
---
clang/lib/Lex/HeaderSearch.cpp | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 0632882b296146..9409b97ba0e64a 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -1313,6 +1313,14 @@ OptionalFileEntryRef
HeaderSearch::LookupSubframeworkHeader(
// File Info Management.
//===--===//
+static bool
+headerFileInfoModuleBitsMatchRole(const HeaderFileInfo *HFI,
+ ModuleMap::ModuleHeaderRole Role) {
+ return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+ (HFI->isTextualModuleHeader ==
+ ((Role & ModuleMap::TextualHeader) != 0));
+}
+
static void mergeHeaderFileInfoModuleBits(HeaderFileInfo ,
bool isModuleHeader,
bool isTextualModuleHeader) {
@@ -1432,7 +1440,7 @@ void HeaderSearch::MarkFileModuleHeader(FileEntryRef FE,
if ((Role & ModuleMap::ExcludedHeader))
return;
auto *HFI = getExistingFileInfo(FE);
-if (HFI && HFI->isModuleHeader)
+if (HFI && headerFileInfoModuleBitsMatchRole(HFI, Role))
return;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
