Skip to content

[clang] Non-object types are non-trivially relocatable #69734

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.

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
Nov 28, 2023

Conversation

AMP999
Copy link
Contributor

@AMP999 AMP999 commented Oct 20, 2023

Both active C++ proposals (P1144 and P2786) agree that is_trivially_relocatable_v<int&> and is_trivially_relocatable_v<int()> should be false, not true. Only complete object types can be trivially relocatable.

Fixes #67498

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 20, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 20, 2023

@llvm/pr-subscribers-clang

Author: Amirreza Ashouri (AMP999)

Changes

Both active C++ proposals (P1144 and P2786) agree that is_trivially_relocatable_v&lt;int&amp;&gt; and is_trivially_relocatable_v&lt;int()&gt; should be false, not true. Only complete object types can be trivially relocatable.

Fixes #67498


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

1 Files Affected:

  • (modified) clang/lib/AST/Type.cpp (+2)
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 8389b1423058197..bdeff1e4ac5b604 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2649,6 +2649,8 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
 
   if (BaseElementType->isIncompleteType()) {
     return false;
+  } else if (!BaseElementType->isObjectType()) {
+    return false;
   } else if (const auto *RD = BaseElementType->getAsRecordDecl()) {
     return RD->canPassInRegisters();
   } else {

@cor3ntin
Copy link
Contributor

cor3ntin commented Oct 25, 2023

Can you add tests / release notes? (the change itself looks fine to me)

@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from 6433528 to a67c7b8 Compare November 4, 2023 10:15
@AMP999
Copy link
Contributor Author

AMP999 commented Nov 4, 2023

@cor3ntin Are these tests what you had in mind? Are they sufficient? What else should I add?

@AMP999
Copy link
Contributor Author

AMP999 commented Nov 11, 2023

@cor3ntin Gentle ping!

@LYP951018 LYP951018 requested a review from cor3ntin November 11, 2023 15:02
@cor3ntin
Copy link
Contributor

@AMP999 Sorry, I'm in a committee meeting this week. tests look good. it's still missing a release note though

@AMP999
Copy link
Contributor Author

AMP999 commented Nov 11, 2023

Thank you! No problem! I'll add the release notes.

@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from a67c7b8 to ffc19dd Compare November 14, 2023 16:43
@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from ffc19dd to 187bfbf Compare November 16, 2023 11:42
@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from 187bfbf to befa993 Compare November 26, 2023 11:38
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@AMP999
Copy link
Contributor Author

AMP999 commented Nov 26, 2023

LGTM, thanks!

You're welcome!

@AMP999
Copy link
Contributor Author

AMP999 commented Nov 28, 2023

@cor3ntin Could you land this for me, please?

@cor3ntin
Copy link
Contributor

@AMP999 can you resolve the conflict?

@AMP999
Copy link
Contributor Author

AMP999 commented Nov 28, 2023

Oh, Sure. I didn't notice that.

@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from befa993 to a0b9798 Compare November 28, 2023 15:17
Both active C++ proposals (P1144 and P2786) agree that
`is_trivially_relocatable_v<int&>` and `is_trivially_relocatable_v<int()>`
should be false, not true. Only complete object types
can be trivially relocatable.

Fixes llvm#67498
@AMP999 AMP999 force-pushed the pr3-non-trivially-relocatable-object branch from a0b9798 to 8cad094 Compare November 28, 2023 15:18
@AMP999
Copy link
Contributor Author

AMP999 commented Nov 28, 2023

@cor3ntin I've fixed that.

@cor3ntin cor3ntin merged commit 98e674c into llvm:main Nov 28, 2023
@AMP999 AMP999 deleted the pr3-non-trivially-relocatable-object branch November 30, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__is_trivially_relocatable of non-object type is true
3 participants