cl,ssa,runtime: safely clear dead local finalizer roots - #2036
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
831cf7f to
05dc73a
Compare
d877a90 to
4a23ec7
Compare
4a23ec7 to
4d535b8
Compare
cpunion
left a comment
There was a problem hiding this comment.
Need full explaination in PR body about how this PR works
3b06973 to
6530a45
Compare
There was a problem hiding this comment.
Review: safely clear dead local finalizer roots
The revised design is a substantial safety improvement over the earlier whole-stack-scan / register-clobber / late-argument approach. The pass is now deliberately narrow and fails closed on every uncertain shape (control flow, Phi, defer, goroutine, closure, cross-block, cyclic block, heap escape). I verified the central safety premise empirically and it holds.
Verified safe (no action needed): The whole approach rests on shouldClearAlloc excluding v.Heap allocs so an address that escapes the frame is never cleared. I checked this against golang.org/x/tools/go/ssa directly: whenever a local's address escapes — stash(&box), takesPtrPtr(&box.p), Sink = &box, or a slice values[:] passed to a call — go/ssa marks the Alloc as Heap == true, so shouldClearAlloc rejects it. Only allocs whose sole non-local use is by value (a load) remain Heap == false, and clearing the slot after that load is safe. The mfinal.go eface handling, runtime_gc.go/bdwgc.go GC_clear_stack usage, and StoreVolatile are all correct.
The findings below are maintainability / documentation only. None are blocking.
Findings
-
cl/liveness_internal_test.go— vacuous "must not use" assertions.TestCompileConservativeLivenessDoesNotScanWholeStack(and the two otherfor _, helper := range []string{"llgo_clear_stack_ptr", "llgo_clobber_pointer_regs"}loops) assert that helper names from the removed design do not appear in the IR. Those symbols now exist nowhere in the repo, so these checks pass unconditionally and cannot ever catch a regression. Consider replacing them with positive assertions against the current volatile-store design (e.g. that astore volatile ... zeroinitializeris/ isn't emitted for the expected slots). See inline comment. -
cl/compile.go—blockIsCyclicrecomputed per allocation.collectStackClearPlanscallsblockIsCyclic(useBlk)for every candidateAlloc, and each call runs a fresh DFS (blockCanReach) over the CFG. Cyclicity is a per-block property independent of the alloc, so this is O(allocs × (V+E)) per function and multiple allocs in the same block recompute the identical result. Caching per block (map[*ssa.BasicBlock]bool) or computing cyclic blocks once makes it O(V+E). Low impact in practice since the pass only runs for packages that callSetFinalizer. See inline comment. -
runtime/internal/lib/runtime/runtime_gc.go— newClearStack(nil)calls undocumented. The twobdwgc.ClearStack(nil)calls added toGC()are the runtime-side counterpart to the compiler-side slot clearing (scrubbing stale conservative roots before each collection), but the adjacent comment only explains the extra collection cycle. A one-line note on the first call would keep a future reader from assuming it is redundant withGcollectand removing it. See inline comment. -
cl/compile.go—shouldClearAllocrelies onssa.Alloc.Commentstring literals ("varargs","makeslice") from x/tools SSA. These are load-bearing for correctness; a brief comment noting the source and why each is excluded would harden the guard against upstream drift. (Minor.)
|
@fennoai review again |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Review: conservative dead-local finalizer-root clearing
Reviewed the diff with four subagents (quality, performance, security, docs). The design is deliberately fail-closed and well-tested: every ambiguous case (cross-block use, Phi, Defer/Go/MakeClosure, cyclic blocks, heap/varargs/makeslice allocs) bails out and skips clearing, so a missed clear only retains a stale root. The StoreVolatile mechanism, the SetFinalizer package gate, and the block/last-use analysis are sound, and the internal + end-to-end regression tests map closely onto each fail-closed branch.
Findings below; the first is the one worth resolving before merge.
1. Correctness — the walk stops at pointer-into-memory sinks (Store/MapUpdate/Send) without following where the address flows. lastUseInBlock transitively follows referrers that are themselves ssa.Values, but a non-Value referrer that consumes a slot-derived pointer as its value operand is recorded as the last use and terminates the chain. If the address of the slot (or a FieldAddr/IndexAddr into it) is stored into another non-escaping stack local in the same block and later loaded and dereferenced, that read is not a referrer of the alloc — so the slot is treated as dead and cleared before the aliased read. This is only safe as long as go/ssa marks such an alloc Heap==true. Heap is not defined as a full escape analysis, so the safety of this optimization rests on an implicit, load-bearing invariant. Consider failing closed (return nil, false) when a slot-derived pointer reaches a Store/MapUpdate/Send as the stored/sent value, and adding a regression case for stack-to-stack address aliasing. See inline comments.
2. Performance — runtime.GC() now unconditionally runs two full collection cycles plus two GC_clear_stack calls, roughly doubling the cost of every explicit GC() even when no finalizers/weak cleanup are pending. Consider gating the second cycle on whether the first actually enqueued finalizer/cleanup work. See inline comment.
3. Minor.
cl/compile.go— the*ssa.Phicase can be merged into the preceding*ssa.Defer, *ssa.Go, *ssa.MakeClosurecase so the "merge/escape boundaries fail closed" set reads as one list.cl/compile.go—instructionUsesValuecallsinstr.Operands(nil), which allocates a fresh slice per referrer on a compile hot path; a reusable buffer would avoid the per-instruction garbage. Also worth a one-line comment that the re-scan is intentional fail-closed defense against malformed referrer lists so it isn't "optimized" away later.runtime/internal/lib/runtime/runtime_gc.go:39— "stale conservative stack roots" could read as clearing in-scope slots;GC_clear_stackscrubs the unused stack region below SP. Wording like "stale conservative pointers left in unused stack space" is more precise.
Non-blocking overall. Nice, careful change.
|
Follow-up to the latest review is in f479770. In addition to the inline fixes, operand re-scans now reuse one scratch buffer per liveness traversal and document why the re-scan is intentional. A full raw audit of |
There was a problem hiding this comment.
Review summary
Reviewed the conservative dead-local finalizer-root clearing pass (cl/compile.go), the StoreVolatile helper (ssa/memory.go), the BDWGC GC_clear_stack wiring (runtime_gc.go, bdwgc.go), and the tests/xfail updates.
The soundness-critical logic holds up well: the liveness proof is consistently fail-closed on Phi/Defer/Go/MakeClosure referrers, cross-block uses, cyclic blocks, address-retaining instructions (instructionRetainsAddress), and malformed/stale referrer lists (the deliberate Operands re-scan). The load-ends-provenance rule (UnOp MUL → slotAddress=false) is correctly scoped to loads only, so derived interior pointers stay tracked. GC_clear_stack(nil) matches BDWGC's public contract (the arg is a passthrough, never dereferenced) and cannot clear live frames. Test coverage is unusually thorough, including forcing Heap=false to prove safety does not depend on x/tools escape analysis.
No soundness or miscompilation defect was found. The findings below are maintainability / clarity notes (none blocking); inline comments mark the specific locations.
Additional observations without a precise diff anchor:
packageUsesRuntimeSetFinalizer/ method-set detection is name+path based; an indirectruntime.SetFinalizerreference (via a stored func value or unresolved interface method) would disable the pass for that package. This only causes a missed optimization (stale root retained), never a cleared live root — acceptable given the fail-closed intent, but worth noting as a completeness gap.- Minor per-alloc allocation:
shouldClearAlloccallshasConservativeGCPointers(..., map[types.Type]bool{}), allocating a fresh map per candidate alloc; a scratch/reused map or a shallow-type fast path would avoid it. Low impact. - PR-description nuance: the removed darwin/arm64
deferfin.goxfails were only the go1.24 and go1.25 versioned entries (no go1.26 darwin/arm64 entry ever existed), so the "passes on 1.24/1.25/1.26" claim holds only if go1.26 darwin/arm64 was already green before this PR.
|
Follow-up on the latest review summary: the PR body now states explicitly that the removed Darwin |
|
Follow-up CI fixes are in 70876a6, e4ee930, and 7b1cfbc. The macOS primary test shard was not failing assertions: three runs were canceled exactly at the old 30-minute job limit; one reached 191/193 packages, and the latest #2215 control kept emitting PASS through test/std/unsafe before cancellation. Only the single macOS test job now has a 45-minute timeout; Ubuntu remains at 30 minutes, and no shard or runner was added. The final #2036 run passed all 193 packages in 18m26s job time (16m19s in run llgo test), including std/weak and test/syncpool. The wide run-to-run variance confirms that 30 minutes had no reliable headroom, while 45 minutes keeps the one-runner layout robust. |
|
Final soundness follow-up is in fdce2ed. I checked all 37 x/tools v0.38.0 SSA instruction classes against the address-retention invariant; no additional production sink or blocking issue was found. The analysis now also fails closed when a referrer claims the right block but is no longer in that block’s instruction order, with a regression test. The comments/PR body now state the exact prerequisite: completeness comes from x/tools’ documented function-local Referrers/Operands inverse (validated by LLGo’s SanityCheckFunctions mode), while the defensive rescan rejects stale/foreign entries. GC_clear_stack wording is also narrowed to BDWGC’s actual guarantee of clearing only some inaccessible stack space. |
|
CI follow-up 7b1cfbc makes the ESP QEMU runtime dependency explicit on Ubuntu. The sole failure on fdce2ed occurred before build/tests: qemu-system-riscv32 could not load libslirp.so.0; the other three LLGo matrix jobs were only fail-fast cancellations. setup-embed-deps now installs libslirp0 alongside SDL instead of relying on the runner image to provide it. |
|
The failed Ubuntu |
|
@fennoai CI is now fully green on the latest head |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Re-review @ head 7b1cfbc1b
Re-ran all four subagents (quality, performance, security, docs) against the updated diff. This revision resolves the main finding from my previous review.
Prior blocker — fixed. The stack-to-stack address-aliasing hole (slot address flowing as a value into a Store/MapUpdate/Send/Call/Select-send without the walk following it) is now closed by instructionRetainsAddress plus the slotAddress flag threaded through lastUseInBlockValue. The security pass adversarially traced invoke-mode receivers, Go/Defer/MakeClosure/Phi (fail-closed referrers), MakeInterface/Convert→unsafe/Slice/FieldAddr/IndexAddr (followed as values with slotAddress preserved), load de-aliasing via UnOp{MUL}, zero-referrer values, and Return/Panic terminators — and could not construct a path that clears a live slot. My other minor items (Phi case merged, Operands scratch buffer, clearAlloc reusing p.bvals, and the runtime_gc.go comment) are all addressed too.
Remaining findings are minor / defensive; none blocking.
1. Defensive (worth considering) — instructionRetainsAddress fails open on unknown instructions. The switch is a hand-maintained mirror of the SSA instruction set with default → return false (non-retaining). Safety currently holds because every non-ssa.Value retaining instruction is either in this switch or in the fail-closed referrer switch. But if a future x/tools release adds a new non-Value side-effecting instruction — or adds a value operand to an existing one — this defaults to "does not retain" and the analysis silently fails open. Consider inverting to a whitelist of known non-retaining instructions with a fail-closed default, or a build-time assertion over the instruction set. The doc block documents the invariant but doesn't enforce it. See inline comment.
2. Minor (cleanup) — dead seen seeding in lastUseInBlock. The shim builds states from the incoming seen set (seeding both slotAddress variants per value), but every production and test caller passes an empty map, so the loop never runs. It's dead code, and the dual-seeding semantics would be questionable if exercised (pre-marking slotAddress:true would suppress a legitimate alias traversal). Suggest dropping the seen parameter and constructing the empty states map internally. See inline comment.
3. Minor (perf, one-time compile cost) — blockIsCyclic recomputes reachability per successor. It allocates a fresh seen-map and runs a full reachability DFS for each successor edge. It's memoized per block in collectStackClearPlans, so the cost is bounded and paid once per block with a clearable alloc, but a single CFG-wide back-edge/SCC pass would be cheaper on large dense functions. See inline comment.
4. Noted (intentional) — runtime.GC() still runs two unconditional collection cycles (two ClearStack+Gcollect+runFinalizers). This is a deliberate correctness choice for weak/unique finalizer ordering, so it's reasonable as-is; the second cycle could optionally be gated on whether the first actually ran any finalizers, since runtime.GC() is user-invoked and sometimes called in loops.
As requested, this is review-only — no merge.
|
@fennoai Latest head |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Re-review @ head 6710edbf1
Re-ran all four subagents against the delta since 7b1cfbc1b. All three of my prior follow-up threads are resolved, and I found no unsafe defect.
Threads addressed:
- Unknown SSA instructions now fail closed.
instructionRetainsAddressdefaults toreturn true(retaining), with an explicit whitelist of pure value-producing instructions returningfalse, and the destination-operand checks correctly inverted (Storeretains unlessvis onlyAddr;MapUpdateunless onlyMap;Sendunless onlyChan;Callunconditionally retains;Selectretains a send value but treats a channel-only operand as non-retaining). A new/unlisted instruction now fails closed. The security pass adversarially confirmed each whitelisted value-producer is genuinely non-retaining of a slot address (address operands only ever reachFieldAddr/IndexAddr/Slice/SliceToArrayPointer, which preserveslotAddress;Field/Index/Lookup/Range/Next/Extracttake value operands a slot address never reaches whileslotAddress==true), and the recursion still follows every whitelisted producer so a downstream retain is caught. - Dead
seenseeding removed.lastUseInBlockdropped the unused parameter and buildsstatesempty internally;stackLivenessStateis still live in the recursive walk (not dead). - Tarjan SCC replaces per-successor DFS.
cyclicBlockscomputes SCCs once per function in O(V+E). Verified it marks all members of multi-block SCCs cyclic and single-block self-loops, usesindices[succ](notlowlinks[succ]) in the on-stack branch — avoiding the classic false-negative that would be unsafe here — and guards nil blocks/successors.minis the Go 1.21+ builtin (repo is go 1.24). RemovedblockIsCyclic/blockCanReachare fully cleaned up, and tests cover the fail-closed default, multi-block SCC, and self-loop cases.
One new minor finding (performance, low): in collectStackClearPlans the per-block order map is rebuilt once per qualifying alloc rather than once per block — alloc.Block() is always the enclosing block, so a block with K clearable allocs rebuilds the identical map K times. Hoisting it to per-block (and lifting the cyclic check to the block level) removes the redundancy. Bounded impact; non-blocking. See inline comment.
Everything else was minor/documentation-level or informational (e.g. the Tarjan/liveness walks are recursive to CFG depth — acceptable for generated SSA, and unchanged in risk from before). Overall: the conservative liveness design is sound and the follow-ups meaningfully tightened it.
As requested, review-only — no merge.
|
@fennoai Latest head |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Final delta confirmed @ head 8fc1aa05c
Reviewed the delta since 6710edbf1 (one file, cl/compile.go, +10/-7 in collectStackClearPlans). It's exactly the hoist I suggested, and it's correct:
- Cyclic blocks skipped at the outer loop (
if blockCyclicity[blk] { continue }) instead of per-alloc. Equivalent to the oldblockCyclicity[useBlk]check because only allocs whoseBlock() == blkare processed. ordermap now block-scoped and built lazily once (var order …+if order == nil { … }), reused for every qualifying alloc in the block — removing the per-alloc rebuild.- New
useBlk != blkguard is a sound safety belt now thatorderis block-scoped: it ensures the block-local order map is only ever applied to allocs actually in that block. In go/ssa an alloc iterated fromblk.Instrsalways hasBlock() == blk, so this is a defensive no-op that also documents the invariant.
No behavioral change to the liveness result — purely the intended compile-time cleanup. No new findings.
With this, all findings from my earlier reviews are resolved: the stack-to-stack aliasing hole (closed via instructionRetainsAddress + slotAddress), the fail-closed default for unknown SSA instructions, the removed dead seen seeding, the Tarjan SCC cyclicity pass, and now the per-block order-map reuse. The conservative dead-local finalizer-root clearing looks sound and well-tested from my perspective.
As requested, review-only — no merge.
Re-expresses xgo-dev#1906 on the xgo-dev#2023 base (its remaining ~11k diff lines were the pre-xgo-dev#2012 funcinfo draft, superseded by the stage-5 chain): - cl gains a liveness analysis for stack-allocated objects: allocas whose last use has passed are cleared so bdwgc's conservative stack scan stops keeping dead stack objects (and what they point to) alive; pointer registers are clobbered around the trigger points (llgo_clobber_pointer_regs) and dead stack slots holding the target are zeroed (llgo_clear_stack_ptr, pthread stack-bounds walk). - runtime.SetFinalizer paths (mfinal, runtime_gc, bdwgc binding) hook the cleared-slot machinery so finalizers for dead stack objects run. - xfail: retire deferfin.go, stackobj.go, stackobj3.go, validated on darwin/arm64 go1.24 + go1.26 (stackobj2 already passed). Carries the xgo-dev#2035 shared-GOCACHE commit temporarily (same patch-id, auto-dedups when the chain rebases after xgo-dev#2035 merges). Supersedes xgo-dev#1906.
8fc1aa0 to
af71577
Compare
|
Rebased the functional series onto current The only rebase-sensitive test expected an older fully-qualified main-package IR type name; it now checks the current |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
No main baseline exists yet; all metrics are marked Warning
|
|
Grouped the remaining related GOROOT run xfails against latest
These are one broader precise-liveness boundary, not six unrelated runtime bugs. I also tested the tempting They should not be folded into this PRs local-clear implementation. This pass intentionally requires an exact non-escaping A general fix needs PC-specific pointer liveness for LLGo frames and registers (plus cross-frame stack-object tracing), so the current xfails remain valid. This comment records the shared root and avoids opening duplicate PRs for the individual cases. |
Problem
BDWGC conservatively scans native stacks. Pointer-shaped data left in a dead
LLGo stack allocation can therefore keep an otherwise unreachable object alive
and delay its finalizer.
Safety review and design
An earlier revision tried to remove roots by scanning an entire pthread stack,
clobbering registers and parameters, and rematerializing
SetFinalizerarguments late. Review counterexamples showed that those mechanisms could
rewrite live aliases and
uintptrvalues, clear escaping allocations, breakloop/defer/goroutine/closure semantics, and change normal Go evaluation order.
Those mechanisms are removed. The implementation is deliberately narrow and
fail-closed:
runtime.SetFinalizeror LLGo's alternate runtime implementation. Realmodule package paths, unexported methods, generic method origins, closures,
and instantiated generic functions are covered; C wrapper lowering is
excluded.
ssa.Allocvalues whose element typecontains conservative GC pointers. Heap allocations and synthetic
varargs/makeslice slots are excluded.
(the documented x/tools
Referrers/Operandsinverse, checked by LLGo'sSanityCheckFunctionsbuild mode) to remain in one acyclic basic block.Operand lists and block scheduling are re-checked to reject stale or
foreign referrer entries.
(SSA value, still aliases original stack allocation)provenance.A load ends address provenance. Phi, defer, goroutine, closure, cross-block,
malformed, and unsupported paths fail closed. A still-slot-derived address
reaching
Store,MapUpdate,Send,Call, or a select send also failsclosed because later memory/callee aliases are not represented by the
referrer graph.
final SSA use. No surrounding stack words, independent aliases, parameters,
or registers are touched.
GC_clear_stack(nil)before each existing explicitcollection cycle as best-effort cleanup of the current thread's unused stack
space. Correctness does not depend on it. The two collection cycles already
exist on
main; this PR does not add another cycle.Normal SSA instruction order is preserved. Late
SetFinalizerlowering,dynamic load-and-clear fusion, whole-stack scanning, and register clobbering
are gone.
Proven improvement
The positive regression uses a dynamically indexed
[8]*HeapObjectstackallocation. Separate write/read parameters prevent store/load forwarding while
the caller supplies the same valid runtime index. Disassembly confirms that
mainleaves the target pointer in the native stack allocation, whereas thisbranch emits eight volatile zero stores after
SetFinalizerand before GC.mainfails 20/20; this branch passes 20/20.mainfails 10/10; this branch passes 10/10.This establishes the intended scope: an exact, non-escaping, pointer-bearing
local whose complete use graph stays in one acyclic block and whose stale
pointer is actually resident in native stack memory.
Remaining GOROOT stack-object boundaries
Go 1.26 has three
stackobj*.gotests. Raw runs against currentmainand thisbranch show that this PR does not make broad stack-object claims:
stackobj.gomainand PR collect at phase 3; phase 1 is expectedf'sStkObjis an SSA heap allocation,ghas only a parameter, and pointer copies remain in active caller/callee frames and a callee-saved register during GC. Clearing before the call would break the validKeepAlivepath; clearing after return misses the GC insideg.stackobj2.goSetFinalizer, so this pass is disabled. Its recursive address-takena/bvalues are SSA heap allocations. This is a non-regression result, not evidence of precise live stack-object tracing.stackobj3.gofTrue; isolatedfTrueandfFalseretain the finalizer target through all three GCs (c=-1, n=3)f. Clearing the caller afterfreturns is too late; unconditional early clearing would break the true path.These same two failure mechanisms account for the remaining Darwin entries and
Linux Go 1.24/1.25/1.26 xfails; they are repeated version/platform instances,
not additional stack-object failure classes.
The related
deferfin.goboundary is different:mainalready passes raw with Go 1.24, 1.25, and 1.26 (including50/50 repeated Go 1.24 runs). The stale versioned Go 1.24/1.25 Darwin xfails
are removed (there was no Go 1.26 Darwin entry), but that result is not
attributed to this PR.
exactly one of ten finalizers pending. The captured value is an SSA heap
allocation behind
MakeClosure/Defer. The failure shape is consistent withthe last worker pthread signaling completion before its BDWGC stack/register
roots are unregistered.
GC_clear_stack(nil)affects only the GC-callingthread's unused stack space.
Go solves these broader cases with PC-specific argument/local pointer maps plus
stack-object metadata and a runtime mini-GC over address-taken stack objects.
LLGo/BDWGC currently lacks equivalent typed frame/register metadata. A complete
fix therefore needs precise safepoint maps and cross-frame stack-object tracing,
or reliable backend-level spill/register clearing;
deferfinadditionallyneeds pthread completion coordinated with BDWGC thread deregistration. Those
are separate designs and are intentionally not approximated here.
One additional
stackobj3.goobservation is that LLGo/LLVM eliminates theempty
//go:noinline usefunction used by the upstream test as an orderingbarrier. That is a separate Go-compatibility issue, not changed in this PR.
Regression coverage
Compiler tests cover the review counterexamples and deliberately force current
x/tools heap-promoted allocations back to
Heap=falseto prove safety does notdepend on that implementation detail:
&box.pstored into another stack local and read later;Store/MapUpdate/Send/Call/select-send operand classification;goroutine, closure, heap, varargs, makeslice, package gating, and
whole-aggregate volatile stores.
The end-to-end probe builds one real module with host Go and LLGo, then runs ten
isolated liveness cases: loop, closure, global escape, defer, independent alias,
goroutine, live
uintptr, argument evaluation order, genuine same-blockfinalization, and stored stack-to-stack alias.
Validation
main(551626a1de6b6c47a4dcfad7190b074ae9b0d809, merge of cmd: add LLDB runtime views for strings and slices #2240);instructionRetainsAddressandlastUseInBlockare 100% covered,cyclicBlocks95.7%,lastUseInBlockValue94.7%, andcollectStackClearPlans95.2%;test/gofinalizer test selection passes, including every review counterexample and the end-to-end liveness cases;deferfin.gopass with an empty xfail configuration; Go 1.26.5deferfin.goalso passes;stackobj.goandstackobj3.gostill fail with the two documented precise-liveness boundaries, confirming the PR does not overclaim their coverage;git diff --checkpasses.Supersedes #1906.
Latest-main boundary audit
Rechecked on
2310ff87f1cfa57dda5d4736f98b34ee679bca6dwith Go 1.26.5/Linux amd64:fixedbugs/issue15277.gostill retains about 10 MiB after the dropped value (after drop ... 10489856). The same failure reproduces on this PR.This PR deliberately handles dead local slots that can be cleared at safe points. It does not solve roots retained in callee-saved registers/backend spills, values crossing channel calls, fault/defer cross-frame liveness, or stale slice/string/interface copies. Those require a broader precise safepoint/stack-object or backend register-spill strategy; treating
runtime.MemStatsspecially would only hide the underlying correctness issue. The remaining stack-object/finalizer cases should therefore be grouped by those mechanisms in follow-up work rather than folded into this PR.