[clang] Fix behavior of `__is_trivially_relocatable(volatile int)` #77092

AMP999 · 2024-01-05T13:15:49Z

Consistent with __is_trivially_copyable(volatile int) == true and __is_trivially_relocatable(volatile Trivial) == true, __is_trivially_relocatable(volatile int) should also be true.

Fixes #77091

[clang] [test] New tests for __is_trivially_relocatable(cv-qualified type)

llvmbot · 2024-01-05T13:16:19Z

@llvm/pr-subscribers-clang

Author: Amirreza Ashouri (AMP999)

Changes

Consistent with __is_trivially_copyable(volatile int) == true and __is_trivially_relocatable(volatile Trivial) == true, __is_trivially_relocatable(volatile int) should also be true.

Fixes #77091

[clang] [test] New tests for __is_trivially_relocatable(cv-qualified type)

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

2 Files Affected:

(modified) clang/lib/AST/Type.cpp (+2)
(modified) clang/test/SemaCXX/type-traits.cpp (+12)

diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 160a725939ccd4..96278370d9ff7e 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2651,6 +2651,8 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
     return false;
   } else if (!BaseElementType->isObjectType()) {
     return false;
+  } else if (BaseElementType.isTriviallyCopyableType(Context)) {
+    return true;
   } else if (const auto *RD = BaseElementType->getAsRecordDecl()) {
     return RD->canPassInRegisters();
   } else {
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index c5d196a2590f8d..36991fa2ca1dc8 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -3092,6 +3092,8 @@ namespace is_trivially_relocatable {
 static_assert(!__is_trivially_relocatable(void), "");
 static_assert(__is_trivially_relocatable(int), "");
 static_assert(__is_trivially_relocatable(int[]), "");
+static_assert(__is_trivially_relocatable(const int), "");
+static_assert(__is_trivially_relocatable(volatile int), "");
 
 enum Enum {};
 static_assert(__is_trivially_relocatable(Enum), "");
@@ -3104,6 +3106,8 @@ static_assert(__is_trivially_relocatable(Union[]), "");
 struct Trivial {};
 static_assert(__is_trivially_relocatable(Trivial), "");
 static_assert(__is_trivially_relocatable(Trivial[]), "");
+static_assert(__is_trivially_relocatable(const Trivial), "");
+static_assert(__is_trivially_relocatable(volatile Trivial), "");
 
 struct Incomplete; // expected-note {{forward declaration of 'is_trivially_relocatable::Incomplete'}}
 bool unused = __is_trivially_relocatable(Incomplete); // expected-error {{incomplete type}}
@@ -3113,6 +3117,8 @@ struct NontrivialDtor {
 };
 static_assert(!__is_trivially_relocatable(NontrivialDtor), "");
 static_assert(!__is_trivially_relocatable(NontrivialDtor[]), "");
+static_assert(!__is_trivially_relocatable(const NontrivialDtor), "");
+static_assert(!__is_trivially_relocatable(volatile NontrivialDtor), "");
 
 struct NontrivialCopyCtor {
   NontrivialCopyCtor(const NontrivialCopyCtor&) {}
@@ -3131,12 +3137,16 @@ struct [[clang::trivial_abi]] TrivialAbiNontrivialDtor {
 };
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialDtor), "");
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialDtor[]), "");
+static_assert(__is_trivially_relocatable(const TrivialAbiNontrivialDtor), "");
+static_assert(__is_trivially_relocatable(volatile TrivialAbiNontrivialDtor), "");
 
 struct [[clang::trivial_abi]] TrivialAbiNontrivialCopyCtor {
   TrivialAbiNontrivialCopyCtor(const TrivialAbiNontrivialCopyCtor&) {}
 };
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialCopyCtor), "");
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialCopyCtor[]), "");
+static_assert(__is_trivially_relocatable(const TrivialAbiNontrivialCopyCtor), "");
+static_assert(__is_trivially_relocatable(volatile TrivialAbiNontrivialCopyCtor), "");
 
 // A more complete set of tests for the behavior of trivial_abi can be found in
 // clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -3145,6 +3155,8 @@ struct [[clang::trivial_abi]] TrivialAbiNontrivialMoveCtor {
 };
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialMoveCtor), "");
 static_assert(__is_trivially_relocatable(TrivialAbiNontrivialMoveCtor[]), "");
+static_assert(__is_trivially_relocatable(const TrivialAbiNontrivialMoveCtor), "");
+static_assert(__is_trivially_relocatable(volatile TrivialAbiNontrivialMoveCtor), "");
 
 } // namespace is_trivially_relocatable

