[Clang][Sema] fix crash of attribute transform #78088

jcsxky · 2024-01-14T07:47:49Z

Try to fix issue

During transforming FunctionProtoType, if ThisContext is nullptr and CurrentContext is ClassTemplateSpecializationDecl, Constructor of CXXThisScopeRAII and Sema::getCurrentThisType won't set CXXThisTypeOverride of Sema. This will lead to building this in RebuildCXXThisExpr with a invalid type(NULL type) and cause crash.
During transforming attribute type, if modifiedType of attribute type is changed, EquivalentType need to be transformed. If EquivalentType is FunctionProtoType, its ParamVarDecl will not be copyed(but parameter num does) and will not be instanced in TransformFunctionTypeParams since ParamVarDecl is nullptr. This will lead to crash in findInstantiationOf(can't find the instance of ParamVarDecl).

This patch tries to fix these issues above.

If CurrentContext is ClassTemplateSpecializationDecl, Use it.
Use EquivalentTypeLoc instead of EquivalentType since it has parameter info. But, if use current TypeLocBuilder, it will crash in TypeLocBuilder::push since LastType is mismatch. Use an auxiliary TypeLocBuilder instead and get transformed EquivalentType.

llvmbot · 2024-01-14T07:48:19Z

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Try to fix issue

During transforming FunctionProtoType, if ThisContext, is nullptr and CurrentContext is ClassTemplateSpecializationDecl, Constructor of CXXThisScopeRAII and Sema::getCurrentThisType won't set CXXThisTypeOverride of Sema. This will lead to building this in RebuildCXXThisExpr with a invalid type and cause crash.
During transforming attribute type, if modifiedType of attribute type is changed, EquivalentType will be transformed. If EquivalentType is FunctionProtoType, its ParamVarDecl will not be copyed(but parameter num does) and will not be instanced in TransformFunctionTypeParams since ParamVarDecl is nullptr. This will lead to crash in findInstantiationOf.

This patch tries to fix these issue above.

If CurrentContext is ClassTemplateSpecializationDecl, Use it.
Use EquivalentTypeLoc instead of EquivalentType since it has parameter info. But, if use current TypeLocBuilder, it will crash in TypeLocBuilder::push since LastType is mismatch. Use an auxiliary TypeLocBuilder instead and get transformed EquivalentType.

Full diff: https://github.com/llvm/llvm-project/pull/78088.diff

3 Files Affected:

(modified) clang/include/clang/AST/TypeLoc.h (+4)
(modified) clang/lib/Sema/TreeTransform.h (+8-3)
(added) clang/test/Sema/attr-lifetimebound-no-crash.cpp (+15)

diff --git a/clang/include/clang/AST/TypeLoc.h b/clang/include/clang/AST/TypeLoc.h
index 471deb14aba51f..04780fdeae3bc1 100644
--- a/clang/include/clang/AST/TypeLoc.h
+++ b/clang/include/clang/AST/TypeLoc.h
@@ -884,6 +884,10 @@ class AttributedTypeLoc : public ConcreteTypeLoc<UnqualTypeLoc,
     return getInnerTypeLoc();
   }
 
