Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusing "module file out of date" errors with C++20 modules #70569

Closed
rnikander opened this issue Oct 28, 2023 · 7 comments
Closed

Confusing "module file out of date" errors with C++20 modules #70569

rnikander opened this issue Oct 28, 2023 · 7 comments
Labels
clang:modules C++20 modules and Clang Header Modules duplicate Resolved as duplicate

Comments

@rnikander
Copy link

I will attach a zipped up project, with some source code and a Makefile. At the top of the Makefile, there are some variables that point to my build of clang. Those need to be changed, but then I hope you can reproduce this.

files.tgz

The commands in the Makefile are my attempt to follow the docs here.

Steps to demonstrate the issue

  1. Change this top part of the Makefile to fit your build of clang. I'm building on a Mac so I need that macos_sdk setting.
CXX = /Users/rob/Dev/llvm-project/build_phase1/bin/clang++
macos_sdk = /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk
flags = -isysroot $(macos_sdk) -std=c++2b
  1. Run make, and you should see this output.
% make
Precompiling interface_part1.ccm -> interface_part1.pcm
Precompiling interface_part2.ccm -> interface_part2.pcm
Precompiling foo.ccm -> foo.pcm
Compiling main.cc -> main.o
Compiling foo.pcm -> foo.o
Compiling interface_part1.pcm -> interface_part1.o
Compiling interface_part2.pcm -> interface_part2.o
Linking program
  1. You can run the program to confirm that it compiled.
% ./program 
Starting main()
hello from foo:interface_part1
hello from foo:interface_part2
  1. Now, change the variable name in the other() function in interface_part1.ccm. For example change int a to int aa.
void other() {
    int aa = 1;
}
  1. Run make again. I see this:
% make     
Precompiling interface_part1.ccm -> interface_part1.pcm
Precompiling foo.ccm -> foo.pcm
foo.ccm:5:8: fatal error: module file 'interface_part1.pcm' is out of date and needs to be rebuilt: module file out of date
    5 | export import :interface_part2;
      |        ^
foo.ccm:5:8: note: imported by module 'foo:interface_part2' in 'interface_part2.pcm'

This seems like a problem to me, for a couple reasons:

  1. The error is misleading. It says interface_part1.pcm is out of date and "needs to be rebuilt", but I just rebuilt it.
  2. If interface_part2.pcm needs to be rebuilt, because it depends on part1, that seems like these modules are worse than headers at hiding implementation details. I changed a private non-exported function other(), and I changed an implementation detail - the name of the local variable. The exposed interface of interface_part1 should not change in this case. And I don't think it should trigger the need to recompile things that depend on it.

It's interesting that if I only change the value of int a in other(), say int a = 1 to int a = 2, then then make rebuilds things as expected without a problem.

@EugeneZelenko EugeneZelenko added clang:modules C++20 modules and Clang Header Modules and removed new issue labels Oct 29, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 29, 2023

@llvm/issue-subscribers-clang-modules

Author: None (rnikander)

I will attach a zipped up project, with some source code and a Makefile. At the top of the Makefile, there are some variables that point to my build of clang. Those need to be changed, but then I hope you can reproduce this.

files.tgz

The commands in the Makefile are my attempt to follow the docs here.

Steps to demonstrate the issue

  1. Change this top part of the Makefile to fit your build of clang. I'm building on a Mac so I need that macos_sdk setting.
CXX = /Users/rob/Dev/llvm-project/build_phase1/bin/clang++
macos_sdk = /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk
flags = -isysroot $(macos_sdk) -std=c++2b
  1. Run make, and you should see this output.
% make
Precompiling interface_part1.ccm -> interface_part1.pcm
Precompiling interface_part2.ccm -> interface_part2.pcm
Precompiling foo.ccm -> foo.pcm
Compiling main.cc -> main.o
Compiling foo.pcm -> foo.o
Compiling interface_part1.pcm -> interface_part1.o
Compiling interface_part2.pcm -> interface_part2.o
Linking program
  1. You can run the program to confirm that it compiled.
% ./program 
Starting main()
hello from foo:interface_part1
hello from foo:interface_part2
  1. Now, change the variable name in the other() function in interface_part1.ccm. For example change int a to int aa.
