Skip to content

Expose function cloning and local IR transformation utilities - #56

Open
zhouguangyuan0718 wants to merge 1 commit into
xgo-dev:xgofrom
zhouguangyuan0718:codex/ir-cloning-go-api-20261007
Open

zhouguangyuan0718 wants to merge 1 commit into
xgo-dev:xgofrom
zhouguangyuan0718:codex/ir-cloning-go-api-20261007

Conversation

@zhouguangyuan0718

Copy link
Copy Markdown

Go clients cannot currently clone a function, retain a mandatory tail-call boundary, or clean up a specialized CFG through the bindings. Expose LLVM's existing cloning and local IR utilities so clients can implement transformations in Go without a custom pass.

The API copies functions with their attributes and remapped debug scopes, folds local instructions and terminators, removes unreachable blocks, reads/rebuilds inline assembly properties, converts metadata values, changes a subprogram's linker name, and looks up symbols of any global kind. Tail-call kinds distinguish musttail from optional hints. The C++ layer only forwards to generic LLVM APIs and contains no client transformation policy.

Tests verify independent function/debug cloning, declaration attributes, PHI cleanup, external references, symbol lookup including aliases, metadata identity, inline-assembly flags, and mandatory tail-call verification. Full binding tests passed locally with LLVM 22 on macOS/arm64 and LLVM 19/22 on Linux/amd64.

Needed for the Go implementation of xgo-dev/llgo#2748.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

This adds a clean, well-tested set of Go bindings for IR cloning and local transformation utilities missing from llvm-c. The wrappers correctly reuse existing helpers (boolToLLVMBool), follow the (receiver) (named return) style of ir.go, pair C.CString with defer C.free, and use length-carrying string accessors (StringRef(Name, Len) / C.GoStringN) rather than relying on NUL termination. Test coverage is strong: it verifies the original is unmodified, debug-info linkage names stay independent, external refs/attributes survive cloning, declaration clones keep attributes, round-trips tail-call kinds and inline-asm properties, and re-verifies the module.

The findings below are mostly minor. The most substantive is the unchecked unwrap<T>() casts at the cgo boundary; the rest are robustness/clarity notes.

Verified: the repo targets LLVM 14–22 (LLVM 22 is the default via build tags), so the InlineAsm::getAsmString() return-type change (LLVM 21+ returns StringRef by value) is relevant. I confirmed with the referenced upstream headers that the current const auto & binding is not a dangling-pointer bug — lifetime extension keeps it alive and .data() points into the InlineAsm's persistent storage in both regimes — but see the inline note on clarity.

Inline comments below.

Comment thread transforms.cpp
}
LLVMValueRef LLVMGoCloneFunction(LLVMValueRef Fn) {
ValueToValueMapTy Map;
return wrap(CloneFunction(unwrap<Function>(Fn), Map));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unchecked unwrap<T>() narrowing casts (reachable from Go misuse).

unwrap<Function> here, and unwrap<CallInst> / unwrap<DISubprogram> / unwrap<InlineAsm> / *unwrap<Function> below (lines 31, 35, 40, 44, 47), are implemented via cast<T>(). In an assertions-enabled build this aborts on a wrong-kind input, but in a release/no-assertions LLVM (what -lLLVM-22 typically links) it is an unchecked static_cast, so passing a non-function Value to CloneFunction, a non-call to SetTailCallKind/TailCallKind, or non-DISubprogram metadata to SetSubprogramLinkageName is undefined behavior (type confusion) with no Go-side kind check. Line 31 (*unwrap<Function>(Fn)) is the sharpest case — an immediate deref, so a null/wrong-kind Fn is an invalid access.

This differs from the repo convention in IRBindings.cpp (e.g. LLVMGoConstFPGetBits uses dyn_cast<ConstantFP> + null return, and null-guards precede unwrap). Consider dyn_cast<T>() with a null/no-op fallback, or at minimum documenting the precondition in transforms.go as SetSubprogramLinkageName already does ("md must be a DISubprogram") — CloneFunction, RemoveUnreachableBlocks, SetTailCallKind/TailCallKind, and InlineAsmInfo currently carry no such note.

Comment thread transforms.cpp
}
LLVMGoInlineAsmInfo LLVMGoGetInlineAsmInfo(LLVMValueRef Asm) {
auto *A = unwrap<InlineAsm>(Asm);
const auto &Text = A->getAsmString();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto & binding is fragile across the supported LLVM range (not a bug).

On LLVM ≤ 20, getAsmString()/getConstraintString() return const std::string &; on LLVM 21+ (and the default-22 build here) they return StringRef by value. const auto &Text = A->getAsmString(); then binds a reference to a temporary StringRef. It is safe in practice — lifetime extension keeps Text alive to end of scope and .data()/.size() point into the InlineAsm's own storage — but the const auto & reads as if it aliases a long-lived string and obscures that distinction. Prefer value binding (auto Text = A->getAsmString(); or an explicit StringRef) so intent is clear and the code is obviously correct whether the API returns a reference or a value.

Comment thread transforms.go
type TailCallKind uint32

const (
TailCallKindNone TailCallKind = iota

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicit coupling to llvm::CallInst::TailCallKind with no compile-time guard.

These iota constants (None=0, Tail=1, MustTail=2, NoTail=3) happen to match LLVM's CallInst::TailCallKind enum, and LLVMGoGetTailCallKind returns a bare unsigned cast straight across. If LLVM ever reorders/extends that enum, the Go constants drift silently. A short comment here (and/or in transforms.cpp) noting "values must stay in sync with llvm::CallInst::TailCallKind" would protect future maintainers. TestMustTailCallKinds round-trips the values, which mitigates but doesn't catch a reordering.

Comment thread transforms.go
C.LLVMGoSetSubprogramLinkageName(md.C, text, C.size_t(len(name)))
}

// AsMetadata converts a constant or MetadataAsValue back into metadata.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comment understates AsMetadata behavior.

The underlying LLVMValueAsMetadata has three paths: Constant → ConstantAsMetadata::get, MetadataAsValue → getMetadata(), and a fallback ValueAsMetadata::get(V) for any other value (e.g. a plain instruction or argument). The comment only lists the constant and MetadataAsValue cases, and "converts ... back into metadata" implies a strict round-trip even though the constant/generic paths wrap a value as metadata. Consider noting the general-value fallback. (Separately, minor: AsMetadata wraps the general LLVMValueAsMetadata while the existing ConstantAsMetadata (ir.go) wraps LLVMConstantAsMetadata; a name like ValueAsMetadata next to those in ir.go would be more discoverable — non-blocking.)

Comment thread transforms_test.go
t.Fatal("clone must own distinct debug information")
}
clone.Subprogram().SetSubprogramLinkageName("copy")
if !strings.Contains(mod.String(), `linkageName: "copy"`) || !strings.Contains(mod.String(), `linkageName: "original"`) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: mod.String() is called twice in this one expression (and clone.String() ~4 times across lines 85–88), each a full IR serialization + cgo copy. Negligible for this tiny test, but hoisting each into a local (ir := mod.String()) is cleaner and avoids serializing the same IR repeatedly if this pattern is ever copied into a benchmark or a loop over many functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant