This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Modules] Disable preferred_name when writing a C++20 Module interface
ClosedPublic

Authored by ChuanqiXu on Jul 21 2022, 10:22 PM.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 21 2022, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 10:22 PM
ChuanqiXu requested review of this revision.Jul 21 2022, 10:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 10:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ChuanqiXu added inline comments.Jul 21 2022, 10:34 PM
clang/lib/Serialization/ASTWriter.cpp
4353

The Writer->isWritingNamedModules() part is necessary. Otherwise we would break the https://github.com/llvm/llvm-project/blob/main/clang/test/PCH/decl-attrs.cpp test. The reason why the bug is not found by the user of PCH or clang modules is that a header generally would be guarded by #ifndef ... #define fashion. And if we remove the guard, the compiler would emit an error for duplicated definition. So the problem only lives in C++20 Named Modules.

So this is perhaps the 'most acceptable' version of this workaround or me. What do you think @aaron.ballman and @tahonermann ?

tahonermann added inline comments.Jul 22 2022, 9:38 AM
clang/lib/Serialization/ASTReaderDecl.cpp
2929–2931

Interesting, this looks like a pre-existing issue. It looks like readBTFTypeTagAttr() in clang/include/clang/Serialization/ASTRecordReader.h has an assumption that readAttr() always returns non-null. I wonder if that should be modified to use cast_or_null rather than cast. I don't see any need to address that issue (if it actually exists) in this review though.

clang/lib/Serialization/ASTWriter.cpp
4353

Correct me if I'm mistaken, but I think this issue only occurs because, in the test, both modules have the problematic declarations in the global module fragment; thus creating duplicate definitions that have to be merged which then exposes the ODR mismatch.

I'm suspicious that this actually fixes all possible scenarios. For example:

//--- X1.cpp
#include "foo.h"
import A;

//--- X2.cpp
import A;
#include "foo.h"

I would expect the proposed change to cause an ODR issue in these scenarios since the definition from the module still needs to be merged in non-modular TUs, but now the imported module will lack the attribute present in the non-modular TUs.

ChuanqiXu updated this revision to Diff 447185.Jul 24 2022, 8:20 PM

Add more tests.

ChuanqiXu added inline comments.Jul 24 2022, 8:31 PM
clang/lib/Serialization/ASTReaderDecl.cpp
2929–2931

Yeah, it surprises me too.

clang/lib/Serialization/ASTWriter.cpp
4353

Correct me if I'm mistaken, but I think this issue only occurs because, in the test, both modules have the problematic declarations in the global module fragment; thus creating duplicate definitions that have to be merged which then exposes the ODR mismatch.

I am not sure if I followed. If you are referring to why this problem only exists in C++20 Named Modules, I think you are correct. Other modules (Clang modules, C++20 Header units) don't have global modules.

I'm suspicious that this actually fixes all possible scenarios. For example:

I've added the two examples below. I understand this is confusing at the first sight. There are two cases.
(1) For X1.cpp, we do ODR checks in ASTReaders by calling ASTContext::isSameEntity. And ASTContext::isSameEntity wouldn't check for attributes. (Another defect?)
(2) For X2.cpp, we do ODR checks in Sema. And it would emit a warning as the tests shows.

So as a conclusion, the current implementation works 'not bad' currently. But I agree that it might bad in the future. Especially WG21 are going to disallow the compilers to ignore the semantics of attributes.

Sorry for the frequent ping since 15.x is going to be branched. @aaron.ballman @tahonermann I know this might not look good to you. But I want to ask if there is any objection? Personally, the preferred_name attribute is not so important and it blocks something important. I really want to get this landed. And as @erichkeane said in D129748, I'll take an eye on this.

aaron.ballman accepted this revision.Jul 26 2022, 8:00 AM

I'm okay with this approach, but please be sure to add a release note explaining the behavior and update AttrDocs.td as well, so that users have some notice as to what's broken.

clang/lib/Serialization/ASTReaderDecl.cpp
2929–2931

Some IDEs do poorly when a variable has the same name as a type (for things like jump to definition, etc).

This revision is now accepted and ready to land.Jul 26 2022, 8:00 AM

I guess this is probably ok as a short term fix for the Clang 15 release. It still makes me nervous though.

clang/lib/Serialization/ASTWriter.cpp
4353

I am not sure if I followed. If you are referring to why this problem only exists in C++20 Named Modules, I think you are correct.

I was just trying to summarize the root cause again; to make sure I understand when and why the problem occurs.

(1) For X1.cpp, we do ODR checks in ASTReaders by calling ASTContext::isSameEntity. And ASTContext::isSameEntity wouldn't check for attributes. (Another defect?)

Yeah, that strikes me as likely being another defect; In your added test cases, I suspect we should be doing ODR checks for both Use1.cpp and Use2.cpp (which would then produce a consistent ODR error instead of the asymmetric warning that is currently issued).

tahonermann accepted this revision.Jul 26 2022, 8:23 AM
tahonermann added inline comments.
clang/lib/Serialization/ASTWriter.cpp
4353

@ChuanqiXu and I chatted briefly during today's biweekly Clang Modules call. He was able to explain that the Use2.cpp test case also encounters the reported problem without his patch. That suffices to address my major concerns, so I'll approve.

I do hope we can continue to investigate and address the root cause of the problem though as I'm sure this issue will bite again.

ChuanqiXu added inline comments.Jul 26 2022, 8:38 AM
clang/lib/Serialization/ASTWriter.cpp
4353

For X1.cpp, we do ODR checks in ASTReaders by calling ASTContext::isSameEntity. And ASTContext::isSameEntity wouldn't check for attributes. (Another defect?)

Oh, my bad writing. The another defect refers that we don't check attributes in ASTReaders. It may surprise some people. (But it may be expected due to some people think attributes as a hint instead of a semantical entity.)

I do hope we can continue to investigate and address the root cause of the problem though as I'm sure this issue will bite again.

Yeah, I'll take an eye on this. Although there are many TODOs, I feel like we could fix the root problem in clang16.

This revision was landed with ongoing or failed builds.Jul 26 2022, 8:59 AM
This revision was automatically updated to reflect the committed changes.