Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ Bug Fixes to Attribute Support
Bug Fixes to C++ Support
^^^^^^^^^^^^^^^^^^^^^^^^

- Fix crash when using lifetimebound attribute in function with trailing return.
Fixes (`#73619 <https://github.com/llvm/llvm-project/issues/73619>`_)

Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/AST/TypeLoc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
12 changes: 7 additions & 5 deletions clang/lib/Sema/TreeTransform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6131,7 +6131,9 @@ 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<CXXRecordDecl>(SemaRef.getCurLexicalContext());
Sema::CXXThisScopeRAII ThisScope(
SemaRef, !ThisContext && RD ? RD : ThisContext, ThisTypeQuals);

ResultType = getDerived().TransformType(TLB, TL.getReturnLoc());
if (ResultType.isNull())
Expand Down Expand Up @@ -7088,10 +7090,10 @@ QualType TreeTransform<Derived>::TransformAttributedType(
// FIXME: dependent operand expressions?
if (getDerived().AlwaysRebuild() ||
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;
AuxiliaryTLB.reserve(TL.getFullDataSize());
Comment on lines +7093 to +7094
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your remind! Fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

QualType equivalentType =
getDerived().TransformType(AuxiliaryTLB, TL.getEquivalentTypeLoc());
if (equivalentType.isNull())
return QualType();

Expand Down
17 changes: 17 additions & 0 deletions clang/test/Sema/attr-lifetimebound-no-crash.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// RUN: %clang_cc1 %s -verify -fsyntax-only

// expected-no-diagnostics

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];
}