[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

rnk wrote: Sorry for the delay, work life does its best to intervene. https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

tru wrote: ping on this. https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/rnk requested changes to this pull request. https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/rnk edited https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -1105,6 +1105,11 @@ bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const { static bool isTrivialForMSVC(const CXXRecordDecl *RD, QualType Ty, CodeGenModule &CGM) { + // If the record is marked with the trivial_abi attribute

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/efriedma-quic approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

tru wrote: @efriedma-quic are you ok with this change? https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/rnk approved this pull request. Thanks! https://github.com/llvm/llvm-project/pull/88857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -0,0 +1,21 @@ +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s + +// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { ptr } +struct __attribute__((trivial_abi)) Trivial { + int *p; + Trivial() : p(0

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/tru updated https://github.com/llvm/llvm-project/pull/88857 >From 08214d87d1a7c83ea25eef3bf18de1568a20a152 Mon Sep 17 00:00:00 2001 From: Tobias Hieta Date: Tue, 16 Apr 2024 09:38:53 +0200 Subject: [PATCH] [clang] Handle trivial_abi attribute for Microsoft ABI. Previou

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -0,0 +1,21 @@ +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s + +// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { ptr } +struct __attribute__((trivial_abi)) Trivial { + int *p; + Trivial() : p(0

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -0,0 +1,21 @@ +// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -std=c++11 -fcxx-exceptions -fexceptions -emit-llvm -o - %s | FileCheck %s + +// CHECK: %[[STRUCT_TRIVIAL:.*]] = type { ptr } +struct __attribute__((trivial_abi)) Trivial { + int *p; + Trivial() : p(0

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

@@ -63,6 +63,10 @@ ABI Changes in This Version MSVC uses a different mangling for these objects, compatibility is not affected. (#GH85423). +- The attribute ``trivial_abi`` now works when targetting the Microsoft ABI. Marking rnk wrote: It's o

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

llvmbot wrote: @llvm/pr-subscribers-clang-codegen Author: Tobias Hieta (tru) Changes Previously the trivial_abi was ignored for records when targetting the microsoft abi, the MSVC rules where always enforced to ensure compatibility with MSVC. This commit changes it to be closer to the

[clang] [clang] Handle trivial_abi attribute for Microsoft ABI. (PR #88857)

https://github.com/tru created https://github.com/llvm/llvm-project/pull/88857 Previously the trivial_abi was ignored for records when targetting the microsoft abi, the MSVC rules where always enforced to ensure compatibility with MSVC. This commit changes it to be closer to the itanium abi

[PATCH] D158532: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza added a comment. @gribozavr2 - ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158532/new/ https://reviews.llvm.org/D158532 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lis

[PATCH] D158532: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza added a comment. The delta from https://reviews.llvm.org/D155895 is in `clang/test/SemaCXX/attr-trivial-abi.cpp` where 1) a comment has been added above the non-trivial move constructor of the `Trivial` test helper and 2) test expectations have been tweaked to account for `_WIN64` and `

[PATCH] D158532: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

/D155895. Consider the test input below: struct [[clang::trivial_abi]] Trivial { Trivial() {} Trivial(Trivial&& other) {} Trivial& operator=(Trivial&& other) { return *this; } ~Trivial() {} }; static_assert(__is_trivially_relocatable(Trivial), &quo

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

thakis added a comment. Reverted in 0342bbf223fa12701a0570a23f9eac433b8b341c for now, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.llvm.org/D155895 ___ cfe-commits ma

[clang] 0342bbf - Revert "Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`."

LOG: Revert "Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`." This reverts commit bddaa35177861545fc329123e55b6a3b34549507. Reverting as requested at https://reviews.llvm.org/D155895#4566945 (for breaking tests on Windows). Added: Modified: clang/inc

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

usUnionsAndStructs::TrivialAbiAttributeAppliedToAnonymousUnion)': error: 'warning' diagnostics seen but not expected: File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 194: 'trivial_abi' cannot be applied to 'BasicStruct'

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

thakis added a comment. Looks like this breaks tests on windows: http://45.33.8.238/win/82239/step_7.txt Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.ll

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbddaa3517786: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`. (authored by gribozavr). Changed prior to commit: https

[clang] bddaa35 - Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

.diff LOG: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`. Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`. Consider the test input below: ``` struct [[clang::trivial_abi]] Trivial { Trivial() {} Trivial(Trivial&& other) {} Trivial&a

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

/SemaCXX/attr-trivial-abi.cpp @@ -1,4 +1,5 @@ // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 +// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribu

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

gribozavr2 accepted this revision. gribozavr2 added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:258 + static_assert(__is_trivially_relocatable(TrivialAbiAttributeAppliedToAnonymousUnion), ""); +#endif +

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

rsmith added a comment. LGTM, though please wait a day or two for any more comments from @gribozavr2 since he's looked at this more closely than I have. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.llvm.org/D155895 _

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

-fsyntax-only -verify %s -std=c++11 +// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}} @@ -169,3 +170,104 @@ sta

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

+1,5 @@ // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 +// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}} @@ -169,

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10339-10343 + if (RT->getDecl()->isAnonymousStructOrUnion()) { +FieldsToCheck.append(RT->getDecl()->field_begin(), + RT->getDecl()->field_end()); +continue; +

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza added a comment. In D155895#4525417 , @gribozavr2 wrote: > @lukasza I think you forgot to upload an updated version of the patch. Ooops, sorry about that. I've been trying to use `arc`, but it seems that it has uploaded separate reviews at htt

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

(__is_trivially_relocatable(S20), ""); #endif } // namespace deletedCopyMoveConstructor + +namespace anonymousUnionsAndStructs { + // Test helper: + struct [[clang::trivial_abi]] Trivial { +Trivial() {} +Trivial(Trivial&& other) {} +Trivial& operator=(Trivial&& other) {

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

gribozavr2 added a comment. @lukasza I think you forgot to upload an updated version of the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155895/new/ https://reviews.llvm.org/D155895 ___ cfe-commi

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza marked 5 inline comments as done. lukasza added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10325 - for (const auto *FD : RD.fields()) { -// Ill-formed if the field is an ObjectiveC pointer or of a type that is -// non-trivial for the purpose of

[PATCH] D156003: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza created this revision. Herald added a project: All. lukasza requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Consider the test input below: struct [[clang::trivial_abi]] Trivial { Trivial() {} Trivial(Trivial&&am

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

attr-trivial-abi.cpp:201-202 + + // `S2` is like `S1`, but `field` is wrapped in an anonymous union (and it + // seems that trivial relocatability of `S1` and `S2` should be the same). + // Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:232 + // `S2` an

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

lukasza added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10299 if (!HasNonDeletedCopyOrMoveConstructor()) { PrintDiagAndRemoveAttr(0); I tried to change this condition to `!RD.isAnonymousStructOrUnion() && !HasNonDeletedCopyOrMoveConst

[PATCH] D123059: re-roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".""

This revision was automatically updated to reflect the committed changes. Closed by commit rGf2b31f06b79a: re-roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable""."" (authored by devin.jeanpierre, committed by gribozavr). Repository: rG

[clang] f2b31f0 - re-roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".""

.diff LOG: re-roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable""."" This reverts commit b0bc93da926a943cdc2d8b04f8dcbe23a774520c. Changes: `s/_WIN32/_WIN64/g` in clang/test/SemaCXX/attr-trivial-abi.cpp. The calling convention is specific to 64

[PATCH] D123059: re-roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".""

rivially_relocatable(NontrivialCopyCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor[]), ""); + +struct NontrivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor), ""); +static_

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

devin.jeanpierre added a comment. In D119017#3400347 , @devin.jeanpierre wrote: > I don't have easy access to a Windows machine at work so am struggling with > the reproduction instructions. Well, after much procrastinating, and an hour of fiddling wit

[clang] b0bc93d - Revert "[clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable""."

/b0bc93da926a943cdc2d8b04f8dcbe23a774520c.diff LOG: Revert "[clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable""." This reverts commit 56d46b36fc231a0beb518602503035bba92043e0. The LIT test SemaCXX/attr-trivial-abi.cpp is failing with 32bit build on Windows. All the

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

aaron.ballman added a comment. In D119017#3400347 , @devin.jeanpierre wrote: > Please revert it, I don't have easy access to a Windows machine at work so am > struggling with the reproduction instructions. Thanks for letting us know! @Manna -- I'll let

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

devin.jeanpierre added a comment. Please revert it, I don't have easy access to a Windows machine at work so am struggling with the reproduction instructions. This has failed twice, and this time with a one month delay! Is there any way to run and confirm all tests pass before I try to merge ag

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

aaron.ballman added a comment. Hello, @devin.jeanpierre! I'm wondering if we should revert this while you investigate the fix, or do you think you'll have this solved sometime today? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119017/new

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

Manna added a comment. In D119017#3392996 , @devin.jeanpierre wrote: > In D119017#3392993 , @Manna wrote: > lf so, what target triple, so that I can reproduce? (New to clang development...) >> >> You c

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

devin.jeanpierre added a comment. In D119017#3392993 , @Manna wrote: >>> lf so, what target triple, so that I can reproduce? (New to clang >>> development...) > > You can reproduce the test failure if you checkout 32 bit clang compiler. Please bear with

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

Manna added a comment. In D119017#3392984 , @devin.jeanpierre wrote: > In D119017#3392961 , @Manna wrote: > >> LIT test (SemaCXX/attr-trivial-abi.cpp) is failing for x86 build of clang. >> The same failures are

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

devin.jeanpierre added a comment. In D119017#3392961 , @Manna wrote: > LIT test (SemaCXX/attr-trivial-abi.cpp) is failing for x86 build of clang. > The same failures are happening with our downstream X86 clang build. > [...] > The test might need to be u

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

Manna added a comment. Herald added a project: All. LIT test (SemaCXX/attr-trivial-abi.cpp) is failing for x86 build of clang. The same failures are happening with our downstream X86 clang build. - TEST 'Clang :: SemaCXX/attr-trivial-abi.cpp' FAILED Script: --- : 'RUN:

[clang] 56d46b3 - [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

.diff LOG: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"". This reverts commit 852afed5e0200b70047c28ccf4424a17089d17b0. Changes since D114732: On PS4, we reverse the expectation that classes whose constructor is deleted are not trivially

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG56d46b36fc23: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially… (authored by devin.jeanpierre, committed by

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

gribozavr2 accepted this revision. gribozavr2 added a comment. This revision is now accepted and ready to land. Approving the delta for the PS4 fix (relying or @rsmith 's approval for the actual patch -- https://reviews.llvm.org/D114732). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D119017: [clang] roll-forward "[clang] Mark `trivial_abi` types as "trivially relocatable"".

{ + NontrivialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor[]), ""); + +struct [[clang::trivial_abi]] TrivialAbiNontrivialDtor { + ~Trivial

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. The core difference of behavior is this, in the logic for setting `canPassInRegisters`: // Clang <= 4 used the pre-C++11 rule, which ignores move operations. // The PS4 platform ABI follows the behavior of Clang 3.2. if (CCK == TargetInfo::CCK_ClangABI4OrP

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

dyung added a comment. In D114732#3294794 , @devin.jeanpierre wrote: > Oops, sorry about that. What is the correct way to test/reproduce the change? > Do I / can I submit builds to the buildbot manually for testing? > > Also, should I be rolling back th

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. Thank you @gribozavr2 ! I'll hopefully have a roll-forward ready for you either today or tomorrow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 __

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

gribozavr2 added a comment. I rolled it back while @devin.jeanpierre investigates. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 ___ cfe-commits mailing list

[clang] 852afed - Revert "[clang] Mark `trivial_abi` types as "trivially relocatable"."

.diff LOG: Revert "[clang] Mark `trivial_abi` types as "trivially relocatable"." This reverts commit 19aa2db023c0128913da223d4fb02c474541ee22. It breaks a PS4 buildbot. Added: Modified: clang/docs/LanguageExtensions.rst clang/include/clang/AST/Type.h clang

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. Oops, sorry about that. What is the correct way to test/reproduce the change? Do I / can I submit builds to the buildbot manually for testing? Also, should I be rolling back this change, or no? Not sure of the protocol here, this is my first change to Clang. P

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

dyung added a comment. Hi, your change is causing a test failure in the test attr-trivial-abi.cpp that you modified on the PS4 Windows box: https://lab.llvm.org/staging/#/builders/204/builds/758/steps/7/logs/FAIL__Clang__attr-trivial-abi_cpp TEST 'Clang :: SemaCXX/attr-tr

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. W! Thank you for submitting, and thanks everyone for the kind and helpful reviews. 😀 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732

[clang] 19aa2db - [clang] Mark `trivial_abi` types as "trivially relocatable".

.diff LOG: [clang] Mark `trivial_abi` types as "trivially relocatable". This change enables library code to skip paired move-construction and destruction for `trivial_abi` types, as if they were trivially-movable and trivially-destructible. This offers an extension to the performance f

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

This revision was automatically updated to reflect the committed changes. Closed by commit rG19aa2db023c0: [clang] Mark `trivial_abi` types as "trivially relocatable". (authored by devin.jeanpierre, committed by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. @rsmith I've pulled+rebased again to avoid the (looks like pre-existing) failure. Copy-pasting the wording on https://llvm.org/docs/MyFirstTypoFix.html#commit-by-proxy: I don’t have commit access, can you land this patch for me? Please use “Devin Jeanpierre je

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

trivialCopyCtor&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor[]), ""); + +struct NontrivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_t

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rsmith added inline comments. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:9-11 +// On Windows, trivial relocatability depends only on a trivial copy constructor existing. +// In this case, it is implicitly deleted. Similar concerns apply to later tests. +static_assert(!

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. In D114732#3256046 , @devin.jeanpierre wrote: > Sorry, I missed your other comments. Let me know if there's anything else I > didn't address. I just noticed that there's a "Done" checkbox next to each comment, and it

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

lCopyCtor(const NontrivialCopyCtor&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor[]), ""); + +struct NontrivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. Sorry, I missed your other comments. Let me know if there's anything else I didn't address. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:146 +#ifdef _WIN32 +static_assert(!__is_trivially_relocatable(CopyDeleted), ""); +#else ---

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:9-11 +// On Windows, trivial relocatability depends only on a trivial copy constructor existing. +// In this case, it is implicitly deleted. Similar concerns apply to later tests. +static_ass

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

> can assume tail padding is not an issue, so therefore no metaprogramming is > needed and it can just always do the efficient thing (which is therefore what > I've implemented in D67524 <https://reviews.llvm.org/D67524>). But I know > that's unlikely to ever happe

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

lCopyCtor(const NontrivialCopyCtor&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor[]), ""); + +struct NontrivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

which is therefore what I've implemented in D67524 <https://reviews.llvm.org/D67524>). But I know that's unlikely to ever happen. > (D63620 <https://reviews.llvm.org/D63620> could in some form also be ported > over, but it needs to be guarded behind ABI stability, sinc

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

me form also be ported over, but it needs to be guarded behind ABI stability, since `[[clang::trivial_abi]]` is an ABI breaking change. For example, the same way it was done for unique_ptr <https://libcxx.llvm.org//DesignDocs/UniquePtrTrivialAbi.html>, with the same benefits.) Repositor

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

;) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialCopyCtor[]), ""); + +struct NontrivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_trivially_relocatable(Non

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 ___

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

e: > > > > I think this documentation change has mixed together two different > > > > things. The revised wording says that `[[trivial_abi]]` implies > > > > trivially-relocatable and that trivially-relocatable implies passing > > > > usi

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

td::move(lhs)); } It's really hard to argue that a type can sensibly call out to library code while in a state that is non-relocatable, despite self-labeling as `[[clang::trivial_abi]]`. If the library is allowed to move the value, it's allowed to move it into a parameter, causing a

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. Trivial relocation doesn't imply that types have to be safe against being suddenly relocated during the middle of operations while they're not in a safe internal state. That is not a consideration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

Therefore, all trivial-abi types are "trivially relocatable" in general. This sounds mostly good so far. For example: class [[clang::trivial_abi]] MyType { ... }; MyType Create() { MyType x; /* do stuff */ return x; } void Consume(MyType) {} int main() { Cons

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rivialMoveCtor { + NontrivialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor[]), ""); + +struct [[clang::trivial_abi]] TrivialAbiNontrivia

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. I am perfectly happy accepting that syllogism. Passing a value in registers is a trivial relocation. If there is a reason your type shouldn't be trivially relocated, you should not make it `trivial_abi`. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

ntation change has mixed together two different > > > things. The revised wording says that `[[trivial_abi]]` implies > > > trivially-relocatable and that trivially-relocatable implies passing > > > using the C ABI, which is wrong: the second implication doe

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

ether two different things. > > The revised wording says that `[[trivial_abi]]` implies > > trivially-relocatable and that trivially-relocatable implies passing using > > the C ABI, which is wrong: the second implication does not hold. What we > > should say is that `[[tr

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. Don't worry about `GC::Strong`. ObjC GC is thoroughly dead (unless there's GNU-runtime interest in it?), and AFAIK we've never made any effort to incorporate it into our treatment of non-trivial structs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rsmith added inline comments. Comment at: clang/lib/AST/Type.cpp:2500 +return BaseElementType.isTriviallyCopyableType(Context) && + !BaseElementType.isDestructedType(); + } devin.jeanpierre wrote: > rsmith wrote: > > You can simplify this a little

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

stroy the object before returning. rsmith wrote: > I think this documentation change has mixed together two different things. > The revised wording says that `[[trivial_abi]]` implies trivially-relocatable > and that trivially-relocatable implies passing using the C

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

ialMoveCtor(NontrivialMoveCtor&&) {} +}; +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor), ""); +static_assert(!__is_trivially_relocatable(NontrivialMoveCtor[]), ""); + +struct [[clang::trivial_abi]] TrivialAbiNontrivialDtor { + ~TrivialAbiNontrivia

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3217 +are balanced, and that this attribute can safely be applied to +reference-counting smart pointers. + I would change the first sentence here to: > Arguments of ``trivial_

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

was first proposed; `__has_builtin` has not always supported type trait builtins. - This patch has an unnecessary check for trivial destruction of trivially copyable types (commented in this review). - This patch treats trivial-for-calls as implying trivially-relocatable, and in particular this mean

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

Quuxplusone added a comment. LgenerallyGTM! - I would say "special member functions" instead of "methods" - instead of "passed indirectly by address", consider saying "passed by hidden reference" — is this something people will get, or is it a me-ism? :) There are lots of Google hits for "by hi

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

rnk created this revision. rnk added reviewers: aaron.ballman, rjmccall, dblaikie, ahatanak. rnk requested review of this revision. Herald added a project: clang. My team recently answered some questions about the trivial_abi attribute. This change attempts to distill those answers into user

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

devin.jeanpierre added a comment. Wow, thanks for the quick response! I really appreciate it. In D114732#3159247 <https://reviews.llvm.org/D114732#3159247>, @Quuxplusone wrote: > - (Major point of contention) To me, `[[trivial_abi]]` does not imply > `[[trivially_relocatable]

  1   2   >