Skip to content

Add bit shifting built-ins #13001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add bit shifting built-ins #13001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/manual/rl-next/bit-shift-builtins.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
synopsis: Add bit shifting built-ins
issues: [13000]
---

Added left and right bit shifting built-ins.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give an example.

10 changes: 10 additions & 0 deletions src/libexpr-tests/primops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,16 @@ namespace nix {
ASSERT_THAT(v, IsIntEq(1));
}

TEST_F(PrimOpTest, bitShiftLeft) {
auto v = eval("builtins.bitShiftLeft 3 2");
ASSERT_THAT(v, IsIntEq(12));
}

TEST_F(PrimOpTest, bitShiftRight) {
auto v = eval("builtins.bitShiftRight 17 3");
ASSERT_THAT(v, IsIntEq(2));
}

TEST_F(PrimOpTest, lessThanFalse) {
auto v = eval("builtins.lessThan 3 1");
ASSERT_THAT(v, IsFalse());
Expand Down
34 changes: 34 additions & 0 deletions src/libexpr/primops.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4007,6 +4007,40 @@ static RegisterPrimOp primop_bitXor({
.fun = prim_bitXor,
});

static void prim_bitShiftLeft(EvalState & state, const PosIdx pos, Value * * args, Value & v)
{
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contains UB if i2 is too large. Do it some other way that is safe. Maybe there's an intrinsic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. fully specified behaviour and a spec
  2. knowledge of edge cases and how they're handled and tests
  3. demonstrated need (though that is somewhat shaky here, doing IFD to achieve this probably isn't a great answer either)
  4. 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

static RegisterPrimOp primop_bitShiftLeft({
.name = "__bitShiftLeft",
.args = {"e1", "e2"},
.doc = R"(
Return the integer *e1* shifted left by *e2*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Insufficiently specific, what happens with negative operands? Are those invalid? What about negative shift values?

I suspect the implementation here does UB with them.

)",
.fun = prim_bitShiftLeft,
});

static void prim_bitShiftRight(EvalState & state, const PosIdx pos, Value * * args, Value & v)
{
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably contains UB if i2 is too large

}

static RegisterPrimOp primop_bitShiftRight({
.name = "__bitShiftRight",
.args = {"e1", "e2"},
.doc = R"(
Return the integer *e1* shifted right by *e2*.
)",
.fun = prim_bitShiftRight,
});

static void prim_lessThan(EvalState & state, const PosIdx pos, Value * * args, Value & v)
{
state.forceValue(*args[0], pos);
Expand Down