[PATCH] MS RTTI Generation

Warren Hunt whunt at google.com
Wed May 21 16:07:24 PDT 2014 

New patch coming.

================
Comment at: lib/CodeGen/CGRTTI.cpp:994
@@ +993,3 @@
+
+/// \brief A Helper class for building MS RTTI types.
+struct MSRTTIBuilder {
----------------
Timur Iskhodzhanov wrote:
> I think an overview comment describing basic ideas, concepts and relations would help here?
> You may also consider splitting these ~400LOC into a separate file.
This can be done, I'll need to move the shared functionality into CodeGenModule.

================
Comment at: lib/CodeGen/CGRTTI.cpp:995
@@ +994,3 @@
+/// \brief A Helper class for building MS RTTI types.
+struct MSRTTIBuilder {
+  /// \brief A Helper class that stores information about a base class for the
----------------
Timur Iskhodzhanov wrote:
> make it a `class`
> http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords
yeah, fixed, I always forget this.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1055
@@ +1054,3 @@
+// Used by MSRTTIBuilder::BaseClass::BaseClass
+static int countNonVirtualBases(const CXXRecordDecl *RD) {
+  int Count = 0;
----------------
Timur Iskhodzhanov wrote:
> how about
> 
>   static unsigned countNonVirtualBases(const CXXRecordDecl *RD) {
>     unsigned Count = RD->getNumBases();
>     // Comment explaining why whe should do it recursively.
>     for (const CXXBaseSpecifier &Base : RD->bases())
>       Count += countNonVirtualBases(Base.getType()->getAsCXXRecordDecl());
>     return Count;
>   }
> 
> ?
This feels like a nit.  I can make the change but I don't think there's a strong argument to be made here.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1056
@@ +1055,3 @@
+static int countNonVirtualBases(const CXXRecordDecl *RD) {
+  int Count = 0;
+  for (const CXXBaseSpecifier &Base : RD->bases())
----------------
David Majnemer wrote:
> This should be `unsigned` to match `CXXRecordDecl::getNumBases`
fixed.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1065-1066
@@ +1064,4 @@
+                                    const CXXBaseSpecifier *Specifier)
+    : RD(RD), Flags(HasHierarchyDescriptor), VirtualRoot(0), OffsetInVbase(0),
+      NumBases(countNonVirtualBases(RD)) {
+  if (!Parent)
----------------
David Majnemer wrote:
> Initialize these in the order you declare them in the class definition, otherwise clang will warn on this.
done.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1092
@@ +1091,3 @@
+  // Forward declare the ClassHierarchyDescriptor because of a cycle in the
+  // rtti types.
+  ClassHierarchyDescriptorType =
----------------
Timur Iskhodzhanov wrote:
> s/rtti/RTTI/?
fixed.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1095
@@ +1094,3 @@
+      llvm::StructType::create(VMContext, "MSRTTIClassHierarchyDescriptor");
+  // Describe the BaseClassDescriptor Type.
+  SmallVector<llvm::Type *, 16> FieldTypes;
----------------
Timur Iskhodzhanov wrote:
> I think adding vertical whitespace between logical blocks would improve the readability
Sure, I have no strong opinion.  My current styling is to avoid vertical white-space and use comments instead, but as I said, no strong opinion.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1097
@@ +1096,3 @@
+  SmallVector<llvm::Type *, 16> FieldTypes;
+  FieldTypes.push_back(CGM.Int8PtrTy);
+  FieldTypes.push_back(CGM.IntTy);
----------------
Timur Iskhodzhanov wrote:
> Is there a (high-level?) comment explaining these structures?
Will add.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1130
@@ +1129,3 @@
+  SmallString<256> TypeDescriptorMangledName, TypeInfoString;
+  llvm::raw_svector_ostream Out(TypeDescriptorMangledName);
+  Mangler.mangleCXXRTTI(Type, Out);
----------------
Timur Iskhodzhanov wrote:
> I remember there was some cool trick to avoid flushing manually -- probably put {} around the stream definition and usage? This also improves the locality of the variables.
Yes, although I wouldn't call scoping your variable a "cool trick" it adds indention and extra lines.  I had originally had them as their own blocks and changed that in favor of flush to avoid the random blocks (nowhere else in clang-style to you have a line with just a { on it).  I can change it back but I'm not really happy with either solution.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1135
@@ +1134,3 @@
+      Module.getNamedGlobal(TypeDescriptorMangledName);
+  if (TypeDescriptor)
+    return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy);
----------------
Timur Iskhodzhanov wrote:
>   ...
>   if (llvm::GlobalVariable *TypeDescriptor = Module.getNamedGlobal(TypeDescriptorMangledName))
>     return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy);
>   
>   lvm::raw_svector_ostream Out2 = llvm::raw_svector_ostream(TypeInfoString);
>   ...
I can do that but the TypeDescriptor variable gets reused later when we actually build one.  Given that the variable actually has the exact same semantic in both places, I see no issue with re-using it.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1137
@@ +1136,3 @@
+    return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy);
+  llvm::raw_svector_ostream Out2 = llvm::raw_svector_ostream(TypeInfoString);
+  Mangler.mangleCXXRTTIName(Type, Out2);
----------------
David Majnemer wrote:
> This will not compile with clang, perhaps use the following instead:
>   llvm::raw_svector_ostream Out2(TypeInfoString);
fixed.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1155
@@ +1154,3 @@
+llvm::StructType *MSRTTIBuilder::getTypeDescriptorType(StringRef Str) {
+  llvm::SmallString<32> TDTypeName = "MSRTTITypeDescriptor";
+  TDTypeName += Str.size();
----------------
David Majnemer wrote:
> This should be written as such to compile with clang:
>   llvm::SmallString<32> TDTypeName("MSRTTITypeDescriptor")
fixed.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1169
@@ +1168,3 @@
+MSRTTIBuilder::getClassHierarchyDescriptor(const CXXRecordDecl *RD) {
+  this->RD = RD;
+  Bases.clear();
----------------
Timur Iskhodzhanov wrote:
> This kinda reminds me of global variables.
> Is it intended that this class is not re-entrant?
> Do you think it's reasonable to split it into shared Context + per-class Builders, similar to MicrosoftVFTableContext/VFTableBuilder ?
Reentrancy is handled by cloning the state, I'll refactor this anyway though.  It could be cleaner.

================
Comment at: lib/CodeGen/CGRTTI.cpp:1240
@@ +1239,3 @@
+    BaseClass &Base = Bases[i];
+    if (Base.Flags & BaseClass::IsVirtual && !VirtualBases.insert(Base.RD))
+      i += Base.NumBases;
----------------
David Majnemer wrote:
> Can we have a comment describing what this does?
> 
> Also, please parenthesize the bitwise-and operation.
Sure.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:900
@@ -902,1 +899,3 @@
+  for (VPtrInfo *Info : VFPtrs) {
+    llvm::GlobalVariable *VTable = getAddrOfVTable(RD, (Info)->FullOffsetInMDC);
     if (VTable->hasInitializer())
----------------
Timur Iskhodzhanov wrote:
> the extra parenthesis are not needed anymore?
fixed

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:907
@@ -905,3 +906,3 @@
     const VTableLayout &VTLayout =
-        VFTContext.getVFTableLayout(RD, (*I)->FullOffsetInMDC);
+      VFTContext.getVFTableLayout(RD, (Info)->FullOffsetInMDC);
     llvm::Constant *Init = CGVT.CreateVTableInitializer(
----------------
Timur Iskhodzhanov wrote:
> ditto
fixed

================
Comment at: test/CodeGenCXX/microsoft-abi-rtti.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple=i386-pc-win32 2>/dev/null %s | FileCheck %s
+
----------------
Timur Iskhodzhanov wrote:
> are you sure it should have 755 permissions?
Nope! It's whatever windows/cygwin+git set them to.

http://reviews.llvm.org/D3833

More information about the cfe-commits mailing list