From a1d17566f7137577e22ed49f2dfea5957952a564 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 18:51:39 -0400 Subject: [PATCH] [MG_Pipe, MG_Test] (P7): the respecify scope pin asks that a per-level producer cover the range it declares, so verify and split can be in the same build --- MobileGL/MG_Pipe/PipeApply.cpp | 81 ++++++++--- MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp | 152 +++++++++++++++++++++ 2 files changed, 216 insertions(+), 17 deletions(-) diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 51c851bf..1b294ef7 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -947,27 +947,74 @@ namespace MobileGL::MG_Pipe { #endif #if MOBILEGL_PIPE_VERIFY - // P5's pin, the same shape and for the same reason. The respecify SCOPE now has a wire - // carrier (MGPResourceDesc's HasRespecifiedLevel + the pair) and no producer: every - // descriptor P5 builds is whole-resource, and the per-level scope still arrives the old - // way, as the trailing MGPRespecifiedLevel* this function does not look at. + // P5's pin, RELAXED BY P7 WAVE 3 (CONTRACT-P7 §6) - and relaxed rather than deleted, + // because the thing it was guarding is now reachable instead of hypothetical. // - // The two must not disagree, and when a later package wires the carrier it will set the - // fields at a call site that also still passes the pointer - so the first thing that can - // go wrong is exactly one of the two moving. A verify build refuses to let that arrive - // unannounced, because a descriptor that says "whole resource" while the pointer says - // "level 1" drops every other level's pending upload with nothing saying so. - void PinWholeResourceRespecifyScope(const MGPResourceDesc& desc, MGPipeHandle res, const char* call) { + // WHAT IT USED TO SAY. "The respecify SCOPE has a wire carrier (MGPResourceDesc's + // HasRespecifiedLevel + the pair) and NO PRODUCER: every descriptor P5 builds is + // whole-resource, and the per-level scope still arrives the old way, as the trailing + // MGPRespecifiedLevel* this function does not look at." So any descriptor arriving with + // the carrier set was, by definition, a producer nobody had announced, and a verify + // build refused it. + // + // WHY THAT IS NOW WRONG, AND EXACTLY HOW FAR. The producer landed: + // Wire_Escape_ResourceRespecify (Client/WireTables.cpp:425-433) writes the carrier from + // the pointer through MGPipeSetRespecifiedLevel, and the server codec + // (Wire/PipeWireCodec.cpp:1677-1683) rebuilds the pointer from the carrier. So under a + // split transport EVERY per-level glTexImage*D arrives here with the carrier set, and + // the old pin made verify x split unmeasurable - it fired on the server's apply thread + // during bring-up, before a single verify case could arm: + // + // Fatal{PipeRespecifyScope} resource_respecify {slot=12, gen=0}: the descriptor + // carries a per-level respecify scope (target=258, level=0), and no path in this + // phase may set one + // + // WHAT SURVIVES, AND IT IS THE WHOLE INVARIANT. The pin never cared that the carrier + // was set; it cared that THE TWO HALVES OF THE SCOPE COULD DISAGREE - "the first thing + // that can go wrong is exactly one of the two moving". That hazard did not go away when + // the producer landed, it became LIVE, and it is silent: the applier below drops pending + // uploads by the POINTER, so a descriptor that declares (target, level) while this call + // hands over a null pointer takes the WHOLE-RESOURCE arm and eats every other level's + // texels, and one that hands over a different pair erases the wrong key and keeps the + // one the client stopped owing. Neither shows up as anything but missing pixels, a + // frame later, on whichever level the wire dropped. + // + // So the rule becomes CONTRACT-P7 §6's: A PRODUCER OF A PER-LEVEL SCOPE MUST COVER THE + // RANGE IT DECLARES. Carrier set => the call passes a pointer, and that pointer names + // exactly the declared (uploadTarget, level). + // + // ONE-DIRECTIONAL ON PURPOSE. A whole-resource descriptor says NOTHING about the + // pointer, and must not: that is the monolith shape and it is still the majority of the + // calls in the tree. MG_Impl/Pipe/TextureEmit.h builds a value-initialized descriptor + // (carrier clear, by MGPipeClearRespecifiedLevel's own "the safe default") and states + // the scope in the trailing pointer alone, which is legal, load-bearing and untouched + // here. Checking that direction too would red every monolith glTexImage*D in the tree + // and would be asserting a convention, not an invariant - the pointer is the caller's + // statement, and a caller that has not been converted to the carrier has not lied. + void PinRespecifyScopeCoversItsDeclaration(const MGPResourceDesc& desc, + const MGPRespecifiedLevel* level, MGPipeHandle res, + const char* call) { if (MGPipeRespecifyIsWholeResource(desc)) return; + const Uint16 declaredTarget = MGPipeRespecifiedUploadTargetOf(desc); + const Uint16 declaredLevel = MGPipeRespecifiedLevelOf(desc); + if (level != nullptr && level->UploadTarget == declaredTarget && + level->Level == declaredLevel) { + return; + } MGP_TRIP_WIRE_REPORT("MGPipe: " MGP_TRIP_WIRE_TAG("PipeRespecifyScope") - " %s {slot=%u, gen=%u}: the descriptor carries a per-level respecify " - "scope (target=%u, level=%u), and no path in this phase may set one", - call, res.Slot, res.Gen, - static_cast(MGPipeRespecifiedUploadTargetOf(desc)), - static_cast(MGPipeRespecifiedLevelOf(desc))); + " %s {slot=%u, gen=%u}: the descriptor declares a per-level respecify " + "scope (target=%u, level=%u) and this call covers %s(target=%u, " + "level=%u), so a per-level producer is not covering the range it " + "declares", + call, res.Slot, res.Gen, static_cast(declaredTarget), + static_cast(declaredLevel), + level == nullptr ? "NOTHING " : "", + level == nullptr ? 0u : static_cast(level->UploadTarget), + level == nullptr ? 0u : static_cast(level->Level)); } #else - void PinWholeResourceRespecifyScope(const MGPResourceDesc&, MGPipeHandle, const char*) {} + void PinRespecifyScopeCoversItsDeclaration(const MGPResourceDesc&, const MGPRespecifiedLevel*, + MGPipeHandle, const char*) {} #endif #if MOBILEGL_PIPE_VERIFY @@ -1995,7 +2042,7 @@ namespace MobileGL::MG_Pipe { MGPipeResourceRecord* record = ResolveResourceIn(*table, "resource_respecify", desc.Resource); if (record == nullptr) return false; PinNoLiveHostWrites(*record, desc.Resource, "resource_respecify"); - PinWholeResourceRespecifyScope(desc, desc.Resource, "resource_respecify"); + PinRespecifyScopeCoversItsDeclaration(desc, level, desc.Resource, "resource_respecify"); // IS THIS A REDEFINITION AT ALL? Asked BEFORE the descriptor is replaced, because the // stored one is the only thing there is to compare against (ID-18 M4). See diff --git a/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp b/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp index cff3a4f9..8170cc36 100644 --- a/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp +++ b/MobileGL/MG_Test/Pipe/ResourceEmitTest.cpp @@ -1338,6 +1338,158 @@ namespace { #endif } + // ------------------------------------------------------------------------------------ + // P7 wave 3 (CONTRACT-P7 §6): the respecify SCOPE pin, relaxed and still falsifiable. + // + // The pin used to say "no path in this phase may set a per-level scope on the descriptor", + // which stopped being true the day Wire_Escape_ResourceRespecify started writing the + // carrier: under split EVERY per-level glTexImage*D arrives with it set, and the pin fired + // on the server apply thread during bring-up, before one verify case could arm. It is now + // "a producer of a per-level scope must COVER the range it declares", and these two cases + // are the positive and the negative the contract asks for. + // + // A TEXTURE, NOT A BUFFER, and ScopedResourceOps with it: the scope is a texture concept, + // and every non-buffer row is refused with RefusedNoConsumer unless a backend table is + // registered - so without the scope this case would not reach the pin at all. + MGPResourceDesc Tex2DDesc(MGPipeHandle res, Uint32 extent, Uint32 levels, Uint32 glName) { + MGPResourceDesc desc{}; + desc.Resource = res; + desc.Target = static_cast(MGPipeResourceTarget::Tex2D); + desc.InternalFormat = 1; + desc.Width = extent; + desc.Height = extent; + desc.Depth = 1; + desc.ArrayLayers = 1; + desc.Levels = levels; + desc.Samples = 1; + desc.GlNameForDiag = glName; + return desc; + } + + // The packed (resource target, upload target) pair a Tex2D level's records carry. 0x0102 - + // and it is the very number the split bring-up's Fatal printed, which is how this case is + // known to be about the same shape. + const Uint16 kTex2DUpload = MGPipePackSubDataTarget( + static_cast(MGPipeResourceTarget::Tex2D), + static_cast(TextureUploadTarget::Texture2D)); + + MGPSubData TextureWrite(MGPipeHandle res, Uint16 uploadTarget, Uint16 level, Uint32 extent) { + MGPSubData record{}; + record.Res = res; + record.Target = uploadTarget; + record.Level = level; + record.UnionBox = MGPBox{0, 0, 0, extent, extent, 1}; + record.LevelWidth = extent; + record.LevelHeight = extent; + record.LevelDepth = 1; + return record; + } + + Bool HasPendingUpload(Uint32 slot, Uint16 uploadTarget, Uint16 level) { + for (const auto& entry : MGPipeApplier().TextureResources[slot].PendingUploads) { + if (entry.UploadTarget == uploadTarget && entry.Level == level) return true; + } + return false; + } + + // THE POSITIVE. A per-level producer that covers its declared range is accepted, and the + // accumulated texels of every OTHER level survive it - which is the thing the pin was + // standing in front of, checked rather than asserted. + TEST(ResourceEmit, APerLevelRespecifyCoveringItsDeclaredRangeKeepsTheOtherLevelsTexels) { +#if !MOBILEGL_PIPE_PUSH + GTEST_SKIP() << "MOBILEGL_PIPE_PUSH is off: there is no applier in this build"; +#else + ApplierGuard guard; + ScopedResourceOps consumer; + + const MGPipeHandle res{12, 1}; + ASSERT_TRUE(MGPipeApplyResourceCreate(Tex2DDesc(res, 4, 2, 91))); + + // Two levels with texels the server owes. The canonical sequence the pending set + // exists for: level 0 emitted, nothing uploaded yet, then level 1 redefined. + const Uint8 texels[64] = {}; + ASSERT_TRUE(MGPipeApplyResourceSubData(TextureWrite(res, kTex2DUpload, 0, 4), texels, nullptr)); + ASSERT_TRUE(MGPipeApplyResourceSubData(TextureWrite(res, kTex2DUpload, 1, 2), texels, nullptr)); + ASSERT_TRUE(HasPendingUpload(res.Slot, kTex2DUpload, 0)); + ASSERT_TRUE(HasPendingUpload(res.Slot, kTex2DUpload, 1)); + + // The split shape, both halves in agreement: the carrier says (kTex2DUpload, 1) and the + // trailing pointer the codec rebuilt from it says the same. + MGPResourceDesc perLevel = Tex2DDesc(res, 4, 2, 91); + MGPipeSetRespecifiedLevel(perLevel, kTex2DUpload, 1); + ASSERT_FALSE(MGPipeRespecifyIsWholeResource(perLevel)); + const MGPRespecifiedLevel covered{kTex2DUpload, 1}; + EXPECT_TRUE(MGPipeApplyResourceRespecify(perLevel, nullptr, &covered)); + + EXPECT_TRUE(HasPendingUpload(res.Slot, kTex2DUpload, 0)) + << "a per-level respecify may not eat the levels it did not redefine"; + EXPECT_FALSE(HasPendingUpload(res.Slot, kTex2DUpload, 1)) + << "the level the call DID redefine has a new coordinate system, so its box goes"; + EXPECT_EQ(ReadLog().find("PipeRespecifyScope"), std::string::npos) + << "the relaxed pin must be silent on a producer that covers its declaration"; +#endif + } + + // THE NEGATIVE, and it is the whole reason the pin is relaxed rather than deleted. A + // producer that DECLARES a range and does not cover it is still refused by name: a null + // pointer takes the whole-resource arm and eats every other level's texels, and a pointer + // naming a different pair erases the wrong key and keeps the one the client stopped owing. + // Neither is visible as anything but missing pixels one frame later. + TEST(ResourceEmit, APerLevelRespecifyThatDoesNotCoverItsDeclaredRangeIsRefusedByName) { +#if !MOBILEGL_PIPE_PUSH + GTEST_SKIP() << "MOBILEGL_PIPE_PUSH is off: there is no applier in this build"; +#elif !MOBILEGL_PIPE_VERIFY + // MOBILEGL_PIPE_VERIFY alone, NOT `POISON || VERIFY`, for PinNoLiveHostWrites' reason + // above: PinRespecifyScopeCoversItsDeclaration is compiled under `#if + // MOBILEGL_PIPE_VERIFY` only, so in a plain split build the wire genuinely is compiled + // out and this case must skip rather than expect a death that cannot happen. + GTEST_SKIP() << "Fatal{PipeRespecifyScope} is a MOBILEGL_PIPE_VERIFY wire and is compiled out here"; +#elif !MGTEST_HAVE_FORK + GTEST_SKIP() << "no fork on this platform; the wire's verdict is std::abort()"; +#else + ApplierGuard guard; + ScopedResourceOps consumer; + + const MGPipeHandle res{13, 2}; + ASSERT_TRUE(MGPipeApplyResourceCreate(Tex2DDesc(res, 4, 2, 92))); + + struct Drive { + const char* What; + bool HasPointer; + Uint16 PointerTarget; + Uint16 PointerLevel; + const char* Wanted; + }; + // Declared: (kTex2DUpload, 1). Covered: nothing, the wrong level, the wrong face. + const Drive drives[] = { + {"a null pointer", false, 0, 0, "covers NOTHING (target=0, level=0)"}, + {"the wrong level", true, kTex2DUpload, 0, "covers (target=258, level=0)"}, + {"the wrong upload target", true, static_cast(kTex2DUpload + 0x0100u), 1, + "covers (target=514, level=1)"}, + }; + for (const Drive& drive : drives) { + const ChildResult child = RunInChild([&res, &drive]() { + MGPResourceDesc perLevel = Tex2DDesc(res, 4, 2, 92); + MGPipeSetRespecifiedLevel(perLevel, kTex2DUpload, 1); + const MGPRespecifiedLevel named{drive.PointerTarget, drive.PointerLevel}; + MGPipeApplyResourceRespecify(perLevel, nullptr, + drive.HasPointer ? &named : nullptr); + }); + EXPECT_TRUE(DiedOfAbort(child)) + << drive.What << ": " << DescribeStatus(child) << "; log: " << child.Log; + const std::string named = + "Fatal{PipeRespecifyScope} resource_respecify {slot=13, gen=2}"; + EXPECT_NE(child.Log.find(named), std::string::npos) + << drive.What << ": wanted \"" << named << "\"; log: " << child.Log; + EXPECT_NE(child.Log.find("declares a per-level respecify scope (target=258, level=1)"), + std::string::npos) + << drive.What << ": the refusal must print the DECLARED range; log: " << child.Log; + EXPECT_NE(child.Log.find(drive.Wanted), std::string::npos) + << drive.What << ": wanted \"" << drive.Wanted << "\"; log: " << child.Log; + } +#endif + } + // M-D. The slot is the one number in the family that reaches an ALLOCATOR, so it is // policed like every other: a slot outside the table's bound is Fatal{ProtocolCorruption} // and never a resize. Removing the bound turns this case into a multi-gigabyte allocation.