[Fix, Test] (gen_pipe, Wire): stop the generated header comment inviting a stamp of the call flags into the ring's framing flags - three of the six bits mean something else there, and only one of the three is masked on one path

MobileGL/MG_Test/Wire/RingTest.cpp

@@ -12,6 +12,10 @@
 
 #include <MG_Remote/Transport/Doorbell.h>
 #include <MG_Remote/Transport/Ring.h>
+// For MGPipeCallFlags. This file is the only place in the tree that sees BOTH flag
+// spaces: MG_Pipe is below MG_Remote and may not include Ring.h, so the cross-enum table
+// below cannot live in a generated .inc beside the call flags themselves.
+#include <MG_Pipe/MGPipe.h>
 
 #include <gtest/gtest.h>
 
@@ -669,3 +673,119 @@ TEST(RingTest, AWrapFillerDoesNotAdvanceTheRecordSequence) {
         << "the two sides' sequence spaces have drifted by the fillers between them";
     EXPECT_TRUE(ring.Invariants());
 }
+
+// ---------------------------------------------------------------------------
+// The two flag spaces (P5, after w1's finding). MGPWireRecHeader::Flags IS
+// RingRecordHeader::flags - the two structs are the same eight bytes - and the
+// two enums that name those bits OVERLAP AND DISAGREE. Stamping
+// MGPipeCallFlagsFor(op) into the header is therefore a silent, data-dependent
+// corruption, and the first two bits agreeing is what makes it look right in a
+// debugger.
+//
+// The table below is exhaustive over both enums, and deliberately spells the
+// agreements as well as the collisions: "these two mean the same thing" is a
+// fact the encoder's translation relies on, so it has to be checked too.
+// ---------------------------------------------------------------------------
+
+namespace {
+    constexpr std::uint32_t Call(MobileGL::MG_Pipe::MGPipeCallFlags f) {
+        return static_cast<std::uint32_t>(f);
+    }
+    constexpr std::uint32_t Rec(RingRecordFlags f) { return static_cast<std::uint32_t>(f); }
+} // namespace
+
+TEST(RingTest, TheTwoFlagSpacesAreDisjointByTranslation) {
+    using namespace MobileGL::MG_Pipe;
+
+    // The two bits that agree. An encoder may pass these through, and w1's does.
+    static_assert(Call(kNeedsAck) == Rec(kRecNeedsAck), "bit 0 stopped agreeing");
+    static_assert(Call(kHasBlob) == Rec(kRecHasBlob), "bit 1 stopped agreeing");
+
+    // The three that collide. Each of these is a live defect if it is ever passed through,
+    // and the middle one is the worst: RingConsumer::Pop treats kRecPad as a wrap filler and
+    // SKIPS the record, so a stamped kVarTail deletes every variable-tail call from the
+    // stream with no error raised anywhere.
+    static_assert(Call(kVarTail) == Rec(kRecPad), "the kVarTail/kRecPad collision moved");
+    static_assert(Call(kHostSpan) == Rec(kRecBorrowSlot), "the kHostSpan/kRecBorrowSlot collision moved");
+    static_assert(Call(kReplySlot) == Rec(kRecVarTail), "the kReplySlot/kRecVarTail collision moved");
+
+    // kOptional has no ring counterpart at all: bit 5 is unused over there today. If a sixth
+    // ring flag is ever added it lands on this bit, so this is where that is noticed.
+    static_assert(Call(kOptional) == (1u << 5), "kOptional moved");
+
+    // Exhaustiveness, from both ends. The generator pins kMGPipeCallFlagsAllBits from
+    // MGPipe.h; this pins the ring's own set against it, so ADDING a flag to either enum is a
+    // build break here rather than a wrong decode in the field.
+    static_assert(kMGPipeCallFlagsAllBits == 0x3Fu, "MGPipeCallFlags grew or shrank");
+    constexpr std::uint32_t kAllRingFlags =
+        Rec(kRecNeedsAck) | Rec(kRecHasBlob) | Rec(kRecPad) | Rec(kRecBorrowSlot) | Rec(kRecVarTail);
+    static_assert(kAllRingFlags == 0x1Fu, "RingRecordFlags grew or shrank");
+
+    // ---- the runtime half, and it is NOT the story the collision table alone suggests ----
+    //
+    // RingProducer::Reserve MASKS kRecPad OUT of whatever the caller passes
+    // (Ring.cpp: `flags & ~kRecPad`). So the worst of the three collisions - a real record
+    // framed as a wrap filler and skipped by Pop - is ALREADY DEFENDED for anyone who goes
+    // through Reserve. That defence is worth knowing about and worth pinning, because it is
+    // also exactly one bit wide: the other two collisions pass straight through.
+    RingFixture ring(1024);
+    const std::uint32_t drawVboCallFlags = MGPipeCallFlagsFor(MGPWireOp::DrawVbo);
+    ASSERT_NE(drawVboCallFlags & Call(kVarTail), 0u) << "draw_vbo stopped being a var-tail call";
+    ASSERT_NE(drawVboCallFlags & Call(kHostSpan), 0u) << "draw_vbo stopped being a host-span call";
+
+    void* payload = ring.Producer().Reserve(
+        static_cast<std::uint16_t>(MGPWireOp::DrawVbo),
+        static_cast<std::uint16_t>(drawVboCallFlags), // the mistake, made on purpose
+        16);
+    ASSERT_NE(payload, nullptr);
+    std::memset(payload, 0xAB, 16);
+    ring.Producer().Publish();
+
+    RingRecordView view{};
+    ASSERT_TRUE(ring.Consumer().Pop(view)) << "Reserve stopped masking kRecPad";
+    EXPECT_EQ(view.flags & static_cast<std::uint16_t>(kRecPad), 0u)
+        << "Reserve is what keeps a stamped kVarTail from deleting this record";
+    // ... and here is what IS wrong with it. kHostSpan landed on kRecBorrowSlot and kReplySlot
+    // on kRecVarTail, neither of which Reserve masks. The consumer now believes this record
+    // borrowed a slot into the GPU timeline - so it retires late, on completedFrameSerial
+    // instead of on apply - and that it carries a variable tail it does not have.
+    EXPECT_NE(view.flags & static_cast<std::uint16_t>(kRecBorrowSlot), 0u)
+        << "the kHostSpan/kRecBorrowSlot collision is what makes a stamped header lie about "
+           "this record's lifetime";
+    EXPECT_TRUE(ring.Invariants());
+
+    // The one that Reserve cannot defend: a producer that writes the header ITSELF rather than
+    // letting Reserve write it - which is precisely what a codec with its own header struct
+    // does, since MGPWireRecHeader and RingRecordHeader are the same eight bytes. Then the
+    // mask is not in the path, Pop sees kRecPad, and the record is skipped as a wrap filler
+    // with no error raised anywhere.
+    void* second = ring.Producer().Reserve(static_cast<std::uint16_t>(MGPWireOp::DrawVbo),
+                                           kRecNone, 16);
+    ASSERT_NE(second, nullptr);
+    std::memset(second, 0xAB, 16);
+    RingRecordHeader stamped{};
+    std::memcpy(&stamped, static_cast<std::uint8_t*>(second) - sizeof(RingRecordHeader),
+                sizeof(stamped));
+    stamped.flags = static_cast<std::uint16_t>(drawVboCallFlags); // no mask in this path
+    std::memcpy(static_cast<std::uint8_t*>(second) - sizeof(RingRecordHeader), &stamped,
+                sizeof(stamped));
+    ring.Producer().Publish();
+
+    EXPECT_FALSE(ring.Consumer().Pop(view))
+        << "a header stamped with the CALL flags outside Reserve should vanish into Pop's "
+           "wrap-filler skip - if it did not, the collision has moved and this case is no "
+           "longer the control it was";
+
+    // And the same record framed the way the encoder actually frames it - translated, with the
+    // ring's own var-tail bit - round-trips intact.
+    void* honest = ring.Producer().Reserve(static_cast<std::uint16_t>(MGPWireOp::DrawVbo),
+                                           kRecVarTail, 16);
+    ASSERT_NE(honest, nullptr);
+    std::memset(honest, 0xCD, 16);
+    ring.Producer().Publish();
+    ASSERT_TRUE(ring.Consumer().Pop(view));
+    EXPECT_EQ(view.kind, static_cast<std::uint16_t>(MGPWireOp::DrawVbo));
+    EXPECT_EQ(view.flags, static_cast<std::uint16_t>(kRecVarTail));
+    EXPECT_EQ(view.payloadSize, 16u);
+    EXPECT_TRUE(ring.Invariants());
+}

