Shouldn't `-fmodules-embed-all-files` be the default?

[boris-kolpackov]

Consider these two translation units:

// hello.mxx

module;

#include <string>
#include <string_view>

export module hello;

export namespace hello
{
  std::string
  say_hello (const std::string_view& name)
  {
    return "Hello, " + std::string (name) + '!';
  }
}

// main.cxx

#include <string_view>

import hello;

void f (const std::string_view&) {}

int
main ()
{
  hello::say_hello ("World");
}

If I compile hello.mxx with Clang 16, then remove hello.mxx, and attempt to compile main.cxx, I get an error:

clang++-16 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-16 -std=c++2b -fmodule-file=hello=hello.pcm -o main.o -c -x c++ main.cxx
main.cxx:5:1: fatal error: malformed or corrupted AST file: 'could not find file '/tmp/hello-simple/hello.mxx' referenced by AST file 'hello.pcm''

This can be fixed by adding -Xclang -fmodules-embed-all-files when compiling hello.mxx.

However, -fmodules-embed-all-files appears to no longer be necessary for this example if using Clang 17 or later:

clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
# no error

But a minor change in main.cxx can bring its requirement back if compiling via -frewrite-includes:

// main.cxx

#include <string_view>

import hello;

void f (const std::string_view&) {}

int
main ()
{
  f (hello::say_hello ("World"));  // <-- NOW CALLING f().
}

clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
# no error

But:

clang++-17 -std=c++2b -x c++-module -E -frewrite-includes -o hello.ii hello.mxx
clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.ii
rm hello.ii
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
fatal error: cannot open file '/tmp/hello-simple1/hello.ii': No such file or directory

And if I add -Xclang -fmodules-embed-all-files to the second command, then everything again compiles fine. I get exactly the same behavior with Clang 18.

I have two questions about this:

1. Is -fmodules-embed-all-files the default starting from Clang 17? If the answer is yes, then there seems to be a bug in the -fdirectives-only interaction.

2. If -fmodules-embed-all-files is not the default, then should it not be made so? A BMI that still has references to source files is quite brittle since it can be moved around. AFAIK, neither GCC nor MSVC have this restriction for their BMIs.

@iains @ChuanqiXu9 @dwblaikie

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Boris Kolpackov (boris-kolpackov)

Details

Consider these two translation units:

// hello.mxx

module;

#include <string>
#include <string_view>

export module hello;

export namespace hello
{
  std::string
  say_hello (const std::string_view& name)
  {
    return "Hello, " + std::string (name) + '!';
  }
}

// main.cxx

#include <string_view>

import hello;

void f (const std::string_view&) {}

int
main ()
{
  hello::say_hello ("World");
}

If I compile hello.mxx with Clang 16, then remove hello.mxx, and attempt to compile main.cxx, I get an error:

clang++-16 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-16 -std=c++2b -fmodule-file=hello=hello.pcm -o main.o -c -x c++ main.cxx
main.cxx:5:1: fatal error: malformed or corrupted AST file: 'could not find file '/tmp/hello-simple/hello.mxx' referenced by AST file 'hello.pcm''

This can be fixed by adding -Xclang -fmodules-embed-all-files when compiling hello.mxx.

However, -fmodules-embed-all-files appears to no longer be necessary for this example if using Clang 17 or later:

clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
# no error

But a minor change in main.cxx can bring its requirement back if compiling via -frewrite-includes:

// main.cxx

#include <string_view>

import hello;

void f (const std::string_view&) {}

int
main ()
{
  f (hello::say_hello ("World"));  // <-- NOW CALLING f().
}

clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.mxx
mv hello.mxx hello.mxx.bak
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
# no error

But:

clang++-17 -std=c++2b -x c++-module -E -frewrite-includes -o hello.ii hello.mxx
clang++-17 -std=c++2b -fmodule-output=hello.pcm -o hello.pcm.o -c -x c++-module hello.ii
rm hello.ii
clang++-17 -std=c++2b -fmodule-file=hello=hello.pcm -o hello.o -c -x c++ main.cxx
fatal error: cannot open file '/tmp/hello-simple1/hello.ii': No such file or directory

And if I add -Xclang -fmodules-embed-all-files to the second command, then everything again compiles fine. I get exactly the same behavior with Clang 18.

I have two questions about this:

1. Is -fmodules-embed-all-files the default starting from Clang 17? If the answer is yes, then there seems to be a bug in the -fdirectives-only interaction.

2. If -fmodules-embed-all-files is not the default, then should it not be made so? A BMI that still has references to source files is quite brittle since it can be moved around. AFAIK, neither GCC nor MSVC have this restriction for their BMIs.

@iains @ChuanqiXu9 @dwblaikie

[dwblaikie]

