[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't #89005

ian-twilightcoder · 2024-04-17T00:11:29Z

HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every textual header to non-external.

llvmbot · 2024-04-17T00:12:04Z

@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 --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 &HFI,
                                           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;
   }

ian-twilightcoder · 2024-04-17T00:14:55Z

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.

ian-twilightcoder · 2024-04-17T00:22:23Z

@ilya-biryukov can you check that this fixes your running out of source location space problem please?

jansvoboda11 · 2024-04-17T00:23:57Z

As a test, maybe you could probe the resulting PCM with -module-file-info.

ilya-biryukov · 2024-04-17T12:02:04Z

@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.

jansvoboda11 · 2024-04-17T15:38:39Z

getExistingFileInfo(, /WantExternal=/false)

Until recently that function still consulted ExternalSource for HeaderFileInfo that IsValid && !External && !Resolved. Maybe you want to try the new getExistingLocalFileInfo()?

ian-twilightcoder · 2024-04-17T17:21:09Z

@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?

zygoloid · 2024-04-17T18:03:30Z

clang/lib/Lex/HeaderSearch.cpp

+  return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&
+         (HFI->isTextualModuleHeader ==
+          ((Role & ModuleMap::TextualHeader) != 0));


Should this be:

Suggested change

return (HFI->isModuleHeader == ModuleMap::isModular(Role)) &&

(HFI->isTextualModuleHeader ==

((Role & ModuleMap::TextualHeader) != 0));

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.

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?

If a file is in multiple modules, we use the logic in findModuleForHeader and isBetterKnownHeader 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 .pcms 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.

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 (and I think anything else?) even though A doesn't have a use B. But wouldn't the better setup A to have the use B? I might not be quite understanding what you're saying.

ian-twilightcoder · 2024-04-17T19:43:38Z

Ok this one should fix the logic I think. @ilya-biryukov can you give this a try please?

ian-twilightcoder · 2024-04-17T19:52:42Z

clang/include/clang/Lex/HeaderSearch.h

+  /// 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`.


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

ian-twilightcoder · 2024-04-17T19:59:59Z

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.

jansvoboda11 · 2024-04-17T20:01:57Z

Also note that ASTWriter uses this logic in couple of places to avoid serializing unrelated headers:

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.

ian-twilightcoder · 2024-04-17T20:07:31Z

Also note that ASTWriter uses this logic in couple of places to avoid serializing unrelated headers:
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.

ian-twilightcoder · 2024-04-19T04:20:46Z

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.

sam-mccall · 2024-04-19T15:51:13Z

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)

sam-mccall · 2024-04-19T18:21:57Z

Unfortunately with this patch I'm still seeing the same source-location-exhausted error.
I'm going to try to understand why...

ian-twilightcoder · 2024-04-19T18:31:47Z

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.

zygoloid · 2024-04-19T18:42:54Z

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.

sam-mccall · 2024-04-19T18:47:25Z

Hmm, I locally reverted #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)

jansvoboda11 · 2024-04-19T19:15:31Z

#89441 might be of interest too.

sam-mccall · 2024-04-19T20:27:37Z

#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)

jansvoboda11 · 2024-04-19T20:34:21Z

Just so you're aware, it's possible that #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.

sam-mccall · 2024-04-19T21:24:30Z

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
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

<stdin>: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)
<built-in>:1:1: note: file entered 202 times using 1451554B of space
    1 | # 1 "<built-in>" 3
      | ^
<stdin>:1:1: note: file entered 1 time using 112B of space
    1 | #include "/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"
      | ^

Bad

<stdin>: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" {
      | ^
<built-in>:1:1: note: file entered 202 times using 1455493B of space
    1 | # 1 "<built-in>" 3
      | ^
<stdin>:1:1: note: file entered 1 time using 112B of space
    1 | #include "/usr/local/google/home/sammccall/modulegen/modules_repro/m1/header"

…ers' HeaderFileInfo non-external when it shouldn't HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every `textual header` to non-external.

ian-twilightcoder · 2024-04-22T19:42:30Z

I guess #89441 is the real issue that #83660 caused, but we should still fix this for correctness, even though I'm not sure what the visible consequence is of this bug.

ian-twilightcoder · 2024-04-25T23:33:53Z

From Jan:

Maybe we can add the number of external HeaderFileInfos to HeaderSearch::PrintStats() and have a test that checks for it.
The flag to enable that output is -show-stats I think.

ian-twilightcoder requested review from zygoloid, Bigcheese, ilya-biryukov, jyknight, jansvoboda11, ributzka and cyndyishida April 17, 2024 00:11

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 17, 2024

ian-twilightcoder mentioned this pull request Apr 17, 2024

[clang][modules] Headers meant to be included multiple times can be completely invisible in clang module builds #83660

Merged

ilya-biryukov mentioned this pull request Apr 17, 2024

[Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs #88893

Open

zygoloid reviewed Apr 17, 2024

View reviewed changes

ian-twilightcoder force-pushed the header-file-info-reuse branch from f45cb72 to 0ea2af8 Compare April 17, 2024 19:43

ian-twilightcoder commented Apr 17, 2024

View reviewed changes

ian-twilightcoder requested a review from zygoloid April 17, 2024 20:00

sam-mccall self-assigned this Apr 19, 2024

jansvoboda11 mentioned this pull request Apr 19, 2024

[clang][modules] Allow module maps with textual headers to be non-affecting #89441

Merged

[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual head…

45e7409

…ers' HeaderFileInfo non-external when it shouldn't HeaderSearch::MarkFileModuleHeader is no longer properly checking for no-changes, and so sets the HeaderFileInfo for every `textual header` to non-external.

ian-twilightcoder changed the title ~~[clang][modules] HeaderSearch::MarkFileModuleHeader creates extra HeaderFileInfo, breaks PCM reuse~~ [clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't Apr 22, 2024

ian-twilightcoder force-pushed the header-file-info-reuse branch from 0ea2af8 to 45e7409 Compare April 22, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't #89005

[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't #89005

ian-twilightcoder commented Apr 17, 2024 •

edited

llvmbot commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ilya-biryukov commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

zygoloid Apr 17, 2024

ian-twilightcoder Apr 17, 2024

zygoloid Apr 17, 2024

ian-twilightcoder Apr 17, 2024 •

edited

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder Apr 17, 2024 •

edited

ian-twilightcoder commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

ian-twilightcoder commented Apr 19, 2024

zygoloid commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

jansvoboda11 commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

jansvoboda11 commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

ian-twilightcoder commented Apr 22, 2024

ian-twilightcoder commented Apr 25, 2024

[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't #89005

Are you sure you want to change the base?

[clang][modules] HeaderSearch::MarkFileModuleHeader sets textual headers' HeaderFileInfo non-external when it shouldn't #89005

Conversation

ian-twilightcoder commented Apr 17, 2024 • edited

llvmbot commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ilya-biryukov commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

zygoloid Apr 17, 2024

Choose a reason for hiding this comment

ian-twilightcoder Apr 17, 2024

Choose a reason for hiding this comment

zygoloid Apr 17, 2024

Choose a reason for hiding this comment

ian-twilightcoder Apr 17, 2024 • edited

Choose a reason for hiding this comment

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder Apr 17, 2024 • edited

Choose a reason for hiding this comment

ian-twilightcoder commented Apr 17, 2024

jansvoboda11 commented Apr 17, 2024

ian-twilightcoder commented Apr 17, 2024

ian-twilightcoder commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

ian-twilightcoder commented Apr 19, 2024

zygoloid commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

jansvoboda11 commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

jansvoboda11 commented Apr 19, 2024

sam-mccall commented Apr 19, 2024

ian-twilightcoder commented Apr 22, 2024

ian-twilightcoder commented Apr 25, 2024

ian-twilightcoder commented Apr 17, 2024 •

edited

ian-twilightcoder Apr 17, 2024 •

edited

ian-twilightcoder Apr 17, 2024 •

edited