although this was approved, we did not need to use it to implement the dependent changes.

this is now
https://github.com/llvm/llvm-project/pull/68076

sorry I did not get to moving this yet - but will try during this week.

In D158769#4632875, @philnik wrote:

Since we are moving to GitHub PRs and there hasn't been any activity here yet, could you move this patch over?

Some background since it is relevant to the testing done so far.

on the subject of is there any way to diagnose:

• if we encounter either a linker-visible or global symbol after a cfi_startproc but before the next cfi_endproc [in the same section], that would seem to indicate bad nesting?
• I suppose that there could be some weird code-gen that multiplexed the output between sections, so seeing a section switch before the closing cfi_endproc is not sufficient

In D126694#4470297, @ChuanqiXu wrote:

Yes, that was the decision at the last time we looked - because removing decls would degrade this - if we have new information that changes our preferred design, then fine.

I remember the major reason for the last time to not remove the decls are that the design of AST doesn't support to remove decls. And my current idea is, we can refuse to write the discardable Decls into the BMIs.

In D126694#4470261, @ChuanqiXu wrote:

That is clearly a big motivation - I will ask the folks we were talking to at WG21 if that is their priority - or maybe they care about language isolation etc.

Yeah, I know the folks in WG21 prefer the language isolation. But you know, there are many folks who are not in WG21...

In D126694#4470250, @ChuanqiXu wrote:

BTW, in my experience for talking about modules to users, they mainly/mostly care about the compilation performance. And I can't image how many people would like to use modules if they know they won't get a compilation performance win.

In D126694#4470139, @ChuanqiXu wrote:

Now I think the feature may be important for the performance of modules. And I feel we should work on the ASTWriter side instead of ASTReader side. Since the size of BMIs is a problem now also it shows that it is not free to load the large BMIs. So while it is semantical correct to work on the reader side, it is better for the performance to work on the writer side.

I'd like to finish the idea. And for the current patch, I'd like to refactor it a little bit:

1. Test it by unittest instead of by matching the dump result.

changes are needed to address review comments - but this revision is needed as a parent to other work (so might need to be rebased from time to time)

rebased, fixed some format issues; again to support p1815 work only.

just rebased to support p1815 work

need to re-check that the intention of the paper is covered (since we currently treat bare 'export' and 'export {}' in a similar manner).

rebased and fixed some formatting.

I think it is a good idea to keep this part of P2615R1 separate from the unnamed exports diagnostics [the parent commit] - it seems likely that there will be more fallout from this (given that there were already PRs about the behaviour of exported specialisations).

not sure why the debian CI is reported clang-format errors; I am not seeing them here.

rebased, addressed review comments.

In D152946#4431974, @h-vetinari wrote:

Mark P2615 as implemented in https://github.com/llvm/llvm-project/blame/main/clang/www/cxx_status.html?

In D145965#4431997, @ChuanqiXu wrote:

It is a case that we have supported; the user puts in a use of a decl but forgets to import the module exporting it (I agree it is not _exactly_ a "typo" in terms of names, but the diagnostics counts it in the same way)

I got your point. But I prefer to implement an all-visible mode (not available for users) for such situations. And I still think it is not problem for the special case. Since this is patch is working for internal decls.

I will look at the rest of the comments once back in the office.

In D145965#4431846, @ChuanqiXu wrote:

if we do not adjust the typo fixes, we will regress diagnostics.

What the kind of diagnostics will be regressed? I mean, it looks weird to me that we suggest typo fixes from hidden names.

thanks to Daniela Engert for reporting this and helping identify the current WG21 status on the topic.

In D146614#4416300, @ChuanqiXu wrote:

BTW, there is a case that I am not sure if it is valid about module linkage.

// Part1.cppm
export module TheMod:Part1;
int loog = 1;

// Part2.cppm
export module TheMod:Part2;
int loog = 2;

// TheMod.cppm
export module TheMod;
export import :Part1;
export import :Part2;

int loog = 3;
export int V = loog;

Here all the loog have modules linkage. And is this case an ODR violation or not?

rebased, added testcase for Issue 61601.

In D146614#4416292, @ChuanqiXu wrote:

Hopefully, we can combine them to resolve both the issue and the lookup blocker.

