Page MenuHomePhabricator

[C++20] [Module] Fix bug47116 and implement [module.interface]/p6
ClosedPublic

Authored by ChuanqiXu on Oct 31 2021, 7:20 PM.

Details

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=47116.

According to https://eel.is/c++draft/module.interface#2, it is meaningless to export an entity which is not in namespace scope.
The reason why the compiler crashes is that the compiler missed ExportDecl when the compiler traverse the subclass of DeclContext. So here is the crash.

Also, this patch implements https://eel.is/c++draft/module.interface#6 in Sema::CheckRedeclaration* functions.

Test Plan: check-all

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Oct 31 2021, 7:20 PM
ChuanqiXu created this revision.
aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

I think this diagnostic text is more clear based on the standards text you cited. This would also come with a note diagnostic to point to the previous declaration.

clang/lib/Sema/SemaDecl.cpp
5795–5797

I don't believe this is sufficient to cover [module.interface]p6. I tried out the example from the paragraph in the standard and we still silently accept it.

ChuanqiXu added inline comments.Nov 1 2021, 6:37 PM
clang/lib/Sema/SemaDecl.cpp
5795–5797

Yes, this patch is not intended to cover [module.interface]p6. The intention is to fix the crash since I feel it would take a longer time to support [module.interface]p6. And crash is not good any way. I plan to support [module.interface]p6 in successive patches. Do you happy with this?
BTW, I have a plan to support clang's c++20 module to a workable state. And I am working on the reachable definition in https://eel.is/c++draft/module.reach#3 and https://eel.is/c++draft/module.global.frag#3.

aaron.ballman added inline comments.Nov 3 2021, 5:52 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

Given that this is not intended for p6 specifically, I think my suggestion is incorrect. But I am also not certain your diagnostic is either, but it's really hard to tell given where our current support is for modules. All of the other compilers suggest that an unqualified id is expected to be found, and I tend to agree -- there's no declaration there *to* export, just the type specifier for a declaration. This makes me think the issue is elsewhere and perhaps we shouldn't even be getting into diagnoseQualifiedDeclaration().

clang/lib/Sema/SemaDecl.cpp
5795–5797

Thank you for the clarification! Fixing the crash is definitely a step in the right direction.

ChuanqiXu updated this revision to Diff 384645.Nov 3 2021, 8:13 PM
ChuanqiXu retitled this revision from [C++20] [Module] Fix front end crashes when trying to export a type out of a module to [C++20] [Module] Fix front end crashes when trying to export a qualified entity which is already declared.

Add more tests.

ChuanqiXu added inline comments.Nov 3 2021, 8:27 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

I think we need to touch diagnoseQualifiedDeclaration() after all. Since there is a potential risk of crashing in it. If we didn't fix it in diagnoseQualifiedDeclaration() and find other place to fix the issue, I think it may crash potentially one day in the process of developing or we might ignore some paths to diagnoseQualifiedDeclaration()`. It would be a disaster.
And you said "there's no declaration there *to* export". And I noticed that there is error/warning in Sema::ParsedFreeStandingDeclSpec which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715. And diagnoseQualifiedDeclaration() would handle the qualified redeclaration. So it looks a good place to me.
BTW, I found the current patch could handle [module.interface]/p6 partially for qualified redeclaration surprisingly. See the newly added test case for example.
Finally, given that we need to touch diagnoseQualifiedDeclaration() to fix the crash, I think the patch should be good and we could try to cover [module.interface]/p6 in successive patches. Do you think so?

clang/lib/Sema/SemaDecl.cpp
5795–5797

BYW, I added a new issue for the example in [module.interface]p6 here: https://bugs.llvm.org/show_bug.cgi?id=52395.

5795–5797

s/BYW/BTW

aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

I think we need to touch diagnoseQualifiedDeclaration() after all. Since there is a potential risk of crashing in it.

My point is that we may have wanted to reject this code before ever needing to call diagnoseQualifiedDeclaration() in the first place. aka, it might be just as valid to assert we never see an ExportDecl here because the caller should have already handled that case.

And you said "there's no declaration there *to* export". And I noticed that there is error/warning in Sema::ParsedFreeStandingDeclSpec which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715.

The declaration is invalid in C++20 because it does not declare anything (this is the same example with the export keywords removed): https://godbolt.org/z/8zc9q7fno

Clang fails the first one because we don't yet implement P0634R3, but the presence of the export keyword should not change diagnostic behavior here.

urnathan added inline comments.Nov 4 2021, 8:04 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

Agreed, 'export' is only applicable to namespace-scope declarations. We should reject it applying to non-namepace-scope entities.

ChuanqiXu planned changes to this revision.Nov 4 2021, 6:53 PM
ChuanqiXu added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

Got it. It is important to make the code semantics consistent with the spec.

ChuanqiXu requested review of this revision.Nov 4 2021, 9:36 PM
ChuanqiXu added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

