This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules] Handle template declarations in header units.
ClosedPublic

Authored by iains on Jan 27 2023, 4:12 AM.

Details

Summary

This addresses part of https://github.com/llvm/llvm-project/issues/60079

The test for external functions was not considering function templates.

Diff Detail

Event Timeline

iains created this revision.Jan 27 2023, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 4:12 AM
iains published this revision for review.Jan 27 2023, 4:19 AM

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

The issue here is that the function decl is extracted from function templates (and looks just like any other function definition at this point), so that we need to remember that it came from a template.

Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 4:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
iains planned changes to this revision.Jan 27 2023, 8:38 AM

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

rsmith added a subscriber: rsmith.Jan 27 2023, 4:50 PM
rsmith added inline comments.
clang/lib/Sema/SemaDecl.cpp
15265

Would it make sense to use !isa<FunctionTemplateDecl>(D) here instead of adding IsFnTemplate?

iains updated this revision to Diff 493014.Jan 28 2023, 9:53 AM

rebased, and revised to handle variable templates and instantiations.

iains marked an inline comment as done.Jan 28 2023, 9:59 AM

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

clang/lib/Sema/SemaDecl.cpp
15265

Would it make sense to use !isa<FunctionTemplateDecl>(D) here instead of adding IsFnTemplate?

I have changed this to use FD->isTemplated() to match the changes for VarDecls - where the template decl is not available. Would it be better to use the isa<>() ?

Arthapz added a subscriber: Arthapz.EditedJan 28 2023, 12:04 PM

tried the patch and it seems to work (but libc++ seem's to have a little problem :D)

> rm -rf .xmake build; xmake f --toolchain=clang; xmake b
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release algorithm
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release list
[ 88%]: compiling.release src/main.cpp
[ 92%]: linking.release stl_headerunit_cpp_only
[100%]: build ok!

> rm -rf .xmake build; xmake f --toolchain=clang --cxxflags="-stdlib=libc++"; xmake b                                                                                               
checking for platform ... linux
checking for architecture ... x86_64
[  0%]: generating.module.deps src/main.cpp
[  4%]: compiling.headerunit.release memory
[  4%]: compiling.headerunit.release exception
[  4%]: compiling.headerunit.release queue
[  4%]: compiling.headerunit.release iomanip
[  4%]: compiling.headerunit.release vector
[  4%]: compiling.headerunit.release utility
[  4%]: compiling.headerunit.release deque
[  4%]: compiling.headerunit.release set
[  4%]: compiling.headerunit.release map
[  4%]: compiling.headerunit.release fstream
[  4%]: compiling.headerunit.release cstdio
[  4%]: compiling.headerunit.release complex
[  4%]: compiling.headerunit.release cstdlib
[  4%]: compiling.headerunit.release iostream
[  4%]: compiling.headerunit.release list
[  4%]: compiling.headerunit.release string
[  4%]: compiling.headerunit.release stack
[  4%]: compiling.headerunit.release algorithm
[ 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&))
                   ^
/usr/bin/../include/c++/v1/ostream:225:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(basic_ios<char_type, traits_type>&
                   ^
/usr/bin/../include/c++/v1/ostream:230:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(ios_base& (*__pf)(ios_base&))
                   ^
/usr/bin/../include/c++/v1/ostream:233:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(bool __n);
                   ^
/usr/bin/../include/c++/v1/ostream:234:20: note: declaration of 'operator<<' does not match
    basic_ostream& operator<<(short __n);
  > in src/main.cpp
  
> cat src/main.cpp
import <iostream>;
import <algorithm>;
import <deque>;
import <iostream>;
import <map>;
import <memory>;
import <set>;
import <utility>;
import <vector>;
import <string>;
import <queue>;
import <cstdlib>;
import <utility>;
import <exception>;
import <list>;
import <stack>;
import <complex>;
import <fstream>;
import <cstdio>;
import <iomanip>;

using namespace std;

int main(int argc, char** argv)
{
    cout << "hello world!" << endl;
    return 0;
}
iains marked an inline comment as done.Jan 28 2023, 12:10 PM

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;
}

These seem to be a different diagnostic, do you find that is in some way related to the changes in D140261 + follow on patches?

AFAICT the failing test is unrelated to this patch.

ChuanqiXu added inline comments.Jan 30 2023, 10:32 PM
clang/lib/Sema/SemaDecl.cpp
15265

It looks not bad to me to use isTemplated (). And it looks like !FD->isTemplateInstantiation() is not tested? And it looks a little bit odd since the instantiated template should be implicitly inline.

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?

clang/lib/Sema/SemaDecl.cpp
15265

It looks not bad to me to use isTemplated ().

Hmm. now I am not sure what you prefer; in a previous review there was an objection to not using the provided function in the decl class.

What would be your suggestion here?

(we cannot use isa<VarTemplateDecl> when testing the variables case because the caller pulls out the templated decl before we get to test it) - so it is more consistent (IMO) to use the same interface in both tests.

And it looks like !FD->isTemplateInstantiation() is not tested?

Please could you expand a bit on what you mean here?
(the test is clearly required, in practice)

And it looks a little bit odd since the instantiated template should be implicitly inline.

Did you see @dlaikie's comment on this topic above?

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?

clang/lib/Sema/SemaDecl.cpp
15265

Did you see @dlaikie's comment on this topic above?

sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)

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?

Yeah, this is a pretty severe problem. Luckily clang16 is going to be released in early March. So we have roughly one month to solve this problem. Even if we failed to solve this in the last minute, I think it is not too bad to revert https://github.com/llvm/llvm-project/commit/335668b116439d13c7555616e126acdc608ce59e.

clang/lib/Sema/SemaDecl.cpp
15265

What would be your suggestion here?

I prefer isTemplated () here. If I get things correct, isTemplated () will cover the case that a non-template function decl inside a function template. And isa<FunctionTemplateDecl>(D) wouldn't handle the case. According to my understanding problem, we should avoid all the dependent cases.

Please could you expand a bit on what you mean here?

I mean I didn't find function template instantiation in tests of this revision page.

sorry @dblaikie 's comment is in the PR (https://github.com/llvm/llvm-project/issues/60079#issuecomment-1406856501)

oh, this is because the inconsistent use of inline in different places.. this may be the price we need to pay.

iains updated this revision to Diff 493546.Jan 31 2023, 3:12 AM
iains marked 2 inline comments as done.

rebased, added tests for instantiated variable/function templates.

clang/lib/Sema/SemaDecl.cpp
15265

Please could you expand a bit on what you mean here?

I mean I didn't find function template instantiation in tests of this revision page.

added.

This revision is now accepted and ready to land.Jan 31 2023, 6:08 PM
This revision was landed with ongoing or failed builds.Feb 2 2023, 2:51 AM
This revision was automatically updated to reflect the committed changes.