void other() {
    int aa = 1;
}
  1. Run make again. I see this:
% make     
Precompiling interface_part1.ccm -> interface_part1.pcm
Precompiling foo.ccm -> foo.pcm
foo.ccm:5:8: fatal error: module file 'interface_part1.pcm' is out of date and needs to be rebuilt: module file out of date
    5 | export import :interface_part2;
      |        ^
foo.ccm:5:8: note: imported by module 'foo:interface_part2' in 'interface_part2.pcm'

This seems like a problem to me, for a couple reasons:

  1. The error is misleading. It says interface_part1.pcm is out of date and "needs to be rebuilt", but I just rebuilt it.
  2. If interface_part2.pcm needs to be rebuilt, because it depends on part1, that seems like these modules are worse than headers at hiding implementation details. I changed a private non-exported function other(), and I changed an implementation detail - the name of the local variable. The exposed interface of interface_part1 should not change in this case. And I don't think it should trigger the need to recompile things that depend on it.

It's interesting that if I only change the value of int a in other(), say int a = 1 to int a = 2, then then make rebuilds things as expected without a problem.

@ChuanqiXu9
Copy link
Member

If interface_part2.pcm needs to be rebuilt, because it depends on part1,

Yes, this is the reason.

I changed a private non-exported function other()

This is not strictly correct. It is true that other() is not exported. But it is not true that other() is private. Technically, other() is reachable to the units that import foo.

It's interesting that if I only change the value of int a in other(), say int a = 1 to int a = 2, then then make rebuilds things as expected without a problem.

This looks like an overlook in the current design intentions. Or maybe other() is not actually get called.


To me, the root cause of the issue may be the expectation/consensus of hiding implementation details. And the topic involves BMI format/optimization/ABI. For example, for the following example:

//--- foo.cppm
export module foo;
int foo_impl() {
    return 43;
}
export int foo() {
    return foo_impl();
}

//--- a.cpp
import foo;
int a() {
    return foo();
}

With optimization enabled, the generated code of a.o may look like:

int a() {
    return 43;
}

Yes, the implementation detail of foo_impl() is actually leaked. Then in this case, if we change the implementation of foo_impl() to return 44;. It is a problem that we don't recompile a.cpp.

It is technically available to not leak the implementation of foo_impl() in this case. But it will prevent all the inter procedure optimization across units. Then it'll hurt the performance. And the runtime performance is a key factor of C++ project nowadays.

Also it is a problem of the ABI. If the non-exported function is an inline function, we have to generate the function body in the calling unit. Otherwise we're violating the ABI.

And the final step may be format design/implementation, which is the direct reason for this issue. But I guess we can't solve the underlying issue if we don't solve the first 2 issues.

@rnikander
Copy link
Author

rnikander commented Oct 29, 2023

Thanks @ChuanqiXu9, what you explained there makes sense to me. I see why the non-exported function causes a recompile, in general ... so I think it's mainly the error message that's confusing me here.

And it would be good to know how to hide the other() function, in cases where that is desired, because sometimes I care less about optimization, and more about hiding the implementation details and reducing compilation dependencies. In older C++ this would mean leaving things out of the header. For example, the "pimpl" idiom.

@ChuanqiXu9
Copy link
Member

Thanks @ChuanqiXu9, what you explained there makes sense to me. I see why the non-exported function causes a recompile, in general ... so I think it's mainly the error message that's confusing me here.

And it would be good to know how to hide the other() function, in cases where that is desired, because sometimes I care less about optimization, and more about hiding the implementation details and reducing compilation dependencies. In older C++ this would mean leaving things out of the header. For example, the "pimpl" idiom.

Currently, maybe we can only put that into the implementation units.

@ChuanqiXu9
Copy link
Member

This may be covered by #71034. Let's track the underlying issue there.

@ChuanqiXu9
Copy link
Member

Let's track this in #71618.

@EugeneZelenko EugeneZelenko added the duplicate Resolved as duplicate label Nov 8, 2023
@EugeneZelenko EugeneZelenko closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2023
@ChuanqiXu9
Copy link
Member

@rnikander FYI, see the newest comments in #71618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:modules C++20 modules and Clang Header Modules duplicate Resolved as duplicate
Projects
None yet
Development

No branches or pull requests

4 participants