diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b9bbd91f..3727fffa 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1380,6 +1380,66 @@ jobs: fi echo "integration-magma-split is red by design on ${marker}" + # ---- P5e (ra2), ID-134 / ID-82: THE CLIENT-ARRAY LANE, EXPECTED RED UNDER RUN-AHEAD ---- + # + # The two DirectGLES.Split.ClientVertexArrayScenario entries draw from a vertex array in + # the application's OWN memory. The server uploads those bytes by dereferencing a raw + # client pointer per draw, which is exactly the read rule F forbids once the client stops + # waiting - so ID-82 makes the draw a NAMED REFUSAL under run-ahead and gives the staging + # form to P8. Aborting is therefore the DESIGNED behaviour, and a lane that demanded these + # entries green would be demanding that the refusal not fire. + # + # THE LABEL, AND WHY IT IS NOT `integration-split-clientarrays` (ID-131): `ctest -L` is a + # REGEX. That spelling is matched by `-L integration-split` as a substring, so the entries + # would still be in the lane they were moved out of while every listing showed them + # renamed. Counted, not assumed: `-L integration-split` went 181 -> 179 and + # `-L integration-gpu` 1359 -> 1357 when this label landed. + # + # IT BRANCHES ON WHETHER RUN-AHEAD IS ACTUALLY ARMED, and that is not hedging - it is the + # only formulation that is a real assertion on BOTH heads. kMGPipeP5eRunAheadReady is a + # BUILD constant (MG_Backend/Init.cpp): on a head where it is false no environment can arm + # run-ahead, the refusal is unreachable, and these entries are green - so a step that + # simply demanded red would fail for the one reason that is not a defect. The client says + # which world it is in, in its own log line, and the step reads that and then demands the + # matching outcome. When the flip lands for good, delete the green branch. + - name: Integration-clientarrays-split - expected red under run-ahead (ID-82 / ID-134) + working-directory: build-split + env: + MOBILEGL_ITEST_REQUIRE_GPU: "1" + MOBILEGL_TRANSPORT: "inproc" + run: | + ulimit -c unlimited + logs=MobileGL/MG_IntegrationTest/split-logs + rm -f "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null || true + rc=0 + ctest -L integration-clientarrays-split --no-tests=error -j 1 \ + > "${RUNNER_TEMP}/clientarrays.txt" 2>&1 || rc=$? + tail -5 "${RUNNER_TEMP}/clientarrays.txt" + if ! grep -rqF "run-ahead ARMED" "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null; then + # kMGPipeP5eRunAheadReady is false on this head: the client is lockstep, vi's arm is + # alive and these entries are the positive control they were registered as. + if [ "${rc}" -ne 0 ]; then + echo "::error::integration-clientarrays-split is red WITHOUT run-ahead armed. Under lockstep the client-memory array path is legal and vi keeps it working, so this is a real regression in that arm and not ID-82's refusal." + exit 1 + fi + echo "integration-clientarrays-split is green: run-ahead is not armed on this build, so ID-82's refusal is unreachable" + exit 0 + fi + if [ "${rc}" -eq 0 ]; then + echo "::error::integration-clientarrays-split went GREEN with run-ahead ARMED. ID-82 refuses a client-memory vertex array under run-ahead by name, so a green run means the refusal did not fire and the server dereferenced a client pointer from an unbarriered record. If P8 has landed the staged form, delete this step and fold the entries back into integration-split." + exit 1 + fi + marker='Fatal{UnmigratedVerb, "' + if ! grep -rhoE 'Fatal\{UnmigratedVerb, "(Multi)?DrawArrays\+CLIENT_ARRAYS"\}' \ + "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null | sort -u | grep -q CLIENT_ARRAYS; then + echo "::error::integration-clientarrays-split is red but no private log carries ${marker}...+CLIENT_ARRAYS\"} - the lane failed for some OTHER reason, which is a defect rather than the expected-red control" + grep -rhoE '(Fatal|Admitted)\{[A-Za-z]*, "[^"]*"\}' "${logs}"/*ClientVertexArrayScenario*.log 2>/dev/null | sort | uniq -c | sort -rn | head + exit 1 + fi + echo "integration-clientarrays-split is red by design on:" + grep -rhoE 'Fatal\{UnmigratedVerb, "(Multi)?DrawArrays\+CLIENT_ARRAYS"\}' \ + "${logs}"/*ClientVertexArrayScenario*.log | sort -u + - name: Upload strict lane logs if: always() uses: actions/upload-artifact@v7 diff --git a/MobileGL/MG_Impl/Pipe/PipeFill.cpp b/MobileGL/MG_Impl/Pipe/PipeFill.cpp index 0c851f34..654eba0f 100644 --- a/MobileGL/MG_Impl/Pipe/PipeFill.cpp +++ b/MobileGL/MG_Impl/Pipe/PipeFill.cpp @@ -423,6 +423,27 @@ namespace MobileGL::MG_Pipe { static_assert(MGPipeVerbForWireOp(MGPWireOp::DrawVbo) == MGPipeVerb::DrawArrays, "the draw family's representative row moved; the fallback below names " "draw_vbo because that is the record every kDraw verb emits"); + // ---- P5e (ra2), ID-133: AND THE CLIENT'S HALF OF ESCALATION (iii) -------------- + // + // The server escalates a PLAIN multi-draw record (`NumDraws > 1` and not indirect) + // because Espryt's indirect tiers still pull the client's GL_DRAW_INDIRECT_BUFFER + // binding at apply. The fill decision is asked HERE, before the record exists, so + // it cannot read NumDraws - it asks the verb instead, and the three verbs below are + // exactly the ones whose record can carry NumDraws > 1 without kDrawIsIndirect. + // + // THE DIRECTION OF THE INEQUALITY IS THE SAFETY ARGUMENT, and it is worth stating + // because the two clauses are spelled differently. The client's set must CONTAIN + // the server's: a record the server calls barriered whose fields the client did not + // fill would be an ADMITTED pull of a stale value - legal-looking and wrong, which + // is worse than the abort. It does contain it, because `NumDraws > 1` can only come + // from a glMultiDraw* entry point, and the `MultiDraw*Indirect*` verbs set + // kDrawIsIndirect and are therefore NOT escalated on either side. The reverse + // inclusion is not needed: a verb the client fills for and the server then calls + // unbarriered costs a fill nobody reads, never a wrong answer. + const Bool plainMultiDraw = verb == MGPipeVerb::MultiDrawArrays || + verb == MGPipeVerb::MultiDrawElements || + verb == MGPipeVerb::MultiDrawElementsBaseVertex; + if (plainMultiDraw) return true; if (op == MGPWireOp::kOpCount && kMGPipeVerbClass[static_cast(verb)] == MGPipeVerbClass::kDraw) { op = MGPWireOp::DrawVbo; diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 4d57f2de..87e49e2e 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -2192,13 +2192,34 @@ if (MOBILEGL_BUILD_DISAGGREGATED) # them by dereferencing a client pointer per draw - and it is therefore both the positive # control for vi's decision to KEEP that arm alive under lockstep and the only entry that # can reach the run-ahead refusal when ra publishes the caps bit. + # + # ---- P5e (ra2), ID-134: AND WITH THE CAPS BIT PUBLISHED IT IS A LANE OF ITS OWN ---------- + # + # "When ra publishes the caps bit" is now, and what these two entries reach is + # `Fatal{UnmigratedVerb, "DrawArrays+CLIENT_ARRAYS"}` / `"MultiDrawArrays+CLIENT_ARRAYS"` - + # ID-82's NAMED REFUSAL, working exactly as ruled. They cannot be green under run-ahead and + # they should not be: a lane that demanded them green would be demanding that the refusal + # not fire, which is the opposite of what vi registered them for. Staging the bytes is P8's. + # + # THE LABEL IS `integration-clientarrays-split` AND THE SPELLING IS THE POINT (ID-131, and + # gl's `integration-magma-split` above is the worked example): `ctest -L` takes a REGEX, so a + # label spelled `integration-split-clientarrays` is still MATCHED by `-L integration-split` + # and the entries would never have left the lane they were moved out of - a split that reads + # as done in every listing and is not. Counted rather than assumed: `ctest -N -L + # integration-split` is 179 after this change and was 181 before, and `-L integration-gpu` + # is 1357 against 1359. `integration-gpu` goes with it for the same reason `integration- + # magma-split` dropped it - a lane that must be green cannot contain an entry that must not. + # + # The two entries keep their private, distinct log paths (SplitLogPaths.cmake.in lists + # MGL_SPLIT_VI_CLIENT_ARRAY_TESTS by name), which is what lets the expected-red CI step + # assert the MARKER rather than merely tolerate a failure. gtest_discover_tests(MobileGLIntegrationTest TEST_PREFIX "DirectGLES.Split." TEST_LIST MGL_SPLIT_VI_CLIENT_ARRAY_TESTS TEST_FILTER "ClientVertexArrayScenario.*" DISCOVERY_TIMEOUT 30 PROPERTIES - LABELS "integration-gpu\;integration-split" + LABELS "integration-clientarrays-split" TIMEOUT ${MGL_ITEST_TIMEOUT} ENVIRONMENT "${MGL_ITEST_GLES_SPLIT_ENVIRONMENT}" ) diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 2fca9c03..5e957459 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -3015,6 +3015,33 @@ namespace MobileGL::MG_Pipe { if (op == MGPWireOp::DrawVbo && payload != nullptr) { const auto& draw = *static_cast(payload); if ((draw.Flags & static_cast(kDrawClientArrays)) != 0) return true; + // 4. Escalation (iii), ID-133: a PLAIN multi-draw - NumDraws > 1 and not indirect. + // Espryt's multi-draw tiers are chosen on the SERVER and per batch + // (MultiDraw.cpp's ResolveTierForBatch), and the two indirect tiers reach + // MultiDrawImpl::RunIndirect, whose apply still reads the client's + // GL_DRAW_INDIRECT_BUFFER binding slot (BoundDrawIndirectBufferId, :87). That + // is a BARRIER_PULLED field whose retiring phase is P8, so under run-ahead an + // unbarriered record reading it is `Fatal{UnmigratedPipeInput, + // "GetBufferBindingSlot@DrawArrays"}` - 18 lane entries on the two tier lanes. + // + // WHY THE KEY IS THE RECORD AND NOT THE TIER, which is the thing a reader will + // want to check: there is exactly ONE draw opcode. Every one of the twenty draw + // entry points collapses onto draw_vbo (MGPDrawInfo's header says so), and the + // tier is resolved on the server, per batch, from driver caps the client does + // not hold - so "escalate the indirect multi-draw op" has no op to name and no + // predicate both roles could compute. NumDraws > 1 is the narrowest wire fact + // that CONTAINS the reaching set: RunIndirect is reachable only from + // DrawElementsBatch, which only a multi-draw record enters. + // + // AND kDrawIsIndirect IS EXCLUDED, which is what keeps Sodium's draw call out of + // it: a genuinely indirect record carries its buffer as a handle in the second + // tail and its apply resolves from that (ResolveIndirectCommandBytes' split + // arm), so it pulls nothing and needs no wait. NumDraws is 0 there, so the test + // is written `> 1` and the flag test is belt to that brace - both are stated + // because the reason each holds is different. + if (draw.NumDraws > 1 && (draw.Flags & static_cast(kDrawIsIndirect)) == 0) { + return true; + } } return false; } diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index 502a9a44..3b95ca7d 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -916,10 +916,13 @@ namespace MobileGL::MG_Pipe { // 1. MGPipeWaitClassFor(op) != kWaitNone the static column (§2.2) // 2. kCtxVerb with an open transform-feedback span escalation (i): XFB stays lockstep // 3. draw_vbo carrying kDrawClientArrays escalation (ii): client vertex arrays + // 4. draw_vbo with NumDraws > 1 and NOT indirect escalation (iii): plain multi-draw // // NO OTHER RUNTIME ESCALATION EXISTS. Adding one is an integrator ruling and a row in the - // contract, not a condition somebody adds at a call site - because a fourth clause that only - // one side computes is the same silent failure one level down. + // contract, not a condition somebody adds at a call site - because a fifth clause that only + // one side computes is the same silent failure one level down. (iii) is ID-133's, and + // ID-118's precedent is the one it follows: move the record into a barriered class rather + // than special-case the field its apply still pulls. // // Escalation (ii) is a REFUSAL under run-ahead (§5.1), so on a run-ahead server it never // reaches the sink at all; it is here so the predicate is TOTAL and so the lockstep arm, diff --git a/MobileGL/MG_Remote/CONTRACT-P5E.md b/MobileGL/MG_Remote/CONTRACT-P5E.md index 36a3dac0..48101830 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5E.md +++ b/MobileGL/MG_Remote/CONTRACT-P5E.md @@ -90,6 +90,8 @@ MGPipeBarriered(op, payload, st) = MGPipeWaitClassFor(op) != kWaitNone // static column, §2.2 || (MGPipeCallClassFor(op) == kCtxVerb && st.IsTransformFeedbackActive) // (i) XFB stays lockstep || (op == DrawVbo && (payload.Flags & kDrawClientArrays)) // (ii) client arrays + || (op == DrawVbo && payload.NumDraws > 1 + && !(payload.Flags & kDrawIsIndirect)) // (iii) plain multi-draw ``` `st.IsTransformFeedbackActive` is `MGPContextValues`' value (`MGPipeTypes.h:1698`): on the client it is `ctx.IsTransformFeedbackActive()` at emit, on the server the applied value — `set_context_values` @@ -103,6 +105,37 @@ must not be told a draw is unbarriered on that account. Escalation (ii) is a ref (§5.1), so on a run-ahead server it never reaches the sink; it is listed so the predicate is total. **No other runtime escalation exists**; adding one is an integrator ruling and a row here. +**Escalation (iii) is ID-133's, added by ra2, and it follows ID-118's precedent** — move the +record into a barriered class rather than special-case the field its apply still pulls. Espryt's +multi-draw tier is chosen on the SERVER and per batch (`MultiDraw.cpp`'s `ResolveTierForBatch`), +and the two indirect tiers reach `MultiDrawImpl::RunIndirect`, whose apply still reads the +client's `GL_DRAW_INDIRECT_BUFFER` binding slot (`BoundDrawIndirectBufferId`, `MultiDraw.cpp:87`) +— `GetBufferBindingSlot`, whose retiring phase is P8. Unbarriered, that is +`Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"}`: 18 lane entries on the two +tier-pinned lanes. + +**Why the key is the RECORD and not the tier.** There is exactly one draw opcode — all twenty +draw entry points collapse onto `draw_vbo` — and the tier is resolved on the server from driver +caps the client does not hold, so "escalate the indirect multi-draw op" has no op to name and no +predicate both roles could compute. `NumDraws > 1` is the narrowest wire fact that CONTAINS the +reaching set (`RunIndirect` is reachable only from `DrawElementsBatch`, which only a multi-draw +record enters). `kDrawIsIndirect` is excluded because a genuinely indirect record carries its +buffer as a handle in the second tail and resolves from that, pulling nothing. **The cost is +therefore NOT confined to the opt-in tier lanes**: a plain `glMultiDraw*` now waits on every tier, +including the `auto` → `ext` default. That is a real charge against §0's goal and the device arm +owns measuring it. **It can be withdrawn outright** once `BoundDrawIndirectBufferId` takes the +handle arm its own neighbour `ResolveBoundIndexBuffer` already has (three lines, `MultiDraw.cpp`, +mv's file per ID-113): the read is a SAVE/RESTORE of a GL binding name around the tier's scratch +buffer, not a data dependency, so the server can answer it from its own binding record and the +pull disappears rather than being legalised. + +The client's half is asked at the validate point, before the record exists, so it cannot read +`NumDraws`; it asks the verb, and `{MultiDrawArrays, MultiDrawElements, MultiDrawElementsBaseVertex}` +is exactly the set whose record can carry `NumDraws > 1` without `kDrawIsIndirect`. **The +inclusion must go that way round**: a record the server calls barriered whose fields the client +did not fill would be an ADMITTED pull of a stale value, which is worse than the abort; a verb the +client fills for and the server then calls unbarriered costs only a fill nobody reads. + ### 2.2 The static column, row by row | class | rows | why | @@ -212,6 +245,39 @@ UNBARRIERED apply is a finding: `MGLOG_E_ONCE` with the kind, Fatal under strict outside a barriered fill is `Fatal{RoleViolation, "gPipeInputs"}` by name. `Fatal{BarrierViolation}` (`:840`) stays for `BatchWaits==0 && !RunAhead`. + **AMENDED by ra2 (ID-132, and it is the sentence this section got wrong).** "Outside a + barriered fill" was the whole test, and a barriered fill was exempted on the strength of what + the caller SAID about itself: *"this touch is the residual fill of a record this thread is + about to park behind"*. That is a claim about the **future**, and the write is in the + **present** — the order at the validate point is fill, then emit, then park, so at the instant + of the write the apply thread is still draining the unbarriered records the client ran ahead + of. **A guard whose exemption cannot be false on the class it exists for is not a guard.** It + never fired on any of the 70 red lane entries; what fired instead was the apply thread, on + `Fatal{UnmigratedPipeInput, "@"}`, after the GL thread had bumped + `CurrentVerbSerial`, withdrawn `m_serverStampedVerb` and renamed `m_currentVerb` underneath a + record it was inside. + + **The rule as landed:** under `RunAheadArmed` a GL-thread write to the block is legal only + while `ApplyThreadIsInsideApplier()` is false, barriered fill or not; and the three fill sites + (`MGPipeValidateForVerb` phase 1, its step-4 residual walk, `MGPipeNoteFrontendMutation`) + establish that by taking §2.5's forced wait first (`QuiesceApplierBeforeFill`, + `PipeFill.cpp`). **Two waits per filling verb, not one**, because the block is written in two + phases that straddle publication — the serial bump / stamp withdrawal / verb rename before the + emitters, the 63-field walk after them — and the records those emitters published are records + whose apply reads the block. The red-once is deleting a wait; the guard then aborts by name on + the first filling verb behind a run-ahead backlog. Pinned by + `RemoteGuards.ClientBarrieredFillWhileTheApplierIsInsideUnderRunAheadIsFatalByName` with its + green control beside it; both run in the `unit` lane, because the strict lane runs under + lockstep and cannot see this class by construction. + + The same correction applies to **§3.2 above, which is UNLANDED and was never marked as such**: + "the server's stamp is server-private … never into the shared block" describes storage the + applier owns, and `MGPipeServerStampVerbBoundary` still writes `m_filled` / `m_currentVerb` / + `m_serverStampedVerb` into `gPipeInputs` itself. That gap is exactly what made the race + possible, and splitting the stamp is the P11 item — but note that per-role stamps **without** + versioning the BARRIER_PULLED values would be worse than today, because it turns a loud + `Fatal{UnmigratedPipeInput}` into a silent stale read. Both, or the values retired first. + --- ## §4 id — handle-keyed twins