[MS] Allow access to ambiguous, inaccessible direct bases

[rnk]

Summary

Clang typically warns that in the following class hierarchy, 'A' is
inaccessible because there is no series of casts that the user can
write to access it unambiguously:

struct A { };
struct B : A { };
struct C : A, B { };

MSVC allows the user to convert from C* to A*, though, and we've
encountered this issue in the latest Windows SDK headers.

This patch allows this conversion when -fms-compatibility is set and
adds a warning for it under -Wmicrosoft-inaccessible-base.

[smeenai]

When you say "latest Windows SDK headers", do you mean SDK version 10.0.16299.15?

[rsmith]

Yuck, LGTM :)

[rnk]

In D39389#909830, @smeenai wrote:

When you say "latest Windows SDK headers", do you mean SDK version 10.0.16299.15?

Yep. Here's the bug about it in Chromium: https://crbug.com/775174

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def MicrosoftFlexibleArray : DiagGroup<"microsoft-flexible-array">;
 def MicrosoftExtraQualification : DiagGroup<"microsoft-extra-qualification">;
 def MicrosoftCast : DiagGroup<"microsoft-cast">;
 def MicrosoftConstInit : DiagGroup<"microsoft-const-init">;
 def MicrosoftVoidPseudoDtor : DiagGroup<"microsoft-void-pseudo-dtor">;
 def MicrosoftAnonTag : DiagGroup<"microsoft-anon-tag">;
 def MicrosoftCommentPaste : DiagGroup<"microsoft-comment-paste">;
 def MicrosoftEndOfFile : DiagGroup<"microsoft-end-of-file">;
+def MicrosoftInaccessibleBase : DiagGroup<"microsoft-inaccessible-base">;
 // Aliases.
 def : DiagGroup<"msvc-include", [MicrosoftInclude]>;
                 // -Wmsvc-include = -Wmicrosoft-include
 
 // Warnings group for warnings about Microsoft extensions.
 def Microsoft : DiagGroup<"microsoft",
     [MicrosoftCharize, MicrosoftInclude, MicrosoftCppMacro, MicrosoftFixedEnum,
      MicrosoftSealed, MicrosoftUnqualifiedFriend, MicrosoftExceptionSpec,

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

def warn_inaccessible_base_class : Warning<
   "direct base %0 is inaccessible due to ambiguity:%1">,
   InGroup<DiagGroup<"inaccessible-base">>;
 // FIXME: better way to display derivation?  Pass entire thing into diagclient?
 def err_ambiguous_derived_to_base_conv : Error<
   "ambiguous conversion from derived class %0 to base class %1:%2">;
 def err_ambiguous_memptr_conv : Error<
   "ambiguous conversion from pointer to member of %select{base|derived}0 "
   "class %1 to pointer to member of %select{derived|base}0 class %2:%3">;
+def ext_ms_ambiguous_direct_base : ExtWarn<
+  "accessing inaccessible direct base %0 of %1 is a Microsoft extension">,
+  InGroup<MicrosoftInaccessibleBase>;
 
 def err_memptr_conv_via_virtual : Error<
   "conversion from pointer to member of class %0 to pointer to member "
   "of class %1 via virtual base %2 is not allowed">;
 
 // C++ member name lookup
 def err_ambiguous_member_multiple_subobjects : Error<
   "non-static member %0 found in multiple base-class subobjects of type %1:%2">;

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

if (!BaseRD)
     return false;
 
   if (!isCompleteType(Loc, Derived) && !DerivedRD->isBeingDefined())
     return false;
 
   return DerivedRD->isDerivedFrom(BaseRD, Paths);
 }
 
