[C++20][Modules] deleted definitions should be allowed in header units #60079

kaimfrai · 2023-01-16T17:22:22Z

Since non-inline external definitions in header units were disallowed in commit/335668b, using standard header units has become impractical.
In particular, the example given at https://clang.llvm.org/docs/StandardCPlusPlusModules.html#header-units

clang++ -std=c++20 -xc++-system-header --precompile iostream -o iostream.pcm

no longer compiles with current Clang 16 (tried libc++ and libstdc++) while it did work before.

Errors include but are not limited to:

libstdc++

include/c++/12/new:206:8: error: non-inline external definitions are not permitted in C++ header units
  void launder(volatile void*) = delete;

include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units
      static const size_t _S_alignment = 0;

libc++

 include/c++/v1/__type_traits/aligned_union.h:29:25: error: non-inline external definitions are not permitted in C++ header units
    static const size_t value = _I0;

include/c++/v1/__functional/reference_wrapper.h:100:27: error: non-inline external definitions are not permitted in C++ header units
template <class _Tp> void ref(const _Tp&&) = delete;

Unfortunately, this issue isn't limited to libstdc++ and libc++. Not only do other headers often include standard headers, but it's entirely resonable to have = delete; in header files. They are therefore also unusable as header units. Transitioning from headers to header units is thus limited to a small subset of headers, while Clang 15 allowed many more. While I think it's the correct default to error out in these cases, I would appreciate an explicit flag that skips over this rule, at least until P1502R1 (Standard library Header Units) or P2465R3 (std module) are implemented. For example:

clang++ -std=c++20 -xc++-system-header -fallow-non-inline-external-definitions-in-header-units --precompile iostream -o iostream.pcm

Without such a flag I would find it very hard to make use of standard header units as they are implemented right now.

llvmbot · 2023-01-16T18:26:59Z

@llvm/issue-subscribers-clang-modules

dwblaikie · 2023-01-16T22:49:10Z

Might be simpler to consider deleted definitions to be implicitly inline - it's more like how it works in practice.

dwblaikie · 2023-01-16T22:52:40Z

I think it's a pretty simple either implementation or spec bug/omission.

I doubt anyone intended to disallow deleted definitions in header units.

ChuanqiXu9 · 2023-01-17T03:12:44Z

Might be simpler to consider deleted definitions to be implicitly inline - it's more like how it works in practice.

Yeah, it sounds good. In the language side, only the names have linkage instead of declarations. So from the perspective of language, the name of the deleted function can be external. But I guess it may be fine for the compiler to treat it as implicitly inline since the deleted definitions should never appear in object files.

I think it's a pretty simple either implementation or spec bug/omission.

According to @iains , some people of wg21 are discussing to remove such rules in future.

For this issue report, I feel it makes sense to skip the rule. Although LLVM16 is going to be branched, we don't need to be hurry since it should be possible to backport the patch since this is a (severe) regression bug.

(This section is not related to the issue report),

Out of curiosity, I want to ask how do you use header units? What is the build system you're using? Currently, wg21 is still discussing how to support header units. It looks like cmake wouldn't support header units until they get a conclusion. I want to say that I feel the header units is not well tested and used a lot in practice now. So I am curious about your practice and using experience for header units.

ChuanqiXu9 · 2023-01-17T07:06:03Z

Sent https://reviews.llvm.org/D141894

iains · 2023-01-17T07:50:34Z

well, we anticipated that this could cause some code to flag an error.

For clarification: there are some WG21 members who are saying they think this rule should be removed. I am not aware of any paper that actually proposes its removal (so that as far as I know, the rule is the same in C++23).

I think that the
include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units static const size_t _S_alignment = 0;

cases look like a bug, tu-local declarations are allowed. (https://eel.is/c++draft/module#import-6
) I guess the const has somehow tripped up the logic.
perhaps we should seek clarification from core on whether this was intended to apply to deleted functions.

Otherwise - from an engineering perspective

If a header contains a non-inline external definition, then it can only be used once (since multiple imports are an ODR violation), therefore actually it probably makes little sense to make it into a header unit, that is just extra build-time work - (it might as well be included textually in the single place it is used).

iains · 2023-01-17T08:05:18Z

The issue with the deleted function is that it is a definition - if it was a declaration, the rule would not fire.

