Page MenuHomePhabricator

iains (Iain Sandoe)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 16 2014, 4:54 AM (437 w, 6 d)

Recent Activity

Thu, Feb 2

iains committed rGcdd44e2c8554: [C++20][Modules] Handle template declarations in header units. (authored by iains).
[C++20][Modules] Handle template declarations in header units.
Thu, Feb 2, 2:51 AM · Restricted Project, Restricted Project
iains closed D142704: [C++20][Modules] Handle template declarations in header units..
Thu, Feb 2, 2:51 AM · Restricted Project, Restricted Project

Tue, Jan 31

iains added inline comments to D142704: [C++20][Modules] Handle template declarations in header units..
Tue, Jan 31, 3:12 AM · Restricted Project, Restricted Project
iains updated the diff for D142704: [C++20][Modules] Handle template declarations in header units..

rebased, added tests for instantiated variable/function templates.

Tue, Jan 31, 3:12 AM · Restricted Project, Restricted Project

Mon, Jan 30

iains added a comment to D142704: [C++20][Modules] Handle template declarations in header units..

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?

Mon, Jan 30, 11:52 PM · Restricted Project, Restricted Project
iains added a comment to D142704: [C++20][Modules] Handle template declarations in header units..

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?

Mon, Jan 30, 11:47 PM · Restricted Project, Restricted Project

Sun, Jan 29

iains added a comment to D142704: [C++20][Modules] Handle template declarations in header units..

AFAICT the failing test is unrelated to this patch.

Sun, Jan 29, 10:04 AM · Restricted Project, Restricted Project

Sat, Jan 28

iains added a comment to D142704: [C++20][Modules] Handle template declarations in header units..

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;
}
Sat, Jan 28, 12:10 PM · Restricted Project, Restricted Project
iains added a comment to D142704: [C++20][Modules] Handle template declarations in header units..

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

Sat, Jan 28, 9:59 AM · Restricted Project, Restricted Project
iains updated the diff for D142704: [C++20][Modules] Handle template declarations in header units..

rebased, and revised to handle variable templates and instantiations.

Sat, Jan 28, 9:53 AM · Restricted Project, Restricted Project

Fri, Jan 27

iains planned changes to D142704: [C++20][Modules] Handle template declarations in header units..

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

Fri, Jan 27, 8:38 AM · Restricted Project, Restricted Project
iains published D142704: [C++20][Modules] Handle template declarations in header units. for review.

@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).

Fri, Jan 27, 4:19 AM · Restricted Project, Restricted Project

Sun, Jan 22

iains committed rG53a1314ed1b5: [C++20][Modules] Fix named module import diagnostics. (authored by iains).
[C++20][Modules] Fix named module import diagnostics.
Sun, Jan 22, 2:23 AM · Restricted Project, Restricted Project
iains closed D140927: [C++20][Modules] Fix named module import diagnostics..
Sun, Jan 22, 2:23 AM · Restricted Project, Restricted Project

Sat, Jan 21

iains committed rGff70e22f08d9: [C++20][Modules] Handle defaulted and deleted functions in header units. (authored by iains).
[C++20][Modules] Handle defaulted and deleted functions in header units.
Sat, Jan 21, 4:57 AM · Restricted Project, Restricted Project
iains closed D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..
Sat, Jan 21, 4:56 AM · Restricted Project, Restricted Project

Wed, Jan 18

iains updated the diff for D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..

rebased.

Wed, Jan 18, 11:34 PM · Restricted Project, Restricted Project
iains updated the diff for D140927: [C++20][Modules] Fix named module import diagnostics..

rebased

Wed, Jan 18, 3:25 AM · Restricted Project, Restricted Project
iains added a comment to D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..

Well.. we have time for another iteration,

I am going to take a vacation for the Chinese New Year since tomorrow to February. So I am a little bit hurried : )

Wed, Jan 18, 1:56 AM · Restricted Project, Restricted Project
iains updated the diff for D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..

address review commments, add an assert and a FIXME.

Wed, Jan 18, 1:56 AM · Restricted Project, Restricted Project
iains added a comment to D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..

LGTM basically. I still feel we need a FIXME there. But I don't want to block this for this reason especially we need to land this before the branch.

Wed, Jan 18, 12:45 AM · Restricted Project, Restricted Project
iains added inline comments to D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..
Wed, Jan 18, 12:24 AM · Restricted Project, Restricted Project
iains updated the diff for D141908: [C++20][Modules] Handle defaulted and deleted functions in header units..

rebased, address review comments

Wed, Jan 18, 12:23 AM · Restricted Project, Restricted Project

Tue, Jan 17

iains added a comment to D141894: [C++20] [Modules] Implement -fallow-non-inline-external-def-in-header-units.

