[ms-cxxabi] Elide dtor access checks for pass-by-val objects in callees

[hans]

Summary

The ABI imposes the practical requirement that the destructors can be called, but the standard does not require access checks here. This is a stab at eliding those access checks to avoid rejecting valid code, as discussed in http://llvm-reviews.chandlerc.com/D2401?id=6089#inline-12829

[rnk]

Rather than threading a CheckDtorAccess bool through everywhere, I think you can inline a small portion of the body of FinalizeVarWithDestructor into CheckFunctionDefinition and skip the access check. Most of it is probably dead. That's what I did initially in http://llvm-reviews.chandlerc.com/D929.

[hans]

Rather than threading a CheckDtorAccess bool through everywhere, I think you can inline a small portion of the body of FinalizeVarWithDestructor into CheckFunctionDefinition and skip the access check. Most of it is probably dead. That's what I did initially in http://llvm-reviews.chandlerc.com/D929.

That just removes one level of threading though. We still need to thread it through MarkFunctionReferenced to suppress access checks for base class dtors etc. when defining an implicit destructor, and that's where the ugliness comes from :/

[hans]

Inlining the relevant part of FinalizeVarWithDestructor makes it look like this.

[hans]

The problem is that MarkFunctionReferenced does access checks as it marks functions as referenced. This makes perfect sense, except here when we want to be able to call the dtor but bypass the access check.

Richard: it would be interesting to hear your thoughts on this. Is the added plumbing of this boolean variable through the various functions an acceptable approach? Would it be ok to avoid that with some mechanism to "trap" access check errors (maybe a flag on Sema)? Or is it not worth the hassle to change this?

[majnemer]

I don't think we should elide these checks. This would lead us to behave
differently than MSVC inside of SFINAE contexts.

Sent from my phone.

[rsmith]

I think you should only be removing the direct access check of the parameter type, and not handling the base and member access checks -- they're effectively part of generating the destructor definition for the parameter's type, and aren't affected by which TU the check is performed in.

I don't think we should elide these checks. This would lead us to behave
differently than MSVC inside of SFINAE contexts.

If I understand correctly, these checks only occur as part of handling a function definition, which is not a SFINAE context, so I think this is not an issue.

[hans]

Thanks Richard! New patch attached/uploaded that only elides the direct access check. Does this look OK?

[rsmith]

LGTM

[hans]

Closed by commit rL199120 (authored by @hans).

cfe/trunk/lib/Sema/SemaChecking.cpp

while (const ArrayType *AT = Context.getAsArrayType(PType)) {
         Diag(Param->getLocation(), diag::err_array_star_in_function_definition);
         break;
       }
       PType= AT->getElementType();
     }
 
     // MSVC destroys objects passed by value in the callee.  Therefore a
     // function definition which takes such a parameter must be able to call the
-    // object's destructor.
+    // object's destructor.  However, we don't perform any direct access check
+    // on the dtor.
     if (getLangOpts().CPlusPlus && Context.getTargetInfo()
                                        .getCXXABI()
                                        .areArgsDestroyedLeftToRightInCallee()) {
-      if (const RecordType *RT = Param->getType()->getAs<RecordType>())
-        FinalizeVarWithDestructor(Param, RT);
+      if (const RecordType *RT = Param->getType()->getAs<RecordType>()) {
+        CXXRecordDecl *ClassDecl = cast<CXXRecordDecl>(RT->getDecl());
+        if (!ClassDecl->isInvalidDecl() &&
+            !ClassDecl->hasIrrelevantDestructor() &&
+            !ClassDecl->isDependentContext()) {
+          CXXDestructorDecl *Destructor = LookupDestructor(ClassDecl);
+          MarkFunctionReferenced(Param->getLocation(), Destructor);
+          DiagnoseUseOfDecl(Destructor, Param->getLocation());
+        }
+      }
     }
   }
 
   return HasInvalidParm;
 }
 
 /// CheckCastAlign - Implements -Wcast-align, which warns when a
 /// pointer cast increases the alignment requirements.

cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp

 }
 
 namespace Test2 {
 
 // In the MSVC ABI, functions must destroy their aggregate arguments.  foo
 // requires a dtor for B, but we can't implicitly define it because ~A is
 // private.  bar should be able to call A's private dtor without error, even
 // though MSVC rejects bar.
-
 class A {
 private:
-  ~A(); // expected-note 2{{declared private here}}
+  ~A(); // expected-note {{declared private here}}
   int a;
 };
 
 struct B : public A { // expected-error {{base class 'Test2::A' has private destructor}}
   int b;
 };
 
 struct C {
   ~C();
   int c;
 };
 
 struct D {
   // D has a non-trivial implicit dtor that destroys C.
   C o;
 };
 
 void foo(B b) { } // expected-note {{implicit destructor for 'Test2::B' first required here}}
-void bar(A a) { } // expected-error {{variable of type 'Test2::A' has private destructor}}
+void bar(A a) { } // no error; MSVC rejects this, but we skip the direct access check.
 void baz(D d) { } // no error
 
 }
 
 #ifdef MSVC_ABI
 namespace Test3 {
 
 class A {
   A();
-  ~A(); // expected-note 2{{implicitly declared private here}}
+  ~A(); // expected-note {{implicitly declared private here}}
   friend void bar(A);
   int a;
 };
 
 void bar(A a) { }
-void baz(A a) { } // expected-error {{variable of type 'Test3::A' has private destructor}}
+void baz(A a) { } // no error; MSVC rejects this, but the standard allows it.
 
 // MSVC accepts foo() but we reject it for consistency with Itanium.  MSVC also
 // rejects this if A has a copy ctor or if we call A's ctor.
 void foo(A *a) {
   bar(*a); // expected-error {{temporary of type 'Test3::A' has private destructor}}
 }
 }
 #endif
 
 namespace Test4 {
 // Don't try to access the dtor of an incomplete on a function declaration.
 class A;
 void foo(A a);
 }

cfe/trunk/test/SemaObjCXX/microsoft-abi-byval.mm

 // RUN: %clang_cc1 -fsyntax-only -verify -cxx-abi microsoft -Wno-objc-root-class %s
+// expected-no-diagnostics
 
 class Foo {
-  ~Foo(); // expected-note {{implicitly declared private here}}
+  ~Foo();
 };
 
 @interface bar
 - (void) my_method: (Foo)arg;
 @end
 
 @implementation bar
-- (void) my_method: (Foo)arg { // expected-error {{variable of type 'Foo' has private destructor}}
+- (void) my_method: (Foo)arg { // no error; MS ABI will call Foo's dtor, but we skip the access check.
 }
 @end