I suppose that we could implement @dwblaikie 's suggestion to make them implicitly inline. I wonder if there are other similar cases that will trip us up.

I will file a question with core now - obviously, clang can elect to make a dialect as @ChuanqiXu9 has proposed in https://reviews.llvm.org/D141894 but let's see what answer we get first?

ChuanqiXu9 · 2023-01-17T08:06:20Z

well, we anticipated that this could cause some code to flag an error.

For clarification: there are some WG21 members who are saying the think this rule should be removed. I am not aware of any paper that actually proposes its removal (so that as far as I know, the rule is the same in C++23).

Thanks for the clarification. That's what I understood. Maybe I didn't tell things clearly.

I think that the include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units static const size_t _S_alignment = 0;

cases look like a bug, tu-local declarations are allowed. (https://eel.is/c++draft/module#import-6
) I guess the const has somehow tripped up the logic.

I think the behavior is correct. Since the name '_S_alignment' has not internal linkage. According to http://eel.is/c++draft/basic.link, _S_alignment doesn't live in namespace scope so [basic.link]p3 doesn't apply for it. And _S_alignment lives in aligned_union and aligned_union has external linkage, so _S_alignment has external linkage according to [basic.link]p5. So I think the behavior here is fine.

perhaps we should seek clarification from core on whether this was intended to apply to deleted functions.

Otherwise - from an engineering perspective

If a header contains a non-inline external definition, then it can only be used once (since multiple imports are an ODR violation), therefore actually it probably makes little sense to make it into a header unit, that is just extra build-time work - (it might as well be included textually in the single place it is used).

Year, since this is the defect of the language spec. It would be better to ask this in core. But clang16 is going to be branched. Even if we can backport patches before the formal release, the time is really limited. I really feel upset that we can't compile a hello world example for header units. So I think we should both accept the patch and ask questions in wg21. Otherwise, we have to mark header units as unusable in clang16, which seems like a big regression.

ChuanqiXu9 · 2023-01-17T08:10:56Z

The issue with the deleted function is that it is a definition - if it was a declaration, the rule would not fire.

I suppose that we could implement @dwblaikie 's suggestion to make them implicitly inline. I wonder if there are other similar cases that will trip us up.

Since Clang16 is going to be branched. I don't think we should perform the action now. It has wider impact.

I will file a question with core now - obviously, clang can elect to make a dialect as @ChuanqiXu9 has proposed in https://reviews.llvm.org/D141894 but let's see what answer we get first?

Yeah, it should be fine to wait for few days.

iains · 2023-01-17T08:41:18Z

the response from core is:

They already are, see https://eel.is/c++draft/dcl.fct.def.delete#4

"A deleted function is implicitly an inline function ([dcl.inline])."

.. so that particular issue can be resolved (I will take a look at it today).

ChuanqiXu9 · 2023-01-17T08:46:39Z

Yeah, it looks like we did some bad for the deleted function in header units or modules. Since clang have already handle the case in Sema::SetDeclDeleted().

ChuanqiXu9 · 2023-01-17T08:55:37Z

@iains After I took a quick look, I found the reasons.

llvm-project/clang/lib/Parse/Parser.cpp