According to the discussion in wg21, this one should be abandoned.

Could you summarize the rough ideas from the wg21 discussion here?

Tue, Jan 17, 7:46 AM · Restricted Project
iains published D141908: [C++20][Modules] Handle defaulted and deleted functions in header units. for review.
Tue, Jan 17, 2:39 AM · Restricted Project, Restricted Project
iains accepted D141905: [C++20] [Modules] Only diagnose the non-inline external variable definitions in header units.

thanks, LGTM.

Tue, Jan 17, 1:36 AM · Restricted Project, Restricted Project

Mon, Jan 16

iains added a comment to D141894: [C++20] [Modules] Implement -fallow-non-inline-external-def-in-header-units.

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

Mon, Jan 16, 11:52 PM · Restricted Project

Sun, Jan 8

iains committed rG335668b11643: [C++20][Modules] Do not allow non-inline external definitions in header units. (authored by iains).
[C++20][Modules] Do not allow non-inline external definitions in header units.
Sun, Jan 8, 4:20 AM · Restricted Project, Restricted Project
iains closed D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..
Sun, Jan 8, 4:20 AM · Restricted Project, Restricted Project

Jan 5 2023

iains accepted D140867: [C++20] [Modules] Don't generate global ctors/dtors for variables which are available externally.

@iains Oh, I used the incorrect word. I should use available externally. Header Units are external sources but the declarations in header units won't be available externally. I added a test to show this.

Jan 5 2023, 12:24 AM · Restricted Project, Restricted Project, Restricted Project

Jan 4 2023

iains added a comment to D140867: [C++20] [Modules] Don't generate global ctors/dtors for variables which are available externally.

OK. So I've been thinking about this some more and ISTM that if the external source represents a Header Unit, then that has no stand-alone object or initialiser.

Jan 4 2023, 3:15 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..

how does that look?

Jan 4 2023, 2:03 AM · Restricted Project, Restricted Project
iains updated the diff for D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..

rebase, added release notes

Jan 4 2023, 2:02 AM · Restricted Project, Restricted Project
iains added a comment to D140927: [C++20][Modules] Fix named module import diagnostics..

[module.import]p1 says:

In a module unit, all module-import-declarations and export-declarations exporting module-import-declarations shall appear before all other declarations in the declaration-seq of the translation-unit and of the private-module-fragment (if any).

So the following case is invalid:

//--- b.hpp
void func() {}
import a;

//--- c.cpp
module;
#include "b.hpp"
export module c;

I feel we'd better to address such cases in the test.

Jan 4 2023, 1:10 AM · Restricted Project, Restricted Project

Jan 3 2023

iains published D140927: [C++20][Modules] Fix named module import diagnostics. for review.
Jan 3 2023, 2:59 PM · Restricted Project, Restricted Project
iains added a comment to D140867: [C++20] [Modules] Don't generate global ctors/dtors for variables which are available externally.

your test case only covers itanium ABI - does the process still work properly for non-itanium ABIs?
.. if I remember correctly, the ctor/dtors are all run from the top level module (i.e. it pulls in the imported ones and constructs a composite ctor),

Jan 3 2023, 1:42 AM · Restricted Project, Restricted Project, Restricted Project

Dec 19 2022

iains added a comment to D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

Dec 19 2022, 10:23 PM · Restricted Project, Restricted Project
iains added a comment to D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

Dec 19 2022, 4:31 AM · Restricted Project, Restricted Project
iains updated the diff for D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..

rebased, addressed comments.

Dec 19 2022, 4:29 AM · Restricted Project, Restricted Project
iains added inline comments to D140261: [C++20][Modules] Do not allow non-inline external definitions in header units..
Dec 19 2022, 1:08 AM · Restricted Project, Restricted Project

Dec 18 2022

iains committed rGbd7f4c561f5e: [C++20][Modules] Elide unused guard variables in Itanium ABI module… (authored by iains).
[C++20][Modules] Elide unused guard variables in Itanium ABI module…
Dec 18 2022, 1:17 AM · Restricted Project, Restricted Project
iains closed D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..
Dec 18 2022, 1:17 AM · Restricted Project, Restricted Project

Dec 17 2022

iains updated the diff for D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..

rebased, amended a comment as suggested

Dec 17 2022, 11:36 AM · Restricted Project, Restricted Project
iains published D140261: [C++20][Modules] Do not allow non-inline external definitions in header units. for review.

this came up during discussion of other header unit constraints, we had not implemented it yet.
found a test case where we'd accidentally broken this rule too.

Dec 17 2022, 8:54 AM · Restricted Project, Restricted Project

Dec 9 2022

iains accepted D137059: [Driver] [C++20] [Modules] Support -fmodule-output= (2/2).