-void Sema::BuildBasePathArray(const CXXBasePaths &Paths,
+static void BuildBasePathArray(const CXXBasePath &Path,
                                CXXCastPath &BasePathArray) {
-  assert(BasePathArray.empty() && "Base path array must be empty!");
-  assert(Paths.isRecordingPaths() && "Must record paths!");
-
-  const CXXBasePath &Path = Paths.front();
-
   // We first go backward and check if we have a virtual base.
   // FIXME: It would be better if CXXBasePath had the base specifier for
   // the nearest virtual base.
   unsigned Start = 0;
   for (unsigned I = Path.size(); I != 0; --I) {
     if (Path[I - 1].Base->isVirtual()) {
       Start = I - 1;
       break;
     }
   }
 
   // Now add all bases.
   for (unsigned I = Start, E = Path.size(); I != E; ++I)
     BasePathArray.push_back(const_cast<CXXBaseSpecifier*>(Path[I].Base));
 }
 
+
+void Sema::BuildBasePathArray(const CXXBasePaths &Paths,
+                              CXXCastPath &BasePathArray) {
+  assert(BasePathArray.empty() && "Base path array must be empty!");
+  assert(Paths.isRecordingPaths() && "Must record paths!");
+  return ::BuildBasePathArray(Paths.front(), BasePathArray);
+}
 /// CheckDerivedToBaseConversion - Check whether the Derived-to-Base
 /// conversion (where Derived and Base are class types) is
 /// well-formed, meaning that the conversion is unambiguous (and
 /// that all of the base classes are accessible). Returns true
 /// and emits a diagnostic if the code is ill-formed, returns false
 /// otherwise. Loc is the location where this routine should point to
 /// if there is an error, and Range is the source range to highlight
 /// if there is an error.
Sema::CheckDerivedToBaseConversion(QualType Derived, QualType Base,
   // explore multiple paths to determine if there is an ambiguity.
   CXXBasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/true,
                      /*DetectVirtual=*/false);
   bool DerivationOkay = IsDerivedFrom(Loc, Derived, Base, Paths);
   assert(DerivationOkay &&
          "Can only be used with a derived-to-base conversion");
   (void)DerivationOkay;
 
-  if (!Paths.isAmbiguous(Context.getCanonicalType(Base).getUnqualifiedType())) {
+  const CXXBasePath *Path = nullptr;
+  if (!Paths.isAmbiguous(Context.getCanonicalType(Base).getUnqualifiedType()))
+    Path = &Paths.front();
+
+  // For MSVC compatibility, check if Derived directly inherits from Base. Clang
+  // warns about this hierarchy under -Winaccessible-base, but MSVC allows the
+  // user to access such bases.
+  if (!Path && getLangOpts().MSVCCompat) {
+    for (const CXXBasePath &PossiblePath : Paths) {
+      if (PossiblePath.size() == 1) {
+        Path = &PossiblePath;
+        if (AmbigiousBaseConvID)
+          Diag(Loc, diag::ext_ms_ambiguous_direct_base)
+              << Base << Derived << Range;
+        break;
+      }
+    }
+  }
+
+  if (Path) {
     if (!IgnoreAccess) {
       // Check that the base class can be accessed.
-      switch (CheckBaseClassAccess(Loc, Base, Derived, Paths.front(),
-                                   InaccessibleBaseID)) {
-        case AR_inaccessible:
-          return true;
-        case AR_accessible:
-        case AR_dependent:
-        case AR_delayed:
-          break;
+      switch (
+          CheckBaseClassAccess(Loc, Base, Derived, *Path, InaccessibleBaseID)) {
       }
     }
 
     // Build a base path if necessary.
     if (BasePath)
-      BuildBasePathArray(Paths, *BasePath);
+      ::BuildBasePathArray(*Path, *BasePath);
     return false;
   }
 
   if (AmbigiousBaseConvID) {
     // We know that the derived-to-base conversion is ambiguous, and
     // we're going to produce a diagnostic. Perform the derived-to-base
     // search just one more time to compute all of the possible paths so
     // that we can print them out. This is more expensive than any of

cfe/trunk/test/CodeGenCXX/microsoft-inaccessible-base.cpp

+// RUN: %clang_cc1 -fms-compatibility -triple x86_64-windows-msvc %s -emit-llvm -o - | FileCheck %s
+
+// Make sure we choose the *direct* base path when doing these conversions.
+
+// CHECK: %struct.C = type { %struct.A, %struct.B }
+// CHECK: %struct.D = type { %struct.B, %struct.A }
+
+struct A { int a; };
+struct B : A { int b; };
+
+struct C : A, B { };
+extern "C" A *a_from_c(C *p) { return p; }
+// CHECK-LABEL: define %struct.A* @a_from_c(%struct.C* %{{.*}})
+// CHECK: bitcast %struct.C* %{{.*}} to %struct.A*
+
+struct D : B, A { };
+extern "C" A *a_from_d(D *p) { return p; }
+// CHECK-LABEL: define %struct.A* @a_from_d(%struct.D* %{{.*}})
+// CHECK: %[[p_i8:[^ ]*]] = bitcast %struct.D* %{{.*}} to i8*
+// CHECK: getelementptr inbounds i8, i8* %[[p_i8]], i64 8

cfe/trunk/test/SemaCXX/accessible-base.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DMS -fms-compatibility -Wmicrosoft-inaccessible-base -fsyntax-only -verify %s
 
 struct A {
   int a;
 };
 
-struct X1 : virtual A 
+struct X1 : virtual A
 {};
 
 struct Y1 : X1, virtual A
 {};
 
 struct Y2 : X1, A // expected-warning{{direct base 'A' is inaccessible due to ambiguity:\n    struct Y2 -> struct X1 -> struct A\n    struct Y2 -> struct A}}
 {};
 
-struct X2 : A 
+struct X2 : A
 {};
 
 struct Z1 : X2, virtual A // expected-warning{{direct base 'A' is inaccessible due to ambiguity:\n    struct Z1 -> struct X2 -> struct A\n    struct Z1 -> struct A}}
 {};
 
 struct Z2 : X2, A // expected-warning{{direct base 'A' is inaccessible due to ambiguity:\n    struct Z2 -> struct X2 -> struct A\n    struct Z2 -> struct A}}
 {};
+
+A *y2_to_a(Y2 *p) {
+#ifdef MS
+  // expected-warning@+4 {{accessing inaccessible direct base 'A' of 'Y2' is a Microsoft extension}}
+#else
+  // expected-error@+2 {{ambiguous conversion}}
+#endif
+  return p;
+}
+
+A *z1_to_a(Z1 *p) {
+#ifdef MS
+  // expected-warning@+4 {{accessing inaccessible direct base 'A' of 'Z1' is a Microsoft extension}}
+#else
+  // expected-error@+2 {{ambiguous conversion}}
+#endif
+  return p;
+}
+
+A *z1_to_a(Z2 *p) {
+#ifdef MS
+  // expected-warning@+4 {{accessing inaccessible direct base 'A' of 'Z2' is a Microsoft extension}}
+#else
+  // expected-error@+2 {{ambiguous conversion}}
+#endif
+  return p;
+}