Lines 1392 to 1417 in 951cf65

    
           Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D, 
        
                                                       TemplateInfo.TemplateParams 
        
                                                           ? *TemplateInfo.TemplateParams 
        
                                                           : MultiTemplateParamsArg(), 
        
                                                       &SkipBody, BodyKind); 
        
           if (SkipBody.ShouldSkip) { 
        
             // Do NOT enter SkipFunctionBody if we already consumed the tokens. 
        
             if (BodyKind == Sema::FnBodyKind::Other) 
        
               SkipFunctionBody(); 
        
             return Res; 
        
           } 
        
           // Break out of the ParsingDeclarator context before we parse the body. 
        
           D.complete(Res); 
        
           // Break out of the ParsingDeclSpec context, too.  This const_cast is 
        
           // safe because we're always the sole owner. 
        
           D.getMutableDeclSpec().abort(); 
        
           if (BodyKind != Sema::FnBodyKind::Other) { 
        
             Actions.SetFunctionBodyKind(Res, KWLoc, BodyKind); 
        
             Stmt *GeneratedBody = Res ? Res->getBody() : nullptr; 
        
             Actions.ActOnFinishFunctionBody(Res, GeneratedBody, false); 
        
             return Res;

We diagnose in the call of the line 1392. But the deleted function is marked as implicit inline at line 1414. So here is the problem. We should find other place to diagnose this.

iains · 2023-01-17T08:56:52Z

Yeah, it looks like we did some bad for the deleted function in header units or modules. Since clang have already handle the case in Sema::SetDeclDeleted().

edit:
Well, in the commit referenced - we are checking isInlined() so that should be set for setImplicitlyInline(..) [it obviously already works for friend cases] so some debugging is needed.

edit2:
Ah - OK I see the point above.

iains · 2023-01-17T09:02:07Z

OK. So I can look for an alternate place for the function diagnostic.

Not sure what we should do about the other cases, it seems that the short-term solution would be a dialect and then some discussion in WG21 about the intent.

ChuanqiXu9 · 2023-01-17T09:26:08Z

The form static const size_t _S_alignment = 0; should be fine too since the spec says it is only a declaration even if it has the initializer.

Sent https://reviews.llvm.org/D141905

ChuanqiXu9 · 2023-01-17T09:26:31Z

So it looks like we can fix things properly. So the dialect patch would be abandoned.

iains · 2023-01-17T09:37:56Z

yeah, I have a patch in test for the deleted/defaulted functions.

definitions in header units Address part of #60079. Since the the declaration of a non-inline static data member in its class definition is not a definition. The following form: ``` class A { public: static const int value = 43; }; ``` should be fine to appear in a header unit. From the perspective of implementation, it looks like we simply forgot to check if the variable is a definition... Reviewed By: iains Differential Revision: https://reviews.llvm.org/D141905

iains · 2023-01-17T10:40:06Z

https://reviews.llvm.org/D141908

kaimfrai · 2023-01-17T10:50:03Z

(This section is not related to the issue report),

Out of curiosity, I want to ask how do you use header units? What is the build system you're using? Currently, wg21 is still discussing how to support header units. It looks like cmake wouldn't support header units until they get a conclusion. I want to say that I feel the header units is not well tested and used a lot in practice now. So I am curious about your practice and using experience for header units.

I am using CMake, however I have some custom functions set up that make modules and header units usable for Clang with custom commands. While by no means perfect, complete, or even well portable to other proejcts, it's sufficient to experiment until official support lands.
There are 3 ways I use header units right now:

The first is a header that includes all of the standard library. The reason for this are some functions (-objects) marked static, which are defined in one header and used in another. With both being header units, the function (-object) becomes unavailable due to internal linkage, so I put all of the standard library into one header unit for now. That header unit is re-exported by a named module Std, which I import throughout one project until import std; becomes available.

The second is precompiling third party headers (such as boost). Even though they are only used at one place each, I found this useful to separate the build times for measuring how long a specific module takes to compile if all dependencies are already precompiled.

The third is for headers that contain (almost) only macros. It's true however that there would be no need for a header unit in this case and it could as well stay an include.

In general, I prefer using named modules to header units though. My overall experience with header units and modules has been great. Build times can go way down, it's easier to narrow down the location of bugs, and for tests cases it's possible to simply omit one import and replace it with a mock. That wouldn't be possible with transitive headers. The most trouble I had was transitioning from headers with a lot of type forward declarations to named modules, as you can't have a forward declaration in one module and it's definition in another. The related link errors aren't the most intuitive, but that's what I expect from new features.

iains · 2023-01-17T12:40:42Z

(This section is not related to the issue report),
Out of curiosity, I want to ask how do you use header units? What is the build system you're using?
.... snip....

My overall experience with header units and modules has been great. Build times can go way down, it's easier to narrow down the location of bugs, and for tests cases it's possible to simply omit one import and replace it with a mock.

That is very encouraging.

ChuanqiXu9 · 2023-01-18T02:49:37Z

Thanks for the written-up! It is helpful. I didn't image that the named modules are helpful for testing. Interesting.

The most trouble I had was transitioning from headers with a lot of type forward declarations to named modules, as you can't have a forward declaration in one module and it's definition in another.

Yeah, it needs some refactoring. I am not sure if you know you can have a forward declaration in a partition module unit and define it in another partition module unit. In this way, we may avoid the link errors you mentioned.

definitions in header units Address part of llvm/llvm-project#60079. Since the the declaration of a non-inline static data member in its class definition is not a definition. The following form: ``` class A { public: static const int value = 43; }; ``` should be fine to appear in a header unit. From the perspective of implementation, it looks like we simply forgot to check if the variable is a definition... Reviewed By: iains Differential Revision: https://reviews.llvm.org/D141905

kaimfrai · 2023-01-18T10:08:46Z

I didn't image that the named modules are helpful for testing.

Let me clarify that this works only for free functions found by ADL. For example defining operator == for Class in a submodule Class.Compare. You import that throughout the project, but in the test you import Class.CompareMock instead. Class.CompareMock may import but not export Class.Compare. As long as the mock operator == can be found by lookup (using declaration in the test file or global namespace) and the other one can't, this works great.

I am not sure if you know you can have a forward declaration in a partition module unit and define it in another partition module unit. In this way, we may avoid the link errors you mentioned.

I have made use of partitions for exactly that reason. However there are 2 different uses of forward declarations. One is for cyclic dependencies in which case there is no way around partitions. This isn't a problem as files with cyclic dependencies are often close together on the filesystem and putting them in the same module is easy. Of course, resolving the cycles in the first place would be preferable.

The other use case is to save an include in the header to shorten build time. This can happen with 2 files that are in very different parts of the project (or even 2 different projects). Putting those in the same module may just require putting most of the entire project in the same module due to dependency chains. That defeats one of the main purposes of modules. With modules however, there is no need for these kinds of forward declarations and they should just be imports. Finding just those forward declarations and replacing them by imports can be bothersome, but given the improved build time I think it's worth the effort. I think include-what-you-use with --no_fwd_decls can be helpful in replacing forward-declarations with includes before transitioning to modules, but it's not perfect either.

ChuanqiXu9 · 2023-01-19T02:51:10Z

Interesting.

One is for cyclic dependencies in which case there is no way around partitions. This isn't a problem as files with cyclic dependencies are often close together on the filesystem and putting them in the same module is easy. Of course, resolving the cycles in the first place would be preferable.

Yeah, it would be best to solve the dependencies in the first place. Another solution may be the use of extern "C++" in module units. The spec related note is http://eel.is/c++draft/module#unit-7.2.2. In short, the declarations in extern "C++" of module units are considered to be the declarations in the global module fragment. So their mangled name wouldn't change. An example can be found at: https://github.com/alibaba/async_simple/blob/934d2757c7b0c9f83ba69ae425b7469051270eec/async_simple_module/uthread/internal/thread.cppm#L41-L44 (Although this uses extern "C")

iains · 2023-01-28T18:01:35Z

firstly, I apologise that this is taking some time (and iterations) to resolve; I fell into the trap (I should know better) of looking at the line in the standard and the examples and thinking it looked straight forward. As often happens, it is the exclusions that are most tricky to pin down.

https://reviews.llvm.org/D142704 is now updated and (locally at least) consumes the libc++ headers individually.

Of course, we can also consider backing out of the original patch and the two follow-on ones if we are not converging on a working solution.

Arthapz · 2023-01-28T19:59:03Z

tryied the patch seems to work

> clang --version                                                                                                                                                                           
clang version 17.0.0 (/home/arthapz/llvm-git/llvm-project fc5da3969f2fd6065ab6b93f9586ece9002366ce)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

>rm -rf .xmake build; xmake f --toolchain=clang; xmake b                                                                                                                                    
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  0%]: generating.module.deps src/hello.mpp
[ 11%]: compiling.headerunit.release iostream
[ 33%]: compiling.module.release hello
[ 66%]: compiling.release src/main.cpp
[ 77%]: linking.release stl_headerunit
[100%]: build ok!

> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                                        
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  0%]: generating.module.deps src/hello.mpp
[ 11%]: compiling.headerunit.release iostream
[ 33%]: compiling.module.release hello
[ 66%]: compiling.release src/main.cpp
[ 77%]: linking.release stl_headerunit
[100%]: build ok!

Arthapz · 2023-01-28T20:01:07Z

with a more complexe scenario i get

> rm -rf .xmake build; xmake f --toolchain=clang; xmake b
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release algorithm
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release list
[ 88%]: compiling.release src/main.cpp
[ 92%]: linking.release stl_headerunit_cpp_only
[100%]: build ok!

> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release list
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release algorithm
[ 88%]: compiling.release src/main.cpp
error: /usr/bin/../include/c++/v1/ostream:254:20: error: 'std::basic_ostream<char>::operator<<' from module '/usr/bin/../include/c++/v1/complex' is not present in definition of 'std::ostream' in module '/usr/bin/../include/c++/v1/iostream'
    basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
                   ^
/usr/bin/../include/c++/v1/ostream:221:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&))
                   ^
/usr/bin/../include/c++/v1/ostream:225:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ios<char_type, traits_type>&
                   ^
/usr/bin/../include/c++/v1/ostream:230:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(ios_base& (*__pf)(ios_base&))
                   ^