this LGTM ( but please wait for an ack from @dblaikie ) and again thanks for patience in seeing this through.

Dec 9 2022, 2:07 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D137058: [Driver] [Modules] Support -fmodule-output (1/2).

thanks all for the patience on this one - and for the collaborative discussion - I do think the outcome is going to be an easier to remember option name ;)

Dec 9 2022, 12:42 AM · Restricted Project, Restricted Project, Restricted Project

Dec 5 2022

iains added a comment to D137059: [Driver] [C++20] [Modules] Support -fmodule-output= (2/2).

I'm still curious what about the details of other compilers - I think from the sounds of it, @iains suggested GCC doesn't support this yet so we'll need to pick/name the flag ourselves & he's happy to implement whatever we pick? I guess Microsoft's flag naming is sufficiently differently styled as to offer no useful inspiration? Though wouldn't hurt to know what they name it.

Any other examples you had in mind, Ben?

GCC supports naming the output file by asking the "module mapper" where a module with a given name lives (also used for finding imported modules).

Dec 5 2022, 8:55 PM · Restricted Project, Restricted Project, Restricted Project

Nov 8 2022

iains added a comment to D137609: [C++20] [Modules] Remove unmaintained header modules.

I think that (if this change is approved) there will be also some simplifications possible in the driver (since the mode that produces a wrapper header for multiple command-line headers is different from the mode where multiple command line headers would each produce a single C++ standard header unit) ..

Nov 8 2022, 11:39 AM · Restricted Project, Restricted Project, Restricted Project

Nov 3 2022

iains added a comment to D137058: [Driver] [Modules] Support -fmodule-output (1/2).

I realize I got this jumbled up and the thread about "why do we need to name things" is meant to be over in D137059 (sorry @ben.boeckel :/ I know this is all confusing to follow at the best of times) so I'll pick that up there.

But maybe a relevant question /here/ (maybe @iains has some context here, and @ben.boeckel ): What are GCC's (& any other implementations) command line interfaces for things like this? How're the command line flags spelled? To see about inspiration for this.

Nov 3 2022, 4:32 PM · Restricted Project, Restricted Project, Restricted Project

Nov 1 2022

iains added a comment to D137059: [Driver] [C++20] [Modules] Support -fmodule-output= (2/2).