scripts/gen_pipe.py

@@ -487,13 +487,34 @@ 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
+FLAG_ENUM_RE = re.compile(r"enum\s+MGPipeCallFlags\s*:\s*Uint32\s*\{(.*?)\}\s*;", re.S)
-# header because this list is what the generated kMGPipeCallFlags[] table spells into C++:
+FLAG_MEMBER_RE = re.compile(r"^\s*(k\w+)\s*=\s*1u\s*<<\s*(\d+)\s*,", re.M)
-# 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
+def parse_call_flags():
-# combined with anything.
+    """The MGPipeCallFlags enumerators and their bit positions, READ OUT OF MGPipe.h.
-KNOWN_CALL_FLAGS = ("kNeedsAck", "kHasBlob", "kVarTail", "kHostSpan", "kReplySlot", "kOptional")
+
+    Parsed rather than hard-coded, and that is the point: this list is what the generated
+    kMGPipeCallFlags[] table spells into C++, and it is also what the emitted assertions pin
+    against MG_Remote's RingRecordFlags. A seventh flag added to the header has to reach both,
+    so it must reach this function - a copy here would go stale in exactly the case that
+    matters. kNone is deliberately absent: it is the empty set, not a flag."""
+    text = read(os.path.join(PIPE_DIR, "MGPipe.h"))
+    body = FLAG_ENUM_RE.search(text)
+    if not body:
+        sys.exit("MGPipe.h: enum MGPipeCallFlags : Uint32 is missing or has changed shape")
+    flags = [(name, int(bit)) for name, bit in FLAG_MEMBER_RE.findall(body.group(1))]
+    if not flags:
+        sys.exit("MGPipe.h: enum MGPipeCallFlags declares no 1u << N members")
+    seen = {}
+    for name, bit in flags:
+        if bit in seen:
+            sys.exit("MGPipe.h: %s and %s are both bit %d" % (seen[bit], name, bit))
+        seen[bit] = name
+    return flags
+
+
+KNOWN_CALL_FLAGS = tuple(name for name, _ in parse_call_flags())
 
 
 def check_call_flags_are_known(calls):
