Bug in C++ standard library header <bit>

[Delta-dev-99]

llvm-project/libcxx/include/bit

Line 161 in b3077f5

__t = __rotr(__t, __ulldigits);

The rotation direction there seems wrong
Left rotation makes sense, because it moves the most-significant-bits block (of same size as unsigned long long) to the least significant positions (it wraps around), where you can apply the usual function to that fragment of the value, by interpreting it as unsigned long long.

I would have fixed the issue myself (I would love to be included in the list of contributors) but I found the process to be too complicated.
The change is merely one character. Why not accept pull requests for this kind of changes?
So, I decided to at least open an issue so you can fix it.

[philnik777]

I would have fixed the issue myself (I would love to be included in the list of contributors) but I found the process to be too complicated.

What's so complicated? You have to register at https://reviews.llvm.org and create a PR with arc.

The change is merely one character.

This change definitely isn't a single character. Other than a typo in a comment or error message almost no change is. The tests have to be updated, or more likely added, to the test suite for this change.

Why not accept pull requests for this kind of changes?

(I assume you mean on GitHub) I don't know the official reason, but I would guess that it is just a lot more work to support multiple ways to contribute. I personally find the workflow with phabricator a lot nicer too.

[Delta-dev-99]

What's so complicated? You have to register at https://reviews.llvm.org and create a PR with arc.

Yeah, I cannot even load that page. Thanks for the reply tho'.

[shafik]

Perhaps I am reading this wrong but it looks like this would apply in the case of __uint128_t and I did a quick check using godbolt and it looks like it does the right thing.

[Delta-dev-99]

In the case of __uint128_t the rotation direction does not matter, because rotating 64 bits either way is equivalent to swapping the two 64-bit halves. I think that's the reason the bug is still there: it only affects very rare situations (more than 128 bits or not power-of-2 bit size between 64 and 128 (never seen that)).
A valid test thus requires using a larger number (256 bits). I think a class emulating it should do, just need to implement shift operations and of course, conversions to usual integers.

EDIT: Assumed usual integer size: unsigned long long (64-bit)

[frederick-vs-ja]

This is fixed by d78ca73 (https://reviews.llvm.org/D134625).

Although there doesn't seem any behavioral change for supported types. Maybe we can test something when unsigned _BitInt(256) becomes supported by these functions.