ssa: use ptrtoaddr for unsafe builtin bounds checks - #2515
zhouguangyuan0718 wants to merge 1 commit into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: switch unsafe bounds check to ptrtoaddr
The core change in ssa/expr.go is well-reasoned and correct. Using ptrtoaddr with the DataLayout address width instead of ptrtoint with the pointer size is the right approach for a provenance-free address observation, and the cross-architecture test coverage (amd64/386/custom datalayout) is thorough.
I specifically verified the main correctness concern — that truncating lastOffset from pointer width to the (possibly narrower) address width could hide an overflow — and it does not materialize. The lenTooLarge guard runs and traps first: any length reaching the truncation satisfies byteSize = length*elemSize <= maxAddr, so lastOffset = byteSize-1 fits losslessly in the address width. The length == 0 case is correctly masked by the isNonZero gate on the final assert. The overflow detection is sound, and for narrow-address targets it is more correct than the old pointer-width approach.
The main blocking concern is the personal-fork replace directive; two minor readability notes are inline.
|
|
||
| replace github.com/xgo-dev/llgo/runtime => ./runtime | ||
|
|
||
| replace github.com/xgo-dev/llvm => github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906104504-18183e7fb5bf |
There was a problem hiding this comment.
[P1] Fork replace directive should not merge to main
This points the LLVM binding at a personal fork pinned to a pseudo-version. Since ptrtoaddr/ConstPtrToAddr only exist in that fork, merging the source changes without publishing the binding upstream breaks the build for anyone not using the fork, and ties the project to a personal repo that may disappear or diverge (supply-chain surface). Recommendation: land the ptrtoaddr/ConstPtrToAddr bindings in github.com/xgo-dev/llvm, bump the require to a tagged release, and drop this replace (and the corresponding go.sum entries) before merge. Also note ptrtoaddr requires LLVM 21+; consider documenting the minimum LLVM version.
| } | ||
| lastOffset := b.impl.CreateSub(byteSize, llvm.ConstInt(uptr.ll, 1, false), "") | ||
| addr := llvm.CreatePtrToInt(b.impl, data.impl, uptr.ll) | ||
| if lastOffset.Type() != addrTy { |
There was a problem hiding this comment.
[P3] Document the invariant that makes the trunc lossless
The truncation of lastOffset to the address width is provably safe, but the invariant that guarantees it — byteSize <= maxAddr, enforced by the lenTooLarge check above — is implicit. A one-line comment (e.g. "byteSize <= maxAddr per the len check above, so the trunc is lossless") would spare future readers from worrying that high bits are silently dropped.
| b.Return() | ||
| b.EndBuild() | ||
| ir := fn.impl.String() | ||
| if !strings.Contains(ir, fmt.Sprintf("ptrtoaddr ptr %%p to i%d", tc.width)) || strings.Contains(ir, "ptrtoint") { |
There was a problem hiding this comment.
[P3] Blanket ptrtoint assertion is broader than its message
The negative check rejects any ptrtoint anywhere in the function IR, while the failure message speaks only about pointer-provenance of the address observation. This passes today, but if unrelated future codegen in this path ever emits a ptrtoint, the test would fail with a misleading provenance message. Consider scoping the check to the operand (e.g. ptrtoint ptr %p) to match the stated intent. Not a current bug.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
1 participant
unsafe.Stringandunsafe.Slicebounds checks only need the input pointer's address to detect wraparound, but currently emitptrtoint, which also exposes pointer provenance. Use LLVM 22ptrtoaddrfor this check while retaining the original pointer in the resulting string or slice.The maximum byte length and wraparound arithmetic use the DataLayout address width. For a 64-bit pointer representation with a 32-bit index width, the check rejects oversized lengths before truncating the offset and performs the end-address addition in 32 bits. General
unsafe.Pointer/uintptrconversions, map keys, and FFI paths keep their existing behavior. The DCE override constant cloner also preservesptrtoaddrconstant expressions.Depends on xgo-dev/llvm#53. The module replacement temporarily pins its exact personal-fork commit
18183e7fb5bf4204465c04ab41ec8682caf47631; replace it with the upstream binding release after that PR lands.Validation on macOS arm64 with LLVM and LLD 22.1.8:
go test ./ssa ./internal/dcepass -count=1, including new 32-bit, 64-bit, and mixed pointer/index-width IR regressions and constant-cloning coverage.llgo test ./test/go ./test/std/unsafe -run 'TestUnsafe|TestSlice|TestString' -count=1. Existing unsafe builtins tests cover negative/oversized lengths, nil pointers, element-size overflow, end-address wraparound, and the last-byte boundary.go mod verifyandgit diff --check.Cross-platform CI is pending; no end-to-end performance claim is made.