-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[cxxmodules] Start generating modules in rootcling. #930
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
[cxxmodules] Start generating modules in rootcling. #930
Conversation
|
Starting build on |
|
Build failed on ubuntu14/native. Errors:
|
|
Build failed on centos7/gcc49. Errors:
|
|
Build failed on slc6/gcc49. Errors:
|
|
Build failed on slc6/gcc62. Errors:
|
|
Build failed on mac1012/native. Errors:
|
3685e10 to
3164d9c
Compare
|
Starting build on |
1 similar comment
|
Starting build on |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on ubuntu14/native. Failing tests: |
|
Build failed on slc6/gcc62. Failing tests: |
|
Build failed on mac1012/native. Failing tests: |
|
Build failed on slc6/gcc49. Failing tests: |
|
Build failed on centos7/gcc49. Failing tests: |
Axel-Naumann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice progress!
core/dictgen/src/rootcling_impl.cxx
Outdated
| auto &headerList = module->Headers[i]; | ||
| // Include all headers from this module. | ||
| for (const clang::Module::Header &moduleHeader : headerList) { | ||
| // FIXME: Shall we include the absolute path here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's much more performant to first generate the string containing all #includes and then declare() that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
| for (clang::Module *subModule : M->submodules()) | ||
| modules.insert(subModule); | ||
| } | ||
| // Now we include all header files from the previously collected modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the includes be ordered leaf-modules to module, i.e. the inverse? In case any of the headers has a missing #include?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to do this the way clang is doing it which is by marking each header as a submodule and then let the visibility settings inside the module enforce this (e.g., the order doesn't matter, but clang ensures that each header first needs to import the submodules of the used headers, and if it doesn't it can't see the specific decls from this header even if they were parsed before and errors out with a nice error message that shows what include is missing).
This will either happen when we get the GenerateModuleAction from clang (which would still be the best solution) or until I get around and check out how to do this from rootcling.
I'll add a FIXME: that this is documented.
core/dictgen/src/rootcling_impl.cxx
Outdated
| if (StringRef(moduleName).endswith("_rdict.pcm")) { | ||
| auto lengthWithoutSuffix = moduleName.size() - strlen("_rdict.pcm"); | ||
| moduleName = moduleName.substr(0, lengthWithoutSuffix) + ".so"; | ||
| while (!LinkdefPath.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this warrants at least a comment. I also think that it's generally wrong, as it seems to e.g. add my home directory as include path for every ROOT build that's within my home directory structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was a workaround we need for finding the modulemaps. I move this to its own PR and repropose when we see the hit the issue in the master branch.
core/dictgen/src/rootcling_impl.cxx
Outdated
| } | ||
|
|
||
| if (sharedLibraryPathName.empty()) { | ||
| sharedLibraryPathName = dictpathname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the requirement (of the following if) that the library path name must be stated for the multidict case. I'm also not sure why this assignment is valid in the general case. E.g. IIRC CMS is using the same dictionary name for basically all its libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is from the master branch, I just moved it up because I needed to replicate the TModuleGenerator logic (due to the cyclic dependency that I explained in the comment below):
root/core/dictgen/src/rootcling_impl.cxx
Line 4424 in 94a28b7
| if (sharedLibraryPathName.empty()) { |
| unsigned Verbose : 1; | ||
| unsigned CxxModules : 1; | ||
| // The output path of any C++ PCMs we're building on demand. | ||
| std::string CachePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you throw in what you're caching, I guess ModuleCachePath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
|
|
||
| // If user has enabled C++ modules we add some special module flags to the | ||
| // compiler invocation. C++ modules don't make any sense when we are anyway | ||
| // supposed to Lex the input, so let's not do it in the OnlyLex mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't OnlyLex the tab completion flag? In that case modules would help, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to document what we discussed: It's only used by the LinkdefReader from what I can see. And here we can't use modules (as the LinkdefReader can't parse headers because __CINT__is defined when we parse Linkdefs, and this define breaks user code).
But this check was indeed only back when we used the global ROOT_MODULES setting. so I think this check can indeed can go away so one can use this setting in cling.
| // Code below from the (private) code in the GenerateModuleAction class. | ||
| llvm::SmallVector<char, 256> Output; | ||
| llvm::sys::path::append(Output, COpts.CachePath, | ||
| COpts.ModuleName + ".pcm"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we mark these ROOT-specific PCMs by giving them a more distinctive name?
| // With C++ modules, we now attach the consumers that will handle the | ||
| // generation of the PCM file itself. | ||
| if (COpts.CxxModules) { | ||
| // Code below from the (private) code in the GenerateModuleAction class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the use of WriteAST() simplify things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you were on vacation when the discussion about this happened, but to sum it up: Modules can't just be written via WriteAST(). They need a listener that we attach here that keeps track on things like what other module we load and in what order. This code wraps the WriteAST, the creation/attaching of the listener, etc. in the way with the least amount of copy pasted code from clang.
In the future we can either drop this code once I get rootcling into a form where we can reuse the whole GenerateModuleAction from clang or we make it public that we can easily reuse it. But first I want to see how far the rootcling refactor idea allows us to go :)
| CI->getFrontendOpts().ModuleFileExtensions, | ||
| /*AllowASTWithErrors=*/false, | ||
| /*IncludeTimestamps=*/ | ||
| +CI->getFrontendOpts().BuildingImplicitModule)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That leading + is ... weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the way it's in the original source code, I'll drop it and also remove it upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this seems to had a purpose. Back to this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to ask Richard about the semantics of this but forgot...
| case options::OPT_nostdincxx: NoCXXInc = true; break; | ||
| case options::OPT_v: Verbose = true; break; | ||
| case options::OPT_fmodules: CxxModules = true; break; | ||
| case options::OPT_fmodule_name_EQ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add // explicit fall-through here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in LLVM here, so we can even use the explicit LLVM fallthrough :). Fixed, thanks!
|
@phsft-bot build with flags -Druntime_cxxmodules=On |
|
Starting build on |
|
Build failed on centos7/gcc49. Failing tests: |
|
Build failed on ubuntu14/native. Failing tests: |
|
Build failed on mac1012/native. Failing tests: |
|
Build failed on slc6/gcc49. Failing tests: |
|
Build failed on slc6/gcc62. Failing tests: |
|
Starting build on |
e7117a2 to
f89d5f1
Compare
|
Starting build on |
This makes it easier for cling to check if -fmodules and/or -fmodule-name is passed so that we can act on these flags in the CIFactory.
This patch adds support for writing out modules to the CIFactory. If the user has passed -fmodule-name and -fmodules, we start extending the CI instance with support for C++ modules and also optionally setup AST consumers for writing out modules files alongside the interpreter.
This patch enables C++ modules support in rootcling. We now add the correct cling flags that will trigger the modules setup in the CIFactory. We also start reusing the normal clang code for generating modules as far as possible. For this we had to hack around a few bugs in ROOT like the fact that we actually never call many of the interpreter destructors (or if we do, we don't call the in a consistent way between rootcling and rootcling_stage1), mainly manually calling the end of the translation unit.
|
Build failed on slc6/gcc49. Errors:
|
|
@phsft-bot build with flags -Druntime_cxxmodules=On |
|
Starting build on |
|
Build failed on slc6/gcc62. Errors:
And 10 more Warnings:
And 11 more |
|
Build failed on slc6/gcc49. Errors:
And 10 more Warnings:
And 7 more |
|
Build failed on centos7/gcc49. Errors:
And 10 more Warnings:
And 7 more |
|
Build failed on mac1012/native. Errors:
And 10 more |
|
Build failed on ubuntu14/native. Errors:
And 10 more Warnings:
And 11 more |
|
Ok, the warnings are for the obvious modulemap mismatches we have and the errors are due to the yet missing VFS mapping. Can we get this in that I can make the follow up PRs? |
See the specific commits for changes.