[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 ==

[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

[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

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

[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 ::

[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

[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 ACTION

[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

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

devin.jeanpierre updated this revision to Diff 401419. devin.jeanpierre added a comment. pull/rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 Files: clang/docs/LanguageExtensions.rst

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

[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".

devin.jeanpierre updated this revision to Diff 401379. devin.jeanpierre added a comment. Fix copy-paste error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 Files: clang/docs/LanguageExtensions.rst

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

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

devin.jeanpierre added a comment. CI test finished successfully before windows setup did . My workplace's Windows VMs are a bit hosed at the moment... In D114732#3253416 , @Quuxplusone wrote: > In D114732#3253142

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

devin.jeanpierre updated this revision to Diff 401358. devin.jeanpierre added a comment. Clarify Windows comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 Files:

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

Quuxplusone added a comment. In D114732#3253142 , @devin.jeanpierre wrote: > OK, while I'm struggling to set up a new Windows machine so I can make sure > this works on Windows... @Quuxplusone, after this is merged, do you want to > rebase D67524

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

devin.jeanpierre added a comment. OK, while I'm struggling to set up a new Windows machine so I can make sure this works on Windows... @Quuxplusone, after this is merged, do you want to rebase D67524 on top of this, or should I? I can review it -- I think

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

devin.jeanpierre updated this revision to Diff 401035. devin.jeanpierre added a comment. Update to pass on Windows (untested right now). On Windows, unlike Itanium ABI, the only special member functions used are the copy constructor and the destructor. If the copy constructor is deleted, then

[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".

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215 +The compiler will pass and return a trivially relocatable type using the C ABI +for the underlying type, even when the type would otherwise be considered +non-trivially-relocatable.

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

devin.jeanpierre added a comment. In D114732#3178312 , @rjmccall wrote: > 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

[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

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

devin.jeanpierre added a comment. (Sorry, I think I'm doing threading wrong here due to lack of experience with phabricator. The reply buttons are grayed out!) > 1. All trivial-abi types are in fact trivially relocated, specifically when > they are passed to functions. > 2. Therefore, all

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

devin.jeanpierre updated this revision to Diff 392606. devin.jeanpierre added a comment. Use `PCK_ARCStrong` to check for ObjC strong pointers, marking them as trivially relocatable as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[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 LAST

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

rsmith added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215 +The compiler will pass and return a trivially relocatable type using the C ABI +for the underlying type, even when the type would otherwise be considered +non-trivially-relocatable. If a

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

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215 +The compiler will pass and return a trivially relocatable type using the C ABI +for the underlying type, even when the type would otherwise be considered +non-trivially-relocatable.

[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

[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".

devin.jeanpierre added a comment. (Sorry for delayed reply -- I made the mistake of signing up to phabricator with my personal email, which I don't check very well, apparently!) Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215 +The compiler will pass and return a

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

devin.jeanpierre updated this revision to Diff 392214. devin.jeanpierre added a comment. Suggested changes from code review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/ https://reviews.llvm.org/D114732 Files:

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

rsmith added a comment. In D114732#3159247 , @Quuxplusone wrote: > - D50119 has more extensive tests, and has > been live on https://p1144.godbolt.org/z/axx9Wj3r5 for a couple years now; if > the consensus is that

[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 , @Quuxplusone wrote: > - (Major point of contention) To me, `[[trivial_abi]]` does not imply > `[[trivially_relocatable]]` at all. I

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

Quuxplusone added a comment. On the one hand, I like that someone else is pushing a patch that's basically the same as D50119 , because maybe if enough people reimplement the same thing, eventually Clang will accept one of them. ;) However, I'd like to point

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

devinj.jeanpierre added a comment. Just a heads up, I think this is my first change to clang or llvm, and I'd appreciate any feedback you have on the code, review process, etc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114732/new/

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

devinj.jeanpierre created this revision. devinj.jeanpierre added reviewers: rsmith, aaron.ballman, rjmccall, ahatanak. Herald added a subscriber: dexonsmith. devinj.jeanpierre requested review of this revision. Herald added a project: clang. This change enables library code to skip paired