This is an archive of the discontinued LLVM Phabricator instance.

Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel()
ClosedPublic

Authored by adr26 on May 9 2018, 3:31 PM.

Details

Summary

Ensure latest MPT decl has a MSInheritanceAttr when instantiating templates, to avoid null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel().

See PR#37399 for repo / details.

Diff Detail

Event Timeline

adr26 created this revision.May 9 2018, 3:31 PM
rnk added a comment.May 9 2018, 5:36 PM

Is it possible to fix this in assignInheritanceModel instead? I'd imagine we'd get the most recent decl. If that's not the issue, maybe you're fixing the bug in the right spot, but we need to find out where other class template attributes are moved from instantiation to real declaration.

lib/Sema/SemaTemplate.cpp
7079 ↗(On Diff #146016)

This seems like an overly-generic name for what it does, which currently is specific to class template instantiations.

adr26 updated this revision to Diff 148044.May 22 2018, 10:15 AM

Update to change MSInheritanceAttr to always be attached to the latest non-injected class name decl, as suggested by Reid.

rnk accepted this revision.May 30 2018, 11:35 AM

As far as workarounds go, this looks great! Please add a test case, I think you had one in the initial patch.

Should I go ahead and commit this?

This revision is now accepted and ready to land.May 30 2018, 11:35 AM
adr26 updated this revision to Diff 149300.EditedMay 31 2018, 8:20 AM

Hi Reid,

I've added testcases matching the issues I hit in microsoft-abi-member-pointers.cpp (no change to the rest of the fix). If you are happy with this, please feel free to push.

I checked with MSVC, and as per the added testcases, it appears to always lock in the inheritance model as virutal when there's a member funtion pointer in a dependant base, irrespective of whether the most-derived class ends up using single/multiple/virtual inheritance. In fact if, for example, you annotate DerivedFunctor (in the test I've added) using an MS inheritance keyword - i.e. as "class __single_inheritance DerviceFunctor" - then MSVC complains that it's already fixed the representation to be virtual:

C:\Temp\clang\clang>cl C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.14.26428.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

microsoft-abi-member-pointers.cpp
C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp(81): error C2286: pointers to members of 'pr37399::DerivedFunctor<void>' representation is already set to virtual_inheritance - declaration ignored
C:\workspaces\llvm\tools\clang\test\CodeGenCXX\microsoft-abi-member-pointers.cpp(27): note: see declaration of 'pr37399::DerivedFunctor<void>'

as such, these tests cover the cases I was hitting and appear to have behavioural parity with MSVC (as is the intention!).

Thanks, Andrew R

rnk added a comment.May 31 2018, 11:46 AM

Thanks for the patch, I committed it as r333680 with a minor modification.

This revision was automatically updated to reflect the committed changes.

That's a nice change to avoid duplication in ClassTemplateSpecializationDecl::getMostRecentDecl(). Thanks for your help getting this out the door!