/usr/bin/../include/c++/v1/ostream:233:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(bool __n);
                   ^
/usr/bin/../include/c++/v1/ostream:234:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(short __n);
  > in src/main.cpp
  
> cat src/main.cpp
import <iostream>;
import <algorithm>;
import <deque>;
import <iostream>;
import <map>;
import <memory>;
import <set>;
import <utility>;
import <vector>;
import <string>;
import <queue>;
import <cstdlib>;
import <utility>;
import <exception>;
import <list>;
import <stack>;
import <complex>;
import <fstream>;
import <cstdio>;
import <iomanip>;

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

iains · 2023-01-28T20:05:02Z

with a more complexe scenario i get

<SNIP>
[ 88%]: compiling.release src/main.cpp
error: /usr/bin/../include/c++/v1/ostream:254:20: error: 'std::basic_ostream<char>::operator<<' from module '/usr/bin/../include/c++/v1/complex' is not present in definition of 'std::ostream' in module '/usr/bin/../include/c++/v1/iostream'
    basic_ostream& operator<<(basic_streambuf<char_type, traits_type>* __sb);
                   ^
/usr/bin/../include/c++/v1/ostream:221:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ostream& (*__pf)(basic_ostream&))
                   ^
<SNIP>

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

That seems to be a different diagnostic - do you find it to be in some way related ?

Arthapz · 2023-01-28T20:07:25Z

maybe it is not a header unit support problem but a bug of libc++ ?

iains · 2023-01-28T20:20:09Z

maybe it is not a header unit support problem but a bug of libc++ ?

perhaps, but when I build ostream in isolation, this does not fire:
./bin/clang++ -std=c++20 -stdlib=libc++ -xc++-system-header ostream --precompile --sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -ferror-limit=5 -v

gives only a warning about #pragma system_header

my changes are based on main 7a3e87298e2f

edit - and the (start of the) output of ./bin/clang -module-file-info ostream.pcm looks like

Information for module file 'ostream.pcm':
  Module format: raw
  ====== C++20 Module structure ======
  Header Unit '/scratch/12-mon/llvm-modules/./bin/../include/c++/v1/ostream' is the Primary Module at index #1
   Sub Modules:
    Global Module Fragment '<global>' is at index #2
   Macro Definitions:

Arthapz · 2023-01-28T20:24:23Z

i just build a llvm from df34581 with ChianXqi p1689 and clang-scan-deps patches

building the header only, the error is in the .cpp file when importing ostream and complex headers

iains · 2023-01-28T20:27:37Z

OK .. at present, this does not seem to be the same bug, is it possible for you to produce an isolated reproducer (and maybe file a separate bug) ?

Arthapz · 2023-01-28T20:29:19Z

/usr/bin/clang -Qunused-arguments -m64 -fvisibility=hidden -fvisibility-inlines-hidden -O3 -std=c++20 -fmodules -fno-implicit-module-maps -stdlib=libc++ -DNDEBUG -fmodules-cache-path=build/stlmodules/cache -c -Wno-everything -o build/stlmodules/cache/iostream.pcm -x c++-system-header iostream

/usr/bin/clang -Qunused-arguments -m64 -fvisibility=hidden -fvisibility-inlines-hidden -O3 -std=c++20 -fmodules -fno-implicit-module-maps -stdlib=libc++ -DNDEBUG -fmodules-cache-path=build/stlmodules/cache -c -Wno-everything -o build/stlmodules/cache/complex.pcm -x c++-system-header complex

