[PATCH] D129748: [Modules] Disable preferred_name attribute in C++20 Modules

Sun Jul 17 20:18:46 PDT 2022

ChuanqiXu added a comment.

In D129748#3654918 <https://reviews.llvm.org/D129748#3654918>, @aaron.ballman wrote:

> For example, I would be IBOutletCollection, OwnerAttr, PointerAttr, and TypeTagForDatatypeAttr all behave the same way as they all take a type argument. I don't think we want to selectively disable so many attributes (for example, disallowing owner and pointer attributes means we lose out on C++ Core Guideline features that people will likely expect to be able to use with modules.

I agree that there may be other problematic cases. But all of `IBOutletCollection`,`OwnerAttr`, `PointerAttr` and  `TypeTagForDatatypeAttr` wouldn't meet the same problem. And from what I can see, now `PreferredName` is special indeed. Since `PreferredName` is the only attribute whose argument type is `TypeArgument<"TypedefType">`. The reason is stated below.

In D129748#3655572 <https://reviews.llvm.org/D129748#3655572>, @tahonermann wrote:

> I'm not following what the root problem is. You stated:
>
>> When we write the attribute preferred_name(foo) in ASTWriter, the compiler would try to write the type for the argument foo. Then when the compiler write the type for foo, the compiler find the type for foo is a TypeDef type. So the compiler would write the corresponding type foo_templ<char>. The key point here is that the AST for foo_templ<char> is complete now. Since the AST for foo_templ<char> is constructed in Sema.
>
> Are you saying that the act of writing the AST is triggering the instantiation/completion of `foo_templ<char>`? Serialization should certainly not cause such side effects.

No, I don't mention that.

> I'm likewise not following the concern with reading the AST:
>
>> When we read the attribute preferred_name(foo), we would read the type for the argument foo and then we would try to read the type foo_templ<char> later. However, the key problem here is that when we read foo_templ<char>, its AST is not constructed yet! So we get a different type with the writer writes. So here is the ODR violation.
>
> I wouldn't expect the same `Sema` instance to be used to both write and read the same AST.

Yeah, I wouldn't expect that too.

> Can you try again to explain the root problem? Perhaps with references to relevant code?

Yeah, following of  is my explanation to the root problem. It might be a little bit long. First, for the following code:

  C++
  template<class _CharT>
  class basic_string_view;

  typedef basic_string_view<char> string_view;

  template<class _CharT>
  class
  __attribute__((__preferred_name__(string_view)))
  basic_string_view {
  public:
      basic_string_view() 
      {
      }
  };

  inline basic_string_view<char> foo()
  {
    return basic_string_view<char>();
  }

The dumped AST would be:

  |-ClassTemplateDecl 0x7913ac8 <./string_view.h:1:1, line:2:7> col:7 in A.<global> hidden basic_string_view
  | |-TemplateTypeParmDecl 0x7913950 <line:1:10, col:16> col:16 in A.<global> hidden class depth 0 index 0 _CharT
  | |-CXXRecordDecl 0x7913a18 <line:2:1, col:7> col:7 in A.<global> hidden class basic_string_view
  | `-ClassTemplateSpecializationDecl 0x7913cd0 <line:1:1, line:2:7> line:9:1 in A.<global> class basic_string_view definition
  |   |-DefinitionData pass_in_registers empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
  |   | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial constexpr
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c77b0 'char'
  |   |-PreferredNameAttr 0x79143c8 <line:8:16, col:46> string_view
  |   |-CXXRecordDecl 0x7914698 <line:7:1, line:9:1> col:1 in A.<global> implicit class basic_string_view
  |   |-AccessSpecDecl 0x7914748 <line:10:1, col:7> col:1 in A.<global> public
  |   |-CXXConstructorDecl 0x7935158 <line:11:5, line:13:5> line:11:5 in A.<global> used basic_string_view 'void ()'
  |   | `-CompoundStmt 0x79143a8 <line:12:5, line:13:5>
  |   |-CXXConstructorDecl 0x7935300 <line:9:1> col:1 in A.<global> implicit constexpr basic_string_view 'void (const string_view &)' inline default trivial noexcept-unevaluated 0x7935300
  |   | `-ParmVarDecl 0x7935420 <col:1> col:1 in A.<global> 'const string_view &'
  |   |-CXXConstructorDecl 0x79354d0 <col:1> col:1 in A.<global> implicit constexpr basic_string_view 'void (string_view &&)' inline default trivial noexcept-unevaluated 0x79354d0
  |   | `-ParmVarDecl 0x79355f0 <col:1> col:1 in A.<global> 'string_view &&'
  |   `-CXXDestructorDecl 0x79357d0 <col:1> col:1 in A.<global> implicit referenced constexpr ~basic_string_view 'void () noexcept' inline default trivial
  |-TypedefDecl 0x7913e78 <line:4:1, col:33> col:33 in A.<global> hidden referenced string_view 'basic_string_view<char>':'string_view'
  | `-TemplateSpecializationType 0x7913df0 'basic_string_view<char>' sugar basic_string_view
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c77b0 'char'
  |   `-RecordType 0x7913dd0 'string_view'
  |     `-ClassTemplateSpecialization 0x7913cd0 'basic_string_view'
  |-ClassTemplateDecl 0x7914040 prev 0x7913ac8 <line:6:1, line:14:1> line:9:1 in A.<global> hidden basic_string_view
  | |-TemplateTypeParmDecl 0x7913ed8 <line:6:10, col:16> col:16 in A.<global> hidden class depth 0 index 0 _CharT
  | |-CXXRecordDecl 0x7913fa8 prev 0x7913a18 <line:7:1, line:14:1> line:9:1 in A.<global> hidden class basic_string_view definition
  | | |-DefinitionData empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
  | | | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment simple trivial has_const_param needs_implicit implicit_has_const_param
  | | | |-MoveAssignment exists simple trivial needs_implicit
  | | | `-Destructor simple irrelevant trivial constexpr needs_implicit
  | | |-PreferredNameAttr 0x7914100 <line:8:16, col:46> string_view
  | | |-CXXRecordDecl 0x7914168 <line:7:1, line:9:1> col:1 in A.<global> hidden implicit referenced class basic_string_view
  | | |-AccessSpecDecl 0x7914218 <line:10:1, col:7> col:1 in A.<global> public
  | | `-CXXConstructorDecl 0x79142d0 <line:11:5, line:13:5> line:11:5 in A.<global> hidden basic_string_view<_CharT> 'void ()'
  | |   `-CompoundStmt 0x79143a8 <line:12:5, line:13:5>
  | `-ClassTemplateSpecialization 0x7913cd0 'basic_string_view'

