Page MenuHomePhabricator

[MSVC][DLL] use local vftable mangling only exported classes with virtual destructor
AbandonedPublic

Authored by DmitryPolukhin on Jul 6 2016, 12:15 AM.

Details

Reviewers
rnk
Summary

MSVC uses non-local mangling for vftable if class with dllexport attribute has no virtual destructor. I checked all examples that I fixed with MSVC and it uses '_7' in all cases. For more info see old thread in microsoft.public.vc.language: https://groups.google.com/d/msg/microsoft.public.vc.language/atSh_2VSc2w/EgJ3r_7OzVUJ

Diff Detail

Event Timeline

DmitryPolukhin retitled this revision from to [MSVC][DLL] use local vftable mangling only exported classes with virtual destructor .
DmitryPolukhin updated this object.
DmitryPolukhin added a reviewer: rnk.
DmitryPolukhin added a subscriber: cfe-commits.

Hmm, I'm not so sure this will work with constexpr:

#include <stdio.h>
struct __declspec(dllimport) S {
    virtual void fn() const {printf("%s\n", "hi");}
    constexpr S() = default;
};

constexpr S s;
auto &x = s;

int main() {
    x.fn();
}

Before my change we would reference the imported vftable symbol from the .rdata section which will fail at link time.
IIRC, MSVC will miscompile this example.

Your change, if I understand correctly, will cause us to import the vftable symbol which would cause this example to break again.

FWIW, I think that we would be OK to import vftables (and thus use the _7 mangling) in contexts where their construction doesn't need to be constant. It seemed pretty complex to implement but I'd be open to such an implementation.

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC? If not, I think we might want to report error message instead of miss-compile it as MSVC does. Anyway current implementation is not compatible with MSVC in much more common case without constexp. At the moment my patch works with your example just because it only changes mangling but don't use imported vftable that seems to be wrong. Is it you changes somewhere that prevent using imported vftable?

I found the patch that prevents using imported vftable, http://llvm.org/viewvc/llvm-project?view=revision&revision=260548 but the patch has no examples of constexpr + dllimport.

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC?

Yes, Chrome relied on these semantics.

Anyway current implementation is not compatible with MSVC in much more common case without constexp.

I don't see how it is not compatible. The address of the vftable is not observable as far as I know...

At the moment my patch works with your example just because it only changes mangling but don't use imported vftable that seems to be wrong.

Why is it wrong? It should obey the ABI in all practical matters but it might be less efficient space-wise.

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC?

Yes, Chrome relied on these semantics.

Hm, does it mean that Chrome has some workaround to bypass that MSVC doesn't support it?

Anyway current implementation is not compatible with MSVC in much more common case without constexp.

I don't see how it is not compatible. The address of the vftable is not observable as far as I know...

You can easily observe the difference if vftable is exported from DLL but some virtual functions are not (for example, private). MSVC will uses imported vftable and Clang will try to rebuild fvtable that will fail on link time.

At the moment my patch works with your example just because it only changes mangling but don't use imported vftable that seems to be wrong.

Why is it wrong? It should obey the ABI in all practical matters but it might be less efficient space-wise.

I think it is not ABI compatible use local vftable when MSVC uses imported. The easy way to observe the difference I already described but it might be other side effect if user start unloading DLL from address space. So user may expect that it is safe to unload library #1 that created object from library #2 because it doesn't have references to library #1 addresses but due to local fvtable it might result in crash.

rnk edited edge metadata.Jul 6 2016, 7:28 AM

Hmm, I'm not so sure this will work with constexpr:
...

Hah, my version of MSVC fails to install the vtable into 's', and crashes on the call to x.fn().

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC?

Yes, Chrome relied on these semantics.

Hm, does it mean that Chrome has some workaround to bypass that MSVC doesn't support it?

Not an explicit workaround, other MSVC bugs prevent them from miscompiling the exact sequence used in Chrome.

Anyway current implementation is not compatible with MSVC in much more common case without constexp.

I don't see how it is not compatible. The address of the vftable is not observable as far as I know...

You can easily observe the difference if vftable is exported from DLL but some virtual functions are not (for example, private). MSVC will uses imported vftable and Clang will try to rebuild fvtable that will fail on link time.

Wait, can you give an example of MSVC exporting a vftable but not all the virtual methods (other than the deleting destructor)? I don't believe I've ever come across an example of this.

Wait, can you give an example of MSVC exporting a vftable but not all the virtual methods (other than the deleting destructor)? I don't believe I've ever come across an example of this.

It is possible in code like this:
class B {

virtual void foo();

public:

virtual void bar();

};

class __declspec(dllexport) D : public B {
public:

virtual void bar();

};

void B::foo() {}
void B::bar() {}
void D::bar() {}

Here is B::foo is not exported but required to build vftable for D. Also user may want to explicitly control what should be exported from his library and may decide to remove some functions from exported interface.

Wait, can you give an example of MSVC exporting a vftable but not all the virtual methods (other than the deleting destructor)? I don't believe I've ever come across an example of this.

It is possible in code like this:
class B {

virtual void foo();

public:

virtual void bar();

};

class __declspec(dllexport) D : public B {
public:

virtual void bar();

};

void B::foo() {}
void B::bar() {}
void D::bar() {}

Here is B::foo is not exported but required to build vftable for D.

What happens if D also had a virtual destructor?

rnk added a comment.Jul 6 2016, 11:11 AM

Here is B::foo is not exported but required to build vftable for D. Also user may want to explicitly control what should be exported from his library and may decide to remove some functions from exported interface.

Sure, but there are other ways for clients of D to end up referencing the unexported symbol for B::foo, such as devirtualization. Surely you aren't proposing that we power that down just so that we can get the same import/export list as MSVC?

