[MG_Pipe, MG_Impl, MG_IntegrationTest, CI, Docs] (Disaggregated): P5e ra2 - ID-133's escalation, ID-134's lane, and the contract sentence that could not be checked

- ID-133: escalation (iii) in MGPipeBarriered - a draw_vbo with NumDraws > 1 that is NOT
  kDrawIsIndirect is barriered. Espryt's indirect multi-draw tiers reach
  MultiDrawImpl::RunIndirect, whose apply reads the client's GL_DRAW_INDIRECT_BUFFER binding
  (BoundDrawIndirectBufferId, MultiDraw.cpp:87) - GetBufferBindingSlot, retiring in P8 - and
  unbarriered that is Fatal{UnmigratedPipeInput, "GetBufferBindingSlot@DrawArrays"} on 18 lane
  entries. Backtrace of the aborting thread, verbatim frames: CountBarrierPull <-
  MultiDrawImpl::RunIndirect <- MultiDrawImpl::DrawElementsBatch <- ServerVerbSink::OnDrawVbo
  <- PipeWireDecoder::DecodeAndApply <- PipeApplier::ApplyOne.
- TWO PREMISE CORRECTIONS, both measured, both in the report and in the contract. (a) There is
  no indirect multi-draw OP: all twenty draw entry points collapse onto draw_vbo, and the tier
  is resolved on the server per batch (ResolveTierForBatch), so the escalation cannot be keyed
  on the tier and its cost is NOT confined to the opt-in tier lanes - a plain glMultiDraw* now
  waits on the auto -> ext default too. NumDraws > 1 is the narrowest wire fact that contains
  the reaching set; kDrawIsIndirect is excluded so Sodium's indirect draw, which resolves by
  handle and pulls nothing, keeps running ahead. (b) The read is a SAVE/RESTORE of a GL binding
  name around the tier's own scratch buffer, not a data dependency - so it can be retired
  outright by giving BoundDrawIndirectBufferId the handle arm its neighbour
  ResolveBoundIndexBuffer already has, and this escalation withdrawn. That is three lines in
  MultiDraw.cpp, which ID-113 gives to mv, so it is reported rather than edited here.
- The client's half is verb-keyed because the fill decision is asked before the record exists:
  {MultiDrawArrays, MultiDrawElements, MultiDrawElementsBaseVertex} is exactly the set whose
  record can carry NumDraws > 1 without kDrawIsIndirect. The inclusion goes that way round on
  purpose - 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.
- ID-134: the two ClientVertexArrayScenario entries get the label integration-clientarrays-split
  and an expected-red CI step that asserts the MARKER, shaped like gl's Magma lane. ID-131 heeded
  by COUNTING, not by assuming: -L integration-split 181 -> 179, -L integration-gpu 1359 -> 1357,
  the new label selects exactly 2. The step branches on whether run-ahead is actually ARMED,
  because kMGPipeP5eRunAheadReady is a BUILD constant: on a head where it is false the refusal is
  unreachable and the entries are legitimately green, so an unconditional demand for 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; delete the green branch when the flip lands for good.
- CONTRACT-P5E §3.5 amended to the code: "outside a barriered fill" rested on the caller's claim
  about the FUTURE, and 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 entries. The landed rule is "not while the applier
  is inside, barriered fill or not", with the fill sites taking §2.5's forced wait first, twice
  per filling verb because the block is written either side of the emitters.
- AND §3.2 IS MARKED UNLANDED, which nobody had noticed: "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 what made the race possible. Splitting the stamp is the P11
  item - but per-role stamps WITHOUT versioning the BARRIER_PULLED values would be worse than
  today, because it turns a loud Fatal into a silent stale read.
- §2.1 carries escalation (iii) as a row, per its own rule that a new escalation is a ruling and
  a row in the contract rather than a condition at a call site.
- Gate on the flipped head: unit 2256/2256, isplit 179/179 HARD GREEN, gens rc=0, doc citations
  rc=0 (the three remaining are CONTRACT-P5.md's and pre-date this branch).
This commit is contained in:
2026-09-18 13:56:51 -04:00
parent 2d03565f87
commit f286548010
6 changed files with 201 additions and 3 deletions
+60
View File
@@ -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
+21
View File
@@ -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<SizeT>(verb)] == MGPipeVerbClass::kDraw) {
op = MGPWireOp::DrawVbo;
+22 -1
View File
@@ -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}"
)
+27
View File
@@ -3015,6 +3015,33 @@ namespace MobileGL::MG_Pipe {
if (op == MGPWireOp::DrawVbo && payload != nullptr) {
const auto& draw = *static_cast<const MGPDrawInfo*>(payload);
if ((draw.Flags & static_cast<Uint8>(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<Uint8>(kDrawIsIndirect)) == 0) {
return true;
}
}
return false;
}
+5 -2
View File
@@ -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,
+66
View File
@@ -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, "<field>@<the CLIENT's verb>"}`, 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