[clang] Don't call inheritDefaultTemplateArguments() on CXXDeductionGuideDecl's template parameters

[aeubanks]

Summary

A CXXDeductionGuideDecl references its class's template parameters
(rather than cloning them). This causes us to currently call
inheritDefaultTemplateArguments() on the class template parameters
twice, which causes crashes because DefaultArgStorage::setInherited()
should only be called once.

[aeubanks]

ping

[rnk]

Friendly ping for a modules CTAD bugfix.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

3784–3790

An implicit deduction guide can get template parameters from both the class template and the constructor template. The ones from the class template are not copied and owned by the deduction guide, but the ones from the constructor template are. So I think we may need to do this for *some* of the template parameters and not others. Perhaps we could check to see if the template parameter is actually owned by this template and skip updating it if not.

(Alternatively, I think it'd be fine, and probably simpler and cleaner, to make implicit deduction guide generation always clone the template parameters of the class template. The confusion caused by having the template parameters appear in the "wrong" template is probably not justified by the time / memory savings.)

[aeubanks]

clang/lib/Serialization/ASTReaderDecl.cpp

3784–3790

I tried https://reviews.llvm.org/D116983 and it still doesn't fix this issue, although I'm having trouble understanding the Clang AST and the decl chains.
How would you check that the template parameter is owned by this template?

[ChuanqiXu]

In D114908#3223981, @rnk wrote:

Friendly ping for a modules CTAD bugfix.

Not sure if you met the same problem with me. In our downstream, we did a workaround like:

if (Context.getLangOpts().CPlusPlusModules &&
      To->getOwningModule() != From->getOwningModule())
    return false;

We workarounded the problem by inserting the above checking in inheritDefaultTemplateArgument() in ASTReaderDecl. Hope this helps.
(I understand this is not a good solution)

clang/lib/Serialization/ASTReaderDecl.cpp

#include "clang/AST/DeclNodes.inc"
   // FIXME: In this case, the declaration should only be visible if a module
   //        that makes it visible has been imported.
   D->IdentifierNamespace |=
       Previous->IdentifierNamespace &
       (Decl::IDNS_Ordinary | Decl::IDNS_Tag | Decl::IDNS_Type);
 
   // If the declaration declares a template, it may inherit default arguments
   // from the previous declaration.
-  if (auto *TD = dyn_cast<TemplateDecl>(D))
+  if (auto *TD = dyn_cast<TemplateDecl>(D)) {
+    // CXXDeductionGuideDecls reference the class template parameters so we need
+    // to make sure not to call this twice on the same template parameters.
+    if (!isa<CXXDeductionGuideDecl>(TD->getTemplatedDecl()))
       inheritDefaultTemplateArguments(Reader.getContext(),
                                       cast<TemplateDecl>(Previous), TD);
+  }
 
   // If any of the declaration in the chain contains an Inheritable attribute,
   // it needs to be added to all the declarations in the redeclarable chain.
   // FIXME: Only the logic of merging MSInheritableAttr is present, it should
   // be extended for all inheritable attributes.
   mergeInheritableAttributes(Reader, D, Previous);
 }
 

clang/test/Modules/Inputs/ctad/a.h

+#pragma once
+
+template<typename T=int> struct A {
+  A(T) {}
+};
+
+A _a(2);

clang/test/Modules/Inputs/ctad/b.h

+#pragma once
+
+template<typename T=int> struct A {
+  A(T) {}
+};
+
+A _b(5);

clang/test/Modules/Inputs/ctad/module.modulemap

+module a { header "a.h" export * }
+module b { header "b.h" export * }

clang/test/Modules/ctad.cpp

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c++ -emit-module %S/Inputs/ctad/module.modulemap -fmodule-name=a -o %t/a.pcm -std=c++17
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c++ -emit-module %S/Inputs/ctad/module.modulemap -fmodule-name=b -o %t/b.pcm -std=c++17
+// RUN: %clang_cc1 -fmodules -fno-implicit-modules -x c++ -emit-llvm -I%S/Inputs/ctad -o - %s -fmodule-file=%t/a.pcm -fmodule-file=%t/b.pcm -std=c++17 | FileCheck %s
+
+// CHECK: @_a = global
+// CHECK: @_b = global
+// CHECK: call void @__cxx_global_var_init()
+// CHECK: call void @__cxx_global_var_init.1()
+
+#include "a.h"
+#include "b.h"