Open frederick-vs-ja opened 1 month ago
Thanks - this is a regression presumably going back to #804 in VS 2019 16.8.
I think this should be fixed by implementing the paper.
I have some special algorithms implemented directly in application code, that also have this assumption, and making the traits behaving expected would fix those too.
Until the paper can't be implemented, can have workaround with transition comment though.
I think I can implement the workaround.
Workaround for Leopard
in WG21-P3279R0 seems implementable to me. It should be sufficient to detect whether a T::operator=
has the one of the expected types. Although such detection will be extreme boilerplate.
Author of P3279R0 here. (I admit R0 is a mess; I'm planning an R1 that will be a different kind of mess. Sorry.)
@AlexGuteniev says "I think this should be fixed by implementing [P3279]." P3279 proposes that C++ should redefine the meaning of is_trivially_copyable
such that is_trivially_copyable<pair<int&, int&>>
becomes 100% guaranteed false
(because pair<int&, int&>
does assign-through and therefore after P3279 its assignment would be non-trivial by definition).
Therefore, the appropriate way to "implement P3279" today, if you want to take @AlexGuteniev's advice (and mine), is to anticipate the paper's changes and make Microsoft's is_trivially_copyable<pair<int&, int&>>
yield false
today. The easiest way to do that is to give it an assignment operator that is non-trivial according to today's C++. Basically, "don't lie to the compiler" — don't write pair
in such a way that the compiler thinks it's trivially copyable when in fact you want it not to be trivially copied.
On my blog I have a whole series of "#sd-8-adjacent" guidelines; and though I haven't written it up yet, certainly one of those guidelines should be "Don't go out of your way to make a type falsely advertise trivial copyability when you don't want people to trivially copy it." I think you can (and should) assume that users will follow that guideline instinctively. Your single solitary problem here is that your own std::pair
fails to follow the guideline: it falsely advertises trivial copyability when in fact you don't want it to be trivially copied. So just stop advertising it as such, and you're golden!
The bug report here concerns the false-advertising of std::pair
, not really anything wrong with reverse_copy
. I really strongly encourage you not to "fix" your reverse_copy
. Because even if you "fix" your own reverse_copy
, you can't fix all the library algorithms that are already out there in the wild that will fail when presented with your falsely-advertising std::pair
— but you can fix your std::pair
so that it will work correctly with all those existing algorithms (and also with your own reverse_copy
).
See further pre-filing discussion on the cpplang Slack.
Therefore, the appropriate way to "implement P3279" today, if you want to take @AlexGuteniev's advice (and mine), is to anticipate the paper's changes and make Microsoft's
is_trivially_copyable<pair<int&, int&>>
yieldfalse
today.
Hmm, the crux of MSVC STL's pair
(and tuple
) is that it's copy- and move-assigned by assignment operator templates that are not copy or move assignment operators. Perhaps it would be better to switch to use real copy and move assignment operators, but I'm not sure whether we can do this before vNext due to ABI issues, see #3013.
I really strongly encourage you not to "fix" your
reverse_copy
. Because even if you "fix" your ownreverse_copy
, you can't fix all the library algorithms that are already out there in the wild that will fail when presented with your falsely-advertisingstd::pair
I think this is possible. I'm preparing a patch making vectorization correctly applied for regular trivially copyable classes (which are trivially copy & move constructible & assignable with expected semantics), while ruling out your Leopard
in WG21-P3279R0. The trick is extremely verbose and forbidden for user code because it detects conversion from &T::operator=
to certain types. But I think it's fine for the library implementation itself and it can work.
BTW, now I guess it would be better to add new traits of names is_bitwise_constructible
, is_bitwise_convertible
, and is_bitwise_assignable
, and deprecate corresponding is_trivially_meowable
.
There're still some unresovlable problems though.
Given struct VolatileMem { volatile int x; };
, it seems that vectorization (including memcpy
/memmove
) shouldn't be applied for a range of it. But it's generally impossible to detect the volatile member and I'm not seeing any paper addressing this (except for those for reflection).
Edit: there's already one such paper, see below.
There're still some unresovle problems though.
Given
struct VolatileMem { volatile int x; };
, it seems that vectorization (includingmemcpy
/memmove
) shouldn't be applied for a range of it. But it's generally impossible to detect the volatile member and I'm not seeing any paper addressing this (except for those for reflection).
I covered that case in "When is a trivially copyable type not trivially copyable?" (2018-07-13), and in the paper P1153R0 Copying volatile subobjects is not trivial (2018); but ended up dropping P1153. IIRC, I dropped it simply because I thought it wasn't worth spending time on: The library never goes out of its way to care about volatile
, so there didn't seem to be a good reason for is_trivially_fooable
specifically to go out of its way to care, either. But I could see an argument for picking it back up.
I really strongly encourage you not to "fix" your reverse_copy. Because even if you "fix" your own reverse_copy, you can't fix all the library algorithms that are already out there in the wild that will fail when presented with your falsely-advertising std::pair
I think this is possible. I'm preparing a patch [...] ruling out your
Leopard
. The trick is extremely verbose and forbidden for user code [...] But I think it's fine for the library implementation itself and it can work.
Please don't use any "trick" in Microsoft's reverse_copy
that can't also be applied to
etc. etc. Microsoft's pair
is currently giving bad behavior on all third-party libraries, which is why fixing pair
should be higher-priority than "fixing" reverse_copy
alone. It would be more intellectually honest, IMO, to leave the 32-bit reverse_copy
"broken" (and leave this bug report open) until such time as you can fix pair
itself.
I disagree. With the Core Language as it stands today, our pair
type is doing nothing wrong (it is something that the Core Language allows to exist, and user types could behave like it too), and our type traits are doing nothing wrong. It's the algorithm's use of is_trivially_copyable
when we're about to (bypass) an assignment, where is_trivially_copy_assignable
would be the proper question to ask.
We should patch this optimization, and audit the STL for all other uses of is_trivially_copyable
that would be vulnerable to similar problems.
If other third-party libraries made the same mistake that we did, they should also patch themselves.
We'll revisit this in July 2024.
I disappointedly found that while it's possible to rule out Leopard
, it doesn't seem possible to rule out a class wrapping it, e.g. struct LeopardHouse { Leopard _; };
. MSVC and Clang behave differently - MSVC doesn't think LeopardHouse
is trivially copy assignable while Clang does, and currently Clang seems right. However, P3279 seemingly makes MSVC's behavior correct.
I guess at least we need the __is_bitwise_assignable
intrinsic if we can't change __is_trivially_assignable
directly.
Describe the bug
From DevCom-10662768.
MSVC STL's
pair<int&, int&>
is trivially copyable but not trivially copy assignable. Currently, vectorization ofreverse_copy
incorrectly detects the element type withis_trivially_copyable
, which is problematic withpair<int&, int&>
and its friends.https://github.com/microsoft/STL/blob/63354c3fa9c1fb2ab1fccb58c47d23c6af1c290f/stl/inc/algorithm#L5095-L5115 https://github.com/microsoft/STL/blob/63354c3fa9c1fb2ab1fccb58c47d23c6af1c290f/stl/inc/algorithm#L5175-L5192
Command-line test case
Expected behavior
The output should be
A 64-bit target program outputs the correct results.
STL version
Additional context
See also WG21-P3279R0.