https://github.com/guillem-bartrina-sonarsource updated https://github.com/llvm/llvm-project/pull/214213
>From 8ebfea30aafbe2ee6ae17b584b51aa38d9be6b23 Mon Sep 17 00:00:00 2001 From: guillem-bartrina-sonarsource <[email protected]> Date: Wed, 5 Aug 2026 13:34:46 +0200 Subject: [PATCH 1/5] [ASTImporter] Propagate MSInheritanceAttr onto imported redeclarations Under the Microsoft C++ ABI, Sema::assignInheritanceModel() attaches an implicit MSInheritanceAttr to whichever declaration of a class is most recent when a pointer-to-member type first has to be complete, and CXXRecordDecl::getMSInheritanceModel() reads it back off getMostRecentCXXRecordDecl(). Importing a declaration of that class appends it to the redeclaration chain and makes it the most recent one, but the attribute is not inherited along the chain, so a later query for the model -- e.g. laying out a record with a pointer-to-member field -- reaches a declaration without it and asserts, or dereferences null in a release build. Clone it onto the imported declaration instead, searching the whole chain since Sema may have attached it to any node. Add a CTU regression test reproducing the crash. --- clang/lib/AST/ASTImporter.cpp | 22 ++++ .../msinheritance-member-pointer.cpp | 115 ++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 3ad71a223903c..740e857b625cc 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -3386,6 +3386,24 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) { return D2; } +/// Carry over the inheritable attributes onto a freshly imported declaration. +static void importInheritableAttrs(ASTContext &Ctx, Decl *To, Decl *Prev) { + if (!To || !Prev || To == Prev) + return; + + if (!isa<CXXRecordDecl>(To) || To->hasAttr<MSInheritanceAttr>()) + return; + + for (Decl *R : Prev->redecls()) { + if (const auto *IA = R->getAttr<MSInheritanceAttr>()) { + auto *Clone = cast<InheritableAttr>(IA->clone(Ctx)); + Clone->setInherited(true); + To->addAttr(Clone); + return; + } + } +} + ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { bool IsFriendTemplate = false; if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) { @@ -3568,6 +3586,8 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { addDeclToContexts(D, D2); } + importInheritableAttrs(Importer.getToContext(), D2, PrevDecl); + if (auto BraceRangeOrErr = import(D->getBraceRange())) D2->setBraceRange(*BraceRangeOrErr); else @@ -6621,6 +6641,8 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl( ClassTemplate->AddSpecialization(D2, InsertPos); } + importInheritableAttrs(Importer.getToContext(), D2, PrevDecl); + D2->setSpecializationKind(D->getSpecializationKind()); // Set the context of this specialization/instantiation. diff --git a/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp b/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp new file mode 100644 index 0000000000000..d639f69e9b552 --- /dev/null +++ b/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp @@ -0,0 +1,115 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t + +// Pathological case: under the Microsoft C++ ABI a class's inheritance model is +// assigned lazily. Sema::assignInheritanceModel() attaches an implicit +// MSInheritanceAttr to whichever declaration of the class is most recent at the +// moment a pointer-to-member type first has to be complete, and +// CXXRecordDecl::getMSInheritanceModel() later reads it back off +// getMostRecentCXXRecordDecl(). Importing a declaration of that class appends it +// to the redeclaration chain, so it becomes the most recent one, but the +// attribute stays behind on the declaration Sema picked -- it is not inherited +// along the chain. Any later query then reaches a declaration without the +// attribute and asserts; a release build dereferences null instead. + +// RUN: %clang_cc1 -std=c++17 -triple x86_64-pc-windows \ +// RUN: -fms-compatibility -fms-extensions -fms-compatibility-version=19.40 \ +// RUN: -fdelayed-template-parsing \ +// RUN: -emit-pch -o %t/MavLinkConnectionImpl.cpp.ast %t/MavLinkConnectionImpl.cpp + +// RUN: %clang_extdef_map %t/MavLinkConnectionImpl.cpp -- -std=c++17 \ +// RUN: --target=x86_64-pc-windows -fms-compatibility -fms-extensions \ +// RUN: -fms-compatibility-version=19.40 -fdelayed-template-parsing \ +// RUN: > %t/externalDefMap.tmp.txt +// On windows, absolute paths generated by extdef_map are not recognized, +// so CSA prepends the workdir path to them. Force relative paths to work +// around this issue. The match is anchored and excludes spaces because the USR +// of Holder::Holder contains one (its member-pointer parameter) -- which is what +// the "<length>:" prefix of every defmap entry exists to tolerate. +// RUN: sed -e 's| [^ ]*/MavLinkConnectionImpl\.cpp$| MavLinkConnectionImpl.cpp.ast|' \ +// RUN: %t/externalDefMap.tmp.txt > %t/externalDefMap.txt + +// RUN: %clang_cc1 -std=c++17 -triple x86_64-pc-windows \ +// RUN: -fms-compatibility -fms-extensions -fms-compatibility-version=19.40 \ +// RUN: -fdelayed-template-parsing -analyze \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-config experimental-enable-naive-ctu-analysis=true \ +// RUN: -analyzer-config ctu-dir=%t \ +// RUN: -verify %t/main.cpp + +//--- main.cpp + +// expected-no-diagnostics + +namespace impl { +class Inner; +} + +class Outer { + void entry(); + +public: + impl::Inner *pInner; +}; + +// This definition is the one Sema hands the MSInheritanceAttr to, because it is +// the most recent declaration of impl::Inner while this TU is parsed. +namespace impl { +class Inner { +public: + void entry(); + void target(); +}; +} // namespace impl + +void Outer::entry() { + // Importing impl::Inner::entry brings the other TU's forward declaration of + // impl::Inner along, appending it after the definition above and making it the + // most recent declaration -- without the attribute. Inlining the imported body + // then lays out Holder, which asks for the inheritance model again. + pInner->entry(); // no-crash +} + +//--- MavLinkConnectionImpl.cpp + +namespace impl { +// This forward declaration is what ends up appended to the main TU's chain. +class Inner; +} // namespace impl + +class Outer { +public: + impl::Inner *pInner; +}; + +namespace impl { +class Inner { + void entry(); + void target(); +}; +} // namespace impl + +// Holds a pointer-to-member by value, so laying this record out needs the MS +// inheritance model of impl::Inner. It must not be a template: an implicit +// instantiation of a template member never reaches the external definition map, +// so naive CTU could not import it, and an uninlined constructor never triggers +// the layout. +class Holder { + void (impl::Inner::*pm)(); + +public: + Holder(void (impl::Inner::*p)()) : pm(p) {} +}; + +Outer *g; + +void impl::Inner::target() { + // A field access through a pointer, so the analyzer builds the region whose + // getAsOffset() forces the record layout. An empty body does not reproduce. + g->pInner; +} + +void impl::Inner::entry() { + Holder h(&impl::Inner::target); +} >From de079430eb78fd48a5ea812e56f1b73771cdc69a Mon Sep 17 00:00:00 2001 From: guillem-bartrina-sonarsource <[email protected]> Date: Wed, 19 Aug 2026 10:40:34 +0200 Subject: [PATCH 2/5] Add unittest test --- clang/unittests/AST/ASTImporterTest.cpp | 39 +++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index 503f5da8af90f..d08f365f38d6a 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -8260,6 +8260,42 @@ TEST_P(ImportAttributes, ImportC99NoThrowAttr) { checkImported(FromAttr->getAttrName(), ToAttr->getAttrName()); } +struct ImportMSInheritanceAttr : ASTImporterOptionSpecificTestBase { + std::vector<std::string> getExtraArgs() const override { + return {"--target=x86_64-pc-windows-msvc"}; + } +}; + +// The implicit MSInheritanceAttr must be cloned onto an imported +// redeclaration, since it becomes the most recent one and +// getMSInheritanceModel() reads the attribute off of that. +TEST_P(ImportMSInheritanceAttr, PropagatedOntoImportedRedecl) { + Decl *ToTU = getToTuDecl( + R"( + namespace NS { class Inner {}; } + struct HasPM { void (NS::Inner::*PM)(); }; + )", + Lang_CXX17); + auto *ToInner = FirstDeclMatcher<CXXRecordDecl>().match( + ToTU, cxxRecordDecl(hasName("Inner"), isDefinition())); + ASSERT_TRUE(ToInner->hasAttr<MSInheritanceAttr>()); + + Decl *FromTU = + getTuDecl("namespace NS { class Inner; }", Lang_CXX17, "from.cc"); + auto *FromInner = + FirstDeclMatcher<CXXRecordDecl>().match(FromTU, cxxRecordDecl(hasName("Inner"))); + + auto *ToImportedInner = cast<CXXRecordDecl>(Import(FromInner, Lang_CXX17)); + ASSERT_TRUE(ToImportedInner); + + // The newly imported redeclaration must have become the most recent one. + ASSERT_EQ(ToImportedInner, ToInner->getMostRecentDecl()); + + EXPECT_TRUE(ToImportedInner->hasAttr<MSInheritanceAttr>()); + EXPECT_EQ(ToImportedInner->getMSInheritanceModel(), + MSInheritanceModel::Single); +} + template <typename T> auto ExtendWithOptions(const T &Values, const std::vector<std::string> &Args) { auto Copy = Values; @@ -10867,6 +10903,9 @@ INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportWithExternalSource, INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportAttributes, DefaultTestValuesForRunOptions); +INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportMSInheritanceAttr, + ::testing::Values(std::vector<std::string>())); + INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportInjectedClassNameType, DefaultTestValuesForRunOptions); >From 07b68fb27409eec36e33ea8fe86298d323cc6737 Mon Sep 17 00:00:00 2001 From: guillem-bartrina-sonarsource <[email protected]> Date: Wed, 19 Aug 2026 10:48:38 +0200 Subject: [PATCH 3/5] Remove lit test --- .../msinheritance-member-pointer.cpp | 115 ------------------ 1 file changed, 115 deletions(-) delete mode 100644 clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp diff --git a/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp b/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp deleted file mode 100644 index d639f69e9b552..0000000000000 --- a/clang/test/Analysis/ctu/regression/msinheritance-member-pointer.cpp +++ /dev/null @@ -1,115 +0,0 @@ -// RUN: rm -rf %t -// RUN: mkdir -p %t -// RUN: split-file %s %t - -// Pathological case: under the Microsoft C++ ABI a class's inheritance model is -// assigned lazily. Sema::assignInheritanceModel() attaches an implicit -// MSInheritanceAttr to whichever declaration of the class is most recent at the -// moment a pointer-to-member type first has to be complete, and -// CXXRecordDecl::getMSInheritanceModel() later reads it back off -// getMostRecentCXXRecordDecl(). Importing a declaration of that class appends it -// to the redeclaration chain, so it becomes the most recent one, but the -// attribute stays behind on the declaration Sema picked -- it is not inherited -// along the chain. Any later query then reaches a declaration without the -// attribute and asserts; a release build dereferences null instead. - -// RUN: %clang_cc1 -std=c++17 -triple x86_64-pc-windows \ -// RUN: -fms-compatibility -fms-extensions -fms-compatibility-version=19.40 \ -// RUN: -fdelayed-template-parsing \ -// RUN: -emit-pch -o %t/MavLinkConnectionImpl.cpp.ast %t/MavLinkConnectionImpl.cpp - -// RUN: %clang_extdef_map %t/MavLinkConnectionImpl.cpp -- -std=c++17 \ -// RUN: --target=x86_64-pc-windows -fms-compatibility -fms-extensions \ -// RUN: -fms-compatibility-version=19.40 -fdelayed-template-parsing \ -// RUN: > %t/externalDefMap.tmp.txt -// On windows, absolute paths generated by extdef_map are not recognized, -// so CSA prepends the workdir path to them. Force relative paths to work -// around this issue. The match is anchored and excludes spaces because the USR -// of Holder::Holder contains one (its member-pointer parameter) -- which is what -// the "<length>:" prefix of every defmap entry exists to tolerate. -// RUN: sed -e 's| [^ ]*/MavLinkConnectionImpl\.cpp$| MavLinkConnectionImpl.cpp.ast|' \ -// RUN: %t/externalDefMap.tmp.txt > %t/externalDefMap.txt - -// RUN: %clang_cc1 -std=c++17 -triple x86_64-pc-windows \ -// RUN: -fms-compatibility -fms-extensions -fms-compatibility-version=19.40 \ -// RUN: -fdelayed-template-parsing -analyze \ -// RUN: -analyzer-checker=core \ -// RUN: -analyzer-config experimental-enable-naive-ctu-analysis=true \ -// RUN: -analyzer-config ctu-dir=%t \ -// RUN: -verify %t/main.cpp - -//--- main.cpp - -// expected-no-diagnostics - -namespace impl { -class Inner; -} - -class Outer { - void entry(); - -public: - impl::Inner *pInner; -}; - -// This definition is the one Sema hands the MSInheritanceAttr to, because it is -// the most recent declaration of impl::Inner while this TU is parsed. -namespace impl { -class Inner { -public: - void entry(); - void target(); -}; -} // namespace impl - -void Outer::entry() { - // Importing impl::Inner::entry brings the other TU's forward declaration of - // impl::Inner along, appending it after the definition above and making it the - // most recent declaration -- without the attribute. Inlining the imported body - // then lays out Holder, which asks for the inheritance model again. - pInner->entry(); // no-crash -} - -//--- MavLinkConnectionImpl.cpp - -namespace impl { -// This forward declaration is what ends up appended to the main TU's chain. -class Inner; -} // namespace impl - -class Outer { -public: - impl::Inner *pInner; -}; - -namespace impl { -class Inner { - void entry(); - void target(); -}; -} // namespace impl - -// Holds a pointer-to-member by value, so laying this record out needs the MS -// inheritance model of impl::Inner. It must not be a template: an implicit -// instantiation of a template member never reaches the external definition map, -// so naive CTU could not import it, and an uninlined constructor never triggers -// the layout. -class Holder { - void (impl::Inner::*pm)(); - -public: - Holder(void (impl::Inner::*p)()) : pm(p) {} -}; - -Outer *g; - -void impl::Inner::target() { - // A field access through a pointer, so the analyzer builds the region whose - // getAsOffset() forces the record layout. An empty body does not reproduce. - g->pInner; -} - -void impl::Inner::entry() { - Holder h(&impl::Inner::target); -} >From 213129bd396964c9238d2d43a36623f032e7db15 Mon Sep 17 00:00:00 2001 From: guillem-bartrina-sonarsource <[email protected]> Date: Wed, 19 Aug 2026 12:48:22 +0200 Subject: [PATCH 4/5] Refactor attr propagation, avoid duplication --- clang/lib/AST/ASTImporter.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 740e857b625cc..d30014b24e2ca 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -3386,12 +3386,9 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) { return D2; } -/// Carry over the inheritable attributes onto a freshly imported declaration. -static void importInheritableAttrs(ASTContext &Ctx, Decl *To, Decl *Prev) { - if (!To || !Prev || To == Prev) - return; - - if (!isa<CXXRecordDecl>(To) || To->hasAttr<MSInheritanceAttr>()) +static void importInheritableAttrs(ASTContext &Ctx, CXXRecordDecl *To, + CXXRecordDecl *Prev) { + if (!To || !Prev || To == Prev || To->hasAttr<MSInheritanceAttr>()) return; for (Decl *R : Prev->redecls()) { @@ -3586,8 +3583,6 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) { addDeclToContexts(D, D2); } - importInheritableAttrs(Importer.getToContext(), D2, PrevDecl); - if (auto BraceRangeOrErr = import(D->getBraceRange())) D2->setBraceRange(*BraceRangeOrErr); else @@ -6641,8 +6636,6 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl( ClassTemplate->AddSpecialization(D2, InsertPos); } - importInheritableAttrs(Importer.getToContext(), D2, PrevDecl); - D2->setSpecializationKind(D->getSpecializationKind()); // Set the context of this specialization/instantiation. @@ -10159,6 +10152,12 @@ Expected<Decl *> ASTImporter::Import(Decl *FromD) { return ToAttrOrErr.takeError(); } + // Now that ToD's own attributes have been imported above, carry over + // an MSInheritanceAttr from a previous redeclaration if ToD doesn't + // already have one of its own. + if (auto *RD = dyn_cast<CXXRecordDecl>(ToD)) + importInheritableAttrs(getToContext(), RD, RD->getPreviousDecl()); + // Notify subclasses. Imported(FromD, ToD); >From 68ba225dc6e75ddfde6390fb8a99852917a7fa59 Mon Sep 17 00:00:00 2001 From: guillem-bartrina-sonarsource <[email protected]> Date: Wed, 19 Aug 2026 13:05:44 +0200 Subject: [PATCH 5/5] format --- clang/lib/AST/ASTImporter.cpp | 2 +- clang/unittests/AST/ASTImporterTest.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index d30014b24e2ca..9e3a3bf0d7296 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -3387,7 +3387,7 @@ ExpectedDecl ASTNodeImporter::VisitEnumDecl(EnumDecl *D) { } static void importInheritableAttrs(ASTContext &Ctx, CXXRecordDecl *To, - CXXRecordDecl *Prev) { + CXXRecordDecl *Prev) { if (!To || !Prev || To == Prev || To->hasAttr<MSInheritanceAttr>()) return; diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index d08f365f38d6a..6776befc765d2 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -8282,8 +8282,8 @@ TEST_P(ImportMSInheritanceAttr, PropagatedOntoImportedRedecl) { Decl *FromTU = getTuDecl("namespace NS { class Inner; }", Lang_CXX17, "from.cc"); - auto *FromInner = - FirstDeclMatcher<CXXRecordDecl>().match(FromTU, cxxRecordDecl(hasName("Inner"))); + auto *FromInner = FirstDeclMatcher<CXXRecordDecl>().match( + FromTU, cxxRecordDecl(hasName("Inner"))); auto *ToImportedInner = cast<CXXRecordDecl>(Import(FromInner, Lang_CXX17)); ASSERT_TRUE(ToImportedInner); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
