[cfe-commits] [Review] First step towards PR13231 - vftable generation with -cxx-abi microsoft

[John McCall]

On Nov 15, 2012, at 8:43 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> Can you please review the attached patch?

Sorry about the delay.

> With this patch, I can compile and run simple single-inheritance tests;
> even method calls from Clang-generated obj files to CL-generated
> methods work (and vice versa).
> 
> Some notes:
> a) It only works with -fno-rtti since we can't mangle RTTI yet,

Okay, but I'm curious about something here.  It looks like you're not even
allocating space in the v-table for the RTTI pointer if RTTI is disabled?
That means that every translation unit using the class needs to agree
about whether RTTI is enabled, or else they might miscompile.  If that's
what MSVC does, fine, but if not and what you're really trying to do is
just punt on RTTI for now, why not just always emit RTTI pointers as null?

> b) I had to remove "AddressPoint != 0" assertion since AddressPoint
> should be 0 for some classes in -cxx-abi microsoft,

You could weaken the assertion instead of removing it.

> c) I've added conditionals rather than a layer of abstraction per
> http://llvm.org/bugs/show_bug.cgi?id=13231#c5,
> 
> d) The complete/deleting dtor logic will change when we support MSVC
> dtor types, but not now
>    [shouldn't matter now that we still don't support virtual inheritance]

You're thinking about complete vs. base destructors.  Complete vs. deleting
destructors isn't about virtual inheritance;  it's about whether the destructor
calls operator delete.  (This is important because the 'delete' operator uses
the operator delete from the most-derived class.)

How does MSVC handle the difference between
  foo->~Foo(); // virtually dispatched but doesn't call operator delete
and
  delete foo; // virtually dispatched and calls operator delete
?

Code notes:

+
+  // FIXME: Should probably add a layer of abstraction for vtable
+  // generation, see http://llvm.org/bugs/show_bug.cgi?id=13231#c5
+  bool MsABI = (Context.getTargetInfo().getCXXABI() == CXXABI_Microsoft);

Please don't litter the code with PR references.

Also, please name this isMicrosoft or something.  This looks like "Ms. ABI".

+    if (MsABI)
+      llvm_unreachable("Haven't seen an implicit virtual dtor in MS mode");
+

If this is right, then this should be an assert.  But I don't understand if it's
right, because I don't understand what the message is trying to say.
Is this an invariant?  A language rule? An unimplemented feature?

John.