tamnd / tamnd/firepanda

Mojo miscompiles an Optional of a struct with trailing padding, and AnyArray pads around it

Open
#286 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Mojo
Stars
1
Forks
0
Avg merge
1h 31m
Merged PRs (30d)
640

Description

`AnyArray` in `firepanda/array/any.mojo` carries a field called `_slack` that nothing reads. It is a workaround for a Mojo 1.0.0 code generation bug and it should come out when the bug does. This issue is what the field's docstring points at.

### The bug

Mojo 1.0.0 miscompiles `Optional[T]` when `T` has trailing padding. A value moved into the optional reports absent when the optional is asked whether it holds anything, so `Bool(held)` is `False` immediately after `Optional[T](value^)`, and `take()` on it aborts.

### How it was found

It arrived as eight test files failing in #282, none of them anywhere near the change. The change added a `List[NestedNode]` field to `AnyArray`, and the symptom was every `Index` in the library quietly becoming the range 0, 1, 2, plus one crash unwrapping an empty optional. `Index` holds its labels in an `Optional[AnyArray]`, and an index whose labels report absent is a range index, which is a perfectly ordinary thing for an index to be, so nothing raised and every lookup answered the wrong row.

### The bisection

A struct of the same shape as `AnyArray` with a trailing `InlineArray[UInt8, n]`, moved into an `Optional` and asked whether it holds anything, at the sizes where the struct total stays fixed:

| n | fields | size_of | round trips |
| --- | --- | --- | --- |
| 1 | 464 | 480 | no |
| 8 | 464 | 480 | no |
| 16 | 464 | 480 | yes |
| 17 | 480 | 496 | no |
| 24 | 480 | 496 | no |
| 32 | 480 | 496 | yes |
| 64 | 512 | 528 | yes |

Same size, different answer, and the thing that changes with the answer is whether the fields fill the struct. It is the padding and not the size.

### The workaround

`var _slack: UInt64` on `AnyArray`, which pads the fields back out to the size of the struct. Two tests hold it in place, `test_a_column_survives_being_put_in_an_optional` and `test_a_column_has_no_trailing_padding`, so the next person to add a field here gets a failing test rather than broken indexes. The second is the one that matters, because the first would go on passing at some sizes.

### What to do

- [ ] Cut the probe down to something small enough to file upstream and file it
- [ ] Check each toolchain bump against a probe rather than against the suite, since the failure is silent in seven of the eight files it caused
- [ ] Delete `_slack` and `test_a_column_has_no_trailing_padding` when the bump that fixes it lands, and keep `test_a_column_survives_being_put_in_an_optional`

Every other struct in the library that goes into an `Optional` is worth checking too. `AnyArray` is the one that was caught because `Index` is the one thing that hides the failure instead of raising.

Contributor guide

Open the contributing guide

Research direction

Start in firepanda/array/any.mojo, read the _slack docstring, and run test_a_column_survives_being_put_in_an_optional and test_a_column_has_no_trailing_padding. Reduce the Optional padding probe enough to file upstream and check toolchain bumps against it. Done means removing _slack and the padding test after the compiler fix, while keeping the surviving-column test.

Written by the indexing model from the issue text.

Assessment

Domain
data-engineering, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.