[patch] Add support for putting constructors and destructos in explicit comdats

[Reid Kleckner]

I've always wished clang-format would make these into tables to reduce the
length, but oh well. :(

+inline CXXCtorType toCXXCtorType(StructorType T) {
+  switch (T) {
+  case StructorType::Complete:
+    return Ctor_Complete;
+  case StructorType::Base:
+    return Ctor_Base;
+  case StructorType::Deleting:
+    llvm_unreachable("foo");

The message should have some meaning, i.e. cannot have deleting ctor.

+  }
+}
+
+inline StructorType getFromCtorType(CXXCtorType T) {
+  switch (T) {
+  case Ctor_Complete:
+    return StructorType::Complete;
+  case Ctor_Base:
+    return StructorType::Base;
+  case Ctor_CompleteAllocating:
+    llvm_unreachable("foo");

I didn't realize we even had this enum. Anyway, "invalid enum" seems like a
better message.

+  }
+}
+
+inline CXXDtorType toCXXDtorType(StructorType T) {
+  switch (T) {
+  case StructorType::Complete:
+    return Dtor_Complete;
+  case StructorType::Base:
+    return Dtor_Base;
+  case StructorType::Deleting:
+    return Dtor_Deleting;
+  }
+}
+
+inline StructorType getFromDtorType(CXXDtorType T) {
+  switch (T) {
+  case Dtor_Deleting:
+    return StructorType::Deleting;
+  case Dtor_Complete:
+    return StructorType::Complete;
+  case Dtor_Base:
+    return StructorType::Base;
+  }
+}

If you want to satisfy MSVC I believe you have to put llvm_unreachable at
the end of all of these functions. I don't think it does switch case
coverage analysis.

Otherwise, the refactoring side looks good.

-----


+enum CXXCtorComdatType { Ctor_Comdat = Ctor_CompleteAllocating + 1 };
+
 /// \brief C++ destructor types.
 enum CXXDtorType {
     Dtor_Deleting, ///< Deleting dtor
@@ -34,6 +36,8 @@ enum CXXDtorType {
     Dtor_Base      ///< Base object dtor
 };

+enum CXXDtorComdatType { Dtor_Comdat = Dtor_Base + 1 };

Any strong reason to keep these out of the CXXCtorType and CXXDtorType
enums? We have very few switches over them. Especially in the ctor case,
where we only implement Ctor_Base and Ctor_Complete.

+  virtual void mangleCXXCtorComdat(const CXXConstructorDecl *D,
raw_ostream &);
+  virtual void mangleCXXDtorComdat(const CXXDestructorDecl *D, raw_ostream
&);

These methods are Itanium-specific, so they can live on
ItaniumMangleContext in the same header. We get a little more type safety
that way, since the caller has to explicitly downcast to
ItaniumMangleContext if they want to call this method.

+  // The complete constructor is equivalent to the base constructor
+  // for classes with no virtual bases.
+  if (MD->getParent()->getNumVBases())
+    return StructorCodegen::Emit;

Maybe flip the logic of the comment to match the code, like "The complete
and base structors are not equivalent if there are any virtual bases, so
emit separate functions."?

The logic of emitCXXStructor and getCodegenToUse is starting to feel very
Itanium-specific, because it has this requirement that if you must emit the
structor in one TU, you must emit all structors. The MS ABI doesn't have
this constraint. First, there are no ctor variants, and for dtors, anyone
using a complete dtor or deleting dtor is required to emit it like an
inline function if they call it. Speaking of which, we never call deleting
dtors, so far as I can tell. So, I wonder if we should push emitCXXStructor
through the CGCXXABI interface.

+  if (CompleteCG == StructorCodegen::COMDAT) {
+    SmallString<256> Buffer;
+    llvm::raw_svector_ostream Out(Buffer);
+    if (DD)
+      ABI->getMangleContext().mangleCXXDtorComdat(DD, Out);
+    else
+      ABI->getMangleContext().mangleCXXCtorComdat(CD, Out);
+    llvm::Comdat *C = TheModule.getOrInsertComdat(Out.str());
+    Fn->setComdat(C);
+  }

If you push emitCXXStructor into CGCXXABI, the above code will live in
ItaniumCXXABI.cpp, and you can access the Itanium-specific
mangleCXXCtorComdat mangler methods.


On Thu, Sep 4, 2014 at 2:19 PM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> On 4 September 2014 14:41, Reid Kleckner <rnk at google.com> wrote:
> > I don't like calling arrangeCXXDtorDeclaration from ctor-related
> functions.
> > It looks like a typo. In the past I've used 'structor' as the generic
> term
> > for ctors and dtors. You can see it in the Itanium mangling code, but I
> > don't think it's very widely used. Does that seem like a better
> > nomenclature? Then we would probably have StructorType, etc.
>
> The attached patches uses the Structor nomenclature.
>
> > I'm excited to finally have this functionality, though. :)
>
> +1 :-)
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140904/d7684464/attachment.html>