ir: bind constant-range attribute creation - #52
Conversation
There was a problem hiding this comment.
Review: CreateConstantRangeAttribute Go binding
A clean, well-constructed addition. The LLVMGo* C shim guarding on LLVM_VERSION_MAJOR >= 19 matches the existing binding style, the word-count/width validation is sound, and the cgo pointer pass-through is safe: the upstream LLVMCreateConstantRangeAttribute copies the words into an APInt and does not retain the Go slice storage.
Verified clean:
- Memory safety —
&lowerWords[0]/&upperWords[0]are only reached afternumBits > 0(sonwords >= 1) and the exact-length check, so no empty-slice deref; C reads exactlyceil(numBits/64)words, matching the Go count. Overflow guard onnumBitspreventsC.unsignedtruncation. - Performance — no allocations or per-word marshalling; single cgo call.
- Docs — the word order (least-significant-word-first),
ceil(numBits/64)requirement, panic conditions, and pre-19 nil behavior all match the implementation and are confirmed by the tests (e.g. the 128-bit case3 + 2*2^64 = 36893488147419103235). - Tests —
TestConstantRangeAttributeeven zeroes the input slices after construction to confirm LLVM copied the words; invalid-bounds cases cover the panic paths.
Two minor, non-blocking suggestions inline. No blocking issues found.
| if len(lowerWords) != nwords || len(upperWords) != nwords { | ||
| panic("llvm: constant range bounds have incorrect word counts") | ||
| } | ||
| a.C = C.LLVMGoCreateConstantRangeAttribute(c.C, C.unsigned(kind), C.unsigned(numBits), |
There was a problem hiding this comment.
[P3] Note the nwords>=1 invariant that keeps &lowerWords[0] safe
The &lowerWords[0] / &upperWords[0] dereference is safe today only because numBits > 0 forces nwords >= 1 and the preceding length check guarantees both slices are non-empty. This is a fragile coupling: any future loosening of the validation could silently introduce an out-of-bounds [0] access across the cgo boundary. A one-line comment noting that nwords >= 1 is guaranteed above would protect the invariant. Non-blocking.
| // CreateConstantRangeAttribute creates a constant-range attribute such as | ||
| // "range". Bounds are unsigned words in least-significant-word-first order; | ||
| // each slice must contain exactly ceil(numBits/64) words and numBits must be | ||
| // positive. Invalid widths or word counts panic. On LLVM before 19, which does |
There was a problem hiding this comment.
[P3] Doc says pre-19 returns nil, but validation runs on all versions first
The comment states "On LLVM before 19 ... it returns a nil Attribute," but the width/word-count validation (which can panic) runs unconditionally before the version-gated C call. So on a pre-19 build an invalid-argument call panics rather than returning nil. The behavior is arguably better, but the doc reads as a pure "always nil" path. Consider clarifying, e.g. "a nil Attribute is returned for otherwise-valid arguments." Non-blocking.
Clients cannot currently construct LLVM constant-range attributes through the Go API. For example, declaring a nonnegative
i32ori64return range requires a private cgo bridge in the client.Add
Context.CreateConstantRangeAttribute(kind, numBits, lowerWords, upperWords)aroundLLVMCreateConstantRangeAttribute. The bounds use least-significant-word-firstuint64slices, support multiple words, and are checked for invalid widths and incorrect word counts before entering C. The context owns the resulting attribute; input slices can be reused immediately.A version-guarded forwarding function preserves compilation against LLVM 14–18, returning a nil attribute where the API is unavailable. LLVM 19 and later use the upstream LLVM-C constructor.
Tests cover 1/32/64/65/128-bit ranges, multiword ordering, return-attribute round-trips, LLVM verification, independence from input slice storage, and malformed bounds. The existing CI matrix also exercises the pre-19 fallback.
Validation on macOS arm64:
go test -tags=byollvm ./... -count=1.go test -tags=byollvm,llvm19 ./... -count=1.Consumer: xgo-dev/llgo#2510 will use this API and remove its private cgo bridge.