This is an archive of the discontinued LLVM Phabricator instance.

Fixing a crash in Sema.
ClosedPublic

Authored by jkorous-apple on Jan 10 2018, 4:02 AM.

Details

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);

Diff Detail

Event Timeline

jkorous-apple created this revision.Jan 10 2018, 4:02 AM
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
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.

Changes based on Aaron's feedback.

aaron.ballman added inline comments.Jan 11 2018, 11:15 AM
SemaCXX/base-class-ambiguity-check.cpp
2

This comment hasn't been applied yet.

jkorous-apple added inline comments.Jan 12 2018, 2:38 AM
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 added inline comments.Jan 12 2018, 7:28 AM
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 added inline comments.Jan 12 2018, 7:33 AM
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.

Fixed test based on Aaron's comments.

aaron.ballman accepted this revision.Jan 12 2018, 7:55 AM

LGTM, thank you!

This revision is now accepted and ready to land.Jan 12 2018, 7:55 AM

Thank you!

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

I do. Will commit this in a minute.

jkorous-apple closed this revision.Apr 18 2018, 4:26 AM

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