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

[ChuanqiXu]

Summary

This is an alternative to D129748.

[ChuanqiXu]

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.

[erichkeane]

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

[tahonermann]

clang/lib/Serialization/ASTReaderDecl.cpp

2925–2926

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]

Add more tests.

[ChuanqiXu]

clang/lib/Serialization/ASTReaderDecl.cpp

2925–2926

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.

[ChuanqiXu]

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]

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

2925–2927

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

[tahonermann]

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]

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]

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.

clang/include/clang/Serialization/ASTWriter.h

public:
   unsigned getDeclRefExprAbbrev() const { return DeclRefExprAbbrev; }
   unsigned getCharacterLiteralAbbrev() const { return CharacterLiteralAbbrev; }
   unsigned getIntegerLiteralAbbrev() const { return IntegerLiteralAbbrev; }
   unsigned getExprImplicitCastAbbrev() const { return ExprImplicitCastAbbrev; }
 
   bool hasChain() const { return Chain; }
   ASTReader *getChain() const { return Chain; }
 
+  bool isWritingNamedModules() const {
+    return WritingModule && WritingModule->isModulePurview();
+  }
+
 private:
   // ASTDeserializationListener implementation
   void ReaderInitialized(ASTReader *Reader) override;
   void IdentifierRead(serialization::IdentID ID, IdentifierInfo *II) override;
   void MacroRead(serialization::MacroID ID, MacroInfo *MI) override;
   void TypeRead(serialization::TypeIdx Idx, QualType T) override;
   void SelectorRead(serialization::SelectorID ID, Selector Sel) override;
   void MacroDefinitionRead(serialization::PreprocessedEntityID ID,

clang/lib/Serialization/ASTReaderDecl.cpp

#include "clang/Serialization/AttrPCHRead.inc"
 
   assert(New && "Unable to decode attribute?");
   return New;
 }
 
 /// Reads attributes from the current stream position.
 void ASTRecordReader::readAttributes(AttrVec &Attrs) {
   for (unsigned I = 0, E = readInt(); I != E; ++I)
-    Attrs.push_back(readAttr());
+    if (auto *Attr = readAttr())
+      Attrs.push_back(Attr);
 }
 
 //===----------------------------------------------------------------------===//
 // ASTReader Implementation
 //===----------------------------------------------------------------------===//
 
 /// Note that we have loaded the declaration with the given
 /// Index.
 ///

clang/lib/Serialization/ASTWriter.cpp

 }
 
 //===----------------------------------------------------------------------===//
 // General Serialization Routines
 //===----------------------------------------------------------------------===//
 
 void ASTRecordWriter::AddAttr(const Attr *A) {
   auto &Record = *this;
-  if (!A)
+  // FIXME: Clang can't handle the serialization/deserialization of
+  // preferred_name properly now. See
+  // https://github.com/llvm/llvm-project/issues/56490 for example.
+  if (!A || (isa<PreferredNameAttr>(A) && Writer->isWritingNamedModules()))
     return Record.push_back(0);
+
   Record.push_back(A->getKind() + 1); // FIXME: stable encoding, target attrs
 
   Record.AddIdentifierRef(A->getAttrName());
   Record.AddIdentifierRef(A->getScopeName());
   Record.AddSourceRange(A->getRange());
   Record.AddSourceLocation(A->getScopeLoc());
   Record.push_back(A->getParsedKind());
   Record.push_back(A->getSyntax());

clang/test/Modules/preferred_name.cppm

+// Tests that the ODR check wouldn't produce false-positive result for preferred_name attribute.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/Use.cppm -verify -fsyntax-only
+//
+//--- foo.h
+template<class _CharT>
+class foo_templ;
+
+typedef foo_templ<char> foo;
+
+template<class _CharT>
+class
+__attribute__((__preferred_name__(foo)))
+foo_templ {
+public:
+    foo_templ() {}
+};
+
+inline foo_templ<char> bar()
+{
+  return foo_templ<char>();
+}
+
+//--- A.cppm
+module;
+#include "foo.h"
+export module A;
+
+//--- Use.cppm
+// expected-no-diagnostics
+module;
+#include "foo.h"
+export module Use;
+import A;