For myself, I think not embedding the source is the right default. (the build depends on scattered source files on disk without modules, I'm OK with it depending on them (even indirectly through module files) with modules)
For Google - we embed source because of constraints of our distributed build system, and we're more than happy for our internal needs/uses to happen to match external users, so we'd benefit from the change in default to something that looked more like our situation.
Probably some performance numbers on the cost of doing this to demonstrate it's not a major cost (probably compile time/module generation time, and disk usage numbers would be useful) would be helpful in making this determination.

[ChuanqiXu9]

Is -fmodules-embed-all-files the default starting from Clang 17? If the answer is yes, then there seems to be a bug in the -fdirectives-only interaction.

No.

If -fmodules-embed-all-files is not the default, then should it not be made so? A BMI that still has references to source files is quite brittle since it can be moved around. AFAIK, neither GCC nor MSVC have this restriction for their BMIs.

I am happy to make it the default at least for C++20 modules. It will be pretty helpful for distributed build and sandbox-based build.

[boris-kolpackov]

While I don't disagree with any of the points made (it's a tradeoff), I want to highlight one aspect that I think is overlooked: not embedding the sources prevents you from moving the BMI. The fact that Clang 17 now seems to be needing the sources in fewer cases might actually make the matter worse since you usually don't get an error even if you moved the BMI. It took me probably half a day yesterday to try to understand what on earth is going on.

[boris-kolpackov]

Probably some performance numbers on the cost of doing this to demonstrate it's not a major cost (probably compile time/module generation time, and disk usage numbers would be useful) would be helpful in making this determination.

Here is one date point towards that: the libc++'s std.cppm BMI is 31MB. Not small but I think not a deal breaker either, seeing that it's probably an outlier, size-wise.

[ChuanqiXu9]

The size of embedded files are much smaller than the size of the BMI.

Let's make it in 2 weeks if no objection comes in.

[dwblaikie]

Here is one date point towards that: the libc++'s std.cppm BMI is 31MB. Not small but I think not a deal breaker either, seeing that it's probably an outlier, size-wise.

Not sure I follow - is it 31MB without embedded files, or with them? How much does it change with/without?

AFAIK, neither GCC nor MSVC have this restriction for their BMIs.

Could we get more details on that? Do they embed the source files? Do they do something else?

[boris-kolpackov]

Not sure I follow - is it 31MB without embedded files, or with them? How much does it change with/without?

It is 31662KB with and 30574KB without.

Could we get more details on that? Do they embed the source files? Do they do something else?

I cannot say for sure whether they embed source files (my guess is that they do not, they just serialize the AST). But I can say for sure they don't have shallow referenced to source files since in build2 we always go through -frewrite-includes (or equivalent) and then remove the partially preprocessed TU (because we compress them).

[dwblaikie]

Not sure I follow - is it 31MB without embedded files, or with them? How much does it change with/without?

It is 31662KB with and 30574KB without.

Fair enough, that sounds alright.

Could we get more details on that? Do they embed the source files? Do they do something else?

I cannot say for sure whether they embed source files (my guess is that they do not, they just serialize the AST). But I can say for sure they don't have shallow referenced to source files since in build2 we always go through -frewrite-includes (or equivalent) and then remove the partially preprocessed TU (because we compress them).

I wouldn't necessarily conclude that - preprocessed files might still have .file directives that could result in the output file referencing real files on disk. Though how the downstream diagnostics work in the absence of those files - fair, if that works it works. But I'd like to know what GCC and MSVC are doing before we go forward with this work.

[boris-kolpackov]

I wouldn't necessarily conclude that - preprocessed files might still have .file directives that could result in the output file referencing real files on disk. Though how the downstream diagnostics work in the absence of those files - fair, if that works it works.

I am not sure I follow the reasoning here: yes, the preprocessed .ii files have #line directives that point to the original source files. And in the diagnostics we see file/line/column of the original source file. This all works as expected, no issues there AFAIK. The issue is that, without -fmodules-embed-all-files, Clang actually tries to access the .ii file from which the .pcm file was built when it compiles consumers of that .pcm.

But I'd like to know what GCC and MSVC are doing before we go forward with this work.

I am probably missing something here, but I don't think that GCC and MSVC not having such a requirement are doing something special. Rather, Clang needing access to the original source code in addition to .pcm is doing an unusual thing that needs explaining, so to speak. I believe both GCC and MSVC just serialize the AST (probably with all the #line directive information for diagnostics) and it's not clear why they would also need access to the source code. But, again, maybe I am missing some inherent reason for why a BMI would normally also have to be accompanied with the source file (other than diagnostics).

[dwblaikie]

I am probably missing something here, but I don't think that GCC and MSVC not having such a requirement are doing something special. Rather, Clang needing access to the original source code in addition to .pcm is doing an unusual thing that needs explaining, so to speak. I believe both GCC and MSVC just serialize the AST (probably with all the #line directive information for diagnostics) and it's not clear why they would also need access to the source code. But, again, maybe I am missing some inherent reason for why a BMI would normally also have to be accompanied with the source file (other than diagnostics).

GCC at least also quotes source code in diagnostics, as I understand it - so I'd be curious to know whether they have the same dependence, or they embed the source code? If they do embed source, that seems helpful to know/double-check our direction here.

[ChuanqiXu9]

CC @iains for the details in GCC.