Add bit shifting built-ins #13001
Add bit shifting built-ins #13001
Conversation
spacekitteh
force-pushed
the
add-bitshift-primops
branch
2 times, most recently
from
54f5fb5 to
6271ec7
Compare
spacekitteh
force-pushed
the
add-bitshift-primops
branch
from
6271ec7 to
4eed1b3
Compare
There was a problem hiding this comment.
Please address the design issue that these functions don't have specified and safe edge case behaviour in their specification.
@RaitoBezarius iirc we didn't want these in lix at all (though they aren't of terribly high maintenance cost), but do you remember more context? Also, should these be namespaced somehow rather than in the top level builtins attrset?
| issues: [13000] | ||
| --- | ||
|
|
||
| Added left and right bit shifting built-ins. |
| auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitShiftLeft"); | ||
| auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitShiftLeft"); | ||
|
|
||
| v.mkInt(i1.value << i2.value); |
There was a problem hiding this comment.
This contains UB if i2 is too large. Do it some other way that is safe. Maybe there's an intrinsic.
There was a problem hiding this comment.
A brief look at the rust source code reveals that you have to check that i2 is in range: https://doc.rust-lang.org/stable/src/core/num/int_macros.rs.html#1222. Probably don't have to do anything else. You should add this to the checked-arithmetic library used by the other arithmetic.
You also have to make sure the shift amount is positive. BUT this reveals that perhaps the correct design is a single bitShift builtin, see haskell: https://hackage.haskell.org/package/base-4.21.0.0/docs/GHC-Bits.html#v:shift
There was a problem hiding this comment.
Personally I think it would be easier and more understandable if we split the two implementations and enforce a positive shift amount. I think most of the time you know beforehand which direction you want to shift and this way we'll catch errors if that assumption is broken.
There was a problem hiding this comment.
The reason I'm suggesting one of them is that it has clear and continuous behaviour over its domain: it's got less partial behaviour.
If there's two, negative shift amounts are always illegal on both.
There was a problem hiding this comment.
Yeah good point. But then we might have to implement a logical right shift not the current arithmetic one. Else it might lead to a scenario where shift (shift x 1) -1 != x
There was a problem hiding this comment.
Right. Do we want to allow shifting left into the sign bit to change the sign of a number? i dunno! but these are the questions that need to be answered in the spec.
Builtins are permanent parts of derivation ABI that cannot be changed since we don't have a good language versioning story.
The lix position is that builtins need:
- fully specified behaviour and a spec
- knowledge of edge cases and how they're handled and tests
- demonstrated need (though that is somewhat shaky here, doing IFD to achieve this probably isn't a great answer either)
- consideration of back and forward compatibility: if a built-in has an attrset argument how do you detect supported features in it? if it could accept more args in the future how is it accommodated?
We need to write these rules down somewhere because there's multiple sunk built-in projects that regrettably wasted a lot of people's time.
There was a problem hiding this comment.
Honestly after talking with some other people about it I'm less convinced this builtin is strictly needed than before. I still think it's nice to have and would simplify things a bit, but I can see that there is no way of implementing it currently that wouldn't leak the internal integer representation, or wouldn't have some weird/unintuitive edge case behavior.
I'm still open to try and implement it with a proper spec, tests and anything else that's needed, but if the powers that be understandably say they rather not have another semi useful, somewhat obscure niche builtin, I needn't spend more time on this.
So I guess the main question is if this would contain proper specified behavior, handling of edge cases and tests, would you or anyone else be open to merge it or would you rather not?
| auto i1 = state.forceInt(*args[0], pos, "while evaluating the first argument passed to builtins.bitShiftRight"); | ||
| auto i2 = state.forceInt(*args[1], pos, "while evaluating the second argument passed to builtins.bitShiftRight"); | ||
|
|
||
| v.mkInt(i1.value >> i2.value); |
There was a problem hiding this comment.
this probably contains UB if i2 is too large
| .name = "__bitShiftLeft", | ||
| .args = {"e1", "e2"}, | ||
| .doc = R"( | ||
| Return the integer *e1* shifted left by *e2*. |
There was a problem hiding this comment.
Insufficiently specific, what happens with negative operands? Are those invalid? What about negative shift values?
I suspect the implementation here does UB with them.
|
Didn't know these were intentionally not in lix so I opened https://gerrit.lix.systems/c/lix/+/2975 yesterday. I am very much of the opinion that these are a good idea and needed in the language. Some things I want to do are just impossible without (mainly arithmetic on IP/MAC addresses). If possible I would propose to try and coordinate these two requests to keep the two implementations on par. I'll try and implement the suggestions from here over on the lix CL as well. |
|
For comparison I've implemented a logical left/right shift using current nix/lix https://gist.github.com/PatrickDaG/c075f5ef8a7cba59b0999d8a0dd7a7ce. As far as I can tell it works and perfectly mirrors C shift semantics on |
Motivation
I want to shift bits without my nixos configuration taking a minute just to evaluate
Context
Partially addresses #13000 by adding bit shifting operations
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.