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

Mon Jul 18 12:25:56 PDT 2022

aaron.ballman added a comment.

In D129748#3658657 <https://reviews.llvm.org/D129748#3658657>, @ChuanqiXu wrote:

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

All of the attributes I pointed to take a `TypeArgument`, so I'm still not certain I understand why you think these wouldn't be similarly impacted...

>> 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`:

The argument type is `TypeArgument`, not `TypdefType`. The guts of the reading code for the preferred name attribute *should* be:

  case attr::PreferredName: {
    bool isInherited = Record.readInt();
    bool isImplicit = Record.readInt();
    bool isPackExpansion = Record.readInt();
    TypeSourceInfo * typedefType = Record.readTypeSourceInfo();
    New = new (Context) PreferredNameAttr(Context, Info, typedefType);
    cast<InheritableAttr>(New)->setInherited(isInherited);
    New->setImplicit(isImplicit);
    New->setPackExpansion(isPackExpansion);
    break;
  }

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

I'm still not quite seeing that this only impacts one attribute. I *think* this would happen for the others listed if the attribute argument was similarly a typedef as with your example using `preferred_name`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129748