Then when ASTReader reads `ClassTemplateDecl` at `0x7913ac8`, the ASTReader would read `ClassTemplateSpecializationDecl` at `0x7913cd0`. Then the ASTReader would try to read the `PreferredNameAttr` at `0x79143c8`. This happens at https://github.com/llvm/llvm-project/blob/557a471ab353d2be067d12b8cc352706ab089fc5/clang/lib/Serialization/ASTReaderDecl.cpp#L594-L600. An important note here is that the ASTReader reads the attributes in the very earlier state so that the ASTReader haven't read the name! (The ASTReader would read the name of Decl in `ASTReaderDecl::VisitNamedDecl`, which would happen after `ASTReaderDecl::VisitDecl`.)

When the ASTReader reads `PreferredNameAttr`, it would try to read the TypedefType since it's the argument type of `PreferredNameAttr`. The reading code is generated from `clang/include/clang/AST/TypeProperties.td`:

  C++
  QualType readTypedefType() {
      auto &ctx = R.getASTContext();
      Decl* declaration = R.find("declaration").readDeclRef();
      llvm::Optional<QualType> canonicalType = R.find("canonicalType").template readOptional<QualType>();

      QualType finalCanonicalType =
        canonicalType ? ctx.getCanonicalType(*canonicalType)
                      : QualType();
      return ctx.getTypedefType(cast<TypedefNameDecl>(declaration),
                                finalCanonicalType);

  }

The problem comes when we read the canonical type. The ASTReader would try to read `RecordType` at  `0x7913dd0`:

  C++
  QualType readRecordType() {
      auto &ctx = R.getASTContext();
      bool dependent = R.find("dependent").readBool();
      Decl* declaration = R.find("declaration").readDeclRef();

      auto record = cast<RecordDecl>(declaration);
      QualType result = ctx.getRecordType(record);
      if (dependent)
        const_cast<Type *>(result.getTypePtr())
            ->addDependence(TypeDependence::DependentInstantiation);
      return result;

    }

Note the third line of the function, it would try to read the declaration! And from the above AST, we know the ASTReader would read `ClassTemplateSpecializationDecl` at `0x7913cd0`. And this is what we're reading! Note that we wouldn't fall in an infinite loop since we would do a quick return in https://github.com/llvm/llvm-project/blob/bd228a17727ec0f51f8606bbcfb4fe65aebbd0dd/clang/lib/Serialization/ASTReader.cpp#L7488-L7494. But we still haven't completed the reading of `ClassTemplateSpecializationDecl` at `0x7913cd0`. We only started at the very early stage. So the type of that RecordType is broken.

Long story short, the key problem here is that ASTReader don't take care of the dependent loop. It looks like we can't fix the problem by a simple patch. And the problem itself is really small. Currently only `PreferredNameAttr` would trigger the problem. So it is an option to me to ignore the attribute in the specific case.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129748/new/

https://reviews.llvm.org/D129748