+  TypeLoc getEquivalentTypeLoc() const {
+    return TypeLoc(getTypePtr()->getEquivalentType(), getNonLocalData());
+  }
+
   /// The type attribute.
   const Attr *getAttr() const {
     return getLocalData()->TypeAttr;
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 1a1bc87d2b3203..be5ba2000de197 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -6124,7 +6124,11 @@ QualType TreeTransform<Derived>::TransformFunctionProtoType(
       //   "pointer to cv-qualifier-seq X" between the optional cv-qualifer-seq
       //   and the end of the function-definition, member-declarator, or
       //   declarator.
-      Sema::CXXThisScopeRAII ThisScope(SemaRef, ThisContext, ThisTypeQuals);
+      auto *RD =
+          dyn_cast_or_null<CXXRecordDecl>(SemaRef.getCurLexicalContext());
+      Sema::CXXThisScopeRAII ThisScope(
+          SemaRef, ThisContext == nullptr && nullptr != RD ? RD : ThisContext,
+          ThisTypeQuals);
 
       ResultType = getDerived().TransformType(TLB, TL.getReturnLoc());
       if (ResultType.isNull())
@@ -7083,8 +7087,9 @@ QualType TreeTransform<Derived>::TransformAttributedType(
       modifiedType != oldType->getModifiedType()) {
     // TODO: this is really lame; we should really be rebuilding the
     // equivalent type from first principles.
-    QualType equivalentType
-      = getDerived().TransformType(oldType->getEquivalentType());
+    TypeLocBuilder AuxiliaryTLB;
+    QualType equivalentType =
+        getDerived().TransformType(AuxiliaryTLB, TL.getEquivalentTypeLoc());
     if (equivalentType.isNull())
       return QualType();
 
diff --git a/clang/test/Sema/attr-lifetimebound-no-crash.cpp b/clang/test/Sema/attr-lifetimebound-no-crash.cpp
new file mode 100644
index 00000000000000..32e015bab02918
--- /dev/null
+++ b/clang/test/Sema/attr-lifetimebound-no-crash.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+template<typename T>
+struct Bar {
+    int* data;
+
+    auto operator[](const int index) const [[clang::lifetimebound]] -> decltype(data[index]) {
+        return data[index];
+    }
+};
+
+int main() {
+    Bar<int> b;
+    (void)b[2];
+}
\ No newline at end of file

cor3ntin · 2024-01-16T15:04:29Z

clang/lib/Sema/TreeTransform.h

    // TODO: this is really lame; we should really be rebuilding the
    // equivalent type from first principles.


This comment would no longer apply, would it?

Indeed, it has been rebuilt using TypeLoc like ModifiedType.

cor3ntin · 2024-01-16T15:04:44Z

clang/test/Sema/attr-lifetimebound-no-crash.cpp

+int main() {
+    Bar<int> b;
+    (void)b[2];
+}


Missing new line

cor3ntin · 2024-01-17T07:47:59Z

clang/lib/Sema/TreeTransform.h

+    TypeLocBuilder AuxiliaryTLB;
+    AuxiliaryTLB.reserve(TL.getFullDataSize());


an alternative might be to call
TLB.TypeWasModifiedSafely(result) (after the call to getAttributedType below).
I wonder if @AaronBallman has a better alternative

Yeah, I think that's a better approach. Transforming the type should not have changed anything about type location information associated with the type. WDYT @erichkeane ?

Right, yeah, the location shouldn't have changed, just the type itself. I think the suggestions above are the way to go.

Thanks for your remind! Fixed.

I don't think we need to use AuxiliaryTLB any longer, do we?

I‘m afraid not. It will crash on TypeLocBuilder::pushImpl with the new added test case since TLast == LastTy assert failed if we reuse TLB. Because current FunctionProtoType can't be child of other type node.

Ah, you're right, we do still need to do that dance it seems.

@cor3ntin @AaronBallman @erichkeane The test on windows platform failed with TLB.TypeWasModifiedSafely(result);. Maybe we shouldn't change LastTy of TLB since we just transform EquivalentType using AuxiliaryTLB and haven't touched TLB.

I tested address-spaces.cpp on linux and TLast == LastTy assert also failed. Don't know why the test on linux platform passed.

Yeah, I was thinking we didn't need to use AuxiliaryTLB and could instead call TypeWasModifiedSafely but it seems like we need the code as you originally had it (using AuxiliaryTLB but not calling TypeWasModifiedSafely). Sorry for the run-around!

AaronBallman

LGTM but please wait in case @erichkeane has final thoughts.

AaronBallman · 2024-01-18T15:42:41Z

clang/lib/Sema/TreeTransform.h

+    TypeLocBuilder AuxiliaryTLB;
+    AuxiliaryTLB.reserve(TL.getFullDataSize());


Ah, you're right, we do still need to do that dance it seems.

AaronBallman

Please be sure to add a release note about the fix to clang/docs/ReleaseNotes.rst.

AaronBallman · 2024-01-19T16:53:33Z

clang/lib/Sema/TreeTransform.h

+    TypeLocBuilder AuxiliaryTLB;
+    AuxiliaryTLB.reserve(TL.getFullDataSize());


Yeah, I was thinking we didn't need to use AuxiliaryTLB and could instead call TypeWasModifiedSafely but it seems like we need the code as you originally had it (using AuxiliaryTLB but not calling TypeWasModifiedSafely). Sorry for the run-around!

AaronBallman · 2024-01-19T16:56:05Z

clang/lib/Sema/TreeTransform.h

+      auto *RD =
+          dyn_cast_or_null<CXXRecordDecl>(SemaRef.getCurLexicalContext());
+      Sema::CXXThisScopeRAII ThisScope(
+          SemaRef, ThisContext == nullptr && nullptr != RD ? RD : ThisContext,
+          ThisTypeQuals);


Suggested change

auto *RD =

dyn_cast_or_null<CXXRecordDecl>(SemaRef.getCurLexicalContext());

Sema::CXXThisScopeRAII ThisScope(

SemaRef, ThisContext == nullptr && nullptr != RD ? RD : ThisContext,

ThisTypeQuals);

auto *RD =

dyn_cast<CXXRecordDecl>(SemaRef.getCurLexicalContext());

Sema::CXXThisScopeRAII ThisScope(

SemaRef, !ThisContext && RD ? RD : ThisContext,

ThisTypeQuals);

Code Fixed and release note has been added.

cor3ntin · 2024-01-25T09:28:15Z

There seems to be an issue with the rebasing of the PR.
As we branch clang 18, the new changelog (for 19) being empty is expected

jcsxky · 2024-01-25T09:40:48Z

There seems to be an issue with the rebasing of the PR. As we branch clang 18, the new changelog (for 19) being empty is expected

I try to merge this PR and found ReleaseNotes.rst is changed because of clang-19 initialize. I don't know where should I add the release note.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 14, 2024

jcsxky force-pushed the fix_crash_attribute_transform branch from b5dfbd8 to d040754 Compare January 14, 2024 08:13

jcsxky requested review from erichkeane, cor3ntin, tbaederr and philnik777 January 14, 2024 09:16

cor3ntin reviewed Jan 16, 2024

View reviewed changes

jcsxky force-pushed the fix_crash_attribute_transform branch from fb6aaee to 55b433e Compare January 17, 2024 05:01

cor3ntin reviewed Jan 17, 2024

View reviewed changes

jcsxky force-pushed the fix_crash_attribute_transform branch from 55b433e to 7632f63 Compare January 18, 2024 09:21

AaronBallman approved these changes Jan 18, 2024

View reviewed changes

jcsxky force-pushed the fix_crash_attribute_transform branch from 7632f63 to b0fcd3b Compare January 19, 2024 03:40

AaronBallman reviewed Jan 19, 2024

View reviewed changes

jcsxky force-pushed the fix_crash_attribute_transform branch 3 times, most recently from 0e74e6c to 2d6a39f Compare January 25, 2024 08:45

jcsxky force-pushed the fix_crash_attribute_transform branch from 2d6a39f to 707c471 Compare January 26, 2024 06:14

[Clang][Sema] fix crash of attribute transform

9e7ba80

jcsxky force-pushed the fix_crash_attribute_transform branch from 707c471 to 9e7ba80 Compare January 26, 2024 06:16

jcsxky merged commit d9d1ae6 into llvm:main Jan 26, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] fix crash of attribute transform #78088

[Clang][Sema] fix crash of attribute transform #78088

jcsxky commented Jan 14, 2024 •

edited

llvmbot commented Jan 14, 2024

cor3ntin Jan 16, 2024

jcsxky Jan 17, 2024

cor3ntin Jan 16, 2024

jcsxky Jan 17, 2024

cor3ntin Jan 17, 2024

AaronBallman Jan 17, 2024

erichkeane Jan 17, 2024

jcsxky Jan 18, 2024

AaronBallman Jan 18, 2024

jcsxky Jan 18, 2024

AaronBallman Jan 18, 2024

jcsxky Jan 19, 2024

jcsxky Jan 19, 2024

AaronBallman Jan 19, 2024

AaronBallman left a comment

AaronBallman Jan 18, 2024

AaronBallman left a comment

AaronBallman Jan 19, 2024

AaronBallman Jan 19, 2024

jcsxky Jan 21, 2024

cor3ntin commented Jan 25, 2024

jcsxky commented Jan 25, 2024

		// TODO: this is really lame; we should really be rebuilding the
		// equivalent type from first principles.

		TypeLocBuilder AuxiliaryTLB;
		AuxiliaryTLB.reserve(TL.getFullDataSize());

[Clang][Sema] fix crash of attribute transform #78088

[Clang][Sema] fix crash of attribute transform #78088

Conversation

jcsxky commented Jan 14, 2024 • edited

llvmbot commented Jan 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cor3ntin commented Jan 25, 2024

jcsxky commented Jan 25, 2024

jcsxky commented Jan 14, 2024 •

edited