This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules] Build module static initializers per P1874R1.
ClosedPublic

Authored by iains on May 23 2022, 2:14 AM.

Details

Summary

Currently we only implement this for the Itanium ABI since the correct
mangling for the initializers in other ABIs is not yet known.

Intended result:

For a module interface [which includes partition interface and implementation
units] (instead of the generic CXX initializer) we emit a module init that:

  • wraps the contained initializations in a control variable to ensure that the inits only happen once, even if a module is imported many times by imports of the main unit.
  • calls module initializers for imported modules first. Note that the order of module import is not significant, and therefore neither is the order of imported module initializers.
  • We then call initializers for the Global Module Fragment (if present)
  • We then call initializers for the current module.
  • We then call initializers for the Private Module Fragment (if present)

For a module implementation unit, or a non-module TU that imports at least one
module we emit a regular CXX init that:

  • Calls the initializers for any imported modules first.
  • Then proceeds as normal with remaining inits.

For all module unit kinds we include a global constructor entry, this allows
for the (in most cases unusual) possibility that a module object could be
included in a final binary without a specific call to its initializer.

Implementation:

  • We provide the module pointer in the AST Context so that CodeGen can act on it and its sub-modules.
  • We need to account for module build lines like this: clang -cc1 -std=c++20 Foo.pcm -emit-obj -o Foo.o or clang -cc1 -std=c++20 -xc++-module Foo.cpp -emit-obj -o Foo.o
  • in order to do this, we add to ParseAST to set the module pointer in the ASTContext, once we establish that this is a module build and we know the module pointer. To be able to do this, we make the query for current module public in Sema.
  • In CodeGen, we determine if the current build requires a CXX20-style module init and, if so, we defer any module initializers during the "Eagerly Emitted" phase.
  • We then walk the module initializers at the end of the TU but before emitting deferred inits (which adds any hidden and static ones, fixing https://github.com/llvm/llvm-project/issues/51873 ).
  • We then proceed to emit the deferred inits and continue to emit the CXX init function.

Diff Detail

Event Timeline

iains created this revision.May 23 2022, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 2:14 AM
iains added a subscriber: Restricted Project.
iains edited the summary of this revision. (Show Details)May 23 2022, 2:22 AM
iains edited the summary of this revision. (Show Details)
iains edited the summary of this revision. (Show Details)
iains edited the summary of this revision. (Show Details)May 23 2022, 2:24 AM
iains edited the summary of this revision. (Show Details)
iains updated this revision to Diff 431313.May 23 2022, 2:33 AM

just updated description

iains published this revision for review.May 23 2022, 5:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 5:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
urnathan added inline comments.May 23 2022, 5:53 AM
clang/lib/CodeGen/CGDeclCXX.cpp
626–627

'at this time' is ambiguous. I think it means 'the current compiler implementation state', but it could also mean 'at this point in the compilation'. I don't think there's a general problem with such an optimization -- we could emit a flag into the BMI. It's just we don't have the smarts to do that just yet. right?

629

I'm with Morse about this, even before coming to live where I do. You've used both 'ise' and 'ize'. s/ise/ize/

ChuanqiXu added inline comments.May 26 2022, 12:15 AM
clang/include/clang/AST/ASTContext.h
477

The name PrimaryModule looks confusing. PrimaryModule looks like primary module interface unit to me. I think it refers to current module unit only. I think it is better to rename to CurrentModuleUnit.

Then why is it mutable? I don't find it is changed in a const member function.

1086
clang/lib/CodeGen/CGDeclCXX.cpp
621

Recommend to comment related part in your summary. It should be much helpful for other to read the codes.

645–646

mangleModuleInitializer is virtual function and we don't need to cast it.

671

I find the process here for PrioritizedCXXGlobalInits is much simpler than the one done in CodeGenModule::EmitCXXGlobalInitFunc(). It would generate more global init function. Doesn't it matter?

684

Given CodeGenModule is designed as ABI independent, I feel better to avoid see Itanium ABI in this function. (Although we don't have other ABI for modules now..)

692–693
699

Should the Guard be internal linkage? I image that it is possible to be manipulated by different TUs. So I feel like it might be better to be linkonce or linkonce_odr?

706–722

The codes look highly similar to CodeGenModule::EmitCXXGlobalInitFunc. I feel it is better to hoist a new function to avoid repeating the same logic twice.

867

This looks odd since it might be possible to not handling modules. But I understand it might be time-consuming if we want to use CXXGloablInits and we want ModuleInits to be front.

clang/lib/CodeGen/CodeGenModule.cpp
2521

Nit: I feel better to add methods like Module *Module::getGlobalModuleFragment(). I feel odd to use magic string here. But I am OK with it since there many magic string in CodeGen codes and we got in consensus before that we would need to do a lot of refactor work in the future.

2535

Nit: with above: Module *Module::getPrivateModuleFragment().

2979–2981
3254–3255

Is this change needed? Could you elaborate more on this?

clang/lib/CodeGen/CodeGenModule.h
1603

Let's keep consistent with the following. Given CodeGen part uses LLVM IR tensely, clang::Module looks better than Module*

clang/test/CodeGen/module-intializer.cpp
57

I think it is necessary to check the load for the guard and the check.

173

It looks like we lack a test for PrivateModuleFragment.

ChuanqiXu added inline comments.May 26 2022, 12:31 AM
clang/lib/CodeGen/CGDeclCXX.cpp
699

Oh, I realized I'm wrong. Don't remind this one.

iains updated this revision to Diff 432241.May 26 2022, 4:02 AM
iains marked 14 inline comments as done.
iains added a comment.May 26 2022, 4:02 AM

address review comments

iains added a comment.May 26 2022, 4:03 AM

not sure why the Debian clang-format is complaining - I am using llvm-14's clang-format with git clang-format + arc.

clang/include/clang/AST/ASTContext.h
477

The name PrimaryModule looks confusing. PrimaryModule looks like primary module interface unit to me. I think it refers to current module unit only. I think it is better to rename to CurrentModuleUnit.

I think we want to be clear that this is the Top Level Module (and not some dependent or sub-module) - so I have renamed to "TopLevelModule".

Then why is it mutable? I don't find it is changed in a const member function.

Ah well caught; the implementation went through several iterations and I missed to remove this.

clang/lib/CodeGen/CGDeclCXX.cpp
621

Hmm. I am not sure exactly what you mean here - there is a comment about the purpose of the method, where the method is declared.
The commit message describes what this method does in the first part. I'm happy to make things more clear, but not sure where you want to see some change,

626–627

yeah, I meant "as currently implemented" - adding flags to the BMI attracts concomitant churn in the serialisation, so probably better done separately. Amended the comment.

629

tools tools ... my editor wants "ise" unless I remember to set it to USian .. and sometimes I miss a change,, hopefully will catch them all as I go.

645–646

Actually, 'mangleModuleInitializer' is not a virtual function of the 'MangleContext' class, only of the 'ItaniumMangleContext'.
(see D122741).

This because we do not know the mangling for MS and, in principle, there might never be such a mangling (e.g. that implementation might deal with P1874 in some different way).

I did have a version of this where I amended the mangling to make the function virtual in the 'MangleContext' class, but then that means generating a dummy MS version that, as noted above, might never exist in reality.

671

In the general CXX initializer the prioritized inits are grouped into functions named in a way that allows them to be collated by other tools (e.g. the static linker) so that init priority can be honoured over multiple TUs.

This is not feasible for module initializers, there is no way to fuze them across TUs (it would have no meaning). So I think all we need to do here is to emit the calls in the correct order (in the end there are no more initializer functions, we just skip the intermediated grouping functions,

Of course, init priority is not mentioned in the standard, so that really what would matter is that compiler 'vendors' agree on a sensible approach to make sure code behaves in an expected manner for common toolchains.

684

Until we have an answer on MS's approach to this, I think it is better to be clear that this only applies to Itanium ABI uses (but if consensus is to remove this, I can do so).

692–693

we need the cast, see above.

706–722

actually, I had considered trying to factor this a bit, but it looks kind of artificial (will consider a second iteration).

867

Yes - otherwise we end up inserting a bunch of stuff, the code is not too long, and the pre-pending is described at the stage it is done.

clang/lib/CodeGen/CodeGenModule.cpp
2521

yeah, I was intending to do this actually - although it only moves the magic strings to a different place. It might be that we have enough bits in the module type enumerator (already streamed) so that we could use up two values with "GMF" and "PMF" module types. However IMO that should be a separate change.

3254–3255

the test is quite heavy, and is used twice when the assertions are enabled (I do not mind to revert this part if that does not seem worthwhile).

clang/test/CodeGen/module-intializer.cpp
57

can you say why?
the main purpose here is to check that we have the right mangled symbols (and some check that there is useful ordering). In what case would you think code-gen could emit the store and somehow not the other parts?

173

yeah, needs to be a second test since we cannot have PMF in a module with partitions.
added one.

ChuanqiXu added inline comments.May 26 2022, 8:15 PM
clang/lib/CodeGen/CGDeclCXX.cpp
621

I mean the paragraph:

For a module (instead of the generic CXX initializer) we emit a module init
that:

- wraps the contained initializations in a control variable to ensure that the inits only happen once, even if a module is imported many times by imports of the main unit.
- calls module initialisers for imported modules first. Note that the order of module import is not significant, and therefore neither is the order of imported module initializers.
- We then call initializers for the Global Module Fragment (if present)
- We then call initializers for the current module.
- We then call initializers for the Private Module Fragment (if present)

I understand people might feel like this is wordy. But, personally, I prefer readability.

645–646

I was just afraid of people might blame us to bring ABI specific things to CodeGen* things. I found there similar uses in CGVTables.cpp and CGVTT.cpp. So this might be acceptable too.

671

Yeah, the prioritized inits are really odd fashion to me.. I only saw such things in assembly. So my understanding to this one is that:
(1) If we have prioritized inits in modules' code , it is not guaranteed to be initialized first. This is the vendors agreement.
(2) If we link a static library (e.g., libgcc.a), the prioritized inits in that would still be initialized first.

Do I understand right?

clang/lib/CodeGen/CodeGenModule.cpp
2521

We still need to use auto * instead of auto here. I remember this is recommended in clang's coding standards.

2535

Use auto* here.

3254–3255

I prefer to do such changes in a standalone NFC patch.

iains added inline comments.May 27 2022, 3:35 AM
clang/lib/CodeGen/CGDeclCXX.cpp
621

so, to clarify - you would like me to add this description as a comment block on the new initializer method (I am OK with doing this)

ChuanqiXu added inline comments.May 27 2022, 5:36 AM
clang/lib/CodeGen/CGDeclCXX.cpp
621

Yeah, I feel it is helpful.

iains updated this revision to Diff 433049.May 31 2022, 4:05 AM
iains marked 7 inline comments as done.

rebased, addressed review comments.

clang/lib/CodeGen/CGDeclCXX.cpp
645–646

yes, let us see - if there are any other opinions - personally, I prefer not to generate a dummy function in the MS code.

671

We should avoid side-tracking this patch with too much discussion of the problems of C++ global initialisers. Behaviour of things like static libraries depends on the toolchain, binary container, platform ....

  1. prioritised initializers are not part of the standard
  2. they are optional (some toolchains do not handle them at all - e.g. macOS)
  3. where they are handled, usually the toolchain makes no guarantees about behaviour between TUs
  4. (for a regular ELF-style C++ global init) there is a convention to group initializers for a specific priority into a function with a specific mangled name that allows (optionally) external tools - like the linker -to group / order the inits _between_ TUs.

None of this is relevant to Module initializers, we just have to ensure that imported module inits are run before the current module's.

clang/lib/CodeGen/CodeGenModule.cpp
3254–3255

OK .. fair enough, reverted for now.

ChuanqiXu accepted this revision.May 31 2022, 7:01 PM

LGTM, thanks!

This revision is now accepted and ready to land.May 31 2022, 7:01 PM
iains updated this revision to Diff 434003.Jun 3 2022, 5:26 AM
iains edited the summary of this revision. (Show Details)

rebased and updated

previously, this was not doing the right thing for module implementation units
which were being processed as if they were interfaces, so we've introduced a module
implemetation type, and that is now checked for in the emitting of the inits.

iains updated this revision to Diff 434781.Jun 7 2022, 4:57 AM

rebased

iains updated this revision to Diff 436487.Jun 13 2022, 11:20 AM

rebased. Amended to include a global CTOR entry for each module kind.

@urnathan - I believe that this now implements the same scheme as you have done for GCC (less any of the optimisations).
In particular, we now emit global CTOR entries for module initializers, even though these should really be called explicitly, but as was discussed on the core/sg15 reflectors it is possible (at least for unix-like systems) to construct cases where module objects are included in a final binary without their initializers being called.

please sed /initialiser/initializer/, I noticed a few had crept in.

@iains may I ask what's the issue to not land this? It looks like you're waiting for the behavior to be consistency with GCC?

Since this patch could fix https://github.com/llvm/llvm-project/issues/51873, which breaks the users to compile a hello world example.

@iains may I ask what's the issue to not land this? It looks like you're waiting for the behavior to be consistency with GCC?

Since this patch could fix https://github.com/llvm/llvm-project/issues/51873, which breaks the users to compile a hello world example.

I need to make some typo corrections; there is no issue (I'm not waiting for anything) just prioritising posting patches to complete C++20 support .. will land this soon.

iains updated this revision to Diff 440924.Jun 29 2022, 3:41 AM

rebased, corrected some spellings.

iains edited the summary of this revision. (Show Details)Jun 29 2022, 3:42 AM

please sed /initialiser/initializer/, I noticed a few had crept in.

should be done now.

ChuanqiXu added a comment.EditedJun 30 2022, 10:44 PM

@iains may I ask what's the issue to not land this? It looks like you're waiting for the behavior to be consistency with GCC?

Since this patch could fix https://github.com/llvm/llvm-project/issues/51873, which breaks the users to compile a hello world example.

I need to make some typo corrections; there is no issue (I'm not waiting for anything) just prioritising posting patches to complete C++20 support .. will land this soon.

Thanks! The reason why I am a little bit hurry is that I'm trying to implement std modules: https://github.com/ChuanqiXu9/stdmodules

Now it runs good for libcxx. But it would meet segfault in https://github.com/ChuanqiXu9/stdmodules/blob/master/test/HelloWorld/HelloWorld.cpp for libstdc++ if we don't land this revision.

This revision was landed with ongoing or failed builds.Jul 9 2022, 1:09 AM
This revision was automatically updated to reflect the committed changes.

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

Undefined symbols for architecture x86_64:
  "__ZGIW10std_config", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW4span", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5array", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5stdio", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6string", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6vector", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/

iains added a comment.Jul 11 2022, 9:39 AM

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

Undefined symbols for architecture x86_64:
  "__ZGIW10std_config", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW4span", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5array", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5stdio", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6string", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6vector", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/

ack, (I develop on macOS so usually catch these things)...
do you need me to revert the commit - or can we try to find out what's broken and fix it?

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

Undefined symbols for architecture x86_64:
  "__ZGIW10std_config", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW4span", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5array", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW5stdio", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6string", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o
  "__ZGIW6vector", referenced from:
      __GLOBAL__sub_I_main.cpp in main.o

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/

ack, (I develop on macOS so usually catch these things)...
do you need me to revert the commit - or can we try to find out what's broken and fix it?

If you think you'll be able to get to the bottom of this quickly then I don't think we need to revert, but the bot has been red for a while (which means that other issues can get buried) so I'd like to get it green ASAP.

iains added a comment.Jul 11 2022, 9:42 AM

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

Undefined symbols for architecture x86_64:

<snip>..

"__ZGIW6vector", referenced from:
    __GLOBAL__sub_I_main.cpp in main.o
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/

ack, (I develop on macOS so usually catch these things)...
do you need me to revert the commit - or can we try to find out what's broken and fix it?

hmm there seems to be a compiler error, which looks somewhat unrelated to the active patch:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp:125:27: error: no type named 'size_t' in namespace 'std'
  void* operator new(std::size_t, T) {
                     ~~~~~^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp:132:29: error: no type named 'size_t' in namespace 'std'
  void* operator new[](std::size_t, T) {
iains added a comment.Jul 11 2022, 9:58 AM

This breaks TestDataFormatterLibcxxSpan.py on GreenDragon:

....

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45207/

ack, (I develop on macOS so usually catch these things)...
do you need me to revert the commit - or can we try to find out what's broken and fix it?

If you think you'll be able to get to the bottom of this quickly then I don't think we need to revert, but the bot has been red for a while (which means that other issues can get buried) so I'd like to get it green ASAP.

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f and 4328b960176f4394416093e640ad4265bde65ad7 locally and I'm still getting a linker error about missing symbols:

Undefined symbols for architecture arm64:
  "vtable for std::__1::format_error", referenced from:
      std::__1::format_error::format_error(char const*) in main.o
      std::__1::format_error::format_error(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in main.o

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f and 4328b960176f4394416093e640ad4265bde65ad7 locally and I'm still getting a linker error about missing symbols:

those refs come up as 'bad object' for me .. can you identify the upstream changes?

Undefined symbols for architecture arm64:
  "vtable for std::__1::format_error", referenced from:
      std::__1::format_error::format_error(char const*) in main.o
      std::__1::format_error::format_error(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in main.o

That seems also unrelated to the modules code, but I could always be surprised :)

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f and 4328b960176f4394416093e640ad4265bde65ad7 locally and I'm still getting a linker error about missing symbols:

those refs come up as 'bad object' for me .. can you identify the upstream changes?

That's odd, both Github and Phab think those are the canonical hashes:

https://github.com/llvm/llvm-project/commit/ac507102d258b6fc0cb57eb60c9dfabd57ff562f
https://github.com/llvm/llvm-project/commit/4328b960176f4394416093e640ad4265bde65ad7

Undefined symbols for architecture arm64:
  "vtable for std::__1::format_error", referenced from:
      std::__1::format_error::format_error(char const*) in main.o
      std::__1::format_error::format_error(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in main.o

That seems also unrelated to the modules code, but I could always be surprised :)

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f and 4328b960176f4394416093e640ad4265bde65ad7 locally and I'm still getting a linker error about missing symbols:

those refs come up as 'bad object' for me .. can you identify the upstream changes?

That's odd, both Github and Phab think those are the canonical hashes:

https://github.com/llvm/llvm-project/commit/ac507102d258b6fc0cb57eb60c9dfabd57ff562f
https://github.com/llvm/llvm-project/commit/4328b960176f4394416093e640ad4265bde65ad7

pilot error (pasto...)

Undefined symbols for architecture arm64:
  "vtable for std::__1::format_error", referenced from:
      std::__1::format_error::format_error(char const*) in main.o
      std::__1::format_error::format_error(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in main.o

That seems also unrelated to the modules code, but I could always be surprised :)

OK - so if I cannot figure out what is happening in the next couple or hours, I can revert those two commits (that's 9PM for me so I probably cannot do much more today).

JFTR, I did not get any notification from green dragon (which is odd, AFAIR it's sent email before) or I would have looked right away - kicked off a build will take a look as soon as that's done.

Yes, the bot was already red because of a different issue.

FWIW I tried reverting ac507102d258b6fc0cb57eb60c9dfabd57ff562f and 4328b960176f4394416093e640ad4265bde65ad7 locally and I'm still getting a linker error about missing symbols:

those refs come up as 'bad object' for me .. can you identify the upstream changes?

That's odd, both Github and Phab think those are the canonical hashes:

https://github.com/llvm/llvm-project/commit/ac507102d258b6fc0cb57eb60c9dfabd57ff562f
https://github.com/llvm/llvm-project/commit/4328b960176f4394416093e640ad4265bde65ad7

pilot error (pasto...)

Undefined symbols for architecture arm64:
  "vtable for std::__1::format_error", referenced from:
      std::__1::format_error::format_error(char const*) in main.o
      std::__1::format_error::format_error(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in main.o

That seems also unrelated to the modules code, but I could always be surprised :)

OK - so if I cannot figure out what is happening in the next couple or hours, I can revert those two commits (that's 9PM for me so I probably cannot do much more today).

Thanks, if the bot reproduces the other issue I mentioned I'll bisect it down to see if there's another commit that's causing issues. I should've mentioned that that failures only happens when building with -gmodules, so it does sound somewhat related.

iains added a comment.EditedJul 11 2022, 11:54 AM

I need to do some more builds to be able to reproduce this - my guess (at present) is that this is a manifestation of '-fcxx-modules -std=c++20' being almost, but not exactly, the same as C++20 standardised modules. It is possible that the -gmodules flag interacts with that (although one might have expected more than one fail if that were so).

edit: as far as the effort to implement standardised C++20 modules goes, there have been no modifications -gmodules; the intent has been all along to leave clang modules (and c++modules) operating "as before".

hmm there seems to be a compiler error, which looks somewhat unrelated to the active patch:

/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp:125:27: error: no type named 'size_t' in namespace 'std'
  void* operator new(std::size_t, T) {
                     ~~~~~^
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/cross-project-tests/debuginfo-tests/clang_llvm_roundtrip/simplified_template_names.cpp:132:29: error: no type named 'size_t' in namespace 'std'
  void* operator new[](std::size_t, T) {

Added the missing include here 4ca205855267191ccfd191539cf4b3ed792a4257

iains reopened this revision.Jul 15 2022, 8:14 AM

reopening to post the patch I intend to land.

This revision is now accepted and ready to land.Jul 15 2022, 8:14 AM
iains updated this revision to Diff 444989.Jul 15 2022, 8:16 AM

rebased, fixed the interaction with clang module map modules.

Is it realistic for this to land before LLVM 15 branches? Would be great!

iains added a comment.Jul 19 2022, 4:09 AM

Is it realistic for this to land before LLVM 15 branches? Would be great!

that is the intention - I just got side-tracked with trying to replicate the test fails that got the commit reverted. AFAICT it's OK now, and i plan to re-land it soon.

I feel like we could land this sooner to avoid any unimagined failures.