feat(LLVM): add poison tracking to pointers in interpreter - #1463
tobiasgrosser wants to merge 6 commits into
Conversation
78d457f to
d963848
Compare
regehr
left a comment
There was a problem hiding this comment.
this moves us in the right direction, I'd be happy to land this and work on the finer details as we move forward
1e239b5 to
49b27bf
Compare
Every other runtime value could be poison; a pointer could not, so the places that should have produced one returned an address of 0 instead. The pointer load even said so in a FIXME. An address of 0 is a value rather than a bottom element, so the substitute was observable. `Data.LLVM.Ptr` now wraps the address the way `Data.LLVM.Int` wraps its bit vector, and `RuntimeValue.addr` carries it. The memory interface stays indexed by a concrete address, because using a poison pointer for an access is undefined behaviour and that is decided by the operation before memory is reached. A pointer is poison when it is loaded from bytes with any poison in them, when it is bitcast from poisoned bits, or when it is offset from a poison pointer or by a poison index. Storing one writes eight poison bytes, which read back as poison both as a pointer and as an integer. Loading or storing through one, or casting one into a RISC-V register, is undefined behaviour: a register is a plain bit pattern with no poison to carry, which is the same reason Alive2's assembly mode drops poison entirely. `freeze` turns one into null. Refinement follows `Int`: poison is refined by every pointer, and a pointer only by itself.
0787d8a to
44d9650
Compare
luigirinaldi
left a comment
There was a problem hiding this comment.
From what I understand, it looks good. Some really minor nits in the new Ptr file.
Co-authored-by: Luigi Rinaldi <82770895+luigirinaldi@users.noreply.github.com>
Co-authored-by: Luigi Rinaldi <82770895+luigirinaldi@users.noreply.github.com>
Co-authored-by: Luigi Rinaldi <82770895+luigirinaldi@users.noreply.github.com>
nchappe
left a comment
There was a problem hiding this comment.
This looks good, thanks. Nit: the PR body is unclear, only unrealized casts from poison pointers to registers are fail.
Nice. I fixed the PR body wording. |
math-fehr
left a comment
There was a problem hiding this comment.
Nice! I just had 1-2 style comments, but nothing major!
I think however we should change the semantics of unrealized_conversion_cast, otherwise currently our lowering would be unsound
| cases q <;> simp_all | ||
|
|
||
| /-- The pointer whose bits are `b`, poison if any bit is poison. -/ | ||
| def ofByte (b : Byte 64) : Ptr := |
There was a problem hiding this comment.
Should we have the same for conversions with LLVM.Int, or is the conversion only with Byte?
| if bw = 64 then | ||
| match val' with | ||
| | .val v => .ok (.int 64 (LLVM.Int.val v.toBitVec)) | ||
| | .poison => .ok (.int 64 .poison) |
There was a problem hiding this comment.
For instance here, we should probably have a LLVM.Ptr.toInt?
| | .registerType _, [.addr val] => | ||
| /- A register has no poison to carry, so a poison pointer cannot be cast into one. -/ | ||
| let .val val := val | Interp.fail | ||
| return (#[.reg ⟨val.toNat⟩], mem, none) |
There was a problem hiding this comment.
This would be incorrect with
%p = "llvm.mlir.poison"() : () -> !llvm.ptr
%b = "llvm.bitcast"(%p) : (!llvm.ptr) -> !llvm.byte<64>
Here, the current lowering might create this program:
%p = "llvm.mlir.poison"() : () -> !llvm.ptr
%r = "builtin.unrealized_conversion_cast"(%p) : (!llvm.ptr) -> !riscv.reg
%b = "builtin.unrealized_conversion_cast"(%r) : (!riscv.reg) -> !llvm.byte<64>
So returning fail here is making this transformation incorrect. I think the answer is to return non-deterministically any possible value of !riscv.reg, like we do with poison (and in that case here, only return 0 in the current interpreter).
| Together with fresh memory being poison, this is the semantics proposed in | ||
| "Towards Removing Undef Values from LLVM IR" (Lobo et al., PLDI 2026), not | ||
| LangRef's, where uninitialized memory reads as `undef`. | ||
| As Clang on still on poison, e.g., for a bitfield or an integer copy of a |
In LLVM, values of type
ptrcan be poison. This PR lifts our previously poison-free pointer runtime values to support element-level poison. We add the following interpreter semantics:llvm.mlir.poisonyields a poison valuellvm.mlir.zeroyields a non-poison zero addressllvm.loadandllvm.storetrigger UB when accessing a poison addressllvm.getelementptrforwards poison on both operandsllvm.bitcastof a poison pointer yields (all) poison forllvm.intandllvm.byteThese semantics choices are worth taking a closer look:
unrealized_conversion_castfromllvm.ptrtoriscv.regwherellvm.ptris poison isfailfor now.As
riscv.regdoes not carry poison, the cast must either choose a concrete value or trigger UB. We will likely replaceunrealized_conversion_castin the future withzeroext,signext,anyext, and will also have ctrees, which will allow us to define meaningful semantics when casting toriscv.reg.llvm.loadinto allvm.ptrtype yields poison if any bit loaded was labeled poisonCurrently, LLVM models certain uninitialized memory as
undef, butundefis due to be replaced. The paperTowards Removing Undef Values from LLVM IRproposes to set uninitialized memory topoisonand use freezing loads to obtain unknown non-poisonous values. Without ctrees, a freeze is currently equal to picking a fixed concrete value, but eventually this will be a non-deterministic choice of any concrete bit pattern. As we neither want to introduceundef, we lack support for ctree, and also do not have freezing loads the current semantics means that in certain cases our semantics allow poison to propagate further than LLVM's. We document this.For better readability of the test cases, we improve the
toStringmethod used for printing pointers.