Could you link to the email/discourse discussion about supporting this mode (I think you've linked it in other discussions, be good to have it for reference here & Probably in the other review)? (I'm wondering if we need a new flag for this, or if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step, for instance - I realize this is a somewhat breaking change but may be acceptable given that modules aren't widely deployed yet)

Done. From my reading, in that discourse discussing, we're not talking about to add the new flags. I add the flag since I don't want the .pcm file pollutes the user space accidentally.

if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step

I am not sure what you mean. Do you talk about to forbidden the original 2-phase compilation model? If so, I think it is definitely the wrong direction. The 2-phase compilation model should be the correct direction in the long term since it has higher parallelism.

I am not convinced about this second point as motivation for this direction; it comes with some significant resource tradeoffs (compared with the proposed [near] future version of producing the PCM and the object from one invocation of the FE):

  • it requires multiple instantiations of the FE
  • it blocks the objective of reducing the content of module interfaces (so that they only contain the information that pertains to the interface) - since requiring source -> pcm, pcm -> object means that the PCM has to contain all the information necessary to generate the object.
  • in terms of parallelism, the interface PCM has to be generated and distributed - the parsing and serialisation has to be complete before the PCM can be distributed; that process is the same regardless of whether the FE invocation also produces an object.

So, I would suggest that we would move to a single invocation of the compiler to produce the PCM and object as the default; if the user has a specific reason to want to do the two jobs separately then thay could still do so ( -fmodule-only / --precompile ) at the expense of two invocations as now,

(so that they only contain the information that pertains to the interface)

No, we can't do this. It hurts the performance.

it requires multiple instantiations of the FE

Agreed. But if we care about this, I think it may be best to allow the current 2 phase compilation model only. And we forbid the compilation from module unit to object files directly. This is cleanest approach.

in terms of parallelism, the interface PCM has to be generated and distributed - the parsing and serialisation has to be complete before the PCM can be distributed; that process is the same regardless of whether the FE invocation also produces an object.

I think the distribution doesn't matter with parallelism. For parallelism, I mean, for the scan-based build systems, the compilation of A must wait until the dependent module B compiles to object files, which is significantly worse than the 2 phase compilation.

Nov 1 2022, 1:26 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D137059: [Driver] [C++20] [Modules] Support -fmodule-output= (2/2).

Could you link to the email/discourse discussion about supporting this mode (I think you've linked it in other discussions, be good to have it for reference here & Probably in the other review)? (I'm wondering if we need a new flag for this, or if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step, for instance - I realize this is a somewhat breaking change but may be acceptable given that modules aren't widely deployed yet)

Done. From my reading, in that discourse discussing, we're not talking about to add the new flags. I add the flag since I don't want the .pcm file pollutes the user space accidentally.

if it'll be OK to change the driver behavior to always coalesce the .cppm->.pcm->.o path into a single step

I am not sure what you mean. Do you talk about to forbidden the original 2-phase compilation model? If so, I think it is definitely the wrong direction. The 2-phase compilation model should be the correct direction in the long term since it has higher parallelism.

Nov 1 2022, 12:49 AM · Restricted Project, Restricted Project, Restricted Project

Oct 19 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

I grepped options.td and got (incomplete) list for options to take a output name:

# -o and its alias
-o
-object_file_name=
--output=

/Fa (windows for assembly output filename)
/Fe (windows for output executable file name)
/Fi (windows for preprocessed output filename)
/Fo (Windows for object file)

-dependency-dot (for DOT-formatted header dependencies)
-dependency-file (to write dependency output to)

-header-include-file (Filename to write header include output to)

-opt-record-file (Filename to use for YAML optimization record output)

-split-dwarf-output (Filename to use for split dwarf debug info output)

-stack-usage-file (to write stack usage output to)
-coverage-data-file (Emit coverage data to this filename)
-coverage-notes-file (Emit coverage notes to this filename)

And it looks like the -file appears a lot. So may be the suggestion (-fc++-module-file-output) may be better. And for the default location, I feel like my explanation above makes sense. If the end user wants to produce .pcm files, they can use --precompile just like what they do with -c to get the object files. This only matters with end users since the build systems should/would chose other positions.

Oct 19 2022, 11:38 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

hmm (I realise I mentioned this, and hope it has not complicated things) ..

.. I was thinking of the -fmodule-file=<name>=filename variant. The problem with using it without (the <name>) is that -fmodule-file= can (and does) appear multiple times on the command line to specify dependent modules - so that one would have to specify which (named) module was related to the filename.

I /think/ currently -fmodule-file works with Clang header modules presumably because the module files contain within them enough information to enable the compiler to substitute the module for an include where necessary? I'm not sure the <name>= part is necessary, as such, if the BMIs can reasonably contain enough identifying info to make their use clear to the consumer.

-fmodule-file= works with both clang modules and standard C++ ones. What it does (as of now) is cause the FE to open the named file and to pre-load the module name - so that when the FE tries to find a module the lookup mechanism can associate the module name with the module data. So, if we need 3 dependent modules to be available when building a new source .. we would specify the PCM files containing those modules.

At present, if you try to specify a non-existent PCM using -fmodule-file= the compile will fail early on (it is expecting this to indicate a source module, not a destination one).

However, the second syntax -fmodule-file=<name>=filename I think should be able to work around this (since it says that the named module <name> is associated with the PCM filename which would allow us to cater for that file being missing (when we are creating it).

Would the intent be that this might write out multiple different module files? I would hope we don't need to support that - that we only generate the module file for the single source module passed to the compiler. I guess maybe to support clang++ x.cppm y.cppm you might have multiple output files? But in that case you don't get to use -o so maybe you don't get to specify the module paths either and just get the default "same as .o but with .pcm suffix" behavior?

Does that clarify at all ?

It would be great not to add more modules options flags, there are already way to many :/ - but if this seems too complex then one of the spellings suggested by @ChuanqiXu would work.

Oh, right, super sorry - this is about how to specify the output filename. OK.

(I'd still sort of lean towards "make it the same as the .o, but with the .pcm suffix" and if the build system wants to put things in other places, it can move them around - but I understand why that's not something everyone's on board with)

Oct 19 2022, 4:25 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

hmm (I realise I mentioned this, and hope it has not complicated things) ..

.. I was thinking of the -fmodule-file=<name>=filename variant. The problem with using it without (the <name>) is that -fmodule-file= can (and does) appear multiple times on the command line to specify dependent modules - so that one would have to specify which (named) module was related to the filename.

Oct 19 2022, 3:40 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

In a pre-scanned world, the build system does know the info for each source file (published and dependent modules) [which ought to dispel some of the concerns raised about not knowing about possible outputs for implementation/interface cases].

In a discovery world, the interface to the build system carries all of this traffic anyway so that the command line would only be providing pre-set data for that.

This does not cover all build systems. For example, Coverity relies on observing the compiler invocations performed by another build system and relies on interpreting the command lines of those invocations in order to identify all inputs and outputs (and Coverity does require awareness of outputs as well as inputs). Other tools that operate on a compilation database or monitors like Build EAR have similar requirements.

Oct 19 2022, 2:59 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

I'm OK with sticking with the existing -fmodule-file if that works for everyone. Yeah, it's short and ambiguous in a space with many concepts of what a "module file" is, but also the fact that it's a -f flag might help disambiguate it a bit - it's probably not the way anyone would think/expect to be passing source files, those just get passed without flags on the command line. And any use of it will show the .pcm extension or whatever that should make it clear enough what's going on.

Oct 19 2022, 12:18 PM · Restricted Project, Restricted Project, Restricted Project

Oct 18 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

once again we are getting off topic for this patch :)

Oct 18 2022, 2:32 AM · Restricted Project, Restricted Project, Restricted Project

Oct 17 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

I am also OK with doing this in two steps (first in the driver with this patch and then by updating the FE to allow the two outputs from one invocation - my draft patch series).

Oct 17 2022, 11:55 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

Although modular code is user-facing - BMIs are an implementational detail, right?

but I don't think BMIs are an implementation detail, anymore than object files are - users should/will be as aware of BMIs as they are of .o files - there build artifacts that need to be cached, can be out of date, can be deleted/maybe copied/moved around in limited ways, have to be passed to other tools in the build pipeline. Not sure what term to use, I guess compilers don't often have warnings/errors/diagnostics that explicitly talk about "object files" - so maybe in a similar sense they won't explicitly talk about "binary module artifact files" but maybe?

Yeah, agreed. I guess @iains 's meaning may be "if the tool chains are ready enough one day, then the end user won't need to touch BMIs directly". Yeah, it looks true. Nowadays, many c++ programmers don't need to touch the .o files since the build systems have taken care for it (from my personal experience). But from the perspective of a compiler, the concept of BMI may be necessary. I remember when I wrote the documents for modules, I used the term "module file" at first. Then @ruoso corrected me that we should use BMI here. So I think the use of the term BMI may be correct here.

The build system still needs to know that B.cppm depends on A.cppm - and once it knows that, it's not a huge cost for it to know the name of the file that represents that dependency and is produced by A.cppm and passed to B.cppm, I think?

In a private chat with Kitware guys, they told me if the one phase compilation wasn't supported, they can mock it by replacing the original command by the combination of:

clang++ -std=c++20 --precompile src.cppm -o src.pcm
clang++ -std=c++20 src.pcm --precompile src.o

but the pcm files won't be depended directly. So it may be harder, I am not sure. @ben.boeckel any update?

In short - seems like we should separate out the cache discussion from the "one phase" compilation in the sense of a single build action that takes a .cppm and generates both a .o and a .pcm in one compiler/driver invocation. (maybe something like this is what @iains has already sent out in another review?)

Agreed.

to @iains point about "it'd be good if we didn't have to invoke two underlying commands from the one drivter invocation" - yeah, agreed. Though I wouldn't mind one step being "add the driver interface" and another being "fix whatever serialization isuse/etc/ might stand in the way of doing .cppm->{.o,.pcm} in a single action without serialization, so we can then start stripping stuff out of the .pcm since it'll only need to contain the interface, and not have to worry about having enough info for .o generation anymore"

(I guess you're saying without deserialization)

Agreed in the higher level.

But what do you mean about stripping stuff out of the .pcm? Do you mean to remove some function bodies when writing the BMI? If yes, it'll be problematic. When we did so, when other TU imports the BMI, it won't see the function bodies it could see before. This will prevent optimization. It'll be good as an optional flag so that the user know what the effect is. But it might not be good to do so by default. (Although this paragraph starts to talk about other topics)

Oct 17 2022, 11:42 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

Address @tschuett's opinion:

  • Use new introduced -fc++-module-cache-path= instead of -fc++-module-path to avoid many logics about modules cache in clang modules.
  • Use new introduced -fmodule-bmi-output= instead of -fmodule-name for the same reason. Also -fmodule-bmi-output= is more flexible since -fmodule-name require its argument to be the same with the actual module name.
Oct 17 2022, 5:33 AM · Restricted Project, Restricted Project, Restricted Project
iains accepted D135958: [clang] modules: Fix callback argument thinko.

LGTM (the CI test fail seems spurious/unrelated, and does not repeat for me on macOS)

Oct 17 2022, 12:25 AM · Restricted Project, Restricted Project

Oct 13 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

For example, my experimental support for P1689 is at: [...]. The implementation looks relatively trivial to me. The simplicity is pretty important.

Oct 13 2022, 1:13 AM · Restricted Project, Restricted Project, Restricted Project

Oct 12 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

To avoid more tangential discussion here - I've opened https://discourse.llvm.org/t/generating-pcm-module-interfaces-and-regular-object-files-from-the-same-compiler-invocation/65918 ... it would be great if the major stakeholders here especially @rsmith could comment on the design intentions.

Oct 12 2022, 8:35 AM · Restricted Project, Restricted Project, Restricted Project

Oct 11 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.
Oct 11 2022, 9:09 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

(trying not derail this discussion further)

  • Yes, the alternate solution proposed for the "Hello World" case would work with the module mapper interface. However, the initial discussion was simply about being able to produce both the .PCM and .O artefacts from one invocation of the FE.
  • It seemed reasonable to mention (since it's not vapourware, there are draft patches), but clearly some technical issues to address.
  • I do not think this patch fully addresses the issue of harmonising GCC and clang's command lines for modular builds (because it does not deal with discovery of modular code in 'normally named' sources), and my investigation of trying to do this in the driver suggests that it would be much more complex than doing it in the FE.
Oct 11 2022, 8:25 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

If we are going to require the serialisation/deserialisation for correctness then IMO we should have an error for an incorrect compilation.

Of course.

I still feel that deserialisation should do deserialisation, and Sema should be doing the diagnoses when it tries to use a decl (in fact, I would expect that to be required for correctness in the C++20 scheme, since the point of use is significant). I will defer to folks who know the history better - but this seems like a layering bug to me still.

Another reason I forgot to mention is that the deserialization need to do merging. It is common in practice that a similar declarations lives in multiple module files (e.g., by GMF). Then if the deserialization needs to merge things, it is naturally that the deserilizer need to do ODR checks. But if you feel like the merging job belongs to Sema too, I think it might not be bad/wrong.

I would be concerned that doing this work in the deserialisation could:

  • produce false positives (diagnostics) where there should be no error until an object is actually used.
  • make it more difficult to track cases where what is merged at the point of use is semantically significant.

It would seem to better that the consumer of the AST should not have to know whether it had been round-tripped through the serialisation/deserialization process.

The reading process of modules are lazy. So the problem is relatively mitigated. But I understand your concern in some level.

Oct 11 2022, 3:41 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

If we are going to require the serialisation/deserialisation for correctness then IMO we should have an error for an incorrect compilation.

Of course.

I still feel that deserialisation should do deserialisation, and Sema should be doing the diagnoses when it tries to use a decl (in fact, I would expect that to be required for correctness in the C++20 scheme, since the point of use is significant). I will defer to folks who know the history better - but this seems like a layering bug to me still.

Another reason I forgot to mention is that the deserialization need to do merging. It is common in practice that a similar declarations lives in multiple module files (e.g., by GMF). Then if the deserialization needs to merge things, it is naturally that the deserilizer need to do ODR checks. But if you feel like the merging job belongs to Sema too, I think it might not be bad/wrong.

Oct 11 2022, 2:32 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

BTW, in the current context, when I say "my" and "your", I just want to refer the corresponding things. There is no means to offend.

sure - no problem.

I guess we ought to be a bit more clear:
There is implementation divergence between GCC and clang and from the point to view of users and build systems having two different command line sets is not helpful.

The patches I drafted were aimed at removing that divergence.
If that is not possible (because the ODR analysis requires steps that make it very difficult or inefficient to do so) then some more thought and/or restructuring is needed.

Yeah, now this is more clear.

(2) Your scheme saves the time for deserialization. However, clang implement many ODR checks in the deserialization part. Skipping the deserialization will omit these ODR checks, which will break the semantics. And this is the reason why that scheme may not be good.

What you seem to be saying here is that:

clang++ -std=c++20 foo.cxxm -c -o foo,o

edit; maybe I should have written -xc++ foo.cxxm

Does different ODR analysis from:

clang++ -std=c++20 foo.cxxm --precompile -o foo.pcm
clang++ -std=c++20 foo.pcm -o foo.o

If so, then that is of some concern since those are both valid compilations in the current compiler.

In the first case, the back end is connected as an AST consumer to the front - there is no serialise/deserialise.

No. Currently, in clang, the following two is the same:

clang++ -std=c++20 foo.cxxm -c -o foo.o

That's why I added the edit ...

clang++ -std=c++20 -xc++ foo.cxxm -c -o foo.o

AFAIU that will not produce any error for the user?

suppose I have foo.cxx which includes modules and has local declarations?
clang++ -std=c++20 foo.cxx -c -o foo.o

with

clang++ -std=c++20 foo.cxxm --precompile -o foo.pcm
clang++ -std=c++20 foo.pcm -o foo.o

The first compilation will generate .pcm files in the /tmp directories and compile the .pcm files in the /tmp directories.

yes, I should have added the -xc++ in the first case ;)

Oh, I got it. Now it makes sense. I misses the edit : (.

The answer is:
(1) Now, the behavior is different. And I once sent a bug report for it.
(2) Now there won't be direct error message. And I was planned to add it. This is the reason why I closed the above bug report : )
(3) If we're going to make it an error, this is not decided yet.

Oct 11 2022, 1:56 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

Clang header modules started with implicit builds and module caches. Apple is moving too explicit builds. There are all kinds of problems with implicit module builds. I believe that C++20 modules shouldn't make the same mistake.

Oct 11 2022, 1:37 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

BTW, in the current context, when I say "my" and "your", I just want to refer the corresponding things. There is no means to offend.

Oct 11 2022, 1:20 AM · Restricted Project, Restricted Project, Restricted Project

Oct 10 2022

iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.
Oct 10 2022, 11:55 PM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

FAOD: I think this should not be about "who's patches" - but about the compilation model and command lines that we want to end up with.

Oct 10 2022, 11:18 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

I have implemented a similar scheme to GCC's where C/BMIs can be produced "on demand" when the FE encounters the module keywords. The idea was to make it possible to have the same command lines on both compilers (basically to remove the need for the user to identify that a c++ source is is modular - with the sole exception of header units which the compiler cannot figure out on its own).

This scheme interfaces with the module-mapper approach (but how the mapping is determined could just as easily be from command line flags, module map files etc. etc. that is the beauty of that approach - the compiler has no need to know about how the mapping is made - it can be a fixed in-process scheme (like using the module cache path + some well-known mapping of names) or a full-blown build system - e.g. attaching to build2 (I think Boris did try out an earlier version of my implementation.)

draft patches (need rebasing) here: D118896, D118898,D118895, D118899 (these do not include the libCody stuff, which would support the module mapper, but concentrate on the compiler side).
edit: D118895 is probably no longer relevant.

Oct 10 2022, 2:42 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

I have implemented a similar scheme to GCC's where C/BMIs can be produced "on demand" when the FE encounters the module keywords. The idea was to make it possible to have the same command lines on both compilers (basically to remove the need for the user to identify that a c++ source is is modular - with the sole exception of header units which the compiler cannot figure out on its own).

This scheme interfaces with the module-mapper approach (but how the mapping is determined could just as easily be from command line flags, module map files etc. etc. that is the beauty of that approach - the compiler has no need to know about how the mapping is made - it can be a fixed in-process scheme (like using the module cache path + some well-known mapping of names) or a full-blown build system - e.g. attaching to build2 (I think Boris did try out an earlier version of my implementation.)

draft patches (need rebasing) here: D118896, D118898,D118895, D118899 (these do not include the libCody stuff, which would support the module mapper, but concentrate on the compiler side).
edit: D118895 is probably no longer relevant.

Reworking/rebasing this scheme and posting the patches is next on my TODO after (current work on) P1815 part 2.

Hopefully, there is some way to combine these pieces of work to give the user greatest flexibility.

@iains oh, I remembered just now that you said you'd like to take this in one of GitHub issues long ago. My bad. I forgot it when I started this. I think you can add me as subscriber such drafts next time to avoid similar things happen again.
In fact, I had some drafts about modules and they conflicted with yours previously. So I'm sure I understand this : (

And for this concrete job (generate pcm and object file in one compilation job), I think my implementation looks better. It is shorter and reuses the current behavior (generate .pcm files in /tmp directories and we just redirect it). So I think it may be better to pursue this patch for this task. (The option design could be changed of course).

Oct 10 2022, 1:54 AM · Restricted Project, Restricted Project, Restricted Project
iains added a comment to D134267: [C++] [Modules] Support one phase compilation model for named modules.

@ChuanqiXu BTW, I think we (unfortunately) have some overlap in work in progress here;

Oct 10 2022, 12:15 AM · Restricted Project, Restricted Project, Restricted Project

Oct 1 2022

iains updated the diff for D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..

rebased and updated to elide the guard only for the case of no inits _and_ no imports.

Oct 1 2022, 7:44 AM · Restricted Project, Restricted Project

Sep 26 2022

iains planned changes to D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..

So, in light of these comments;

Sep 26 2022, 8:15 AM · Restricted Project, Restricted Project

Sep 24 2022

iains added a comment to D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..

thanks for the review.

Sep 24 2022, 3:01 PM · Restricted Project, Restricted Project
iains updated the diff for D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers..

address review comments, minor amendments to description.

Sep 24 2022, 3:00 PM · Restricted Project, Restricted Project
iains published D134589: [C++20][Modules] Elide unused guard variables in Itanium ABI module initializers. for review.
Sep 24 2022, 8:20 AM · Restricted Project, Restricted Project
iains updated the diff for D126694: [C++20][Modules] Implementation of GMF decl elision..

rebased and reworked.

Sep 24 2022, 4:31 AM · Restricted Project, Restricted Project, Restricted Project

Aug 21 2022

iains committed rGfee3cccc6cda: [C++20][Modules] Improve handing of Private Module Fragment diagnostics. (authored by iains).
[C++20][Modules] Improve handing of Private Module Fragment diagnostics.
Aug 21 2022, 2:21 AM · Restricted Project, Restricted Project
iains closed D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics..
Aug 21 2022, 2:20 AM · Restricted Project, Restricted Project

Aug 20 2022

iains added inline comments to D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics..
Aug 20 2022, 6:32 AM · Restricted Project, Restricted Project
iains updated the diff for D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics..

rebased, addressed remaing comments.

Aug 20 2022, 6:29 AM · Restricted Project, Restricted Project

Aug 19 2022

iains added a comment to D128981: [C++20][Modules] Implement include translation..

Hi @iains, upstream Clang crashes on the attached test case due to an assertion failure. Git bisect pointed me to this commit. Can you please take a look? Previously, the test would result in a warning of incomplete umbrella header.

yes, I can repeat this - will take a look.

Aug 19 2022, 1:33 AM · Restricted Project, Restricted Project
iains committed rGa2dd6130d497: [clang][Modules] Fix a regression in handling missing framework headers. (authored by iains).
[clang][Modules] Fix a regression in handling missing framework headers.
Aug 19 2022, 1:13 AM · Restricted Project, Restricted Project

Aug 17 2022

iains added a comment to D128981: [C++20][Modules] Implement include translation..

Hi @iains, upstream Clang crashes on the attached test case due to an assertion failure. Git bisect pointed me to this commit. Can you please take a look? Previously, the test would result in a warning of incomplete umbrella header.

Aug 17 2022, 4:27 AM · Restricted Project, Restricted Project

Aug 15 2022

iains added inline comments to D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics..
Aug 15 2022, 2:58 AM · Restricted Project, Restricted Project
iains updated the diff for D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics..

rebased, addressed review comments

Aug 15 2022, 2:56 AM · Restricted Project, Restricted Project

Aug 8 2022

iains added inline comments to D131388: [docs] Add "Standard C++ Modules".
Aug 8 2022, 3:56 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
iains added a comment to D131388: [docs] Add "Standard C++ Modules".

general comment.

Aug 8 2022, 3:41 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
iains added a comment to D131062: [docs] Add "C++20 Modules".

@h-vetinari you are right, this has become difficult to review - I will try and do some more later - just the one comment for now.

Aug 8 2022, 2:49 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Aug 2 2022

iains added a comment to D130864: [NFC] Introduce ASTContext::isInSameModule().

Thanks for working on this. I have a few major concerns here:

  • Do we know that it's a bottleneck in practice? E.g. if the module name have different sizes, comparing strings is actually fast. If the module names are short, we are also in a pretty good situation. Having some benchmarks numbers would help to quantify whether the change is needed.

No, we don't know it is a bottleneck in practice. (Currently there shouldn't be many users for C++20 Modules). The reason to do this is that we (Iain and me) feel bad to compare string frequently. Benchmarks would be good but I am afraid I can't. Since there are not enough existing benchmarks and it looks costful to construct the meaningful benchmarks. Let's not make the perfect to be the enemy of better.

Aug 2 2022, 2:38 AM · Restricted Project, Restricted Project, Restricted Project

Aug 1 2022

iains accepted D130871: [C++20] [Modules] Handle initializer for Header Units.

LGTM, one small point about the test,

Aug 1 2022, 3:06 PM · Restricted Project, Restricted Project, Restricted Project

Jul 25 2022

iains committed rG25558a1bfd79: [C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1] (authored by iains).
[C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1]
Jul 25 2022, 6:29 AM · Restricted Project, Restricted Project
iains closed D129174: [C++20][Modules] Update ADL to handle basic.lookup.argdep p4 [P1815R2 part 1].
Jul 25 2022, 6:29 AM · Restricted Project, Restricted Project
iains committed rGb826567136e3: [C++20][Modules] Add a testcase for [basic.link] p10 [NFC]. (authored by iains).
[C++20][Modules] Add a testcase for [basic.link] p10 [NFC].
Jul 25 2022, 4:20 AM · Restricted Project, Restricted Project

Jul 24 2022

iains added a comment to D129138: [clang] [docs] Update the changes of C++20 Modules in clang15.

@iains I'm going to land this in next Monday if all the dependent patch landed. Do you feel good with this?

Jul 24 2022, 3:14 PM · Restricted Project, Restricted Project, Restricted Project

Jul 23 2022

iains updated the diff for D126694: [C++20][Modules] Implementation of GMF decl elision..

rebase, update testcases for upstream changes.

Jul 23 2022, 11:46 AM · Restricted Project, Restricted Project, Restricted Project