search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.25k stars 12.07k forks source link

Bug in C++ standard library header <bit> #55491

Open Delta-dev-99 opened 2 years ago

Delta-dev-99 commented 2 years ago

https://github.com/llvm/llvm-project/blob/b3077f563d9f15aaf1868e54c91a82e2aab666a3/libcxx/include/bit#L161 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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)