(didn't start to look at the code yet)

I feel like we can't solve https://github.com/llvm/llvm-project/issues/61601 without touching the deserializer.

// Part1.cppm
export module TheMod:Part1;
static int loog = 1;

// Part2.cppm
export module TheMod:Part2;
static int loog = 2;

// TheMod.cppm
export module TheMod;
export import :Part1;
export import :Part2;

static int loog = 3;
export int V = loog;

I feel the compiler will still emit the ODR violation issue.

although I would welcome input from the ABI owners, testing shows that this patch does appear to DTRT and brings clang in line with the operation of GCC in this area. It would be good to land it (or decide what other action is needed) before 17 branches

many thanks to Daniela Engert from bringing this to my attention (and providing a reproducer test case).

In D146614#4407954, @ChuanqiXu wrote:

It would be good to do so - but we need to make sure that the workaround does not break any of the cases (esp. the ADL in instantiation).

Yeah, this is the problem. It looks like my solution in brain would break the ADL example you gave. But I am not sure if your lookup patch could solve https://github.com/llvm/llvm-project/issues/61807.

about the comments that the patch seems to do multiple things; I do not think we can fix the lookup without touching the typo-fixes since it uses the same underlying machinery - if we do not adjust the typo fixes, we will regress diagnostics.

In D146614#4407917, @ChuanqiXu wrote:

We are talking highly related but different things. My first point is on the product/user level.

I think the current status is:

• Clang17 is going to be branched and there is a significant issue we haven't solved yet.
• We have a potential workaround for the issue. But the workaround is incorrect for some not-working-yet cases.
• Also the workaround would affect some other important and big feature implementations (your works). And if we decide to wait for such implementations, we're highly possible not able to get up for clang17 to fix the issues. One the one hand, we need spend a lot of time to test and review it.

rebased and adjusted for upstream changes

BTW, I am generally very uncomfortable with making the deserialisation process do more of the Sema work.

Actually, P1815 is almost complete (95%+ in the current patch and i am currently looking at the lambda) however P1815 is ultimately blocked by this (lookup) issue; That is, P1815 is not a solution to the lookup problem - for it to work lookup has to be fixed.

This looks reasonable to me - but the vtable stuff is not an area that I am familiar with, so I'll defer to the other reviewers.

in the end, we have to deal with the following cases:

rebased, addressed review comments (patch still needs refactoring)

I updated this because I am going to push the latest version of the P1815 patch and that depends on the lookup changes.

rebased

rebased, retested.

+1 for this as an interim solution.

It took me a while to get my local macOS based devt. environment to reproduce the problem - but it was as expected; the implementation module was unowned and nothing was deleting it.

rebased, reworked to have the module owned.

I had a hunch that the issue was the non-ownership of the implementation module.

In D126959#4222637, @bjope wrote:

This seem to case problems when building with asan enabled (LLVM_USE_SANITIZER='Address'):

although this patch does handle the cases needed, it really needs refactoring.
posting here since we are both working in this area and this might be useful input.

rebased, split the changes to module and private linkage out,

rebased, addressed review comments, and amended the description.

I agree it is complex

• I think it needs more than is in this patch
• and there are different rules applied during lookup for typos (which I have had to handle specially, otherwise we get useless suggestions)

rebased.

General comments (at least my opinion).

This was originally created (and @ChuanqiXu approved) for the work on module initialisers. I did not apply it then, since it was possible to determine the correct state for the initialisers without it.

rebased, and reworked for changes in main.

In D144844#4195633, @ChuanqiXu wrote:

However, "performance" also includes compilation speed in the 'no optimisation, debug' case - that is also considered very important. So, perhaps, the short-term approach should be (as @dblaikie suggested) to include the bodies for -O >= 3?

I don't think so. I think "performance" refers to the runtime performance generally. I don't believe the normal users will be happy if modules will decrease the performance of their program in any means. So I think we should include the bodies by default.

In D144844#4195316, @ChuanqiXu wrote:

Got your points. Let's postpone this one.

But I want to emphasize that this patch (and the thin PCM) will decrease the performance. While LTO can save the regression, LTO is not widely used. (ThinLTO can only mitigate this.) I mean the default behavior shouldn't cause performance regression. We can offer a new alternative for the users but we can't enable that by default simply now.

In D145965#4192051, @iains wrote:

In D144844#4193568, @dblaikie wrote:

Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.

In D145965#4192072, @ChuanqiXu wrote:

In D145965#4192051, @iains wrote:

In D145965#4191973, @ChuanqiXu wrote:

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring).

I should definitely check to make sure that this was adopted as a DR against C++20 (otherwise we would need to use the previous test conditionally on < 2b).

OK, I guess one extra sub-module is not too bad.
LGTM.

In D144367#4157589, @ChuanqiXu wrote:

In D144367#4155267, @iains wrote:

In D144367#4154225, @ChuanqiXu wrote:

@iains ping~ I want to land this one in the next week if no comments come in.

I will try to review during this week.

One quick question : does the revised module layout work OK with "-module-file-info" which should print the summary of the C++20 content in the first few lines.

Yeah, I've added the test for it.

In D144367#4154225, @ChuanqiXu wrote:

@iains ping~ I want to land this one in the next week if no comments come in.

OK - the visibility path is already complex, we do not need to make it more so.
I guess you are intending to rebase/update this?

I am wondering why we need to add another sub-module.

rebased, added tests for instantiated variable/function templates.

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

I think we need to find a way to proceed - because this causes a regression on the llvm-16 branch, and that should be resolved soon, if possible.
What is your suggestion for a way forward?

AFAICT the failing test is unrelated to this patch.

In D142704#4088217, @Arthapz wrote:

tried the patch and it seems to work with libstdc++ but not with libc++

<SNIP>
> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
<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;
}

in my local testing, I was able to consume all libc++ headers individually.

rebased, and revised to handle variable templates and instantiations.

this is necessary, but not sufficient (I need to make additions) .. no need to review yet.

@dblaikie - I suspect that this would be useful on the llvm-16 release branch, and so I've added you as a reviewer, if you have some chance to look ..
(I do not think @ChuanqiXu is available until February).