Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
DmitryPolukhin closed this revision. DmitryPolukhin added a comment. Committed as https://reviews.llvm.org/rL282059 https://reviews.llvm.org/D24704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
DmitryPolukhin added a comment. Richard, please take another look. https://reviews.llvm.org/D24704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
DmitryPolukhin updated this revision to Diff 71778.
DmitryPolukhin marked an inline comment as done.
https://reviews.llvm.org/D24704
Files:
lib/AST/ItaniumMangle.cpp
test/CodeGenCXX/mangle-abi-tag.cpp
Index: test/CodeGenCXX/mangle-abi-tag.cpp
===
--- test/CodeGenCXX/mangle-abi-tag.cpp
+++ test/CodeGenCXX/mangle-abi-tag.cpp
@@ -203,3 +203,19 @@
}
// A18::operator A[abi:A][abi:B]() but GCC adds the same tags twice!
// CHECK-DAG: define linkonce_odr {{.+}} @_ZN3A18cv1AB1AB1BEv(
+
+namespace N19 {
+ class A {};
+ class __attribute__((abi_tag("B"))) B {};
+ class D {};
+ class F {};
+
+ template
+ class C {};
+
+ B foo(A, D);
+}
+void f19_test(N19::C, N19::F, N19::D) {
+}
+// f19_test(N19::C, N19::F, N19::D)
+// CHECK-DAG: define void
@_Z8f19_testN3N191CINS_1AEXadL_ZNS_3fooB1BES1_NS_1DENS_1FES2_(
Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -405,12 +405,14 @@
CXXNameMangler(CXXNameMangler &Outer, raw_ostream &Out_)
: Context(Outer.Context), Out(Out_), NullOut(false),
Structor(Outer.Structor), StructorType(Outer.StructorType),
-SeqID(Outer.SeqID), AbiTagsRoot(AbiTags) {}
+SeqID(Outer.SeqID), AbiTagsRoot(AbiTags),
+Substitutions(Outer.Substitutions) {}
CXXNameMangler(CXXNameMangler &Outer, llvm::raw_null_ostream &Out_)
: Context(Outer.Context), Out(Out_), NullOut(true),
Structor(Outer.Structor), StructorType(Outer.StructorType),
-SeqID(Outer.SeqID), AbiTagsRoot(AbiTags) {}
+SeqID(Outer.SeqID), AbiTagsRoot(AbiTags),
+Substitutions(Outer.Substitutions) {}
#if MANGLE_CHECKER
~CXXNameMangler() {
@@ -458,6 +460,8 @@
void addSubstitution(QualType T);
void addSubstitution(TemplateName Template);
void addSubstitution(uintptr_t Ptr);
+ // Destructive copy substitutions from other mangler.
+ void extendSubstitutions(CXXNameMangler* Other);
void mangleUnresolvedPrefix(NestedNameSpecifier *qualifier,
bool recursive = false);
@@ -685,6 +689,10 @@
// Output name with implicit tags and function encoding from temporary
buffer.
mangleNameWithAbiTags(FD, &AdditionalAbiTags);
Out << FunctionEncodingStream.str().substr(EncodingPositionStart);
+
+ // Function encoding could create new substitutions so we have to add
+ // temp mangled substitutions to main mangler.
+ extendSubstitutions(&FunctionEncodingMangler);
}
void CXXNameMangler::mangleFunctionEncodingBareType(const FunctionDecl *FD) {
@@ -4426,6 +4434,14 @@
Substitutions[Ptr] = SeqID++;
}
+void CXXNameMangler::extendSubstitutions(CXXNameMangler* Other) {
+ assert(Other->SeqID >= SeqID && "Must be superset of substitutions!");
+ if (Other->SeqID > SeqID) {
+Substitutions.swap(Other->Substitutions);
+SeqID = Other->SeqID;
+ }
+}
+
CXXNameMangler::AbiTagList
CXXNameMangler::makeFunctionReturnTypeTags(const FunctionDecl *FD) {
// When derived abi tags are disabled there is no need to make any list.
Index: test/CodeGenCXX/mangle-abi-tag.cpp
===
--- test/CodeGenCXX/mangle-abi-tag.cpp
+++ test/CodeGenCXX/mangle-abi-tag.cpp
@@ -203,3 +203,19 @@
}
// A18::operator A[abi:A][abi:B]() but GCC adds the same tags twice!
// CHECK-DAG: define linkonce_odr {{.+}} @_ZN3A18cv1AB1AB1BEv(
+
+namespace N19 {
+ class A {};
+ class __attribute__((abi_tag("B"))) B {};
+ class D {};
+ class F {};
+
+ template
+ class C {};
+
+ B foo(A, D);
+}
+void f19_test(N19::C, N19::F, N19::D) {
+}
+// f19_test(N19::C, N19::F, N19::D)
+// CHECK-DAG: define void @_Z8f19_testN3N191CINS_1AEXadL_ZNS_3fooB1BES1_NS_1DENS_1FES2_(
Index: lib/AST/ItaniumMangle.cpp
===
--- lib/AST/ItaniumMangle.cpp
+++ lib/AST/ItaniumMangle.cpp
@@ -405,12 +405,14 @@
CXXNameMangler(CXXNameMangler &Outer, raw_ostream &Out_)
: Context(Outer.Context), Out(Out_), NullOut(false),
Structor(Outer.Structor), StructorType(Outer.StructorType),
-SeqID(Outer.SeqID), AbiTagsRoot(AbiTags) {}
+SeqID(Outer.SeqID), AbiTagsRoot(AbiTags),
+Substitutions(Outer.Substitutions) {}
CXXNameMangler(CXXNameMangler &Outer, llvm::raw_null_ostream &Out_)
: Context(Outer.Context), Out(Out_), NullOut(true),
Structor(Outer.Structor), StructorType(Outer.StructorType),
-SeqID(Outer.SeqID), AbiTagsRoot(AbiTags) {}
+SeqID(Outer.SeqID), AbiTagsRoot(AbiTags),
+Substitutions(Outer.Substitutions) {}
#if MANGLE_CHECKER
~CXXNameMangler() {
@@ -458,6 +460,8 @@
void addSubstitution(QualType T);
void addSubstitution(TemplateName Template);
void addSubstitution(uintptr_t Ptr);
+ // Destructive copy substitutions from other mangler.
+ void extendSubstitutions(CXXNameMa
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
DmitryPolukhin added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:668 @@ -664,3 +667,3 @@ llvm::raw_svector_ostream FunctionEncodingStream(FunctionEncodingBuf); CXXNameMangler FunctionEncodingMangler(*this, FunctionEncodingStream); // Output name of the function. rsmith wrote: > DmitryPolukhin wrote: > > rsmith wrote: > > > Maybe it'd be simpler to just override the output stream here rather than > > > creating a new mangler? > > I'm not sure that it is simpler because it will also require substitutions > > save/restore for name mangling (mangleNameWithAbiTags) to produce the same > > substitutions twice (without implicate abi_tags and later with implicit > > abi_tags). IMHO, it is almost identical approaches but current one is a bit > > cleaner because it copies explicitly everything required from temp to outer > > mangler. > I think we'd want to override the output stream to write to a temporary > buffer at the point when we would otherwise write out the ABI tags, to avoid > needing to save/restore any substitutions. But I agree, that seems more > invasive than what you're doing here. I'll leave this up to you. It is significant redesign and additional complexity to remember which exactly ABI tags we would like to rewrite later (it may not be the first call of writeAbiTags for current mangler). I would keep it as is. https://reviews.llvm.org/D24704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
DmitryPolukhin added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:668 @@ -664,3 +667,3 @@ llvm::raw_svector_ostream FunctionEncodingStream(FunctionEncodingBuf); CXXNameMangler FunctionEncodingMangler(*this, FunctionEncodingStream); // Output name of the function. rsmith wrote: > Maybe it'd be simpler to just override the output stream here rather than > creating a new mangler? I'm not sure that it is simpler because it will also require substitutions save/restore for name mangling (mangleNameWithAbiTags) to produce the same substitutions twice (without implicate abi_tags and later with implicit abi_tags). IMHO, it is almost identical approaches but current one is a bit cleaner because it copies explicitly everything required from temp to outer mangler. https://reviews.llvm.org/D24704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
rsmith added inline comments.
Comment at: lib/AST/ItaniumMangle.cpp:668
@@ -664,3 +667,3 @@
llvm::raw_svector_ostream FunctionEncodingStream(FunctionEncodingBuf);
CXXNameMangler FunctionEncodingMangler(*this, FunctionEncodingStream);
// Output name of the function.
DmitryPolukhin wrote:
> rsmith wrote:
> > Maybe it'd be simpler to just override the output stream here rather than
> > creating a new mangler?
> I'm not sure that it is simpler because it will also require substitutions
> save/restore for name mangling (mangleNameWithAbiTags) to produce the same
> substitutions twice (without implicate abi_tags and later with implicit
> abi_tags). IMHO, it is almost identical approaches but current one is a bit
> cleaner because it copies explicitly everything required from temp to outer
> mangler.
I think we'd want to override the output stream to write to a temporary buffer
at the point when we would otherwise write out the ABI tags, to avoid needing
to save/restore any substitutions. But I agree, that seems more invasive than
what you're doing here. I'll leave this up to you.
Comment at: lib/AST/ItaniumMangle.cpp:4439
@@ +4438,3 @@
+ if (Other.SeqID > SeqID) {
+Substitutions = Other.Substitutions;
+SeqID = Other.SeqID;
Please avoid the cost of a full copy here by making this a destructive
operation on `Other` -- use `swap` or move-assignment here. (We'll still be
paying for one copy of the substitution set per function mangling, which is
unfortunate / undesirable.)
https://reviews.llvm.org/D24704
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24704: PR30401: Fix substitutions for functions with abi_tag
rsmith added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:668 @@ -664,3 +667,3 @@ llvm::raw_svector_ostream FunctionEncodingStream(FunctionEncodingBuf); CXXNameMangler FunctionEncodingMangler(*this, FunctionEncodingStream); // Output name of the function. Maybe it'd be simpler to just override the output stream here rather than creating a new mangler? https://reviews.llvm.org/D24704 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
