GeezerVison

@claude review

Claude finished @carlos-granados's task in 3m 5s —— View job

Claude PR Review

• Gather context from PR
• Read changed files (src/base/base.c, src/lib/lib.c, src/lib/lib.h, 3 new phpts + 1 inc)
• Trace call sites (xdebug_base_rinit_if_enabled → only called on real debug/on_demand=1 paths)
• Verify zend_alter_ini_entry semantics at STAGE_RUNTIME and interaction with opcache's OnUpdateEnable
• Cross-check xdebug_break() / xdebug_connect_to_client() gating vs the new hook
• Post review

Summary

The fix is well-scoped and correctly addresses a real correctness bug: opcache SHM being shared between instrumented and non-instrumented requests silently causes missed breakpoints (poisoning direction 1) and perpetual EXT_STMT overhead after a debug session ends (poisoning direction 2). The mechanism — flipping opcache.enable off at STAGE_RUNTIME — is the right lever, because it makes opcache's persistent_compile_file delegate straight to the original compiler, bypassing both SHM and the file cache, and PHP's INI modified-list restores the value at request shutdown so the toggle is truly per-request. The block comment in src/lib/lib.c:146-153 clearly documents both the why and the mechanism, which will age well.

The gating in xdebug.c:612-640 is also correct: xdebug_base_rinit_if_enabled() is only reached when (a) a debug session was requested and a client actually connected, or (b) on_demand_debugging_enabled=1. The near-zero-overhead path (start_with_request=no, on_demand=0, or a trigger present but no listener) still returns before the new call, so a "quiet" FPM worker keeps opcache enabled. Test 002 pins that behavior down.

Overall: correctness-focused fix, sensible tests. Approving in spirit — a couple of small notes below, none blocking.

Notes / questions

1. on_demand_debugging_enabled=1 in production FPM — worth calling out louder (src/base/base.c:1039)
When on_demand=1 is set to allow ad-hoc xdebug_break()/xdebug_connect_to_client(), every request in the worker now pays "no opcache" (and, implicitly, "no JIT") — not just requests that end up debugging. On a busy PHP-FPM server this is a meaningful throughput cliff. The PR body acknowledges this ("Another reason to avoid this mode if possible"), but nothing in the code, INI docs, or xdebug_info diagnosis output warns the operator. Worth at least a xdebug_log_ex(..., XLOG_INFO, ...) when we disable opcache due to on-demand mode with no client connected, so it shows up in the log and helps triage "why is my server suddenly slower".

