This patch gives clang the ability to generate cl.exe compatible RTTI structures. Because we don't support sections in llvm properly we can't emit vtables that reference the rtti data yet but this patch allows us to generate the data-structures.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Please see some of my drive-by nitpicking.
lib/CodeGen/CGRTTI.cpp | ||
---|---|---|
994 ↗ | (On Diff #9593) | I think an overview comment describing basic ideas, concepts and relations would help here? |
995 ↗ | (On Diff #9593) | |
1020 ↗ | (On Diff #9593) | http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly |
1055 ↗ | (On Diff #9593) | 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; } ? |
1092 ↗ | (On Diff #9593) | s/rtti/RTTI/? |
1095 ↗ | (On Diff #9593) | I think adding vertical whitespace between logical blocks would improve the readability |
1097 ↗ | (On Diff #9593) | Is there a (high-level?) comment explaining these structures? |
1130 ↗ | (On Diff #9593) | 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. |
1135 ↗ | (On Diff #9593) | ... if (llvm::GlobalVariable *TypeDescriptor = Module.getNamedGlobal(TypeDescriptorMangledName)) return llvm::ConstantExpr::getBitCast(TypeDescriptor, CGM.Int8PtrTy); lvm::raw_svector_ostream Out2 = llvm::raw_svector_ostream(TypeInfoString); ... |
1169 ↗ | (On Diff #9593) | This kinda reminds me of global variables. |
lib/CodeGen/MicrosoftCXXABI.cpp | ||
900 ↗ | (On Diff #9593) | the extra parenthesis are not needed anymore? |
907 ↗ | (On Diff #9593) | ditto |
test/CodeGenCXX/microsoft-abi-rtti.cpp | ||
1 ↗ | (On Diff #9593) | are you sure it should have 755 permissions? |
lib/AST/MicrosoftMangle.cpp | ||
---|---|---|
2265–2268 ↗ | (On Diff #9593) | Instead of making these a cast to int, can you just make the params int32_t? |
lib/CodeGen/CGRTTI.cpp | ||
1056 ↗ | (On Diff #9593) | This should be unsigned to match CXXRecordDecl::getNumBases |
1240 ↗ | (On Diff #9593) | Can we have a comment describing what this does? Also, please parenthesize the bitwise-and operation. |
lib/CodeGen/CGRTTI.cpp | ||
---|---|---|
1065–1066 ↗ | (On Diff #9593) | Initialize these in the order you declare them in the class definition, otherwise clang will warn on this. |
1137 ↗ | (On Diff #9593) | This will not compile with clang, perhaps use the following instead: llvm::raw_svector_ostream Out2(TypeInfoString); |
1155 ↗ | (On Diff #9593) | This should be written as such to compile with clang: llvm::SmallString<32> TDTypeName("MSRTTITypeDescriptor") |
RTTI generation fails for:
struct A { virtual void f(); }; struct B : virtual A { __declspec(align(4)) int field : 5; }; B b;
We generate incompatible RTTI with the following:
struct A {}; struct B : A { virtual void f(); }; struct C : virtual B, A {}; struct D : C {}; D d;
lib/CodeGen/CGRTTI.cpp | ||
---|---|---|
1195 ↗ | (On Diff #9593) | IMO this should return CompleteObjectLocator to reduce statefulness, along with the other Declare* methods. |
lib/CodeGen/CodeGenModule.h | ||
249 ↗ | (On Diff #9593) | This should be a std::unique_ptr to avoid leaks, if you want to keep this persistent on the CodeGenModule. You would also need to move the MSRTTIBuilder definition into a header in order to delete it in ~CodeGenModule. However, it sounds like we might restructure this code. |
721 ↗ | (On Diff #9593) | @brief instead of the name. I'm guessing this was the conflict you encountered. |
New patch coming.
lib/CodeGen/CGRTTI.cpp | ||
---|---|---|
994 ↗ | (On Diff #9593) | This can be done, I'll need to move the shared functionality into CodeGenModule. |
995 ↗ | (On Diff #9593) | yeah, fixed, I always forget this. |
1055 ↗ | (On Diff #9593) | This feels like a nit. I can make the change but I don't think there's a strong argument to be made here. |
1056 ↗ | (On Diff #9593) | fixed. |
1065–1066 ↗ | (On Diff #9593) | done. |
1092 ↗ | (On Diff #9593) | fixed. |
1095 ↗ | (On Diff #9593) | 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. |
1097 ↗ | (On Diff #9593) | Will add. |
1130 ↗ | (On Diff #9593) | 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. |
1135 ↗ | (On Diff #9593) | 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. |
1137 ↗ | (On Diff #9593) | fixed. |
1155 ↗ | (On Diff #9593) | fixed. |
1169 ↗ | (On Diff #9593) | Reentrancy is handled by cloning the state, I'll refactor this anyway though. It could be cleaner. |
1240 ↗ | (On Diff #9593) | Sure. |
lib/CodeGen/MicrosoftCXXABI.cpp | ||
900 ↗ | (On Diff #9593) | fixed |
907 ↗ | (On Diff #9593) | fixed |
test/CodeGenCXX/microsoft-abi-rtti.cpp | ||
1 ↗ | (On Diff #9593) | Nope! It's whatever windows/cygwin+git set them to. |
lgtm
Comments are only mechanical issues.
lib/CodeGen/CodeGenModule.h | ||
---|---|---|
713–718 ↗ | (On Diff #9673) | Can you move these near GetAddrOfRTTIDescriptor? This is an ABI-specific interface, but without exposing MicrosoftCXXABI or merging MicrosoftRTTI.cpp with MicrosoftCXXABI.cpp, we can't reach it any other way. |
954 ↗ | (On Diff #9673) | s/approrpiate/appropriate/ |
lib/CodeGen/MicrosoftRTTI.cpp | ||
24 ↗ | (On Diff #9673) | There's a guideline to "make anonymous namespaces as small as possible": Most of the free functions should be static (I think they were before?), and just the class definition (not the method implementations) should be anonymous. |
46 ↗ | (On Diff #9673) | s/mearly/merely/ |
52 ↗ | (On Diff #9673) | s/what it's/what is it's/ |
60 ↗ | (On Diff #9673) | I'd rename Str to something informative, like maybe MangledTypeName? nit: reflow |
76–83 ↗ | (On Diff #9673) | Any reason not to write: llvm::Type *FieldTypes[] = { CGM.Int8PtrTy, CGM.IntTy, CGM.IntTy, CGM.IntTy, CGM.IntTy, CGM.IntTy, getClassHierarchyDescriptorType(CGM)->getPointerTo() }; ? This pattern appears in lots of other places too. |