@@ -651,7 +672,33 @@ def gen_wire(calls, residual_fields=None):
 
 struct MGPWireRecHeader {
     Uint16 Op;    // MGPWireOp
-    Uint16 Flags; // MGPipeCallFlags of the call, for asserts and tracing
+    // RING FRAMING FLAGS - MG_Remote::Transport::RingRecordFlags - NOT MGPipeCallFlags.
+    //
+    // This field IS RingRecordHeader::flags (Ring.h): the two structs are the same eight bytes
+    // and the ring writes this one. The two flag spaces OVERLAP AND DISAGREE, so stamping
+    // MGPipeCallFlagsFor(op) in here is a silent, data-dependent corruption:
+    //
+    //     bit 0  kNeedsAck  == kRecNeedsAck     agree
+    //     bit 1  kHasBlob   == kRecHasBlob      agree
+    //     bit 2  kVarTail   vs kRecPad          DISAGREE
+    //     bit 3  kHostSpan  vs kRecBorrowSlot   DISAGREE
+    //     bit 4  kReplySlot vs kRecVarTail      DISAGREE
+    //     bit 5  kOptional  vs (nothing)        no ring counterpart
+    //
+    // The first two agreeing is exactly what makes this dangerous: a naive stamp looks right
+    // in a debugger. It is not. RingProducer::Reserve masks kRecPad out of a caller's flags,
+    // so bit 2 is defended for anyone who goes through it - and ONLY bit 2, and only on that
+    // path. A producer that writes this header itself (these are the same eight bytes, so a
+    // codec with its own header struct does exactly that) loses even that: Pop treats kRecPad
+    // as a WRAP FILLER AND SKIPS THE RECORD. Bits 3 and 4 are undefended everywhere - a
+    // stamped kHostSpan tells the consumer this record borrowed a slot into the GPU timeline
+    // and must retire late, and a stamped kReplySlot claims a tail that is not there.
+    //
+    // THE TWO SPACES ARE DISJOINT BY TRANSLATION, NOT SHARED. The encoder maps one to the
+    // other explicitly (MG_Remote/Wire/PipeWireCodec.cpp) and nothing else may write this
+    // field. The CALL's flags are not on the wire at all and do not need to be: the opcode is,
+    // and MGPipeCallFlagsFor(op) recovers them exactly on either side.
+    Uint16 Flags;
     Uint32 Size;  // bytes of this record including the header and the variable tail
 };
 static_assert(sizeof(MGPWireRecHeader) == 8, "the wire header is 8 bytes");
