CF 7289 Review: JIT inlining SIGSEGV in MemoryBuffer::getFile

code-review · generated from canonical Markdown

Type
code-review

Source
postgresql-master

Slug
commitfest-7289-review

Tags

Updated
2026-09-18T04:09:42.202680+00:00

Verdict / Summaryready-for-committer

Front matter

schema: review-report-v2
target: CommitFest 7289
title: "CF 7289: JIT inlining SIGSEGV in MemoryBuffer::getFile with LLVM 17 / GCC 13"
head: 2631e08d791d3b504ddc6a4c790520f9878a1642
base: 2631e08d791d3b504ddc6a4c790520f9878a1642
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
diff_sha256: 9a4095d08470c6347b754489a0ebc01e548c2805294cf08725388ce5c02c5fb9

CF 7289 Review: JIT inlining SIGSEGV in MemoryBuffer::getFile

Scope

CommitFest: 7289
Patch: [PATCH] JIT inlining SIGSEGV in MemoryBuffer::getFile with LLVM 17 / GCC 13
Author: Yuriy Grigoryev (ju.grigorev@ftdata.ru)
Thread: dc9fb68c682c4eec9d18f1007bd568f8@localhost.localdomain (2026-08-19)
Head: 2631e08d791d3b504ddc6a4c790520f9878a1642 (staged uncommitted diff)
Base: 2631e08d791d3b504ddc6a4c790520f9878a1642 (origin/master HEAD)
Diff SHA-256: 9a4095d08470c6347b754489a0ebc01e548c2805294cf08725388ce5c02c5fb9
Changed file: src/backend/jit/llvm/llvmjit_inline.cpp (+30 / -11)
Changed function: llvm_load_summary (lines 767–810)

Change Summary

The patch replaces llvm::MemoryBuffer::getFile(path) (the C++ overload whose default std::optional<Align> argument is instantiated at the call site and SIGSEGVs under a GCC-13/distro-LLVM-17 compiler mismatch) with the C-API call LLVMCreateMemoryBufferWithContentsOfFile, matching the established pattern in load_module() (same file, line 494) and llvm_create_types() (src/backend/jit/llvm/llvmjit.c:1005). The buffer is materialised into a llvm::MemoryBufferRef via *llvm::unwrap(buf), and LLVMDisposeMemoryBuffer(buf) is invoked on both the success path and the elog(FATAL) path. A std::string pathstr(path) is introduced because the C API requires a NUL-terminated const char *. LLVMDisposeMessage(msg) is called on the open-failure path to free the LLVM-allocated error string.

Deterministic Verdict

ready-for-committer

0 BLOCKER findings (verified: 0, unverified: 0)
0 MAJOR findings
1 verified MINOR maintainability finding (PATCH-1: redundant bare { } block)
5 refuted NIT findings (confirmatory non-defect analyses)
No correctness, security, performance, or test defects
No live pgsql-hackers thread objections

Findings

PATCH-1

— MINOR — maintainability — `src/backend/jit/llvm/llvmjit_inline.cpp:789`

Mechanism: Bare { } compound scope around the success/error body (lines 789–808) has no control-flow purpose; the early-return guard at lines 782–787 already narrows flow to the success path, making the remaining compound block purely stylistic.

Failure: Future readers may misread the block as load-bearing (e.g. RAII scope guard or error isolation); pgindent does not touch .cpp files, so the redundant block will persist indefinitely.

Verification: verified — source inspection of HEAD vs staged diff confirmed the pre-patch code used if (EC) { ... } else { ... } where the else-block scoped the locals. The patch replaced it with an early return nullptr guard, leaving the bare block with no control-flow purpose. The repaired postimage was compile-validated (llvmjit.so linked cleanly against LLVM 21).

Repair: Concrete unified diff provided (SHA-256: a366f782bb4f668a539afe527d6fe4e28eb8602b5e7d1e31b8f7c8d082eb61f8), bound to HEAD 2631e08d791d3b504ddc6a4c790520f9878a1642. Validation: passed (apply-check, diff-check, compile with g++ against LLVM 21, and static-check; llvmjit.so linked successfully in a disposable coverage build tree).

PATCH-2

— NIT — maintainability — `src/backend/jit/llvm/llvmjit_inline.cpp:809` — REFUTED

return nullptr; after elog(FATAL) is unreachable. Pre-existing (present in HEAD before the patch); not introduced by CF 7289. Serves compiler-satisfaction purposes. No defect.

PATCH-3

— NIT — docs — `src/backend/jit/llvm/llvmjit_inline.cpp:774` — REFUTED

Comment is accurate and self-contained. Correctly explains the ABI mismatch mechanism (std::optional<Align> default argument instantiated at call site; GCC 13 / distro LLVM 17 mismatch). No defect.

SEC-1

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:782` — REFUTED

Path-traversal surface confirmation (no flaw). Path is $libdir-confined via add_module_to_inline_search_path prefix guard (line 822: modpath.starts_with("$libdir/")). Not user-controlled. No defect.

SEC-2

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:805` — REFUTED

