This is an archive of the discontinued LLVM Phabricator instance.

MS RTTI Generation
ClosedPublic

Authored by whunt on May 19 2014, 6:05 PM.

Details

Summary

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.

Diff Detail

Event Timeline

whunt updated this revision to Diff 9593.May 19 2014, 6:05 PM
whunt retitled this revision from to MS RTTI Generation.
whunt updated this object.
whunt edited the test plan for this revision. (Show Details)
whunt added reviewers: majnemer, rnk.
whunt added a subscriber: Unknown Object (MLST).

Please see some of my drive-by nitpicking.

lib/CodeGen/CGRTTI.cpp
994

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.

995
1020
1055

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

s/rtti/RTTI/?

1095

I think adding vertical whitespace between logical blocks would improve the readability

1097

Is there a (high-level?) comment explaining these structures?

1130

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

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 ?

lib/CodeGen/MicrosoftCXXABI.cpp
900

the extra parenthesis are not needed anymore?

907

ditto

test/CodeGenCXX/microsoft-abi-rtti.cpp
2

are you sure it should have 755 permissions?

majnemer added inline comments.May 20 2014, 9:42 AM
lib/AST/MicrosoftMangle.cpp
2265–2268

Instead of making these a cast to int, can you just make the params int32_t?

lib/CodeGen/CGRTTI.cpp
1056

This should be unsigned to match CXXRecordDecl::getNumBases

1240

Can we have a comment describing what this does?

Also, please parenthesize the bitwise-and operation.

majnemer added inline comments.May 20 2014, 9:47 AM
lib/CodeGen/CGRTTI.cpp
1065–1066

Initialize these in the order you declare them in the class definition, otherwise clang will warn on this.

1137

This will not compile with clang, perhaps use the following instead:

llvm::raw_svector_ostream Out2(TypeInfoString);
1155

This should be written as such to compile with clang:

llvm::SmallString<32> TDTypeName("MSRTTITypeDescriptor")
majnemer edited edge metadata.May 20 2014, 10:04 AM

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;
rnk added inline comments.May 20 2014, 11:39 AM
lib/CodeGen/CGRTTI.cpp
1195

IMO this should return CompleteObjectLocator to reduce statefulness, along with the other Declare* methods.

lib/CodeGen/CodeGenModule.h
249

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

@brief instead of the name. I'm guessing this was the conflict you encountered.

whunt added a comment.May 21 2014, 4:07 PM

New patch coming.

lib/CodeGen/CGRTTI.cpp
994

This can be done, I'll need to move the shared functionality into CodeGenModule.

995

yeah, fixed, I always forget this.

1055

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

fixed.

1065–1066

done.

1092

fixed.

1095

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

Will add.

1130

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

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

fixed.

1155

fixed.

1169

Reentrancy is handled by cloning the state, I'll refactor this anyway though. It could be cleaner.

1240

Sure.

lib/CodeGen/MicrosoftCXXABI.cpp
900

fixed

907

fixed

test/CodeGenCXX/microsoft-abi-rtti.cpp
2

Nope! It's whatever windows/cygwin+git set them to.

whunt updated this revision to Diff 9673.May 21 2014, 6:14 PM
whunt edited edge metadata.

Complete re-factor, also takes into account all of the first round of feedback.

rnk accepted this revision.May 22 2014, 5:30 PM
rnk edited edge metadata.

lgtm

Comments are only mechanical issues.

lib/CodeGen/CodeGenModule.h
713–718

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

s/approrpiate/appropriate/

lib/CodeGen/MicrosoftRTTI.cpp
24

There's a guideline to "make anonymous namespaces as small as possible":
http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

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

s/mearly/merely/

52

s/what it's/what is it's/

60

I'd rename Str to something informative, like maybe MangledTypeName?

nit: reflow

76–83

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.

This revision is now accepted and ready to land.May 22 2014, 5:30 PM
whunt closed this revision.May 23 2014, 9:15 AM
whunt updated this revision to Diff 9762.

Closed by commit rL209523 (authored by @whunt).