Fixing a crash in Sema.

[jkorous-apple]

Summary

The original code is checking for inaccessible base classes but does not expect inheriting from template parameters (or dependent types in general) as these are not modelled by CXXRecord.

Issue was at this line since getAsCXXRecord() returned nullptr:

bool found
        = Class->isDerivedFrom(CanonicalBase->getAsCXXRecordDecl(), Paths);

[aaron.ballman]

Sema/SemaDeclCXX.cpp

2426

that base -> that the base

2427–2429

You can elide the braces here.

SemaCXX/base-class-ambiguity-check.cpp

2

This run line isn't testing anything. Since you're trying to ensure this doesn't crash, I would put -verify on the RUN line and // expected-no-diagnostics on the line below.

10

Can you add a newline at the end of the file, and then run the file through clang-format to properly format it?

10

I would add a comment around here explaining that this used to crash.

[jkorous-apple]

Changes based on Aaron's feedback.

[aaron.ballman]

SemaCXX/base-class-ambiguity-check.cpp

2

This comment hasn't been applied yet.

[jkorous-apple]

Sema/SemaDeclCXX.cpp

2426

Good catch. Thanks.

2427–2429

Of course. Thanks for reminder.

SemaCXX/base-class-ambiguity-check.cpp

2

I know what you mean. I was worried about that as well so I asked other guys. Apparently this is enough. If you run the test on master this is what you get:

> bin/llvm-lit /Users/jankorous/src/oss/llvm/llvm/tools/clang/test/SemaCXX/base-class-ambiguity-check.cpp
llvm-lit: /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/llvm/config.py:334: note: using clang: /Users/jankorous/src/oss/llvm/build/bin/clang
llvm-lit: /Users/jankorous/src/oss/llvm/llvm/utils/lit/lit/util.py:379: note: using SDKROOT: '/Applications/Edge9E77_AllSDKs_fromNFA/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk'
-- Testing: 1 tests, 1 threads --
FAIL: Clang :: SemaCXX/base-class-ambiguity-check.cpp (1 of 1)
Testing Time: 0.31s
********************
Failing Tests (1):
    Clang :: SemaCXX/base-class-ambiguity-check.cpp

  Unexpected Failures: 1

2

Sorry, what do you mean?

10

Good idea.

10

Sure. I forgot to clang-format the test.

[aaron.ballman]

SemaCXX/base-class-ambiguity-check.cpp

2

Weird, it seems Phab didn't have your latest comment when I posted mine -- sorry for the confusion!

"This is enough" != "This is the way it should be done" -- by adding -verify and // expected-no-diagnostics, you wind up testing that this code doesn't produce any diagnostics in addition to not crashing, which is an improvement over just testing there's no crash, as this code is perfectly reasonable and shouldn't produce a diagnostic.

7

The commit hash doesn't do much here (we're still on svn, but the revision information isn't all that helpful), so I'd replace it with "// Test that this code no longer causes a crash in Sema. See PRNNNN/rdar://NNNN" (assuming you have a report somewhere; if not, just leave off the last sentence).

[jkorous-apple]

SemaCXX/base-class-ambiguity-check.cpp

2

No problem, I thought that we are probably not on the same page.

Fair enough. I can do that.

7

Oh, right. Thanks.

[jkorous-apple]

Fixed test based on Aaron's comments.

[aaron.ballman]

LGTM, thank you!

[jkorous-apple]

Thank you!

[aaron.ballman]

In D41897#974618, @jkorous-apple wrote:

Thank you!

Do you have commit privileges, or would you like me to commit this on your behalf?

[jkorous-apple]

I do. Will commit this in a minute.

[jkorous-apple]

Landed as git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322438 91177308-0d34-0410-b5e6-96231b3b80d8

Sema/SemaDeclCXX.cpp

if (KnownBase) {
           Class->addAttr(WeakAttr::CreateImplicit(Context));
       }
     }
   }
 
   // Attach the remaining base class specifiers to the derived class.
   Class->setBases(Bases.data(), NumGoodBases);
 
+  // Check that the only base classes that are duplicate are virtual.
   for (unsigned idx = 0; idx < NumGoodBases; ++idx) {
     // Check whether this direct base is inaccessible due to ambiguity.
     QualType BaseType = Bases[idx]->getType();
+
+    // Skip all dependent types in templates being used as base specifiers.
+    // Checks below assume that the base specifier is a CXXRecord.
+    if (BaseType->isDependentType())
+      continue;
+
     CanQualType CanonicalBase = Context.getCanonicalType(BaseType)
       .getUnqualifiedType();
 
     if (IndirectBaseTypes.count(CanonicalBase)) {
       CXXBasePaths Paths(/*FindAmbiguities=*/true, /*RecordPaths=*/true,
                          /*DetectVirtual=*/true);
       bool found
         = Class->isDerivedFrom(CanonicalBase->getAsCXXRecordDecl(), Paths);

SemaCXX/base-class-ambiguity-check.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template <typename T> class Foo {
+  struct Base : T {};
+
+  // Test that this code no longer causes a crash in Sema. rdar://23291875
+  struct Derived : Base, T {};
+};