Thinking about this some more, it is possible for clang to emit code that will make everybody happy:

If a class is being constructed in a constexpr context and all the vftable entries it references are marked import, emit local vftables and reference them in the object. If a vftable entry it references is not marked import, report an error.

If a class is constructed via operator new and all the vftable entries it references are marked import, emit local vftables and store it in the object after the constructors run. If a vftable entry it references is not marked import, report an error.

If a class is constructed via a local or global variable and all the vftable entries it references are marked import, create a dllimport available_externally vftable. Otherwise create a normal dllimport external vftable.

I believe this behavior captures the best behavior across the spectrum of functionality we all care about: it supports devirtualization, constexpr and importing classes which don't have all their vftable methods exported.

Here is B::foo is not exported but required to build vftable for D.

What happens if D also had a virtual destructor?

It seems that in this case MSVC never inline x-tors so there is no problem with using local vftable. I don't propose to do the same inline decisions like MSVC does so Clang may need/use different set of exported members and MSVC depending on optimization levels and other things may change these sets. But it seems that never use exported vftable is not aligned with ABI. MSVC seems to be very consistent about uses exported vftable when it is possible.

Thinking about this some more, it is possible for clang to emit code that will make everybody happy:

If a class is being constructed in a constexpr context and all the vftable entries it references are marked import, emit local vftables and reference them in the object. If a vftable entry it references is not marked import, report an error.

If a class is constructed via operator new and all the vftable entries it references are marked import, emit local vftables and store it in the object after the constructors run. If a vftable entry it references is not marked import, report an error.

If a class is constructed via a local or global variable and all the vftable entries it references are marked import, create a dllimport available_externally vftable. Otherwise create a normal dllimport external vftable.

I believe this behavior captures the best behavior across the spectrum of functionality we all care about: it supports devirtualization, constexpr and importing classes which don't have all their vftable methods exported.

I'll try to implement this approach except for special handling for constriction via new that seems to be out of scope for this issue and can be implemented independent later. Small addition, I think there is no sense to check if vftable references to entry that is not marked as dllimport. Linker will do this work for us and in some cases function may come from libraries (static or dynamic) but in the source may not be properly marked as dllimport (it is the behavior that MSVC does check it in compiler and relays on linker).

DmitryPolukhin edited edge metadata.

Sorry for delay with patch rework, PTAL!

  • added local vftable for constexpr
  • added use of imported vftable

Running your patch against the following:

struct __declspec(dllimport) S {
  virtual void f() {}
};
S x;
constexpr S y;

results in:

Global is marked as dllimport, but not external
[2 x i8*]* @"\01??_7S@@6B@"
fatal error: error in backend: Broken module found, compilation aborted!
clang-3.9: error: clang frontend command failed with exit code 70 (use -v to see invocation)

I don't think your approach will correctly handle instances where we need to emit a local vftable and reference an imported vftable simultaneously.

A flag on CXXRecordDecl which is sensitive to the most recent expression evaluation might not be the best way to go.
Perhaps we should be able to use the VFTableBuilder to build imported and local vftables for the same vftable? Not entirely sure though...

lib/AST/ExprConstant.cpp
5765

I'd use auto * here.

rnk added inline comments.Jul 13 2016, 9:27 AM
lib/AST/ExprConstant.cpp
5766

Even better:

if (auto *CD = E->getType()->getAsCXXRecordDecl())

A flag on CXXRecordDecl which is sensitive to the most recent expression evaluation might not be the best way to go.
Perhaps we should be able to use the VFTableBuilder to build imported and local vftables for the same vftable? Not entirely sure though...

The flag can be only set to true, there is no way to reset it false. So by the end of translation unit it should have proper value and many things are deferred until the end of translation unit (like CodeGenModule::EmitDeferredVTables). But your example shows that normal var generation is not deferred, I missed it. Deferring var generation in CodeGenModule::MayBeEmittedEagerly seems to work fine.

DmitryPolukhin marked 2 inline comments as done.
  • defer var generation if their type is class with dllimport.
rnk added a comment.Jul 18 2016, 3:09 PM

The approach makes sense to me, but the tests suggest it isn't doing what I'd expect.

test/CodeGenCXX/dllimport-rtti.cpp
7

I would've expected this to remain the same, since the implicit default ctor of 'S' is constexpr by default in C++14. It seems a lot better to emit a local vftable here and get static initialization for 's' than dynamic initialization.

DmitryPolukhin added inline comments.Jul 19 2016, 3:10 AM
test/CodeGenCXX/dllimport-rtti.cpp
7

The context of evaluation of the whole expression is not constexpr so this case can be done both ways. But implemented approach is how MSVC behaves in this case. MSVC has very predictable behavior when local vftable is used - only when class has virtual d-tor. Current Clang behavior is also very consistent - always use local vftable. But this patch makes it hard to predict which table will be used - it becomes use context sensitive instead of depending only on class declaration. Therefore different translation units could use different tables and it could cause strange artifacts. Using dllimported classes in pure constexpr case in my experience is very rare case so it was kind of fine but implicit constructors much more common case.

Also thinking more about my patch I realized that fix in MayBeEmittedEagerly doesn't work if dllimported class is a member of non-imported class so actual fix would require traversing for all base classes and members for the type.

So my proposal is to keep things as is for now and abandon this patch if you have no objection.

rnk added inline comments.Jul 19 2016, 7:05 AM
test/CodeGenCXX/dllimport-rtti.cpp
7

Sounds good, we also thought this was a reasonable compromise position last time we touched this.

DmitryPolukhin abandoned this revision.Jul 19 2016, 7:09 AM