Page MenuHomePhabricator

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

Repository
rL LLVM

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 ↗(On Diff #9593)

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 ↗(On Diff #9593)
1020 ↗(On Diff #9593)
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.
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 ↗(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?

majnemer added inline comments.May 20 2014, 9:42 AM
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.

majnemer added inline comments.May 20 2014, 9:47 AM
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")
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 ↗(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.

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

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.

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 ↗(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":
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 ↗(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.

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