Page MenuHomePhabricator

CodeGen: Refactor RTTI emission
ClosedPublic

Authored by majnemer on Jun 23 2014, 2:30 PM.

Details

Summary

Let's not expose ABI specific minutia inside of CodeGenModule and
instead abstract it through CXXABI. This gets rid of
CodeGenModule::getCompleteObjectLocator,
CodeGenModule::EmitFundamentalTypeDescriptor{s,},
CodeGenModule::getMSTypeDescriptor,
CodeGenModule::getMSCompleteObjectLocator.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 10765.Jun 23 2014, 2:30 PM
majnemer retitled this revision from to CodeGen: Refactor RTTI emission.
majnemer updated this object.
majnemer added reviewers: rsmith, rnk, whunt, rjmccall.
majnemer added a subscriber: Unknown Object (MLST).
rnk added inline comments.Jun 23 2014, 2:48 PM
include/clang/AST/Type.h
1458 ↗(On Diff #10765)

Stray comment?

lib/CodeGen/CodeGenModule.cpp
3390–3391 ↗(On Diff #10765)

Not part of this change, but note this linkage is incorrect for dllimport classes: http://llvm.org/PR20106

lib/CodeGen/ItaniumCXXABI.cpp
1950 ↗(On Diff #10765)

Looks like the Itanium code paths weigh in at ~900 lines and the MS ones at ~500. That's kind of my internal threshold for "this deserves it's own file". IIRC you considered but rejected ItaniumRTTI.cpp. Why? I don't see any problem with a lib/CodeGen/ItaniumRTTI.h that exposes some free functions that ItaniumCXXABI calls to.

lib/CodeGen/MicrosoftCXXABI.cpp
2783 ↗(On Diff #10765)

"creats"

majnemer updated this revision to Diff 10769.Jun 23 2014, 3:18 PM
  • Address review feedback.
include/clang/AST/Type.h
1458 ↗(On Diff #10765)

Fixed.

lib/CodeGen/ItaniumCXXABI.cpp
1950 ↗(On Diff #10765)

As an example:
ItaniumCXXABI::getAddrOfRTTIDescriptor refers to ItaniumRTTIBuilder. Either we somehow expose a CXXABIRTTIBuilder base class for both of them (as well as factory functions to create them) or we just merge them into their respective ABI implementations.

I can do either but it made more sense, to me, to avoid creating another interface.

lib/CodeGen/MicrosoftCXXABI.cpp
2783 ↗(On Diff #10765)

Fixed.

rnk accepted this revision.Jun 23 2014, 3:52 PM
rnk edited edge metadata.

lgtm

lib/CodeGen/ItaniumCXXABI.cpp
1950 ↗(On Diff #10765)

OK, sgtm. We don't want a base class. The interface won't be the same. In Itanium, the type info object has all the relevant data, whereas in MS the other data points to the type_info struct.

This revision is now accepted and ready to land.Jun 23 2014, 3:52 PM
majnemer closed this revision.Jul 6 2014, 11:29 PM
majnemer updated this revision to Diff 11111.

Closed by commit rL212435 (authored by @majnemer).