@@ -718,6 +765,32 @@ static_assert((MGPipeCallFlagsFor(MGPWireOp::DrawVbo) &
                static_cast<Uint32>(kHostSpan | kVarTail)) == static_cast<Uint32>(kHostSpan | kVarTail),
               "draw_vbo is the conditional-tail plus host-span shape the codec is measured on");
 """)
+    # The bit layout, pinned from the header the generator just read. This exists because
+    # MGPWireRecHeader::Flags is RingRecordHeader::flags and three of these bits mean something
+    # ELSE over there (kVarTail/kRecPad, kHostSpan/kRecBorrowSlot, kReplySlot/kRecVarTail). The
+    # cross-enum table cannot live here - MG_Pipe is below MG_Remote and may not include
+    # Ring.h - so it lives in MG_Test/Wire/RingTest.cpp, which sees both. What CAN be stated
+    # here is the half this side owns: these are the flags, at these bits, and this is all of
+    # them. Renumber one and the translation on the other side is wrong; both ends go red.
+    flag_list = parse_call_flags()
+    out.append("")
+    out.append("// The bit layout of MGPipeCallFlags, read out of MGPipe.h by the generator.")
+    out.append("// MG_Test/Wire/RingTest.cpp holds the other half: what each of these bits means")
+    out.append("// in MG_Remote::Transport::RingRecordFlags, which is NOT the same thing.")
+    for name, bit in flag_list:
+        out.append("static_assert(static_cast<Uint32>(%s) == (1u << %d)," % (name, bit))
+        out.append("              \"MGPipeCallFlags::%s moved bit; MG_Remote's translation is keyed on it\");"
+                   % name)
+    union = " | ".join(name for name, _ in flag_list)
+    total = 0
+    for _, bit in flag_list:
+        total |= 1 << bit
+    out.append("// ... and this is ALL of them. A seventh flag changes this number, which is the")
+    out.append("// build break that sends its author to the ring's flag space before it ships.")
+    out.append("inline constexpr Uint32 kMGPipeCallFlagsAllBits = static_cast<Uint32>(%s);" % union)
+    out.append("static_assert(kMGPipeCallFlagsAllBits == 0x%02Xu," % total)
+    out.append("              \"the MGPipeCallFlags bit set changed; see RingTest's cross-enum table\");")
+    out.append("")
     for call in calls:
         out.append("struct alignas(8) MGPWireRec_%s {" % call.Name)
         out.append("    MGPWireRecHeader Header;")