/usr/bin/clang -c -Qunused-arguments -m64 -fvisibility=hidden -fvisibility-inlines-hidden -O3 -std=c++20 -fmodules -fno-implicit-module-maps -stdlib=libc++ -DNDEBUG -fmodule-file=build/stlmodules/cache/iostream.pcm -fmodule-file=build/stlmodules/cache/complex.pcm -o build/.objs/stl_headerunit_cpp_only/linux/x86_64/release/src/main.cpp.o src/main.cpp

import <iostream>;
import <complex>;

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}

iains · 2023-01-28T20:44:10Z

I can repeat with my build, but cannot yet see any connection to the diagnostic that is the subject of this PR.

Arthapz · 2023-01-28T21:04:38Z

so it is not, i've made an other issue #60358

kaimfrai · 2023-01-29T11:37:27Z

firstly, I apologise that this is taking some time (and iterations) to resolve

No worries, I'm well aware this can happen when using unreleased builds. I've been using a build from January 8th in the meantime. The patch seems to work this time, no issues with either libc++ or libstdc++, so thank you. I hope this lands soon on apt.llvm.org.

Regarding the other issue #60358 it seems to be the same issue as #58540.
I've ran into this before. My conclusion at the time was that some implementation detail headers are included into multiple standard headers. When compiling to a header unit, the declarations are bound to multiple units resulting in mismatches. It is solvable by explicitly specifying -fmodule-file="included-header.pcm" for every header included into each header unit. This is very cumbersome to do, will break when header includes change (e.g. granularization of includes) and is very slow to compile. I assume this isn't meant to work without P1502R1 (Standard Library Header units for C++20).

The best current workaround I could come up with was to include all of the standard library headers into one header std.hpp and using this one as the only header unit for the standard library. By force importing it with a header import_std.hpp
import <std.hpp>;

and

--include/path/to/import_std.hpp -fmodule-file=std.hpp.pcm

into third party files, the include guards exported by std.hpp.pcm will disable all includes of the standard library and use the header unit instead.
Not the most elegant solution, but a working one regardless (at least for open source).

This addresses part of #60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704

ChuanqiXu9 · 2023-02-03T02:11:47Z

@kaimfrai @Arthapz Could you help to test if the issue remains? Then we can close the issue.

This addresses part of llvm/llvm-project#60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704 (cherry picked from commit cdd44e2)

This addresses part of llvm/llvm-project#60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704

This addresses part of llvm/llvm-project#60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704 (cherry picked from commit cdd44e2)

Arthapz · 2023-02-03T11:35:03Z

@kaimfrai @Arthapz Could you help to test if the issue remains? Then we can close the issue.

works on my side

kaimfrai · 2023-02-03T12:31:10Z

No issues here. Tested libc++, libstdc++, C++20 and C++2b mode.
Thank you!

This addresses part of llvm/llvm-project#60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704

definitions in header units Address part of llvm/llvm-project#60079. Since the the declaration of a non-inline static data member in its class definition is not a definition. The following form: ``` class A { public: static const int value = 43; }; ``` should be fine to appear in a header unit. From the perspective of implementation, it looks like we simply forgot to check if the variable is a definition... Reviewed By: iains Differential Revision: https://reviews.llvm.org/D141905

Address part of llvm/llvm-project#60079. Deleted and Defaulted functions are implicitly inline, but that state is not set at the point that we perform the diagnostic checks for externally- visible non-inline functions; check the function body type explicitly in the diagnostic. Differential Revision: https://reviews.llvm.org/D141908

@kaimfrai

…owed in header units Summary: These is no sense to report this cases as an error or add `inline` explicitly in this cases. If it is not required in normal headers. Similar to llvm#60079. Test Plan: check-clang Reviewers: @kaimfrai, @ChuanqiXu9 Subscribers: @iains, @EugeneZelenko, @dwblaikie, @Arthapz