@aaron.ballman @urnathan I have sent a draft to reject the codes before diagnoseQualifiedDeclaration(). The draft is here: https://reviews.llvm.org/D113239. The draft is not completed to review the details. The key idea is to insert checks for ExportDecl on the fly to construct the AST. Due to the complexity of semantics analysis , we might need to insert the checks many times. It may not be too terrible since I found many Check* functions in Sema. So may be this is what we used to do.

And the draft could reject the example in https://eel.is/c++draft/module.interface#6. It looks like not so hard as I imaged.

BTW, I have tried to let the compiler to emit "declaration does not declare anything" when it sees "export template <typename T> X<T>::Y". But I failed. It is not easy for me. And I found that the compiler wouldn't emit "declaration does not declare anything" when the declaration is in a DeclContext. Here is an example: https://godbolt.org/z/3j63PnTc7. So the compiler used to handle this case in diagnoseQualifiedDeclaration(). Personally, it looks not bad to handle export template<> X::Y in diagnoseQualifiedDeclaration() at least from the names. And I am wondering if we could export a non-namespace-scope name without using qualifiers? If no, it looks better for me to handle this in diagnoseQualifiedDeclaration(). BTW, the following example is not what I meant:

struct X { struct Y {}; };
export using Z = typename X::Y;

Finally, it looks like we need to touch diagnoseQualifiedDeclaration() after all. Otherwise, it would crash for that example.

ChuanqiXu updated this revision to Diff 384968.Nov 4 2021, 10:05 PM

Updated diagnostic messages. It looks good to me to reject for exporting non-namespace-scope names in diagnoseQualifiedDeclaration().

urnathan added inline comments.Nov 11 2021, 6:08 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807

I think the wording is awkward. 'here' carries the implication there is a somewhere where it would be ok, but there isn't.
'cannot export %0 as it is not at namespace scope'

clang/test/CXX/module/module.interface/p6.cpp
48

um, is that really invalid? it looks to me that one's reexporting an already exported namespace entity. It's perfectly valid to write::
namespace A{ export struct X;}
namespace A { export struct X;}
so I don't see why 'export struct A::X;' is invalid. (I realize this is not a change you;ve made).

also applies to foo.

49

Can we note (a) as the original declaration was not exported, and note where it is?
'cannot export redeclaration %0 as it is not exported'
'note: declared here'

also applies to bar

ChuanqiXu updated this revision to Diff 386728.Nov 11 2021, 9:55 PM
ChuanqiXu retitled this revision from [C++20] [Module] Fix front end crashes when trying to export a qualified entity which is already declared to [C++20] [Module] Fix bug47716 and implement [module.interface]/p6.
ChuanqiXu edited the summary of this revision. (Show Details)
  • Address comments.
  • Implement [module.interface]/p6.

In the process of addressing @urnathan 's comment, I found that it is better to handle [module.interface]/p6 in Sema::CheckRedeclaration* functions. And I add more tests about p6 now. Now it looks much better now.

ChuanqiXu marked 3 inline comments as done.Nov 11 2021, 9:58 PM
ChuanqiXu added inline comments.
clang/test/CXX/module/module.interface/p2-2.cpp
2

I move them here since this is described in p2 instead of p6. I don't merge this into p2.cpp since that I feel it is not harmonious to merge this test with the test in p2 now.

Remove dead codes.

ChuanqiXu retitled this revision from [C++20] [Module] Fix bug47716 and implement [module.interface]/p6 to [C++20] [Module] Fix bug47116 and implement [module.interface]/p6.Nov 11 2021, 10:05 PM
ChuanqiXu updated this revision to Diff 387144.Nov 14 2021, 6:06 PM

Fix a mismatch in test

aaron.ballman added inline comments.Dec 16 2021, 9:11 AM
clang/include/clang/AST/DeclBase.h
610

I think it would be good to add some comments to document the function (as done with surrounding code).

clang/include/clang/Basic/DiagnosticSemaKinds.td
7806–7807
7808–7809

Both of these tweak the formatting somewhat, but the important thing is dropping the trailing punctuation from the diagnostic text.

clang/lib/AST/DeclBase.cpp
999
clang/lib/Sema/SemaDecl.cpp
1631
1652–1654
5797
ChuanqiXu updated this revision to Diff 395045.Dec 16 2021, 7:13 PM

Address comments

ChuanqiXu marked 5 inline comments as done.Dec 16 2021, 7:14 PM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaDecl.cpp
1631

I didn't address this since it belongs to C++20 instead of C++2b.

urnathan accepted this revision.Fri, Jan 21, 5:02 AM

LGTM, thanks for working on this

This revision is now accepted and ready to land.Fri, Jan 21, 5:02 AM
This revision was landed with ongoing or failed builds.Sun, Jan 23, 6:37 PM
This revision was automatically updated to reflect the committed changes.