From 41b4f8dfe32bd8f371176afd5c1bcfa1449d765a Mon Sep 17 00:00:00 2001 From: rereview Date: Fri, 11 Sep 2026 13:05:05 -0400 Subject: [PATCH] [Fix] (MG_Pipe, gen_pipe): export the per-opcode call flags and give GetCaps and CreateSamplerState the kHasBlob their payloads have always owned --- MobileGL/MG_Pipe/PipeCalls.def | 35 ++++++- MobileGL/MG_Pipe/generated/PipeWire.inc | 121 ++++++++++++++++++++++++ scripts/gen_pipe.py | 90 ++++++++++++++++++ 3 files changed, 244 insertions(+), 2 deletions(-) diff --git a/MobileGL/MG_Pipe/PipeCalls.def b/MobileGL/MG_Pipe/PipeCalls.def index 9c368fb1..ff5720fb 100644 --- a/MobileGL/MG_Pipe/PipeCalls.def +++ b/MobileGL/MG_Pipe/PipeCalls.def @@ -23,6 +23,14 @@ // (MGPipeTypes.h) is what says so, which is why the same call still carries // every glBufferData without acknowledging one. // +// kHasBlob MEANS "THE PAYLOAD OWNS AN MGPBlobRef MEMBER", nothing weaker (P5 +// R-13.1). It is not "this call carries bytes": three calls carry bytes with no +// MGPBlobRef at all and they are named in CONTRACT-P5.md table 1, not flagged +// here, because a decoder that trusts kHasBlob has to find a member to read. +// The flags are EXPORTED, once, as kMGPipeCallFlags[] in generated/PipeWire.inc +// (P5 R-13.4): before that table existed every consumer hard-coded its own copy, +// which is how the two carriers below came to disagree with their payloads. +// // RECORD NUMBERING NEVER CHURNS. Entries that are not implemented yet still occupy their // line (plan section 11, P0: "the complete call catalogue, placeholders included"). A new // call is APPENDED to its group; a retired call keeps its slot with a comment. The wire @@ -77,8 +85,20 @@ // clang-format off #define MGP_CALL_LIST(X) \ /* ---- screen: caps, resources, persistent map, fences (plan 4.4.1) ---- */ \ - X(GetCaps, MGPCaps, kScreen, kReplySlot) \ + /* kHasBlob because MGPCaps owns TWO MGPBlobRef members - FormatCapabilities and */ \ + /* RendererInfo (MGPipeTypes.h). It carried none until P5 R-13.1; the flag is a */ \ + /* property of the payload, so its absence was simply wrong. Adding a flag does not */ \ + /* move an opcode: the opcode is this line's 1-based position. */ \ + X(GetCaps, MGPCaps, kScreen, kReplySlot|kHasBlob) \ X(ResourceCreate, MGPResourceDesc, kScreen, kNone) \ + /* NO kHasBlob, BY RULING (P5 R-13.3; CONTRACT-P5.md table 1 row 19). MGPResourceDesc */ \ + /* owns no MGPBlobRef and gains none: under split `initialBytes` is always nullptr and */ \ + /* the initial content of a glBufferData(size, data) / glTexImage*(..., data) arrives */ \ + /* as resource_subdata records IMMEDIATELY AFTER this one. Reuses a path that is */ \ + /* already chunked (MGPipeForEachSubDataRecordRange) and already acceptance-gated; the */ \ + /* cost is one extra record. NOTE the SECOND uncarried companion on this call, the */ \ + /* MGPRespecifiedLevel* scope (PipeApply.h:792) - it is NOT bytes and NOT covered by */ \ + /* this ruling; table 1 row 19 gives it the two pad fields of MGPResourceDesc. */ \ X(ResourceRespecify, MGPResourceDesc, kScreen, kNeedsAck) \ X(ResourceDestroy, MGPHandleOnly, kScreen, kNone) \ X(MapPersistent, MGPHandleOnly, kScreen, kReplySlot|kOptional) \ @@ -101,7 +121,11 @@ X(CreateVertexElements, MGPVertexElements, kCtxCso, kHasBlob) \ X(BindVertexElements, MGPHandleOnly, kCtxCso, kNone) \ X(DeleteVertexElements, MGPHandleOnly, kCtxCso, kNone) \ - X(CreateSamplerState, MGPSamplerDesc, kCtxCso, kNone) \ + /* kHasBlob because MGPSamplerDesc owns an MGPBlobRef member, `Parameters` - the */ \ + /* SamplerParameters POD byte for byte, borderColorForm included (P5 R-13.1). The */ \ + /* companion pointer beside the record today is a TYPED frontend pointer */ \ + /* (const SamplerParameters*), which is exactly what split may not carry. */ \ + X(CreateSamplerState, MGPSamplerDesc, kCtxCso, kHasBlob) \ X(DeleteSamplerState, MGPHandleOnly, kCtxCso, kNone) \ X(CreateSamplerView, MGPSamplerView, kCtxCso, kNone) \ X(DeleteSamplerView, MGPHandleOnly, kCtxCso, kNone) \ @@ -134,6 +158,13 @@ X(ResourceSubData, MGPSubData, kCtxObject, kHasBlob|kVarTail) \ X(BufferSubDataResident, MGPSubData, kCtxObject, kHasBlob|kOptional) \ X(ResourceSubDataComplete, MGPSubDataComplete, kCtxObject, kNone) \ + /* NO kHasBlob AND NO MGPBlobRef, BY RULING (P5 R-13.2; CONTRACT-P5.md table 1 row 21). */ \ + /* It takes a content pointer today, but under split it CARRIES NO BYTES AT ALL: it is */ \ + /* a {range, AccessFlags} control record, and the bytes of [Offset, Offset+Size) arrive */ \ + /* AHEAD of it as resource_subdata records covering exactly that range. The ladder this */ \ + /* drives rewrites its range "from the authoritative shadow" (Managers.cpp:1047-1076), */ \ + /* and under split the authoritative shadow is server-owned (R-11), so subdata is the */ \ + /* only way bytes reach it - a blobref here would be a SECOND way to say the same thing. */ \ X(ResourceFlushRange, MGPFlushRange, kCtxObject, kNone) \ X(ResourceReadback, MGPReadback, kCtxObject, kReplySlot) \ X(ResourceCopyRegion, MGPCopyRegion, kCtxObject, kNone) \ diff --git a/MobileGL/MG_Pipe/generated/PipeWire.inc b/MobileGL/MG_Pipe/generated/PipeWire.inc index df170405..d909caf9 100644 --- a/MobileGL/MG_Pipe/generated/PipeWire.inc +++ b/MobileGL/MG_Pipe/generated/PipeWire.inc @@ -118,6 +118,127 @@ enum class MGPWireOp : Uint16 { kOpCount = 72, }; +// THE FLAGS, EXPORTED ONCE, INDEXED BY OPCODE (P5 R-13.4). MGPWireRecHeader::Flags is +// documented as "MGPipeCallFlags of the call", and until this table existed nothing +// generated said what those were: every consumer that needed to know whether a record owns +// an MGPBlobRef, a variable tail or a reply slot had to hard-code its own copy of +// PipeCalls.def's fourth column, and six of them were about to. A hard-coded copy is how +// GetCaps and CreateSamplerState came to carry an MGPBlobRef member with no kHasBlob on +// their line at all - nothing compared the two, because nothing had both in one place. +// +// Index 0 is MGPWireOp::kInvalid and is kNone: the catalogue is 1-based, and an encoder +// that reads flags for an opcode it never got from the catalogue must see the empty set +// rather than another call's flags. +// +// kHasBlob here means EXACTLY "the payload owns an MGPBlobRef member". Three calls carry +// bytes without one - resource_respecify, resource_flush_range and map_persistent, whose +// companion pointers have no carrier - and they are deliberately NOT flagged; MG_Remote's +// CONTRACT-P5.md table 1 is where those live, because a decoder that trusts kHasBlob has +// to find a member to read. +inline constexpr Uint32 kMGPipeCallFlags[static_cast(MGPWireOp::kOpCount)] = { + /* 0 kInvalid */ static_cast(kNone), + /* 1 GetCaps */ static_cast(kReplySlot | kHasBlob), + /* 2 ResourceCreate */ static_cast(kNone), + /* 3 ResourceRespecify */ static_cast(kNeedsAck), + /* 4 ResourceDestroy */ static_cast(kNone), + /* 5 MapPersistent */ static_cast(kReplySlot | kOptional), + /* 6 UnmapPersistent */ static_cast(kOptional), + /* 7 FenceCreate */ static_cast(kNone), + /* 8 FenceStatus */ static_cast(kReplySlot), + /* 9 FenceWait */ static_cast(kReplySlot), + /* 10 FenceDestroy */ static_cast(kNone), + /* 11 QueryCreate */ static_cast(kNone), + /* 12 QueryBegin */ static_cast(kNone), + /* 13 QueryEnd */ static_cast(kNone), + /* 14 QueryAvailable */ static_cast(kReplySlot), + /* 15 QueryResult */ static_cast(kReplySlot), + /* 16 QueryDestroy */ static_cast(kNone), + /* 17 CreateRenderState */ static_cast(kHasBlob), + /* 18 BindRenderState */ static_cast(kNone), + /* 19 DeleteRenderState */ static_cast(kNone), + /* 20 CreateVertexElements */ static_cast(kHasBlob), + /* 21 BindVertexElements */ static_cast(kNone), + /* 22 DeleteVertexElements */ static_cast(kNone), + /* 23 CreateSamplerState */ static_cast(kHasBlob), + /* 24 DeleteSamplerState */ static_cast(kNone), + /* 25 CreateSamplerView */ static_cast(kNone), + /* 26 DeleteSamplerView */ static_cast(kNone), + /* 27 CreateShaderState */ static_cast(kHasBlob), + /* 28 BindShaderState */ static_cast(kNone), + /* 29 DeleteShaderState */ static_cast(kNone), + /* 30 SetDynamicState */ static_cast(kHasBlob), + /* 31 SetFramebufferState */ static_cast(kNone), + /* 32 SetVertexBuffers */ static_cast(kVarTail), + /* 33 SetIndexBuffer */ static_cast(kNone), + /* 34 SetIndirectBuffers */ static_cast(kNone), + /* 35 SetSamplerViews */ static_cast(kVarTail), + /* 36 BindSamplerStates */ static_cast(kVarTail), + /* 37 SetShaderImages */ static_cast(kVarTail), + /* 38 SetShaderBuffers */ static_cast(kVarTail | kHostSpan), + /* 39 SetStreamOutputTargets */ static_cast(kVarTail), + /* 40 SetGlobalConstants */ static_cast(kHasBlob), + /* 41 SetVertexAttribDefaults */ static_cast(kVarTail), + /* 42 SetPixelPackState */ static_cast(kNone), + /* 43 SetPatchState */ static_cast(kNone), + /* 44 SetDrawProgram */ static_cast(kNone), + /* 45 SetDispatchProgram */ static_cast(kNone), + /* 46 SetResidualValueState */ static_cast(kHasBlob), + /* 47 SetTextureParams */ static_cast(kNone), + /* 48 ResourceSubData */ static_cast(kHasBlob | kVarTail), + /* 49 BufferSubDataResident */ static_cast(kHasBlob | kOptional), + /* 50 ResourceSubDataComplete */ static_cast(kNone), + /* 51 ResourceFlushRange */ static_cast(kNone), + /* 52 ResourceReadback */ static_cast(kReplySlot), + /* 53 ResourceCopyRegion */ static_cast(kNone), + /* 54 GenerateMipmap */ static_cast(kNone), + /* 55 GetTextureImage */ static_cast(kReplySlot), + /* 56 Blit */ static_cast(kNone), + /* 57 Clear */ static_cast(kNone), + /* 58 ReadPixels */ static_cast(kReplySlot), + /* 59 DrawVbo */ static_cast(kHostSpan | kVarTail), + /* 60 LaunchGrid */ static_cast(kNone), + /* 61 MemoryBarrier */ static_cast(kNone), + /* 62 BeginStreamOutput */ static_cast(kNone), + /* 63 EndStreamOutput */ static_cast(kNone), + /* 64 PauseStreamOutput */ static_cast(kNone), + /* 65 ResumeStreamOutput */ static_cast(kNone), + /* 66 Flush */ static_cast(kNone), + /* 67 Present */ static_cast(kNone), + /* 68 SetSwapInterval */ static_cast(kOptional), + /* 69 QueryTimestamp */ static_cast(kReplySlot), + /* 70 QueryCounter */ static_cast(kNone), + /* 71 FenceWaitServer */ static_cast(kNone), +}; +static_assert(sizeof(kMGPipeCallFlags) / sizeof(kMGPipeCallFlags[0]) == + static_cast(MGPWireOp::kOpCount), + "the flags table and the opcode space disagree"); + +// The only supported read of the table. Out-of-range is kNone rather than undefined +// behaviour, because the one caller that can pass a bad opcode is a decoder holding bytes +// off a stream, and it must reach its own Fatal{ProtocolCorruption} rather than read past +// the array on the way there. +inline constexpr Uint32 MGPipeCallFlagsFor(MGPWireOp op) { + const SizeT index = static_cast(op); + return index < static_cast(MGPWireOp::kOpCount) ? kMGPipeCallFlags[index] + : static_cast(kNone); +} + +// Spot checks the generator states about its own output, so that a catalogue edit that +// silently drops a flag is a build break here and not a wrong decode six packages away. +static_assert(MGPipeCallFlagsFor(MGPWireOp::kInvalid) == static_cast(kNone), + "opcode 0 is not a call and carries no flags"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::GetCaps) & static_cast(kHasBlob)) != 0, + "MGPCaps owns two MGPBlobRef members; R-13.1 gave the call its flag"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::CreateSamplerState) & static_cast(kHasBlob)) != 0, + "MGPSamplerDesc owns an MGPBlobRef member; R-13.1 gave the call its flag"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::ResourceFlushRange) & static_cast(kHasBlob)) == 0, + "R-13.2: resource_flush_range carries no bytes on the wire and owns no blobref"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::ResourceRespecify) & static_cast(kHasBlob)) == 0, + "R-13.3: initial bytes follow as resource_subdata; MGPResourceDesc owns no blobref"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::DrawVbo) & + static_cast(kHostSpan | kVarTail)) == static_cast(kHostSpan | kVarTail), + "draw_vbo is the conditional-tail plus host-span shape the codec is measured on"); + struct alignas(8) MGPWireRec_GetCaps { MGPWireRecHeader Header; MGPCaps Payload; diff --git a/scripts/gen_pipe.py b/scripts/gen_pipe.py index db1deeba..7f2b3f11 100644 --- a/scripts/gen_pipe.py +++ b/scripts/gen_pipe.py @@ -487,6 +487,30 @@ def check_call_payloads_have_field_lists(calls, payloads): "would be blind to them: %s" % ", ".join(missing)) +# The MGPipeCallFlags enumerators, MGPipe.h:39-54. Kept here rather than parsed out of the +# header because this list is what the generated kMGPipeCallFlags[] table spells into C++: +# a flag token in PipeCalls.def that is not one of these would generate an expression that +# does not compile, and a build break several minutes later is a worse diagnosis than this +# one line. kNone is listed but is NOT a flag - it is the empty set, and it may not be +# combined with anything. +KNOWN_CALL_FLAGS = ("kNeedsAck", "kHasBlob", "kVarTail", "kHostSpan", "kReplySlot", "kOptional") + + +def check_call_flags_are_known(calls): + """Every flag token in PipeCalls.def must be an MGPipeCallFlags enumerator, and kNone may + not be combined with one. Runs in both modes, --check included: P5 R-13.4 exports these + flags as a table six packages read, so a typo here is a wrong decode rather than a + compile error in the one consumer that used to hard-code its own copy.""" + for call in calls: + for flag in call.Flags: + if flag != "kNone" and flag not in KNOWN_CALL_FLAGS: + sys.exit("PipeCalls.def: %s carries flag %s, which is not an MGPipeCallFlags " + "enumerator (%s)" % (call.Name, flag, ", ".join(KNOWN_CALL_FLAGS))) + if "kNone" in call.Flags and len(call.Flags) != 1: + sys.exit("PipeCalls.def: %s combines kNone with %s; kNone is the empty set" + % (call.Name, "|".join(f for f in call.Flags if f != "kNone"))) + + def parse_coverage(): text = read(os.path.join(PIPE_DIR, "Coverage.def")) accessors = [] @@ -641,6 +665,59 @@ enum class MGPWireOp : Uint16 { out.append(" %s = %d," % (call.Name, call.Index)) out.append(" kOpCount = %d," % (len(calls) + 1)) out.append("};\n") + out.append("""// THE FLAGS, EXPORTED ONCE, INDEXED BY OPCODE (P5 R-13.4). MGPWireRecHeader::Flags is +// documented as "MGPipeCallFlags of the call", and until this table existed nothing +// generated said what those were: every consumer that needed to know whether a record owns +// an MGPBlobRef, a variable tail or a reply slot had to hard-code its own copy of +// PipeCalls.def's fourth column, and six of them were about to. A hard-coded copy is how +// GetCaps and CreateSamplerState came to carry an MGPBlobRef member with no kHasBlob on +// their line at all - nothing compared the two, because nothing had both in one place. +// +// Index 0 is MGPWireOp::kInvalid and is kNone: the catalogue is 1-based, and an encoder +// that reads flags for an opcode it never got from the catalogue must see the empty set +// rather than another call's flags. +// +// kHasBlob here means EXACTLY "the payload owns an MGPBlobRef member". Three calls carry +// bytes without one - resource_respecify, resource_flush_range and map_persistent, whose +// companion pointers have no carrier - and they are deliberately NOT flagged; MG_Remote's +// CONTRACT-P5.md table 1 is where those live, because a decoder that trusts kHasBlob has +// to find a member to read.""") + out.append("inline constexpr Uint32 kMGPipeCallFlags[static_cast(MGPWireOp::kOpCount)] = {") + out.append(" /* 0 %-24s*/ static_cast(kNone)," % "kInvalid") + for call in calls: + out.append(" /* %2d %-24s*/ static_cast(%s)," + % (call.Index, call.Name, " | ".join(call.Flags))) + out.append("};") + out.append("static_assert(sizeof(kMGPipeCallFlags) / sizeof(kMGPipeCallFlags[0]) ==") + out.append(" static_cast(MGPWireOp::kOpCount),") + out.append(" \"the flags table and the opcode space disagree\");") + out.append(""" +// The only supported read of the table. Out-of-range is kNone rather than undefined +// behaviour, because the one caller that can pass a bad opcode is a decoder holding bytes +// off a stream, and it must reach its own Fatal{ProtocolCorruption} rather than read past +// the array on the way there. +inline constexpr Uint32 MGPipeCallFlagsFor(MGPWireOp op) { + const SizeT index = static_cast(op); + return index < static_cast(MGPWireOp::kOpCount) ? kMGPipeCallFlags[index] + : static_cast(kNone); +} + +// Spot checks the generator states about its own output, so that a catalogue edit that +// silently drops a flag is a build break here and not a wrong decode six packages away. +static_assert(MGPipeCallFlagsFor(MGPWireOp::kInvalid) == static_cast(kNone), + "opcode 0 is not a call and carries no flags"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::GetCaps) & static_cast(kHasBlob)) != 0, + "MGPCaps owns two MGPBlobRef members; R-13.1 gave the call its flag"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::CreateSamplerState) & static_cast(kHasBlob)) != 0, + "MGPSamplerDesc owns an MGPBlobRef member; R-13.1 gave the call its flag"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::ResourceFlushRange) & static_cast(kHasBlob)) == 0, + "R-13.2: resource_flush_range carries no bytes on the wire and owns no blobref"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::ResourceRespecify) & static_cast(kHasBlob)) == 0, + "R-13.3: initial bytes follow as resource_subdata; MGPResourceDesc owns no blobref"); +static_assert((MGPipeCallFlagsFor(MGPWireOp::DrawVbo) & + static_cast(kHostSpan | kVarTail)) == static_cast(kHostSpan | kVarTail), + "draw_vbo is the conditional-tail plus host-span shape the codec is measured on"); +""") for call in calls: out.append("struct alignas(8) MGPWireRec_%s {" % call.Name) out.append(" MGPWireRecHeader Header;") @@ -1232,12 +1309,24 @@ def self_test(accessors): calls_for_control = parse_calls() controls.append(("emitted row naming a call that does not exist", lambda: gen_emitted_by( [("GetViewport", "SetDynamicState")], calls_for_control, [("GetViewport", "NotACall")]))) + # P5 R-13.4's gate. The flags are now a GENERATED TABLE six packages read instead of six + # hard-coded copies, so a token that is not an MGPipeCallFlags enumerator has to stop the + # generator rather than emit an expression that fails to compile minutes later - and + # kNone, the empty set, may not be OR'd with a real flag and quietly read as one. + flag_typo = Call(1, "Canned", "MGPHandleOnly", "kScreen", ["kHasBlobb"]) + flag_kNone = Call(1, "Canned", "MGPHandleOnly", "kScreen", ["kNone", "kHasBlob"]) + controls.append(("call flag that is not an MGPipeCallFlags enumerator", + lambda: check_call_flags_are_known([flag_typo]))) + controls.append(("kNone combined with a real flag", + lambda: check_call_flags_are_known([flag_kNone]))) trips = 0 for name, fn in controls: trips += expect_trip(name, fn) # The positive control: the canned struct's exact list passes, and the parser sees the # padding member as padding and the function as not a member. check_field_lists_cover_struct_members({"Canned": ["A", "B", "C"]}, ["Canned"], [canned_struct]) + # ... and the real catalogue's real flags pass the same gate. + check_call_flags_are_known(calls_for_control) if trips == 0: sys.exit("gen_pipe: self-test: no negative control tripped - the gates are not checking anything") if trips != len(controls): @@ -1256,6 +1345,7 @@ def main(): calls = parse_calls() payloads = parse_verify_payloads() + check_call_flags_are_known(calls) check_call_payloads_have_field_lists(calls, payloads) check_field_lists_cover_struct_members(parse_field_lists(), payloads) accessors, deltas, sticky, emitted = parse_coverage()