2. Ordering of the two opcache disables (src/base/base.c:1038-1039)
xdebug_disable_opcache_optimizer() runs first (writing opcache.optimization_level=0 with STAGE_STARTUP), then xdebug_disable_opcache_for_request() turns opcache off entirely. Once opcache.enable=0, the optimizer level is moot for this request. Not a bug (writing an INI entry that's about to become irrelevant is cheap), but functionally the optimizer disable is dead code on the debugging path — only the non-debugging path (where xdebug_base_rinit_if_enabled is skipped entirely) benefits from it. If that's intentional (belt-and-braces), a one-line comment would help future readers.

3. zend_alter_ini_entry return value ignored (src/lib/lib.c:159)
If opcache isn't loaded, zend_alter_ini_entry silently returns FAILURE. That's fine — no-op is the right behavior — but consider a xdebug_log_ex(XLOG_CHAN_CONFIG, XLOG_DEBUG, ...) when the call fails, so someone diagnosing "why is my breakpoint not firing" has a breadcrumb. Same latent issue exists in xdebug_disable_opcache_optimizer, so this is pre-existing and low priority.

4. Documentation surface (docs / changelog / xdebug_info)
This is a visible behavior change (opcache toggling on debug requests, potentially JIT off too). Users have no way to discover it except by reading the source. Worth a note in the changelog and, ideally, a line in the debug-mode section of the docs. xdebug_info() diagnostics could also mention "opcache disabled for this request" when applicable — makes the "why is opcache off?" question self-answering.

5. Test coverage gap: mid-request xdebug_break()/xdebug_connect_to_client() under on_demand=1
Tests 001–003 cover the three main static paths well. There's no test that exercises "on_demand=1, request starts with opcache disabled (per this fix), then connects mid-request via xdebug_break() and successfully hits a breakpoint in a file that opcache would have cached from a prior request." That's exactly the scenario the fix protects against. A test that pre-warms opcache with a helper request (or uses opcache_compile_file() in one process, then debugs in another) would nail down the intended guarantee. Might not be feasible within the current phpt harness — worth a quick think, not a hard requirement.

6. Nit: test 001 uses --EXPECTF-- with no format specifiers
--EXPECT-- would be equivalent and slightly stricter. Consistent with other DBGp tests in the repo, so this is only worth changing if you want to tighten it.

Correctness of the mechanism (verified against opcache source semantics)

• opcache.enable is PHP_INI_ALL, so ZEND_INI_USER/STAGE_RUNTIME is accepted by opcache's OnUpdateEnable, which sets ZCG(enabled) = 0 and returns SUCCESS.
• With ZCG(enabled) = 0, persistent_compile_file short-circuits to accelerator_orig_compile_file, so SHM lookups, SHM stores, and file-cache reads/writes are all bypassed for the rest of the request.
• JIT is implicitly disabled too (JIT operates on cached scripts), which is what we want — a JIT-compiled trampoline would skip our EXT_STMT observer.
• PHP's INI modified-list restores the prior value at request deactivation, so the next request on the same FPM worker is independent. ✓

Nothing scary in the ZTS story either — the modified list is per-request/per-thread.

Nice fix. The core logic is right; the notes above are polish, not blockers.
• Branch: fix/opcache-poisoning

@claude do another review

Claude finished @carlos-granados's task in 3m 57s —— View job

Claude PR Review (round 2)

• Pull latest branch state, diff vs origin/main
• Re-read changed C files (src/base/base.c, src/lib/lib.c/h, src/lib/log.c, src/debugger/com.c, xdebug.c, src/base/base_globals.h)
• Read new tests (opcache-bypass-request-001..008) + README changes
• Verify the fix still holds, evaluate delta since round 1, look for anything new
• Post review

Round-1 follow-up: all five points addressed

Also removed warn_if_opcache_is_loaded_after_xdebug() and is_opcache_enabled() from src/debugger/com.c — that warning existed because opcache caching did need to happen after Xdebug had installed its extended-info opcode; now that we simply bypass opcache for instrumented requests, load order no longer matters, so the diagnostic is correctly retired.

The core comment in src/lib/lib.c:135-146 is worth calling out — it captures the why (per-request instrumentation vs. cross-request opcache), the mechanism (OnUpdateEnable at STAGE_RUNTIME turns the accelerator off including the file cache; INI system restores on shutdown), and the fact that this incidentally covers the optimizer too. And the ZEND_INI_SYSTEM-vs-ZEND_INI_USER comment at src/lib/lib.c:154-157 is a genuine subtle-config gotcha — php_admin_value[opcache.enable] in an FPM pool would silently defeat a USER-level change. Good catch to encode.

Verified mechanism once more:

• opcache.enable is PHP_INI_ALL, so ZEND_INI_SYSTEM at STAGE_RUNTIME passes the permission check; OnUpdateEnable sets ZCG(enabled) = 0 and persistent_compile_file short-circuits to accelerator_orig_compile_file — SHM lookup/store and file-cache lookup/store are all bypassed for the rest of the request. JIT is implicitly disabled (JIT is part of opcache).
• XG_BASE(opcache_disabled) is reset in xdebug_base_rinit() (always called for non-off modes) and set in xdebug_base_rinit_if_enabled() (instrumented paths only), so xdebug_info() reflects the state that actually applied to this request.

New / remaining notes (all polish, none blocking)

1. OPCACHE-OD WARN fires on every request — xdebug.c:643-646
Because on-demand mode reaches this branch on every non-debugged request, a busy FPM worker with xdebug.log set gets one WARN per request forever, not one per process. That's a lot of log noise for what is really a static configuration statement. Consider a per-process logged_once flag so the message appears at the top of the log and once per worker restart. Also, XLOG_INFO would arguably fit better than XLOG_WARN — nothing is broken, this is informational advice about a config choice the user already made — but WARN is defensible if the intent is "make sure it's visible at default log levels" (default is XLOG_INFO=7, so both are visible; changing to INFO wouldn't hide it).

2. Test 004 is fragile against unrelated INFO lines — tests/base/opcache-bypass-request-004.phpt
The test asserts exactly two lines with --EXPECTF--:

[%d] Log opened at %s
[%d] [Config] WARN: OPcache is bypassed …

log_level=7 (INFO) also emits [Config] INFO: PRIVTMP … when systemd private-tmp is detected (src/base/base.c:1032), and [Config] INFO: CTRL-OK … when the control socket is set up (src/base/ctrl_socket.c:478) — both fire during RINIT before your on-demand branch runs. Neither happens in this specific CLI test env today, but a future run in a systemd/container CI could break the test. Two low-effort fixes:

• Drop to xdebug.log_level=3 (WARN) — the WARN line is the only one you actually care about matching.
• Or use %A between the two expected lines to allow arbitrary intervening output.

3. Test 007/008 shell redirection isn't portable — tests/base/opcache-bypass-request-007.phpt:22, opcache-bypass-request-008.phpt:22
... > /dev/null 2>&1 is Unix-only; on Windows the redirect + /dev/null both fail. Existing tests use substr(PHP_OS, 0, 3) == 'WIN' ? 'NUL' : '/dev/null' (see tests/base/bug00665.phpt:16). Either follow that pattern or add --SKIPIF-- linux|osx gating; otherwise the tests will fail on Windows once they get exercised there.

4. xdebug_info() HTML row misses a docs link — src/lib/log.c:633
Every other row uses <td class="d"><a href="…"> for the docs icon; the new OPcache row uses  . Fine if there's no docs anchor for this yet; if you add one to the "OPcache" section of the docs, linking here (or reusing #step_debug) would be consistent.

5. xdebug_disable_opcache_for_request() returns int for a boolean — src/lib/lib.h:227, src/lib/lib.c:145
Assigned into zend_bool XG_BASE(opcache_disabled) — the implicit int → zend_bool narrowing works but a bool return would document the contract and match the field. One-liner.

6. Nit: field naming — src/base/base_globals.h:57
opcache_disabled reads like "opcache is (permanently) disabled". opcache_bypassed_for_request matches both the user-facing string and the actual scope (per-request bypass, INI-modified-list restores it). Cosmetic.

Verdict

Round 1's items are all resolved cleanly and the mechanism is provably correct. The two new tests (007, 008) are the most important addition — they close the loop by actually round-tripping through a warm file cache. Everything above is polish and can go in a follow-up if you'd rather ship this now.
• Branch: fix/opcache-poisoning

@claude I decided not to do number 4, check all the other changes

Claude finished @carlos-granados's task in 2m 30s —— View job

Claude PR Review (round 3)

• Pull latest branch state, verify what's changed since round 2
• Re-check items 1, 2, 3, 5, 6 from round-2 review (skipping Phase 2: RINIT fast-path & deferred hooks — 630% → 10% overhead #4 per author's request)
• Look for anything new introduced since round 2
• Post review

Round-2 follow-ups (excluding #4)

Verification of the delta

Grepped for the old field name — no stragglers:

src/base/base_globals.h:57:    zend_bool     opcache_disabled_for_request;
src/lib/log.c:633:            if (XG_BASE(opcache_disabled_for_request)) {
src/lib/log.c:703:            if (XG_BASE(opcache_disabled_for_request)) {
src/base/base.c:978:          XG_BASE(opcache_disabled_for_request) = false;
src/base/base.c:1039:         XG_BASE(opcache_disabled_for_request) = xdebug_disable_opcache_for_request();
xdebug.c:364:                 xg->opcache_disabled_for_request = false;

Default xdebug.log_level is XLOG_DEFAULT = "7" (src/lib/log.h:36), and XLOG_INFO = 7, so INFO is visible at the default level — the user-facing signal is preserved by the WARN→INFO change; it just stops using WARN for something that isn't broken. Good call.

The mechanism itself is unchanged and still correct: opcache.enable set to 0 at STAGE_RUNTIME with ZEND_INI_SYSTEM flips ZCG(enabled) off (bypassing SHM + file cache + JIT), and PHP's INI modified-list restores it at request shutdown.

One small note on item 1

The severity is now right, but the volume concern from round 2 is unchanged: on a busy FPM worker with xdebug.log pointed at a file, this INFO line still fires once per request forever, not once per worker. If you want to close that loop later, a per-process logged_once flag around the xdebug_log_ex call (reset in MINIT, set on first emit) would give you "log at worker start, silent thereafter". Not a blocker — happy either way, just flagging that the deduplication half of the note wasn't picked up. Fix this →

Verdict

All non-skipped round-2 items are addressed cleanly. Nothing new introduced. Ship it.
• fix/opcache-poisoning