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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3784-3790
+  if (auto *TD = dyn_cast(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(TD->getTemplatedDecl()))
+  inheritDefaultTemplateArguments(Reader.getContext(),
+  cast(Previous), TD);
+  }

rsmith wrote:
> 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.)
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3784-3790
+  if (auto *TD = dyn_cast(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(TD->getTemplatedDecl()))
+  inheritDefaultTemplateArguments(Reader.getContext(),
+  cast(Previous), TD);
+  }

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Friendly ping for a modules CTAD bugfix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114908

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks created this revision.
aeubanks added a reviewer: rsmith.
aeubanks requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114908

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/ctad/a.h
  clang/test/Modules/Inputs/ctad/b.h
  clang/test/Modules/Inputs/ctad/module.modulemap
  clang/test/Modules/ctad.cpp


Index: clang/test/Modules/ctad.cpp
===
--- /dev/null
+++ clang/test/Modules/ctad.cpp
@@ -0,0 +1,12 @@
+// 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"
Index: clang/test/Modules/Inputs/ctad/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/module.modulemap
@@ -0,0 +1,2 @@
+module a { header "a.h" export * }
+module b { header "b.h" export * }
Index: clang/test/Modules/Inputs/ctad/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/b.h
@@ -0,0 +1,7 @@
+#pragma once
+
+template struct A {
+  A(T) {}
+};
+
+A _b(5);
Index: clang/test/Modules/Inputs/ctad/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/a.h
@@ -0,0 +1,7 @@
+#pragma once
+
+template struct A {
+  A(T) {}
+};
+
+A _a(2);
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3781,9 +3781,13 @@
 
   // If the declaration declares a template, it may inherit default arguments
   // from the previous declaration.
-  if (auto *TD = dyn_cast(D))
-inheritDefaultTemplateArguments(Reader.getContext(),
-cast(Previous), TD);
+  if (auto *TD = dyn_cast(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(TD->getTemplatedDecl()))
+  inheritDefaultTemplateArguments(Reader.getContext(),
+  cast(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.


Index: clang/test/Modules/ctad.cpp
===
--- /dev/null
+++ clang/test/Modules/ctad.cpp
@@ -0,0 +1,12 @@
+// 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"
Index: clang/test/Modules/Inputs/ctad/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/module.modulemap
@@ -0,0 +1,2 @@
+module a { header "a.h" export * }
+module b { header "b.h" export * }
Index: clang/test/Modules/Inputs/ctad/b.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/b.h
@@ -0,0 +1,7 @@
+#pragma once
+
+template struct A {
+  A(T) {}
+};
+
+A _b(5);
Index: clang/test/Modules/Inputs/ctad/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/ctad/a.h
@@ -0,0 +1,7 @@
+#pragma once
+
+template struct A {
+  A(T) {}
+};
+
+A _a(2);
Index: clang/lib/Serialization/ASTReaderDecl.cpp