Thanks for the review! New patch coming up.
================
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:
> Hans Wennborg wrote:
> > 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?
> Sure, a FIXME works for me.
OK, added a FIXME.
================
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:
> Hans Wennborg wrote:
> > 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.
> Please either XFAIL or add a FIXME; this test *should* pass without the
> -cxx-abi itanium.
I've added a FIXME.
================
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:
> Hans Wennborg wrote:
> > 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.
> That seems inconsistent: `B` (implicitly) has a virtual destructor, and
> `operator delete` lookup for it would fail, but we don't diagnose it.
I've added "new D" calls to trigger vtable emission, I think it worked out
pretty well in this test.
================
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:
> Hans Wennborg wrote:
> > 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.
> This looks like the same root cause as
> `implicit-virtual-member-functions.cpp` -- we don't trigger vtable emission,
> so we don't implicitly define the destructor. In both that file and this one,
> you could get more consistent behavior in the MS ABI with something like this:
>
> void D::foo() {}
> D d;
> #if MSABI
> // expected-note@-2 {{implicit destructor for 'B' first required here}}
> #else
> // expected-note@-5 {{implicit destructor for 'B' first required here}}
> #endif
This worked great in implicit-virtual-member-functions.cpp, but in this test it
gets a bit more hairy as it we start getting a bunch of notes about requiring
implicit default constructors etc. I don't know if it's ok, if there's a
cleaner way, or if it's not worth the mess to run the test in ms mode.
================
Comment at: test/SemaCXX/destructor.cpp:108-117
@@ -102,6 +107,12 @@
+#ifdef MSABI
+ // expected-note@+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@+2 {{in instantiation of member function
'test6::A<int>::operator delete' requested here}}
+#endif
+ class B : A<int> { B(); };
B::B() {}
----------------
Richard Smith wrote:
> Hans Wennborg wrote:
> > 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'.
> Sure, but that diagnostic doesn't actually say why we're instantiating
> `A<int>`; we're missing the outermost level of the instantiation backtrace.
I think something like this is happening (and correct me if I'm wrong, because
I'm not familiar with the instantiation process):
When we declare B, we require the complete class of A, so we instantiate it
(Sema::InstantiateClass), and then we do Sema::CheckCompletedCXXClass. In MS
ABI mode, this means that we call CheckDestructor() (see r182270), which will
mark the destructor as referenced, and is added to PendingInstantiations.
Later, we process the destructor in PendingInstantiations. But now we don't
have anything on the ActiveInstantiations list, so we fail to emit a note about
why we're instantiating it.
I don't see a straightforward way of fixing this, and since we plan on moving
these checks to some later time (vtable emission or something), would it be
alright to just put a FIXME here in the meantime?
================
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:
> Hans Wennborg wrote:
> > 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.
> So the access check in the callee serves no purpose other than to reject
> valid code? Can we just remove it?
I've sent out a patch for this: http://llvm-reviews.chandlerc.com/D2409
http://llvm-reviews.chandlerc.com/D2401
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
