[PATCH] Use the MS ABI for Win32 target by default
Hans Wennborg
hans at chromium.org
Fri Dec 13 14:34:23 PST 2013
> I think all the test/CodeGen* changes are good, and should probably land now.
OK, I'll split off and commit those. It should make this patch easier to manage :)
================
Comment at: test/CXX/drs/dr2xx.cpp:543
@@ -542,3 +542,3 @@
struct C : A, B {
- virtual ~C();
+ virtual ~C(); // expected-error 0-1 {{'operator delete' found in multiple base classes}}
};
----------------
Richard Smith wrote:
> Reid Kleckner wrote:
> > So, I remember we decided to do it this way, but now we've essentially weakened this test to pass if we fail to diagnose this in the Itanium C++ ABI. Can you remind me what the issue is here? This has to do with differing operator delete lookup behavior?
> Please conditionalize this extra diagnostic on use of the MS ABI. Do we have a predefined macro indicating the ABI? If not, maybe we should add one?
> Can you remind me what the issue is here? This has to do with differing operator delete lookup behavior?
Yes, we diagnose the multiple 'operator delete' when 'virtual ~C()' is declared below, whereas in Itanium we diagnose it when that dtor is defined further on.
================
Comment at: test/CXX/drs/dr2xx.cpp:543
@@ -542,3 +542,3 @@
struct C : A, B {
- virtual ~C();
+ virtual ~C(); // expected-error 0-1 {{'operator delete' found in multiple base classes}}
};
----------------
Hans Wennborg wrote:
> Richard Smith wrote:
> > Reid Kleckner wrote:
> > > So, I remember we decided to do it this way, but now we've essentially weakened this test to pass if we fail to diagnose this in the Itanium C++ ABI. Can you remind me what the issue is here? This has to do with differing operator delete lookup behavior?
> > Please conditionalize this extra diagnostic on use of the MS ABI. Do we have a predefined macro indicating the ABI? If not, maybe we should add one?
> > Can you remind me what the issue is here? This has to do with differing operator delete lookup behavior?
>
> Yes, we diagnose the multiple 'operator delete' when 'virtual ~C()' is declared below, whereas in Itanium we diagnose it when that dtor is defined further on.
> Please conditionalize this extra diagnostic on use of the MS ABI.
Done.
>Do we have a predefined macro indicating the ABI? If not, maybe we should add one?
No, we don't have a macro for this. We could create one, but it's also easy to run the test in both modes and set a macro ourselves, I've done that in a few other tests.
================
Comment at: test/CXX/class.access/p4.cpp:129
@@ -123,1 +128,3 @@
+#endif
+ void foo(A param) { // Okay in Itanium.
A local; // expected-error {{variable of type 'test3::A' has private destructor}}
----------------
Richard Smith wrote:
> Why do we do the access check in the callee under the MS ABI? We can still do dtor access checks in the caller, even though we actually make the call in the callee, can't we? Presumably MSVC redundantly checks access in both places?
>
> (Do we need to omit the access checking in the caller for compatibility somehow?)
IIRC, Reid told me that John had told him to do the access check in the callee too when he implemented this.
We still have the access check in the caller.
================
Comment at: test/CXX/special/class.dtor/p5-0x.cpp:98-104
@@ -96,2 +97,9 @@
} d2; // expected-error {{deleted function}}
+#ifdef MSABI
+// expected-error at +7 {{no suitable member 'operator delete' in 'D3'}}
+// expected-note at +7 {{member 'operator delete' declared here}}
+// expected-note at +7 {{member 'operator delete' declared here}}
+// expected-error at +9 {{attempt to use a deleted function}}
+// expected-note at +9 {{function has been explicitly marked deleted here}}
+#endif
struct D3 { // expected-note {{virtual destructor requires an unambiguous, accessible 'operator delete'}}
----------------
Richard Smith wrote:
> This looks incorrect. We shouldn't be trying to mark the `operator delete` as used if the destructor is deleted.
But neither ~D3() nor ~D4() below are deleted.
================
Comment at: test/Sema/empty1.c:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -Wc++-compat
+// RUN: %clang_cc1 %s -cxx-abi itanium -fsyntax-only -verify -Wc++-compat
----------------
Richard Smith wrote:
> This looks weird. I think you're adding this because empty structs have size 4 under MSVC, but that's controlled by the psABI, not the C++ ABI.
Hmm, it seems to be controlled by -cxx-abi by us :) I agree this is weird, though.
Would you be ok a FIXME here and filing a bug with Warren on the cc list? Or running the test with a fixed target?
================
Comment at: test/SemaCXX/destructor.cpp:86-93
@@ -84,6 +85,10 @@
+#ifndef MSABI
+// This bug, "Clang instantiates destructor for function argument" is intended
+// behaviour in the Microsoft ABI because the callee needs to destruct the arguments.
namespace PR6709 {
template<class T> class X { T v; ~X() { ++*v; } };
void a(X<int> x) {}
}
+#endif
----------------
Richard Smith wrote:
> Please test that this produces diagnostics with the MS ABI, rather than not testing it at all.
Done.
================
Comment at: test/SemaCXX/destructor.cpp:108-117
@@ -102,6 +107,12 @@
+#ifdef MSABI
+ // expected-note at +2 {{in instantiation of member function 'test6::A<int>::operator delete' requested here}}
+#endif
virtual ~A() {}
};
- class B : A<int> { B(); }; // expected-note {{in instantiation of member function 'test6::A<int>::operator delete' requested here}}
+#ifndef MSABI
+ // expected-note at +2 {{in instantiation of member function 'test6::A<int>::operator delete' requested here}}
+#endif
+ class B : A<int> { B(); };
B::B() {}
----------------
Richard Smith wrote:
> There's something odd here. We should be producing a note indicating the place at which the instantiation of `A<int>` was triggered under the MS ABI.
But we are. In MS ABI we emit a note on the declaration of '~A()'. On itanium the note comes later, on the definition of 'class B'.
================
Comment at: test/SemaCXX/implicit-virtual-member-functions.cpp:7-16
@@ -5,10 +6,12 @@
+#ifndef MSABI
struct B : A { // expected-error {{no suitable member 'operator delete' in 'B'}}
virtual void f();
void operator delete (void *, int); // expected-note {{'operator delete' declared here}}
};
void B::f() { // expected-note {{implicit destructor for 'B' first required here}}
}
+#endif
----------------
Richard Smith wrote:
> Can you provide an alternative set of expected diagnostics here for MS ABI rather than disabling that half of the test? (I assume the point is that we trigger `operator delete` lookup earlier?)
In MS ABI we don't require the destructor for B here, so we expect no diagnostics. I've fixed the test to check that.
================
Comment at: test/SemaCXX/undefined-internal.cpp:1-4
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -cxx-abi itanium -fsyntax-only -verify %s
// Make sure we don't produce invalid IR.
-// RUN: %clang_cc1 -emit-llvm-only %s
+// RUN: %clang_cc1 -cxx-abi itanium -emit-llvm-only %s
----------------
Richard Smith wrote:
> Why does this test need to force the Itanium ABI?
We fail during codegen with "error: cannot mangle RTTI descriptors for type 'A' yet", and we need RTTI for the typeid which is used in the test.
This isn't a problem for the '-verify' line though. I'll update that.
================
Comment at: test/SemaCXX/virtual-base-used.cpp:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -cxx-abi itanium -verify %s
// PR7800
----------------
Richard Smith wrote:
> Why does this test need to force the Itanium ABI?
In MS ABI we don't emit any diagnostics in this test. We should probably take a second look and verify that's correct behaviour.
I figured the test mostly seemed to be a regression test for the ICE in PR7800, so maybe we shouldn't run it in MS mode since none of the expectations apply.
================
Comment at: test/SemaCXX/warn-weak-vtables.cpp:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -Wweak-vtables -Wweak-template-vtables
+// RUN: %clang_cc1 %s -fsyntax-only -verify -cxx-abi itanium -Wweak-vtables -Wweak-template-vtables
----------------
Richard Smith wrote:
> Can you also test this in MS ABI mode, to ensure the warning is suppressed there? Something like `-Wweak-vtables -Wweak-template-vtables -Werror` (with no `-verify`)?
Done.
================
Comment at: test/SemaTemplate/virtual-member-functions.cpp:89-93
@@ -82,5 +88,7 @@
+#ifndef MSABI
void test_X(X<int> xi, X<float> xf) {
xi.f();
}
+#endif
}
----------------
Richard Smith wrote:
> Maybe instead change this test to pass the arguments by reference instead of by value?
Done.
http://llvm-reviews.chandlerc.com/D2401
More information about the cfe-commits
mailing list