clang/lib/AST/Type.cpp

cor3ntin · 2024-01-05T16:13:30Z

We probably need a release note for that, don't we?
LGTM otherwise

AMP999 · 2024-01-05T17:16:41Z

We probably need a release note for that, don't we? LGTM otherwise

Sure, I'll add a release note to Clang.

cor3ntin

LGTM

shafik · 2024-01-05T23:23:37Z

I think I would like some more eyes on this, I don't know if it is obvious to me what it means to reallocate a volatile object.

AMP999 · 2024-01-06T12:53:51Z

I think I would like some more eyes on this, I don't know if it is obvious to me what it means to reallocate a volatile object.

"Both active wg21.link/p1144 and wg21.link/p2786 proposals agree that if a type is trivially copyable then it is trivially relocatable; and that the trait ignores top-level cv-qualifiers.

cor3ntin · 2024-01-10T08:01:31Z

@erichkeane @AaronBallman

rjmccall · 2024-01-12T18:41:35Z

clang/lib/AST/Type.cpp

@@ -2669,6 +2669,8 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
    return false;
  } else if (const auto *RD = BaseElementType->getAsRecordDecl()) {
    return RD->canPassInRegisters();
+  } else if (BaseElementType.isTriviallyCopyableType(Context)) {
+    return true;
  } else {
    switch (isNonTrivialToPrimitiveDestructiveMove()) {


I'm not sure why these are different functions, other than maybe having different preconditions.

You mean why is isNonTrivialToPrimitiveDestructiveMove different from isTriviallyRelocatable? I don't know, but I think it has to do with Objective-C, so I left it alone.

cor3ntin · 2024-01-22T07:47:20Z

@AaronBallman @erichkeane ping :)

erichkeane

I don't have a good idea here... I don't really know what it means to 'relocate' a volatile.

AMP999 · 2024-01-23T05:11:34Z

I don't have a good idea here... I don't really know what it means to 'relocate' a volatile.

This PR doesn't take a position on what it means to relocate a volatile; it just makes the type-trait return the correct value on cv-qualified types, for consistency with __is_trivially_copyable and to implement the current Standard proposals.

AMP999 · 2024-02-11T18:47:16Z

@AaronBallman, does this look okay to you? See https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p1144r9.html#abstract-opdef-trivially-relocatable and https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2786r3.pdf#subsection.12.4 which both agree on this point.

AaronBallman

I think the changes make sense in as much as relocatable is supposed to be a subset of copyable (I continue to think volatile-qualified objects should not be trivially copyable... because they aren't... but that's WG21's issue, not ours).

I'm adding in the libc++ reviewers because this trait is used by libc++ and there's a potential for this to be an ABI-breaking change. I hope we don't need to add any ABI tags for this, but I wanted to give them a chance to weigh in.

philnik777 · 2024-02-12T16:33:12Z

This isn't ABI breaking for us currently (at least in a non-benign way). We only use it to optimize vector growing currently, and just define it to is_trivially_copyable if __is_trivially_relocatable isn't available.

AaronBallman · 2024-02-12T16:41:11Z

This isn't ABI breaking for us currently (at least in a non-benign way). We only use it to optimize vector growing currently, and just define it to is_trivially_copyable if __is_trivially_relocatable isn't available.

Excellent, thank you for the confirmation!

AaronBallman

LGTM

Consistent with `__is_trivially_copyable(volatile int) == true` and `__is_trivially_relocatable(volatile Trivial) == true`, `__is_trivially_relocatable(volatile int)` should also be `true`. Fixes llvm#77091 [clang] [test] New tests for __is_trivially_relocatable(cv-qualified type)

AMP999 · 2024-02-15T05:12:01Z

Thanks all for the review! @AaronBallman, Could you please land this patch for me?

mordante · 2024-03-09T12:32:07Z

@AMP999 I wanted to merge this for you but there are merge conflicts. Can you resolve these?

AMP999 · 2024-03-09T13:39:50Z

@mordante Thank you! I've resolved merge conflicts. I've also added an NFC commit to remove empty messages from static_asserts for consistency with the rest of the file.

cor3ntin · 2024-03-10T08:48:14Z

clang/docs/ReleaseNotes.rst

+  Fixes (`#64356 <https://github.com/llvm/llvm-project/issues/64356>`_).
+- ``__is_trivially_relocatable`` no longer returns ``true`` for non-object types
+  such as references and functions, and no longer returns ``false`` for volatile-qualified types.
+  Fixes (`#67498 <https://github.com/llvm/llvm-project/issues/67498>`_) and
+  (`#77091 <https://github.com/llvm/llvm-project/issues/77091>`_)
+


That should be moved after the next line (and you can use the same #GHXXXX syntax )

That's right! I've fixed it.

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

cor3ntin reviewed Jan 5, 2024

View reviewed changes

clang/lib/AST/Type.cpp Outdated Show resolved Hide resolved

AMP999 force-pushed the pr5-__is_trivially_relocatable-for-volatile-int branch from 0562fd1 to 7f59014 Compare January 5, 2024 16:03

AMP999 force-pushed the pr5-__is_trivially_relocatable-for-volatile-int branch 3 times, most recently from e9339f0 to fd01df8 Compare January 5, 2024 21:06

cor3ntin approved these changes Jan 5, 2024

View reviewed changes

shafik requested review from rjmccall, AaronBallman and erichkeane January 5, 2024 23:22

AMP999 force-pushed the pr5-__is_trivially_relocatable-for-volatile-int branch from fd01df8 to 5d8204e Compare January 10, 2024 11:54

rjmccall reviewed Jan 12, 2024

View reviewed changes

erichkeane reviewed Jan 22, 2024

View reviewed changes

AaronBallman requested review from ldionne and a team and removed request for ldionne February 12, 2024 16:26

AaronBallman reviewed Feb 12, 2024

View reviewed changes

AaronBallman approved these changes Feb 12, 2024

View reviewed changes

AMP999 force-pushed the pr5-__is_trivially_relocatable-for-volatile-int branch from 5d8204e to ed94371 Compare February 12, 2024 21:45

AMP999 added 2 commits March 9, 2024 16:51

Merge branch 'main' into pr5-__is_trivially_relocatable-for-volatile-int

0c60ad2

[NFC] Eliminate empty messages in static_asserts

67e69ad

Merge branch 'main' into pr5-__is_trivially_relocatable-for-volatile-int

8d6601d

cor3ntin reviewed Mar 10, 2024

View reviewed changes

AMP999 added 2 commits March 10, 2024 23:40

Fix the code style in ReleaseNotes.rst

d8f6fd4

Revise the release note's text

23d517a

cor3ntin approved these changes Mar 11, 2024

View reviewed changes

cor3ntin merged commit 7dfa839 into llvm:main Mar 11, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Fix behavior of `__is_trivially_relocatable(volatile int)` #77092

[clang] Fix behavior of `__is_trivially_relocatable(volatile int)` #77092

AMP999 commented Jan 5, 2024

llvmbot commented Jan 5, 2024

cor3ntin commented Jan 5, 2024

AMP999 commented Jan 5, 2024

cor3ntin left a comment

shafik commented Jan 5, 2024

AMP999 commented Jan 6, 2024

cor3ntin commented Jan 10, 2024

rjmccall Jan 12, 2024

AMP999 Jan 19, 2024

cor3ntin commented Jan 22, 2024

erichkeane left a comment

AMP999 commented Jan 23, 2024

AMP999 commented Feb 11, 2024

AaronBallman left a comment

philnik777 commented Feb 12, 2024

AaronBallman commented Feb 12, 2024

AaronBallman left a comment

AMP999 commented Feb 15, 2024

mordante commented Mar 9, 2024

AMP999 commented Mar 9, 2024

cor3ntin Mar 10, 2024 •

edited

Loading

AMP999 Mar 10, 2024

[clang] Fix behavior of __is_trivially_relocatable(volatile int) #77092

[clang] Fix behavior of __is_trivially_relocatable(volatile int) #77092

Conversation

AMP999 commented Jan 5, 2024

llvmbot commented Jan 5, 2024

cor3ntin commented Jan 5, 2024

AMP999 commented Jan 5, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

shafik commented Jan 5, 2024

AMP999 commented Jan 6, 2024

cor3ntin commented Jan 10, 2024

rjmccall Jan 12, 2024

Choose a reason for hiding this comment

AMP999 Jan 19, 2024

Choose a reason for hiding this comment

cor3ntin commented Jan 22, 2024

erichkeane left a comment

Choose a reason for hiding this comment

AMP999 commented Jan 23, 2024

AMP999 commented Feb 11, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

philnik777 commented Feb 12, 2024

AaronBallman commented Feb 12, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

AMP999 commented Feb 15, 2024

mordante commented Mar 9, 2024

AMP999 commented Mar 9, 2024

cor3ntin Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

AMP999 Mar 10, 2024

Choose a reason for hiding this comment

[clang] Fix behavior of `__is_trivially_relocatable(volatile int)` #77092

[clang] Fix behavior of `__is_trivially_relocatable(volatile int)` #77092

cor3ntin Mar 10, 2024 •

edited

Loading