llvm: expose callee reuse and non-consuming IR parsing - #55
llvm: expose callee reuse and non-consuming IR parsing#55zhouguangyuan0718 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Review: LLVM 22 C API bindings
Solid, well-scoped PR. The C-shim + LLVM_VERSION_MAJOR dispatch approach matches the existing backports.cpp / switch_llvm22.go idiom, the Go wrappers follow the codebase's named-return style, and the tests genuinely exercise the tricky semantics: buffer ownership on both the success and diagnostic paths, alias/cast returns, and the no-target.1-duplication case.
Verified correct on the subtle points:
- No leak in the legacy
LLVMGoParseIRInContextfallback — theCopybuffer is handed toLLVMParseIRInContext, which frees its input on both success and failure, while the caller's original buffer stays intact. The non-consuming ownership contract documented onParseIRBufferholds on both branches. - No dangling pointer from
.str().c_str()on line 28 — the temporarystd::stringoutlives the call, andLLVMCreateMemoryBufferWithMemoryRangeCopycopies the name synchronously. C.GoString(nil)safely yields"", so a failure with an unseterrmsgwon't crash.
Findings below are minor/optional polish — none are blockers.
Informational: on LLVM < 22, ParseIRBuffer makes a full O(n) copy of the input buffer per call (required to honor the non-consuming contract; avoided entirely on LLVM >= 22 via LLVMParseIRInContext2). Worth awareness for callers doing bulk parsing on pre-22 toolchains.
| @@ -0,0 +1,19 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||
There was a problem hiding this comment.
[P3] Header files miss the LLVM banner/license block used repo-wide
Every other bindings source in this repo (IRBindings.h/.cpp, backports.cpp, SupportBindings.h, ...) opens with the LLVM //===- File - description -*- C++ -*-===// banner plus the full license block. CApiBindings.h and CApiBindings.cpp use only a bare // SPDX-License-Identifier line. Consider adding the standard banner for consistency with the surrounding files.
| // GetOrInsertFunction returns the callee for name, creating an external function | ||
| // declaration with type ft if the name is absent. Existing symbols, types, and | ||
| // attributes are preserved. The result can be an alias or, with typed pointers, | ||
| // a constant-expression cast rather than a Function; callers should use ft when |
There was a problem hiding this comment.
[P3] GetOrInsertFunction doc describes an unreachable typed-pointer case
The comment says the result "can be an alias or, with typed pointers, a constant-expression cast rather than a Function." All LLVM versions this binding compiles against use opaque pointers, so the constant-expression-cast case (typed-pointer mode) can no longer occur. The clause is hedged and not strictly wrong, but it documents a path unreachable here and could lead readers to expect a cast expression. Consider dropping the typed-pointer clause; the alias caveat and IsAFunction guidance remain accurate.
| func (c *Context) ParseIRBuffer(buf MemoryBuffer) (Module, error) { | ||
| var m Module | ||
| var errmsg *C.char | ||
| if C.LLVMGoParseIRInContext(c.C, buf.C, &m.C, &errmsg) != 0 { |
There was a problem hiding this comment.
[P3] No nil-buffer guard in ParseIRBuffer
The legacy shim dereferences Buffer via LLVMGetBufferStart/LLVMGetBufferSize/llvm::unwrap(Buffer)->getBufferIdentifier() with no NULL check, so a nil MemoryBuffer from Go crashes here. This matches upstream LLVMParseIRInContext2 behavior (not a regression), but since buf is a trust boundary from Go callers, a buf.IsNil() early return in ParseIRBuffer (and ParseIR) would be a cheap defense-in-depth guard. Optional.
Expose
Module.GetOrInsertFunctionto reuse existing callees without a separate lookup andContext.ParseIRBufferto parse assembly or bitcode while retaining caller ownership of the input buffer. LLVM 22 usesLLVMGetOrInsertFunctionandLLVMParseIRInContext2; older versions useModule::getOrInsertFunctionand an owned copy for the consuming parser. The copy preserves the buffer identifier for source filenames and diagnostics.Keep
ParseIRsource-compatible and document that it consumes its buffer on success and failure. The new callee API documents that aliases and typed-pointer casts are possible, and preserves existing declarations and attributes. Migrate the switch and metadata parser tests to explicit buffer disposal.Validation on macOS arm64:
go test ./...passed.Remote matrix coverage for other platforms and LLVM versions remains to be confirmed by CI.