…lowed in header units (#98309) Summary: There is no sense to report these cases as an error or add `inline` explicitly in these cases, if it is not required in normal headers. Similar to #60079. Test Plan: check-clang

…lowed in header units (llvm#98309) Summary: There is no sense to report these cases as an error or add `inline` explicitly in these cases, if it is not required in normal headers. Similar to llvm#60079. Test Plan: check-clang

This addresses part of llvm/llvm-project#60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704

github-actions bot added the new issue label Jan 16, 2023

EugeneZelenko added clang:modules C++20 modules and Clang Header Modules and removed new issue labels Jan 16, 2023

dwblaikie changed the title ~~[C++20][Modules] Standard header units impractical since ban of non-inline external definitions~~ [C++20][Modules] deleted definitions should be allowed in header units Jan 16, 2023

ChuanqiXu9 self-assigned this Jan 17, 2023

iains added a commit that referenced this issue Feb 2, 2023

[C++20][Modules] Handle template declarations in header units.

cdd44e2

This addresses part of #60079 The test for external functions was not considering function templates. Differential Revision: https://reviews.llvm.org/D142704

ChuanqiXu9 mentioned this issue Feb 3, 2023

[C++20] [Modules] backport fixes to header units regressions to 16.x #60498

Closed

kaimfrai closed this as completed Feb 3, 2023

dmpolukhin mentioned this issue Jul 10, 2024

[C++20][Modules] static data members of template classes should be allowed in header units #98309

Merged

[C++20][Modules] deleted definitions should be allowed in header units #60079

[C++20][Modules] deleted definitions should be allowed in header units #60079

Comments

kaimfrai commented Jan 16, 2023 • edited by VoltrexKeyva Loading

libstdc++

libc++

llvmbot commented Jan 16, 2023

dwblaikie commented Jan 16, 2023

dwblaikie commented Jan 16, 2023

ChuanqiXu9 commented Jan 17, 2023 • edited Loading

ChuanqiXu9 commented Jan 17, 2023

iains commented Jan 17, 2023 • edited Loading

iains commented Jan 17, 2023

ChuanqiXu9 commented Jan 17, 2023 • edited Loading

ChuanqiXu9 commented Jan 17, 2023

iains commented Jan 17, 2023

ChuanqiXu9 commented Jan 17, 2023

ChuanqiXu9 commented Jan 17, 2023

iains commented Jan 17, 2023 • edited Loading

iains commented Jan 17, 2023

ChuanqiXu9 commented Jan 17, 2023 • edited Loading

ChuanqiXu9 commented Jan 17, 2023 • edited Loading

iains commented Jan 17, 2023

iains commented Jan 17, 2023

kaimfrai commented Jan 17, 2023

iains commented Jan 17, 2023

ChuanqiXu9 commented Jan 18, 2023

kaimfrai commented Jan 18, 2023 • edited Loading

ChuanqiXu9 commented Jan 19, 2023 • edited Loading

iains commented Jan 28, 2023

Arthapz commented Jan 28, 2023

Arthapz commented Jan 28, 2023

iains commented Jan 28, 2023

Arthapz commented Jan 28, 2023 • edited Loading

iains commented Jan 28, 2023 • edited Loading

Arthapz commented Jan 28, 2023 • edited Loading

iains commented Jan 28, 2023

Arthapz commented Jan 28, 2023

iains commented Jan 28, 2023

Arthapz commented Jan 28, 2023 • edited Loading

kaimfrai commented Jan 29, 2023

ChuanqiXu9 commented Feb 3, 2023

Arthapz commented Feb 3, 2023 • edited Loading

kaimfrai commented Feb 3, 2023

kaimfrai commented Jan 16, 2023 •

edited by VoltrexKeyva

Loading

ChuanqiXu9 commented Jan 17, 2023 •

edited

Loading

iains commented Jan 17, 2023 •

edited

Loading

ChuanqiXu9 commented Jan 17, 2023 •

edited

Loading

iains commented Jan 17, 2023 •

edited

Loading

ChuanqiXu9 commented Jan 17, 2023 •

edited

Loading

ChuanqiXu9 commented Jan 17, 2023 •

edited

Loading

kaimfrai commented Jan 18, 2023 •

edited

Loading

ChuanqiXu9 commented Jan 19, 2023 •

edited

Loading

Arthapz commented Jan 28, 2023 •

edited

Loading

iains commented Jan 28, 2023 •

edited

Loading

Arthapz commented Jan 28, 2023 •

edited

Loading

Arthapz commented Jan 28, 2023 •

edited

Loading

Arthapz commented Feb 3, 2023 •

edited

Loading