absl::is_trivially_relocatable now respects assignment operators

[Quuxplusone]

Trivial relocatability also requires that the type not do anything weird with its assignment operator; update the type-trait to reflect this. (This is the definition used by BSL, Folly, HPX, Thrust, Parlay, Amadeus, and P1144.)

This is important if we want to use absl::is_trivially_relocatable as a gate for memcpy optimizations in inlined_vector::erase and/or inlined_vector::swap, because in those cases relocation is used to replace part of a sequence involving assignment; the optimization requires an assignment operator that behaves value-semantically.

Clang's builtin currently fails to check the assignment operator, so we stop using it entirely for now. We already refused to use it on Win32, Win64, and Apple, for various unrelated reasons. I'm working on giving Clang's builtin the behavior that would let us re-enable it here.

Assume that any compiler providing both __cpp_impl_trivially_relocatable and a builtin __is_trivially_relocatable(T) will use the appropriate (P1144) definition for its builtin. Right now there's only one such compiler (the P1144 reference implementation, which forks Clang), so this is largely a moot point, but I'm being optimistic.

[derekmauro]

Thanks. I'm going to try to import this first, then I'll work on the others.

[Quuxplusone]

@derekmauro (or @jacobsa if he's around), just a question: When you turned off the builtin for Windows Clang, why didn't you file a bug against upstream Clang? Would one of you be willing to file an upstream bug at this point?

6b4af24 <-- the Abseil commit that identifies the upstream bug
https://godbolt.org/z/qeKneaon9 <-- the test case, still reproduces, sure looks like a bug to me

(I think what Clang's doing is noticing that the Windows calling convention, unlike the Linux one, allows S to be passed in a register, because parameters on Windows, unlike Linux, are always callee-destroy regardless of triviality. So then Sema sees that S is passable-in-a-register and blithely assumes that it must have been marked [[trivial_abi]]. But it doesn't actually have that attribute, so we can't assume it's meant to be trivially relocatable!)

[jacobsa]

Sorry, I should have said in the public commit that I did report this Google-internally (as bug 275003464). I'm not an expert on Windows toolchains, and at the time I thought it might be only a Google-internal toolchain. @rnk had some thoughts in that thread, and might be able to say more publicly.

[derekmauro]

Update: This is not trivial to import. I've found a small number of tests that that rely on absl::is_trivially_relocatable working with [[clang::trivial_abi]]. Most are easy to workaround, but there is one I'm not sure what do to with. Please be patient while I try to work it out.

[rnk]

It looks like I did write this up and file an issue for it: llvm/llvm-project#69394

[Quuxplusone]

Hi @rnk! I've split out the relevant part of my Clang fork into this commit: Quuxplusone/llvm-project@47859e7
My intent is that this would make Clang's __is_trivially_relocatable(T) completely suitable for Abseil to re-enable, not only on Linux but also on Windows and Apple. Could I interest you (or @derekmauro or @ssbr) in pushing it forward on the Clang side?

[ssbr]

My problem is that I'm really bad at pushing things upstream (a symptom of mostly working internally). For instance, like that time I dragged my feet on pushing your std::vector change to optimize for trivially relocatable types until someone else ended up reimplementing it from scratch.

Realistically, would prefer if someone better at contributing to open source projects did it.

[AMP999]

I'm interested in helping to land that Clang patch! What is the first step?

[Quuxplusone]

I'm interested in helping to land that Clang patch! What is the first step?

@AMP999 My current patch is linked from my comment above. I'd say the first step is to turn it into a PR to https://github.com/llvm/llvm-project/pulls , and then ping llvm/llvm-project#69394 (for the benefit of any Abseilers who might be watching that issue). Then we'd hope that the folks here and at Folly (if not other places too) would take the opportunity to review the patch and weigh in as to whether it would fully suit their libraries' needs. As long as you're willing and able to move it forward and can deal with review comments, that sounds perfect to me.

[AMP999]

@Quuxplusone Okay, I've filed a PR for the builtin's behaviour! With some changes in the code, though.
llvm/llvm-project#84621
@derekmauro Could you take a look at that patch and see if it is indeed compatible with Abseil's usage of the term 'trivially relocatable'?

[Quuxplusone]

@AMP999: Awesome! That PR looks great to me. Hopefully someone from Abseil will also take a look and leave a comment, soon. Abseil certainly ought to have a vested interest in getting the Clang builtin up to speed, because it would help a lot of Abseil-using code. (At least when compiled with Clang 19+.) :)

If the patch is accepted, then we (well, @derekmauro @rnk @jacobsa) should definitely consider re-enabling Abseil's use of the builtin on Clang 19-and-greater.

[Quuxplusone]

Update: Abseil commit df2c771 had already disabled __is_trivially_relocatable for AppleClang 15+. @dangelog (at Qt) has just reported that AppleClang bug as llvm/llvm-project#86354. So we now have two upstream bugs (one for the false positives on Windows, and one for the false positives on Apple). IIUC, @AMP999's llvm/llvm-project#84621 will fix both of them — and also brings the builtin into line with P1144, notably so that it will continue to be safe for Abseil to replace operator= with memcpy in inlined_vector::erase and inlined_vector::swap even after switching back to the builtin if that patch is accepted for Clang 19.

@derekmauro could you please take a look at llvm/llvm-project#84621 and leave a comment from Abseil, or at least give your 👍 to encourage Clang 19 to adopt the patch? Thanks very much!