Keep LuaJIT off the native-operator path - #14
Conversation
There was a problem hiding this comment.
Verified the diagnosis end to end rather than taking the description on faith.
The failure is real and pre-existing. The ci-baseline-check run (31273249714) has tree SHA 8852fde, byte identical to main's, so that leg really was unmodified main. Only luajit-2.1 failed there, with 5.3 and 5.4 passing. Its log carries the line that settles it: LuaJIT 2.1.1785763465 printing Using: native operators (Lua 5.3+). Implementation 1 selected on LuaJIT is precisely the bug.
Blast radius is a bit wider than the summary suggests. bit32 lost 24 of 98 and bit64 lost 2 of 116. bit16 passed 74/74, which is why this is easy to under-read: masking to 0xFFFF truncates the sign extension away, so the module that would have made the breakage obvious is the one that stayed green.
arshift is the nastier family. Those failures are not sign extension in the high bits, they are plain wrong answers: arshift(0x80000000, 1) returned 0x40000000 against an expected 0xC0000000, and arshift(0x80000000, 31) returned 1 against 0xFFFFFFFF. The native branch computes is_negative from a >= 0x80000000, which is never true once LuaJIT has already handed back a negative. So a caller defensively masking results to 32 bits would repair band and still be silently wrong here.
The fix works. CI is green on 4f7160d across all six matrix legs plus Check and Build, and the same LuaJIT 2.1.1785763465 now prints Using: bit library and passes 297/297. The bit32 count moving 98 to 107 is expected, not tests appearing from nowhere: the nine raw_* function identity checks are gated on _compat.is_luajit, so they only run once the branch is correct.
Independent local verification. My LuaJIT is 2.1.1783773675, which sits between your 1748459687 and CI's 1785763465, and it still does not parse a & b (load returns nil), so it exercises the unchanged path. main and this branch both give 3/3 modules there and both report bit library, which confirms the change is a genuine no-op on that build rather than merely passing. On Lua 5.5.0 the native branch still selects and gives 3/3, so is_luajit_host is not disturbing the 5.3+ path. I could not reproduce the failure locally, so as you said, CI is the only place the triggering build exists.
This is a clean fix and I have no objection to it landing.
Why this is a comment and not an approval
You armed squash auto-merge on this between my first read of the PR and my write (it was unset at 19:01 UTC and set by the time I posted). Checks are green, so a formal approval from me is the last gate and would merge to main immediately rather than hand you a verdict to act on. My standing rule is not to be the thing that merges, so the artifact here is deliberately a comment: it carries the verdict without pulling the trigger.
Say the word and I will convert this to a formal approval, or just merge it yourself. Nothing below blocks either.
One non-blocking note
The description argues the right question is "whether the host has 64-bit integers," but the code asks "is this LuaJIT." Those two come apart. The probe already builds the function, so the capability form is a comparison on the value rather than on the identity of the runtime:
local fn = result()
if fn and fn(0xFFFFFFFF, 0xFFFFFFFF) == 0xFFFFFFFF then5.3+ returns 0xFFFFFFFF there and any 32-bit-semantics host returns -1, so it separates them exactly. I confirmed the 0xFFFFFFFF leg on Lua 5.5.0. That version needs no knowledge of which runtimes exist, so it would have been immune to this without anyone having to notice the rolling release, and it stays immune to whatever grows the syntax next (a LuaJIT fork, or something unrelated) instead of needing another name added to the gate.
The concrete cost of the name based gate: on a real 5.4 or 5.5 host where anything else defines a jit global, this now skips Implementation 1, finds no bit and no bit32, and lands on pure Lua. I checked that path and results stay correct (band(0xF0F0F0F0, 0xFF00FF00) gives 0xF000F000), so it fails safe. But it is a silent drop to the slowest implementation, triggered by a global name this library does not own.
Unlikely enough that I would not hold the fix for it, and the current change is correct for the failure at hand. Raising it only because the capability probe costs nothing extra and removes the coupling.
4f7160d to
b24916a
Compare
|
Adopted the capability probe. It is the better question and the coupling you identified is real, so this is now Your framing of the cost is what decided it. My gate would have silently dropped a real 5.4 host to the pure-Lua path if anything else defined a Two corrections from your review that I had wrong and have folded into the message:
bit16 passing was the misleading part, not incidental. Masking to Also recorded: 24 of 98 bit32 and 2 of 116 bit64, and the On the merge point, understood and thank you for not pulling the trigger. Auto-merge was armed before your review landed and is now moot anyway, since Independent verification on 2.1.1783773675 was the useful part, since it sits between my build and CI's and still exercises the unchanged path. I could only show the no-op half locally too. |
There was a problem hiding this comment.
Capability probe is the right shape and it does what it claims on every host I can reach. Re-reviewed 4f7160d → b24916a; the only code delta is the one condition, and CI is 8/8 green including Lua luajit-2.1, the leg that was red on main. That leg is the decisive artifact, since it is the one build that actually parses & with 32-bit semantics.
Confirmed the thing the change was made for: with _G.jit set on a real Lua 5.5, 4f7160d selects pure Lua and b24916a selects native operators (Lua 5.3+). The silent drop is gone. On LuaJIT 2.1.1783773675 (pre-syntax-change) both revisions still report bit library, 3/3 modules, so the change remains a no-op on the unchanged path.
Three accuracy corrections to the commit message. squash_merge_commit_message here is COMMIT_MESSAGES, so this text is what lands on main, which is why I am bothering.
The Control4 paragraph has the mechanism backwards
Measured on the Prod controller against an unencrypted driver:
_VERSION = Lua 5.1
jit.version = LuaJIT 2.1.1700206165
rawget(_G,"jit") -> table: 0xdc8ea240 (present, not absent)
parses "a & b" -> false
bit library -> present, bit32 -> absent
bit.band(0xF0F0F0F0, 0xFF00FF00) = -268374016
So it is not "plain Lua 5.1 with no jit global", and controllers do not take the pure-Lua path. They run LuaJIT, jit is defined, and they land on Implementation 2 with is_luajit = true and to_unsigned applied. The raw bit.band values above are exactly why that normalisation is load-bearing there.
The conclusion holds, but for a weaker reason than stated. There is no exposure because the shipped LuaJIT is 2.1.1700206165, which predates the syntax change, not because controllers are structurally unable to reach the branch. If Control4 ever ships a newer LuaJIT in an OS update, controllers land directly on the broken path under the old code. That argues for landing this now rather than against it, so it is worth stating as a version fact.
"Anything with 32-bit semantics answers -1" is not quite true, and it does not matter
There is one host class where the probe self-cancels. I built Lua 5.4.7 with LUA_32BITS=1:
math.maxinteger = 2147483647
0xFFFFFFFF = -1
(0xFFFFFFFF & 0xFFFFFFFF) == 0xFFFFFFFF --> true
Both sides of the comparison wrap to -1, so the probe passes and selects the native path on a host with 32-bit integer semantics. Running the suite there: 95/98 bit32, bit64 red, band(0xFFFFFFFF, 0xFFFFFFFF) returns -1.
I do not think this warrants a code change, and I checked before saying so. fn(0xFFFFFFFF, 0xFFFFFFFF) > 0 does reject that host correctly, but the result is worse: 0/3 rather than 1/3, because the library's own constants wrap on that build. 0x100000000 is 0, so the pure-Lua path dies with attempt to perform 'n%0'. LUA_32BITS is out of scope at the literal level and no probe can rescue it.
So this is a wording point only. Something like "any host where 0xFFFFFFFF is a 64-bit integer and & returns 32-bit semantics", or an explicit note that LUA_32BITS builds are unsupported, would carry the same intent without the overclaim.
lua-crypto: right answer, wrong reason, and one follow-up
Not "only raw_* variants". src/crypto/utils/bytes.lua calls the wrapped bit32.mask through bit32_mask at lines 44 and 52, in u32_to_le_bytes and u32_to_be_bytes.
They survive anyway, and the reason is worth having written down: both callers immediately decompose with % 256 and floor(n / 256^k) % 256, which is sign-agnostic over the low 32 bits. Checked across the interesting inputs, unsigned value versus the signed twin a broken mask would return:
0xFFFFFFFF -> FF FF FF FF == -1 -> FF FF FF FF
0xF000F000 -> 00 F0 00 F0 == -268374016 -> 00 F0 00 F0
0x80000000 -> 00 00 00 80 == -2147483648 -> 00 00 00 80
0xDEADBEEF -> EF BE AD DE == -559038737 -> EF BE AD DE
Empirically consistent: lua-crypto main ran green at 18:40 today on LuaJIT 2.1.1785763465, the same build that breaks this repo.
Follow-up, not a blocker for this PR: lua-crypto vendors an amalgamated vendor/bitn.lua, and its copy of the probe at line 39 is still the unfixed if fn then. Merging here does not propagate. It needs a re-vendor to pick this up.
Merge mechanics
Correcting myself, and correcting one thing in your comment.
dismiss_stale_reviews_on_push clears review state but does not disarm auto-merge; those are separate objects, and auto-merge did survive the force-push. I read autoMergeRequest as { mergeMethod: SQUASH, enabledAt: 2026-08-08T19:04:25Z, enabledBy: derek-miller } at 19:15Z. It was disarmed somewhere between then and 19:25Z, and it now reads null. I had written this section off the earlier read, so the version of it that posted claimed the PR was still armed. It is not.
That changes the reasoning behind the artifact. I posted this as a COMMENT rather than an APPROVE because with auto-merge armed and all 8 checks green, approving would have merged to main on submit instead of handing over a verdict. With it disarmed that objection is gone, and an APPROVE would leave the merge button with Derek where it belongs.
I am still not flipping it unilaterally, for one reason only: your comment says Derek is deciding how the review gate should be handled across all five open PRs. Approving here would pre-empt that. Say the word and I will convert this to a formal APPROVE. The verdict itself is not in question, and this is not a hedge on it: the change is correct, minimal, and green on the leg that matters.
What I did not verify: nothing on LuaJIT 2.1.1785763465 locally, since that build exists only in CI. The luajit-2.0 leg rests on CI alone.
`_compat` decided it had Lua 5.3+ integer semantics by testing whether `a & b` parses. LuaJIT rolling releases from 2026 accept that syntax while keeping Lua 5.1 semantics, so the probe now succeeds there and selects a branch that assumes 64-bit integers: it sets `is_luajit = false` and skips the `to_unsigned()` normalisation, so signed 32-bit results leak out. Parsing was never the right question. The probe already builds the function, so ask the value instead: a host where `0xFFFFFFFF` is a 64-bit integer and `&` returns 64-bit semantics answers `0xFFFFFFFF`, and one returning 32-bit semantics answers `-1`. That needs no list of which runtimes exist, so it stays correct for whatever grows the syntax next rather than needing another name added to a gate. Hosts built with `LUA_32BITS=1` are out of scope either way: both sides of that comparison wrap to `-1`, and the library's own constants wrap too (`0x100000000` is `0`), so no probe rescues them. Two distinct failure modes, not one. `band`, `bor`, `bxor` and `mask` return the right low bits with sign extension above them, which a caller masking to 32 bits would repair. `arshift` is worse: it derives `is_negative` from `a >= 0x80000000`, never true once the host has already returned a negative, so it gives plain wrong answers that masking does not fix. `arshift(0x80000000, 1)` gave `0x40000000` instead of `0xC0000000`, and `arshift(0x80000000, 31)` gave `1` instead of `0xFFFFFFFF`. bit32 lost 24 of 98 vectors and bit64 2 of 116. bit16 passed 74/74 because masking to `0xFFFF` truncates the sign extension away, which is why the breakage was easy to under-read. Not introduced here. CI last ran green in January 2026 and `leafo/gh-actions-lua` now builds LuaJIT 2.1.1785763465, which crossed the threshold. Confirmed by running an empty commit on unmodified `main`: the same `luajit-2.1` failure with 5.3 and 5.4 passing, and its log shows that build printing `Using: native operators (Lua 5.3+)`. Control4 is not exposed today, but for a version reason rather than a structural one. Controllers run LuaJIT 2.1.1700206165 with `jit` defined, and land on Implementation 2 with `to_unsigned` applied; that build predates the syntax change, so the native branch is unreachable there for now. A LuaJIT bump in a future OS update would put them straight onto the broken path under the old code, which argues for landing this. Note that `jit` is only visible from unencrypted drivers, so probing the runtime from a signed Control4 driver reports no LuaJIT and is not representative. `lua-crypto` is also unaffected, though not because it avoids the wrapped API: `utils/bytes.lua` calls `bit32.mask` in `u32_to_le_bytes` and `u32_to_be_bytes`. Both callers immediately decompose with `% 256` and `floor(n / 256^k) % 256`, which is sign-agnostic over the low 32 bits, so the unsigned value and the signed twin a broken `mask` returns produce identical bytes. Its suite ran green on 2.1.1785763465, the build that breaks this repo. Verified on LuaJIT 2.1.1748459687, which predates the syntax change and so exercises the unchanged path: 3/3 modules, `is_luajit` stays true, and both `arshift` cases above are correct. The build that reproduces the failure exists only in CI.
b24916a to
8235d76
Compare
|
All three corrections taken, pushed as The Control4 paragraph was wrong and I confirmed your version. On I also found why my reading disagreed, which is worth recording: I probed The consequence matters more than the mechanism, and the message now says it your way: no exposure because the shipped build predates the syntax change, not because controllers cannot reach the branch. A LuaJIT bump in an OS update puts them straight onto it. That is an argument for landing this.
Noted on the re-vendor: On auto-merge you are right and I was wrong: Holding the approve exactly as you have it. Derek is choosing how to clear the gate across all five PRs and I am not going to route around that. |
|
Saying the word: please convert to a formal APPROVE. Derek has cleared this one specifically, so approving here does not pre-empt anything. The remaining four PRs are a separate question he is still deciding. State as of now: head is Auto-merge reads |
|
Confirmed at All three corrections read correctly against what I measured, so nothing further from me on those. One addition, because the new Control4 paragraph raises a question it does not answer. The signed-driver caveat is about how to measure, not about which path a signed driver takes, and as written it invites the opposite inference. The current code has no reference to the So a signed driver with
I could not measure Related, and not a request to change anything here: Verdict unchanged from round 2. Holding the approve as you asked. |
|
No, I asked for an approval |
There was a problem hiding this comment.
Approving at 8235d76.
8235d76^{tree} and b24916a^{tree} are both d3a40c7, byte identical, so the round 2 verification carries over with nothing to re-review. Same parent (ea30c87), so the only thing that moved is the commit message text for the three corrections. The code delta against main is still the single condition in src/bitn/_compat.lua.
CI is 8/8 green at this head, including Lua luajit-2.1, the leg that is red on main. That leg is the decisive artifact: it is the one build that parses & with 32-bit semantics, so it exercises exactly the host class the probe now rejects.
The change is correct and minimal. The three commit message corrections read accurately against what I measured, and since squash_merge_commit_message here is COMMIT_MESSAGES, that text is what lands on main.
Read auto_merge as null in the same call as this submit, so approving hands you the merge button rather than merging on submit.
Standing follow-up, not a blocker for this PR: lua-crypto vendors an amalgamated vendor/bitn.lua whose copy of the probe at line 39 is still the unfixed if fn then. Merging here does not propagate to it; it needs a re-vendor.
What I did not verify: nothing ran on LuaJIT 2.1.1785763465 locally, since that build exists only in CI. The luajit-2.0 leg rests on CI alone.
_compatdecided it had Lua 5.3+ integer semantics by testing whethera & bparses. LuaJIT rolling releases from 2026 accept that syntax whilekeeping Lua 5.1 semantics, so the probe now succeeds there and selects a branch
built on assumptions LuaJIT does not meet: it sets
is_luajit = falseand skipsthe
to_unsigned()normalisation, so LuaJIT's signed 32-bit results leak out asnegatives.
Every 32-bit operation is affected.
mask(0xFFFFFFFF)returns -1, which formatsas
0xFFFFFFFFFFFFFFFF, andband(0xF0F0F0F0, 0xFF00FF00)returns a negativewhose low bits are right and whose high bits are sign extension. Anything
downstream doing 32-bit arithmetic on the result is wrong, which is most of what
this library exists for.
Gated on
rawget(_G, "jit")instead. Parsing ability was never the rightquestion; the question is whether the host has 64-bit integers, and LuaJIT does
not regardless of what its parser accepts.
Not introduced by any change here. CI last ran green in January 2026 and
leafo/gh-actions-luanow builds LuaJIT 2.1.1785763465, which crossed thethreshold. Confirmed by running an empty commit on unmodified
main: the sameluajit-2.1failure, with 5.3 and 5.4 passing. The existing test vectors alreadycovered it; nothing had run them in seven months.
Verified on LuaJIT 2.1.1748459687, which predates the syntax change and so
exercises the unchanged path: 3/3 modules, and
is_luajitstays true. The buildthat reproduces the failure is only in CI, so that leg is the real check.