FATAL-path buffer disposal (LLVMDisposeMemoryBuffer(buf) at line 805) is redundant but harmless defensive cleanup before noreturn elog(FATAL). No double-free, no use-after-free. No defect.

SEC-3

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:785` — REFUTED

LLVMDisposeMessage(msg) on the open-failure path is correct LLVM C-API ownership handling. msg is allocated by LLVMCreateMemoryBufferWithContentsOfFile on failure and freed exactly once. No leak. No defect.

Verification Summary

Finding	Severity	Class	Status	Repair
PATCH-1	MINOR	maintainability	verified	provided (passed)
PATCH-2	NIT	maintainability	refuted	not-applicable-refuted
PATCH-3	NIT	docs	refuted	not-applicable-refuted
SEC-1	NIT	security	refuted	not-applicable-refuted
SEC-2	NIT	security	refuted	not-applicable-refuted
SEC-3	NIT	security	refuted	not-applicable-refuted

Repair Validation Summary

PATCH-1: passed — apply-check, diff-check, compile, and static-check in a disposable worktree bound to HEAD 2631e08d791d3b504ddc6a4c790520f9878a1642. The repaired llvmjit_inline.cpp was copied into a disposable coverage build source tree and llvmjit.so relinked cleanly against LLVM 21 with no warnings or errors. The brace removal is purely structural; runtime behaviour is identical.
All refuted findings carry not-applicable-refuted envelopes for every repair field.

Security Summary

No BLOCKER / MAJOR / MINOR security findings. The swap from the C++ MemoryBuffer::getFile() to the C LLVMCreateMemoryBufferWithContentsOfFile is an ABI-hardening change that preserves the existing (non-user-controlled, $libdir-confined) path model and adds correct, exactly-once LLVM buffer disposal on every reachable path. Memory-context false-positive filter applied: all memory in this function is LLVM-heap (LLVMMemoryBufferRef, LLVMDisposeMessage), not PostgreSQL MemoryContext.

Test-Gap Summary

This is LLVM-JIT inline code, not exercised by the default make check / pg_regress suite. Activation requires --with-llvm at configure time and JIT activation (jit=on, jit_above_cost=0, jit_inline_above_cost=0). There is no src/test/jit/ tree. End-to-end verification of the SIGSEGV fix requires the specific toolchain mismatch (GCC 13 / distro LLVM 17); a matching-toolchain build (LLVM 21 / GCC 15) confirms compilation and non-regression but cannot reproduce the original SIGSEGV. Runtime validation was performed in this review (see Changed-Code Coverage).

pgsql-hackers Thread Ledger

Status: no-thread-found

The original patch submission was found (dc9fb68c682c4eec9d18f1007bd568f8@localhost.localdomain, 2026-08-19, from ju.grigorev@ftdata.ru). No review replies, follow-up discussion, or committer responses were found in the pgsql-hackers archive. The thread contains only the initial patch post; all findings are disposition new (not previously raised).

Verified-Clean Section

The following were explicitly checked and confirmed clean:

LLVMDisposeMemoryBuffer on all paths — OK (success: line 799, FATAL: line 805, open-failure: no buffer created)
llvm::unwrap(buf) correctness — OK (types line up via DEFINE_SIMPLE_CONVERSION_FUNCTIONS)
Buffer lifetime vs ModuleSummaryIndex retention — OK (MemoryBufferRef does not retain buffer backing)
pathstr lifetime — OK (std::string materialises NUL-terminated copy)
ilog/elog argument types — OK (all %s arguments are NUL-terminated C strings)
LLVMDisposeMessage(msg) on open-failure — OK (correct LLVM C API ownership)
PG MemoryContext filter — applied, no findings (LLVM heap, not PostgreSQL MemoryContext)
Whitespace / git diff --check — clean
catversion / pg_dump / OIDs — N/A (no catalog/SQL/dump-format impact)
Error style — OK (ilog(DEBUG1) for non-fatal, elog(FATAL) for corrupt summary)

Changed-Code Coverage

Coverage requirement: required — changed instrumentable implementation in src/backend/jit/llvm/llvmjit_inline.cpp

Execution

Selected lane: host-disposable
Status: passed
Build: Fresh coverage build with --coverage CFLAGS/CXXFLAGS, full postgres backend and llvmjit.so compiled against LLVM 21 / GCC 15
Cluster: Disposable initdb cluster with jit=on, jit_above_cost=0, jit_inline_above_cost=0, jit_optimize_above_cost=0
Test: Boolean join query (SELECT t1.b1, NOT t1.b1, t2.b1, NOT t2.b1 FROM booltbl1 t1, booltbl2 t2) — triggered JIT inlining successfully (DEBUG: time to inline: 0.086s), returned 1,000,000 rows, no SIGSEGV
Counter epoch: fresh disposable (gcda cleared before the test run)

Metrics

Metric	Count
Changed executable lines	13
Covered changed lines	8
Compiler-uncovered changed lines	5
Uncovered changed lines	5
Structural uncovered changed lines	0
Changed lines without coverage coordinates	0
Changed branches	4
Covered changed branches	2
Integrity errors	0

Uncovered Regions (all infeasible)

Region 1: llvm_load_summary:785-786 — File-open-failure path

Lines: 785 (LLVMDisposeMessage(msg)), 786 (return nullptr;)
Coverability: infeasible-with-reason — requires missing/unreadable postgres.index.bc
Waiver: required-external-resource

Region 2: llvm_load_summary:803,805,807 — Corrupt-summary FATAL path

Lines: 803 (std::string err = toString(...)), 805 (LLVMDisposeMemoryBuffer(buf)), 807 (pathstr.c_str(), err.c_str())
Coverability: infeasible-with-reason — requires corrupt/invalid postgres.index.bc
Waiver: required-external-resource

Uncovered Branches (all infeasible)

Branch 1: llvm_load_summary:782 (branch_id 4) — file-open-failure branch (count: 0)

Coverability: infeasible-with-reason — requires missing bitcode index file
Waiver: required-external-resource

Branch 2: llvm_load_summary:794 (branch_id 2) — summary-parse-error branch (count: 0)

Coverability: infeasible-with-reason — requires corrupt bitcode index file
Waiver: required-external-resource

Touched Functions

Function	Path	Lines	Entered	Lines (cov/total)	Branches (cov/total)
`llvm_load_summary`	`src/backend/jit/llvm/llvmjit_inline.cpp`	767–810	yes	8/13	2/4

Non-Instrumented Changes

None. All changes are in the single instrumentable C++ file src/backend/jit/llvm/llvmjit_inline.cpp.

Aggregated Test Plan

Compile validation (executed): make -C src/backend/jit/llvm llvmjit.so with --with-llvm against LLVM 21 — passed
Runtime JIT-inlining validation (executed): Boolean join query with jit_above_cost=0, jit_inline_above_cost=0 — JIT inlining triggered successfully (time to inline: 0.086s), no SIGSEGV, correct results (1,000,000 rows)
Coverage measurement (executed): gcov -b -c on llvmjit_inline.gcda — 8/13 changed lines covered, all uncovered lines are infeasible error paths
Repair patch validation (executed): PATCH-1 repair patch apply-checked and compile-validated in disposable build tree (llvmjit.so relinked cleanly)

Provenance

Seat	Model	Role
pg-patch-reviewer	glm-5.2:cloud	patch review, PG memory-context filter, readiness
pg-security-reviewer	glm-5.2:cloud	security + C memory-safety delegation
pg-finding-verifier	glm-5.2:cloud	finding verification + repair validation
pg-change-coverage-reviewer	glm-5.2:cloud	changed-code coverage measurement
pg-review-harness	glm-5.2:cloud	orchestration + v2 artifact assembly

Residual Risk

Low. The patch is a focused, single-function ABI-hardening change with a clear root-cause comment. The only suggested change is cosmetic (PATCH-1: remove redundant bare { } block). The SIGSEGV precondition (GCC 13 / distro LLVM 17 mismatch) cannot be reproduced in this review environment (LLVM 21 / GCC 15), but the patch compiles and runs correctly on the available toolchain, and the C API pattern is already proven in the sibling functions load_module() and llvm_create_types().

Validator

Artifact validated with the run-scoped validator (validate_review_artifact.py, SHA-256 edbcb304ffbd062627196c1ee0ce3d9b60cae80dd7b201e420335fea0a31979e) — ok: pg-review-artifact-v2 is structurally complete

CF 7289 Review: JIT inlining SIGSEGV in MemoryBuffer::getFile

Scope

Change Summary

Deterministic Verdict

Findings

— MINOR — maintainability — src/backend/jit/llvm/llvmjit_inline.cpp:789

— NIT — maintainability — src/backend/jit/llvm/llvmjit_inline.cpp:809 — REFUTED

— NIT — docs — src/backend/jit/llvm/llvmjit_inline.cpp:774 — REFUTED

— NIT — security — src/backend/jit/llvm/llvmjit_inline.cpp:782 — REFUTED

— NIT — security — src/backend/jit/llvm/llvmjit_inline.cpp:805 — REFUTED

— NIT — security — src/backend/jit/llvm/llvmjit_inline.cpp:785 — REFUTED

Verification Summary

Repair Validation Summary

Security Summary

Test-Gap Summary

pgsql-hackers Thread Ledger

Verified-Clean Section

Changed-Code Coverage

Execution

Metrics

Uncovered Regions (all infeasible)

Uncovered Branches (all infeasible)

Touched Functions

Non-Instrumented Changes

Aggregated Test Plan

Provenance

Residual Risk

Validator

— MINOR — maintainability — `src/backend/jit/llvm/llvmjit_inline.cpp:789`

— NIT — maintainability — `src/backend/jit/llvm/llvmjit_inline.cpp:809` — REFUTED

— NIT — docs — `src/backend/jit/llvm/llvmjit_inline.cpp:774` — REFUTED

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:782` — REFUTED

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:805` — REFUTED

— NIT — security — `src/backend/jit/llvm/llvmjit_inline.cpp:785` — REFUTED