[C++20] [Modules] It is unnecessary to contain HeaderSearch table for C++20 named modules.

[mizvekov]

Reproducer:

// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm -verify
// RUN: %clang_cc1 -std=c++20 %t/B.cpp -fprebuilt-module-path=%t -fsyntax-only -verify

//--- foo.hpp
#pragma once
#define FOO

//--- A.cppm
// expected-no-diagnostics
module;
#include "foo.hpp"
export module A;

//--- B.cpp
// expected-no-diagnostics
import A;
#include "foo.hpp"
#ifndef FOO
#error 'pragma once' state should not be exported on module interface
#endif

The inclusion of "foo.hpp" in B.cpp is skipped, due to pragma once considering the inclusion from the imported A.cppm module.

Conventional header guards work as expected.

This is due to the fact we export the HeaderSearch table in standard C++ modules. There are old workarounds for clang modules where, during search for header inclusion, we prefer a header which has been included in another module, over the normal search order. Though this causes us to merge their header infos, and that's where pragma once state lives.

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Matheus Izvekov (mizvekov)

Reproducer: ``` // RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm -verify // RUN: %clang_cc1 -std=c++20 %t/B.cpp -fprebuilt-module-path=%t -fsyntax-only -verify

//--- foo.hpp
#pragma once
#define FOO

//--- A.cppm
// expected-no-diagnostics
module;
#include "foo.hpp"
export module A;

//--- B.cpp
// expected-no-diagnostics
import A;
#include "foo.hpp"
#ifndef FOO
#error 'pragma once' state should not be exported on module interface
#endif

The inclusion of "foo.hpp" in `B.cpp` is skipped, due to pragma once considering the inclusion from the imported `A.cppm` module.

Conventional header guards work as expected.

This is due to the fact we export the HeaderSearch table in standard C++ modules. There are old workarounds for clang modules where, during search for header inclusion, we prefer a header which has been included in another module, over the normal search order. Though this causes us to merge their header infos, and that's where pragma once state lives.
</details>

[ChuanqiXu9]

This should be fixed in 7bc170a. I tried to remove HeaderSearch table completely for C++20 modules. But it shows there are some problems with diagnostic messages... so I don't continue. It will be good if you can remove them completely.

[mizvekov]

I see, I was concurrently working on a patch, taking the approach to remove HeaderSearch table completely, but I saw those other issues and didn't finish looking into all that yet.

I think the diagnostic regressions should be fine, We can't reliably suggest header inclusion based on preprocessor state of another module, which could produce a completely different outcome.

By the way, I think this same issue exists for header-unit modules, but we may not be able to easily drop HeaderSearch table for that case, as there is some other bits in HeaderFileInfo which are used to determine if we should trigger an implicit module import.

[ChuanqiXu9]

I see, I was concurrently working on a patch, taking the approach to remove HeaderSearch table completely, but I saw those other issues and didn't finish looking into all that yet.

Thank you!

I think the diagnostic regressions should be fine, We can't reliably suggest header inclusion based on preprocessor state of another module, which could produce a completely different outcome.

In fact, it is not only regressions. But a crash with diagnostic engine. I didn't look into it.

Also I remember it is not suggesting header inclusion but shows the source locations of declarations. e.g., note there is declaration is not visible. And I think it is fair.

By the way, I think this same issue exists for header-unit modules, but we may not be able to easily drop HeaderSearch table for that case, as there is some other bits in HeaderFileInfo which are used to determine if we should trigger an implicit module import.

I don't feel header units have the same issue. Since header units are highly like standardized clang header modules. My mental model for implementing header uinits are simply around interfaces and drop some unncessary aspects from clang header modules. But header units can share much more code with clang header modules than C++ named modules. Since I think header units really have much more similar semantics with clang header modules.

[mizvekov]

In fact, it is not only regressions. But a crash with diagnostic engine. I didn't look into it.

Yeah that is a runtime check used in the -verify implementation, that asserts we have parsed all files which could contain // expected directives. I believe that can be fixed.

Also I remember it is not suggesting header inclusion but shows the source locations of declarations. e.g., note there is declaration is not visible. And I think it is fair.

That I don't believe requires HeaderInfo though, the relevant AST nodes will still contain source locations which may point to a file parsed when compiling a module, and that will get imported and merged into the consumer's source manager just fine.

I don't feel header units have the same issue. Since header units are highly like standardized clang header modules. My mental model for implementing header uinits are simply around interfaces and drop some unncessary aspects from clang header modules. But header units can share much more code with clang header modules than C++ named modules. Since I think header units really have much more similar semantics with clang header modules.

Right, but unlike clang modules, header units play by the new C++20 rules which allow multiple definitions as long as they are in different TUs, so we don't need to honor their 'pragma onces' in the module consumer, it should work just fine, and we can increase isolation by not serializing it.

[mizvekov]

By the way, I believe we are trying to improve the openssf best practices score, and https://www.bestpractices.dev/en/projects/6124#release_notes in particular, so it would be good if there was an issue about the incorrect '#pragma once' propagation, if that issue got closed, and the relevent entry in the Release Notes created.

[ChuanqiXu9]

In fact, it is not only regressions. But a crash with diagnostic engine. I didn't look into it.

Yeah that is a runtime check used in the -verify implementation, that asserts we have parsed all files which could contain // expected directives. I believe that can be fixed.

Also I remember it is not suggesting header inclusion but shows the source locations of declarations. e.g., note there is declaration is not visible. And I think it is fair.

That I don't believe requires HeaderInfo though, the relevant AST nodes will still contain source locations which may point to a file parsed when compiling a module, and that will get imported and merged into the consumer's source manager just fine.

Great insight. Looking forward to your patch : )

I don't feel header units have the same issue. Since header units are highly like standardized clang header modules. My mental model for implementing header uinits are simply around interfaces and drop some unncessary aspects from clang header modules. But header units can share much more code with clang header modules than C++ named modules. Since I think header units really have much more similar semantics with clang header modules.

Right, but unlike clang modules, header units play by the new C++20 rules which allow multiple definitions as long as they are in different TUs, so we don't need to honor their 'pragma onces' in the module consumer, it should work just fine, and we can increase isolation by not serializing it.

But due to the limitations of implementations, clang is slow to handle redeclarations in multiple TUs. An example may be https://discourse.llvm.org/t/alternatives-to-the-implementation-of-std-modules/71958. I feel this is pretty fundamental and not possible to solve completely and easily. Basically, every time we call redecls() it will iterate all the declarations in all (imported) TUs.

So even if semantically ok to have multiple declarations with header units, I don't feel it will be good.

[ChuanqiXu9]

By the way, I believe we are trying to improve the openssf best practices score, and https://www.bestpractices.dev/en/projects/6124#release_notes in particular, so it would be good if there was an issue about the incorrect '#pragma once' propagation, if that issue got closed, and the relevent entry in the Release Notes created.

Sorry. In clang15, 16 and 17, due to there are a lot of issues with modules, I tried to summarize the state of issues in the end of the release cycle. An example may be f5768ec.

But due to the frequency of modules update getting less in 18, maybe we should better change to the normal practices.

[pogo59]

Is this a dup of #73023 ? or just kind of related?