From 8d0ed5b82c67581465bceb2f867418a85f63a0ba Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 6 Sep 2026 13:06:01 -0400 Subject: [PATCH] [Fix] (Pipe, DirtySurface): read the writes that go through a member's field and the ones the preprocessor pastes together, and decline the rows the write analysis cannot answer - its "UNDER-FIRING" verdicts were an absence proof it did not have, and one of them put a false answer in the map for a bit P2 already ships - the write analysis under-approximated in the exact direction its own claim forbids: written_tokens recorded a write through a member's field (m_foo.bar = v) as FIELD:bar and never as MEM:m_foo, while resolve_reader resolves a shutter's accessor to MEM:m_foo, so for any struct-valued member the two halves could not meet; a new MEMBER_ROOTED_WRITE_RE records both, for m_x.f, m_x[i].f, m_x->f and nested - it could not see RenderState::SetPixelStoreParam's sixteen writes twice over, because they are spelled with the token-pasting operator and the file was read raw - the "field" it recorded was the macro parameter name, paramNameTail. The derivation now expands the function-like macros defined under its two roots (directives blanked, parameters substituted, ## pasted), which is also what makes SET_CAPABILITY's m_parameters.capability##Enabled writes visible - and it now DECLINES rather than answers wherever it cannot say it read every writer: a body carrying a construct it does not model (an unexpandable token paste), anything that reaches such a body through the call-graph fixed point the writes already travel, and any shutter member with a write-shaped occurrence outside the analysed roots. --check prints every decline with its site, plus how many bodies and files the absence claim rests on and the one place it stays coarse - the BitwiseEqual bits' shutter window now also starts at the last `}` before the `dirty |=`, so the pack block's trailing `m_pack = pack;` no longer leaks the pixel store into NEW_PATCH_STATE's reader set - consequence in the map: X(SetPixelStoreParam, NEW_PIXEL_PACK) went from a verdict the gate could not support - no function name in the tree could carry that bit - to an accepted, checked answer, and the row it forced (kPulledEveryVerb, documented as "no shutter exists, and none is needed yet") said that of the only mutator behind the shipped set_pixel_pack_state. The row is now kPulledPartialShutter|NEW_PIXEL_PACK: the pull is what holds on every mutating path, the bit moves on the eight Pack arms, and both facts are machine-readable for the P3a reader D16 writes this file for - --self-test grows from 7 negative controls to 10 - kPulledPartialShutter naming no bit, a mutator whose write analysis is incomplete, and a shutter member written outside the roots, the last two asserting a DECLINE and no verdict - and gains a positive control that fails if SetPixelStoreParam's pasted writes ever go unread again --- MobileGL/MG_Pipe/DirtySurface.def | 85 +++-- scripts/gen_pipe_dirty_surface.py | 496 ++++++++++++++++++++++++++---- 2 files changed, 507 insertions(+), 74 deletions(-) diff --git a/MobileGL/MG_Pipe/DirtySurface.def b/MobileGL/MG_Pipe/DirtySurface.def index 7964944a..6dd22c7b 100644 --- a/MobileGL/MG_Pipe/DirtySurface.def +++ b/MobileGL/MG_Pipe/DirtySurface.def @@ -21,7 +21,10 @@ // ANSWERS. A row lists EVERY publisher that fires on EVERY path through that mutator, // and only those; several are joined with '|'. A publisher that fires on some paths but // not all must not appear, because a shutter built from this file would then UNDER-fire, -// and ARCHITECTURE.md 13.2 names under-firing as the dangerous direction. +// and ARCHITECTURE.md 13.2 names under-firing as the dangerous direction. The one row that +// carries a bit which fires on only some paths says so in its answer - kPulledPartialShutter +// joined with that bit - because the alternative, dropping the bit, tells a reader of this +// file that a bit P2 already emits a call for has no shutter at all. // // "EVERY PATH" MEANS EVERY PATH THAT MUTATES. A setter that returns early because the value // did not change publishes nothing and needs to publish nothing - there is no mutation to @@ -36,13 +39,31 @@ // below. Every other NEW_* answer is checked against the shutter Tracker.h builds for that // bit: gen_pipe_dirty_surface.py resolves what the shutter READS to the members behind it, // computes what each mutator transitively WRITES (through MGP_NOTE_AGGREGATE too, whose hop -// it reads out of MGPipeNoteAggregate's own switch), and fails a row that names a bit whose -// shutter its mutator moves on no path at all. That half is one-directional on purpose - -// "it does write something the shutter reads" cannot prove it does so on EVERY path - so it -// catches under-firing and not over-claiming. The prose answers (kImmediate, kExplicitDestroy, -// kUnpublishedDestroy, kNoBackendRead, kPulledEveryVerb, kReverseChannel) are statements no -// derivation checks; --check prints how many rows carry one, and prints every row it had to -// decline, so "all mapped" can never be read as "all verified". +// it reads out of MGPipeNoteAggregate's own switch, and through the function-like macros of +// MG_State, which it EXPANDS - sixteen of RenderState.cpp's writes exist only after the +// preprocessor has pasted them together), and fails a row that names a bit whose shutter its +// mutator moves on no path at all. That half is one-directional on purpose - "it does write +// something the shutter reads" cannot prove it does so on EVERY path - so it catches +// under-firing and not over-claiming. +// +// AN ABSENCE CLAIM IS ONLY WORTH THE READING BEHIND IT, and this gate learned that the +// expensive way: its write analysis used to record a write through a member's field +// (m_foo.bar = v) as the FIELD alone and never as the member, while the shutter side +// resolves an accessor to the MEMBER - so the two halves could not meet for any +// struct-valued member, and --check printed, as a fact about RenderState.cpp, that +// SetPixelStoreParam "writes nothing NEW_PIXEL_PACK's shutter reads" about a setter whose +// whole body is sixteen writes to exactly that member. The answer below is what that put in +// this file. So the derivation now DECLINES rather than answers whenever it cannot say it +// read every writer: a body carrying a construct it does not model (an unexpandable token +// paste), anything that reaches such a body, and any shutter member written outside +// MG_State/GLState + MG_Impl/Pipe at all. --check prints every decline with the site that +// caused it, and prints how many bodies and files the claim rests on. +// +// The prose answers (kImmediate, kExplicitDestroy, kUnpublishedDestroy, kNoBackendRead, +// kPulledEveryVerb, kPulledPartialShutter, kReverseChannel) are statements no derivation +// checks - except the bits a kPulledPartialShutter row names, which are checked like any +// other bit answer. --check prints how many rows carry a prose answer, so "all mapped" can +// never be read as "all verified". // // For the RenderState family that answer is not a matter of taste and it is CHECKED // rather than asserted: scripts/gen_pipe_dirty_surface.py reads RenderState.cpp and @@ -79,10 +100,25 @@ // stakes. (D13's prose says 'six kinds' while the Core.cpp ranges it // cites also cover MarkProgram/MarkShaderForDeletion; the tree // decides, and the tree has no wire object for those three.) -// kPulledEveryVerb no shutter exists, and none is needed yet: the PipeInputs field this -// writes is in its verb class's may-read mask, so the residual fill copies -// it at EVERY verb of that class. A shutter here is a P3/P4 optimisation, -// not a correctness gap. +// kPulledEveryVerb no shutter exists at all - no MGPipeDirty bit moves on any path through +// this mutator - and none is needed yet: the PipeInputs field it writes is +// in its verb class's may-read mask, so the residual fill copies it at +// EVERY verb of that class. A shutter here is a P3/P4 optimisation, not a +// correctness gap. +// kPulledPartialShutter +// the same pull, but a bit DOES move - on some of the paths that mutate, +// not all of them - so this row must never be read as "no shutter exists". +// The bits that move are named after the '|', which is the one place this +// file joins a prose answer with a bit, and the reason is exactly that a +// P3a shutter builder has to be able to tell "no bit covers this" from "a +// bit covers half of it". The named bits are checked the same way every +// other bit answer is - a dead one is a red gate - but they are NOT a +// licence to narrow: what holds on every mutating path is the pull. +// Which rows need this answer is a human judgement and stays one: the +// derivation's "it does move that shutter" direction over-approximates +// (a call name resolves to every body of that name, a write inside an +// `if` counts), so it can refute a named bit but cannot find the rows +// that should have named one. // // KNOWN BLIND SPOTS OF THE SCANNER, recorded here rather than left implicit // (gen_pipe_dirty_surface.py's own notes plus its scan root): @@ -160,16 +196,21 @@ X(SetViewport, NEW_RENDER_STATE) \ X(SetViewportIndexed, NEW_RENDER_STATE) \ /* ---- the other value-class bits ---- */ \ - /* NOT NEW_PIXEL_PACK, though half of it does move that bit: RenderState::SetPixelStore */ \ - /* Param writes BOTH halves - eight Pack arms and eight Unpack arms - while the */ \ - /* tracker's bit 2 is a byte compare of the PACK half alone (Tracker.h), because */ \ - /* set_pixel_pack_state deliberately has no unpack counterpart (ARCHITECTURE.md 4.6). */ \ - /* So glPixelStorei(GL_UNPACK_ALIGNMENT, 8) and its seven siblings move NOTHING that */ \ - /* bit reads, and naming it here would be an under-firing shutter for eight of the */ \ - /* sixteen arms. What is true on every path is the pull: GetPixelStoreParameters is one */ \ - /* of the two Coverage.def rows an emitted call does not supply completely (PipeFill. */ \ - /* cpp), so the residual fill copies both halves at every verb of the class. */ \ - X(SetPixelStoreParam, kPulledEveryVerb) \ + /* kPulledPartialShutter, NOT kPulledEveryVerb, and NOT a bare NEW_PIXEL_PACK: */ \ + /* RenderState::SetPixelStoreParam writes BOTH halves - eight Pack arms and eight */ \ + /* Unpack arms - while the tracker's bit 2 is a byte compare of the PACK half alone */ \ + /* (Tracker.h), because set_pixel_pack_state deliberately has no unpack counterpart */ \ + /* (ARCHITECTURE.md 4.6). So glPixelStorei(GL_PACK_ALIGNMENT, 8) DOES move bit 2 and */ \ + /* glPixelStorei(GL_UNPACK_ALIGNMENT, 8) moves nothing at all, and a shutter narrowed */ \ + /* to bit 2 would under-fire for eight of the sixteen arms. What is true on every path */ \ + /* is the pull: GetPixelStoreParameters is one of the two Coverage.def rows an emitted */ \ + /* call does not supply completely (PipeFill.cpp), so the residual fill copies both */ \ + /* halves at every verb of the class. The bit is named anyway because P2 already EMITS */ \ + /* set_pixel_pack_state off it: a row that said "no shutter exists" about the only */ \ + /* mutator behind a shipped call would be a false answer to the one question D16 hands */ \ + /* P3a. Splitting this setter into a pack half and an unpack half is what would let the */ \ + /* pack half answer NEW_PIXEL_PACK outright; that is P3's move, not P2's. */ \ + X(SetPixelStoreParam, kPulledPartialShutter|NEW_PIXEL_PACK) \ X(SetPatchDefaultInnerLevel, NEW_PATCH_STATE|NEW_RENDER_STATE|NEW_PIPELINE_STATE) \ X(SetPatchDefaultOuterLevel, NEW_PATCH_STATE|NEW_RENDER_STATE|NEW_PIPELINE_STATE) \ /* Also an immediate publish point, but it has a real bit and the bit is */ \ diff --git a/scripts/gen_pipe_dirty_surface.py b/scripts/gen_pipe_dirty_surface.py index cbda9d23..d24923ca 100644 --- a/scripts/gen_pipe_dirty_surface.py +++ b/scripts/gen_pipe_dirty_surface.py @@ -27,6 +27,14 @@ fires on only some paths through the setter (or omits one that always fires) is it a row could be wrong in exactly the direction ARCHITECTURE.md 13.2 calls dangerous while --check stayed green, which is how two rows in this mapping were wrong for a whole review. +Every other bit answer is derived too, one-directionally, against the shutter Tracker.h +builds for that bit. That half makes an ABSENCE claim ("this mutator writes nothing that +shutter reads"), so it is only as good as the code it reads, and it once was not: it missed +every write through a member's field and every write the preprocessor pastes together, and +reported their absence as a fact about RenderState.cpp. It now expands MG_State's +function-like macros, records the member as well as the field, and DECLINES - prints the row +and the site, and does not fail it - wherever it cannot say it read every writer. + python3 scripts/gen_pipe_dirty_surface.py # human-readable report python3 scripts/gen_pipe_dirty_surface.py --summary # counts only python3 scripts/gen_pipe_dirty_surface.py --check # THE GATE: rc 1 on any hole @@ -157,7 +165,16 @@ DIRTY_NAME_RE = re.compile(r'^\s*"(NEW_[A-Z0-9_]+)",\s*$', re.M) # header; a row that uses anything else is a typo, and a typo that read as "mapped" would be # exactly the silent hole this gate exists to close. NON_BIT_ANSWERS = ("kImmediate", "kReverseChannel", "kNoBackendRead", "kExplicitDestroy", - "kUnpublishedDestroy", "kPulledEveryVerb") + "kUnpublishedDestroy", "kPulledEveryVerb", "kPulledPartialShutter") + +# The one non-bit answer that must NOT stand alone. kPulledEveryVerb says "no shutter exists"; +# kPulledPartialShutter says "the pull is what holds on every mutating path, and these bits DO +# move on some of them" - so it carries those bits, and the derivation below checks that each +# one really is movable by that mutator. Without the distinction, a reader of this file (P3a +# builds its shutters from it) is told that the mutator behind a bit P2 already emits a call +# for has no shutter at all, which is exactly what X(SetPixelStoreParam, kPulledEveryVerb) +# said about NEW_PIXEL_PACK. +PARTIAL_ANSWERS = ("kPulledPartialShutter",) # ---- the render-state answers, DERIVED rather than believed ----------------------------- # The two RenderState counters are the one place in the mapping where "what publishes this" @@ -246,15 +263,47 @@ def render_state_publishers(): # MGPipeNoteAggregate's own switch rather than assumed; # 4. a row claiming bit B for mutator M is UNDER-FIRING when M writes nothing B reads. # -# It is deliberately ONE-DIRECTIONAL. Step 3 is an over-approximation (a call name resolves -# to every body of that name, and a write inside an `if` counts), so "M does write something -# B reads" is not proof that it does so on every path and cannot be turned into a MISSING -# check without false reds. "M writes NOTHING B reads" needs no such assumption, and it is -# the under-firing direction ARCHITECTURE.md 13.2 calls the dangerous one - which is what -# was wrong in this file: X(SetNamedTransformFeedbackBinding, NEW_SO_TARGETS) named a -# shutter that moves on NO path through that mutator. +# STEP 4 IS AN ABSENCE CLAIM, so step 3 must not MISS a write, and that is an obligation this +# script violated rather than a slogan: until this commit a write through a member's FIELD +# (`m_foo.bar = v`) recorded FIELD:bar and never MEM:m_foo, while the reader side resolves an +# accessor to MEM:m_foo - so the two halves could not meet for any struct-valued member, and +# the gate printed "SetPixelStoreParam writes nothing NEW_PIXEL_PACK's shutter reads" about a +# setter whose entire body is sixteen writes to exactly that member. It could not see them +# twice over, because those writes are spelled with the token-pasting operator +# (RenderState.cpp's SET_PIXEL_STORE_PARAM) and the file was read raw, so the "field" it +# recorded was the MACRO PARAMETER NAME. What step 3 models is therefore written down here, +# and what it does not model is DECLINED rather than answered: +# +# modelled ++m_x / m_x = / m_x op=; a write through a member-rooted lvalue (m_x.f, +# m_x[i].f, m_x->f, nested), which records BOTH MEM:m_x and FIELD:f; a bare +# `.f =` write (FIELD:f alone - the FIELD tokens are coarse, and coarse only +# ever WIDENS what a mutator is credited with writing, which is the safe +# direction for an absence claim); MGP_NOTE_AGGREGATE; a call to any function +# whose body is under the two roots; and any function-like macro defined under +# the two roots, which is EXPANDED first, token pasting performed. +# declined a body that still contains `##` after expansion, or that invokes a macro this +# script can see but could not expand and whose body pastes tokens. The taint is +# a token like every other, so it travels the same call-graph fixed point the +# writes do: a mutator that REACHES an unreadable body gets no verdict either. +# A shutter member with a write-shaped occurrence OUTSIDE the two roots is +# declined the same way - the analysis has not read every writer, so it cannot +# say there is none. --check prints every decline with the site that caused it. +# +# What remains one-directional is the OTHER direction: a call name resolves to every body of +# that name, a write inside an `if` counts, and a reader that resolves through a ternary +# yields both members - so "M does write something B reads" is not proof that it does so on +# every path and cannot become a MISSING check without false reds. That is why a mutator +# whose bit moves on half its arms answers kPulledPartialShutter rather than the bit. "M +# writes NOTHING B reads" is the direction the gate fails on, and it is the under-firing one +# ARCHITECTURE.md 13.2 calls dangerous - which is what was wrong in this file: +# X(SetNamedTransformFeedbackBinding, NEW_SO_TARGETS) named a shutter that moves on NO path +# through that mutator. STATE_ROOTS = (os.path.join(REPO_ROOT, "MobileGL", "MG_State", "GLState"), os.path.join(REPO_ROOT, "MobileGL", "MG_Impl", "Pipe")) +# Everything the absence claim has to be checked against, which is wider than what it reads: +# a write to a shutter member from outside STATE_ROOTS is a writer this analysis never looks +# at, and the only honest answer to a row whose shutter has one is "declined". +MOBILEGL_ROOT = os.path.join(REPO_ROOT, "MobileGL") UPDATE_RE = re.compile(r"Uint32\s+Update\s*\(") NOW_RE = re.compile(r"now\[Index\(MGPipeDirty::(\w+)\)\]\s*=\s*([^;]*);") DIRTY_OR_RE = re.compile(r"dirty\s*\|=\s*MGPipeDirtyBit\(MGPipeDirty::(\w+)\)") @@ -269,38 +318,283 @@ MEMBER_CALL_RE = re.compile(r"\b(m_\w+)\s*\.\s*(\w+)\s*\(") CALL_RE = re.compile(r"\b(\w+)\s*\(") WORD_RE = re.compile(r"\b(\w+)\b") AGGREGATE_RE = re.compile(r"MGP_NOTE_AGGREGATE\(\s*(\w+)\s*\)") -MEMBER_WRITE_RE = re.compile(r"\+\+\s*(m_\w+)|\b(m_\w+)\s*(?:\+\+|\+=|=(?!=))") -FIELD_WRITE_RE = re.compile(r"\.\s*(\w+)\s*(?:\[[^\]]*\])?\s*(?:\+\+|\+=|=(?!=))") AGGREGATE_CASE_RE = re.compile(r"case\s+MGPipeAggregate::(\w+)\s*:\s*([^;]*);") +# The assignment operators, as a suffix every write pattern below shares. `=(?!=)` keeps == +# out; !=, <= and >= cannot match at all, because the character where the operator must start +# is then `!`, `<` or `>` and no alternative here begins with one except <<= / >>=. +ASSIGN = r"(?:\+\+|--|\+=|-=|\*=|/=|%=|&=|\|=|\^=|<<=|>>=|=(?!=))" +# A write to the member itself: ++m_x, m_x = v, m_x += v. +MEMBER_WRITE_RE = re.compile(r"\+\+\s*(m_\w+)|\b(m_\w+)\s*%s" % ASSIGN) +# A write THROUGH a member: m_x.f = v, m_x[i].f = v, m_x->f = v, and nested. This is the one +# that was missing, and its absence is why the reader side (which resolves an accessor to +# MEM:m_x) and the writer side could never meet for a struct-valued member. +MEMBER_ROOTED_WRITE_RE = re.compile( + r"\b(m_\w+)\s*(?:\[[^;\n]*?\]|\.\s*\w+|->\s*\w+)+\s*%s" % ASSIGN) +# A write to a field whose base this script did not resolve to a member. Coarse on purpose: +# a FIELD token only ever widens what a mutator is credited with writing. +FIELD_WRITE_RE = re.compile(r"\.\s*(\w+)\s*(?:\[[^\]]*\])?\s*%s" % ASSIGN) + +# ---- the preprocessor's half of the write analysis -------------------------------------- +# Sixteen of RenderState.cpp's writes exist only after the preprocessor has run: SET_PIXEL_ +# STORE_PARAM (:829) pastes `m_pixelStore##paramNameHead##Parameters`, and SET_CAPABILITY +# (:309) pastes `m_parameters.capability##Enabled`. Read raw, neither is a write to any token +# this script can name, so it saw none of them and reported their absence as a fact. So the +# function-like macros defined under the two roots are expanded first, and the ones that +# cannot be expanded are recorded so that a body reaching one is declined rather than +# answered. +MACRO_DIRECTIVE_RE = re.compile( + r"^[ \t]*#[ \t]*(define|undef)[ \t]+(\w+)(\([^()\n]*\))?((?:\\\n|[^\n])*)", re.M) +# MGP_NOTE_AGGREGATE is modelled directly (its hop is read out of MGPipeNoteAggregate's own +# switch), so expanding it would delete the very token AGGREGATE_RE looks for. +NEVER_EXPAND = frozenset(("MGP_NOTE_AGGREGATE", "MGP_NOTE_MUTATION")) +MACRO_BODY_LIMIT = 4000 +PASTE_RE = re.compile(r"##") +# A member DECLARATION - a type, then the name, then the end of the statement. Used to keep +# the containment check below from confusing two classes that spell a member the same way: +# MG_Backend/MGPipe/PipeInputs.h has its own m_transformFeedbackGeneration, and a write to +# THAT one says nothing about who writes GLContext's. +MEMBER_DECL_RE = re.compile( + r"^[ \t]*[A-Za-z_][\w:<>,&*\s]*?[\s&*](m_\w+)\s*(?:\[[^\]\n]*\])?" + r"\s*(?:=[^;\n]*|\{[^}\n]*\})?;", re.M) + + +def source_files(root): + """Every .h/.cpp under `root`, in a stable order.""" + out = [] + for directory, _, files in os.walk(root): + for name in sorted(files): + if name.endswith((".h", ".cpp")): + out.append(os.path.join(directory, name)) + return sorted(out) + + +def macro_definitions(masked): + """(kind, name, params-or-None, body, start, end) for every #define / #undef, with line + continuations joined.""" + for match in MACRO_DIRECTIVE_RE.finditer(masked): + yield (match.group(1), match.group(2), match.group(3), + match.group(4).replace("\\\n", " "), match.start(), match.end()) + + +def blank_directives(masked): + """`masked` with every #define / #undef blanked (newlines kept), so a macro BODY is never + read as code belonging to whatever function encloses the directive - RenderState.cpp + defines SET_PIXEL_STORE_PARAM *inside* SetPixelStoreParam, and read raw its body + contributed a field literally named `paramNameTail`.""" + out = list(masked) + for _, _, _, _, start, end in macro_definitions(masked): + for index in range(start, end): + if out[index] != "\n": + out[index] = " " + return "".join(out) + + +def balanced(text): + counts = {"(": 0, "[": 0, "{": 0} + closing = {")": "(", "]": "[", "}": "{"} + for char in text: + if char in counts: + counts[char] += 1 + elif char in closing: + counts[closing[char]] -= 1 + if counts[closing[char]] < 0: + return False + return not any(counts.values()) + + +def macro_table(paths): + """({name: (params, body)} this script will expand, {name: body} it saw and refused). + + Function-like macros only - an object-like macro is a constant and expanding it buys the + write analysis nothing. A macro is REFUSED when expanding it could corrupt the brace + matching every function body here is found by (an unbalanced body), when it is variadic, + over-long or defined more than once with different bodies, or when the analysis models it + directly. A refused macro is not silently ignored: if its body pastes tokens, every body + that invokes it is declined.""" + seen = {} + for path in paths: + with open(path, "r", encoding="utf-8", errors="replace") as handle: + masked = mask_comments_and_strings(handle.read()) + for kind, name, params, body, _, _ in macro_definitions(masked): + if kind == "undef" or params is None: + continue + seen.setdefault(name, []).append((params, body)) + expandable = {} + refused = {} + for name, definitions in seen.items(): + bodies = set(body for _, body in definitions) + params = [part.strip() for part in definitions[0][0][1:-1].split(",") if part.strip()] + body = definitions[0][1] + if (name in NEVER_EXPAND or len(bodies) > 1 or len(body) > MACRO_BODY_LIMIT + or not balanced(body) or any(not part.isidentifier() for part in params)): + refused[name] = " ".join(sorted(bodies)) + continue + expandable[name] = (params, body) + return expandable, refused + + +def matching_paren(text, open_index): + depth = 0 + for index in range(open_index, len(text)): + if text[index] == "(": + depth += 1 + elif text[index] == ")": + depth -= 1 + if depth == 0: + return index + return None + + +def split_arguments(text): + args = [] + current = [] + depth = 0 + for char in text: + if char in "([{": + depth += 1 + elif char in ")]}": + depth -= 1 + if char == "," and depth == 0: + args.append("".join(current)) + current = [] + continue + current.append(char) + if current or args: + args.append("".join(current)) + return args + + +def substitute(body, params, args): + """One macro expansion: parameters replaced, then `##` pasted away - which is the step + that turns `m_pixelStore##paramNameHead##Parameters` into a member this script can name.""" + text = body + for param, arg in sorted(zip(params, args), key=lambda pair: -len(pair[0])): + text = re.sub(r"\b%s\b" % re.escape(param), lambda _match, value=arg: value, text) + return re.sub(r"\s*##\s*", "", text) + + +def expand_macros(masked, expandable, rounds=4): + """`masked` with its directives blanked and every invocation of an expandable + function-like macro replaced by its expansion.""" + text = blank_directives(masked) + if not expandable: + return text + pattern = re.compile(r"\b(%s)\s*\(" % "|".join(sorted((re.escape(name) for name in expandable), + key=len, reverse=True))) + for _ in range(rounds): + out = [] + index = 0 + changed = False + while True: + match = pattern.search(text, index) + if match is None: + out.append(text[index:]) + break + close = matching_paren(text, match.end() - 1) + params, body = expandable[match.group(1)] + args = split_arguments(text[match.end():close]) if close is not None else None + if args is None or len(args) != len(params): + out.append(text[index:match.end()]) + index = match.end() + continue + out.append(text[index:match.start()]) + out.append(" %s " % substitute(body, params, args)) + index = close + 1 + changed = True + text = "".join(out) + if not changed: + break + return text + + +def unmodelled_sites(body, relative, pasting): + """The constructs in `body` this script does NOT model, as decline reasons. Today there + is exactly one class of them, and it is the one that produced a false verdict: a token + paste it could not expand.""" + sites = set() + if PASTE_RE.search(body): + sites.add("%s: a `##` token paste no visible macro definition expands" % relative) + for match in CALL_RE.finditer(body): + if match.group(1) in pasting: + sites.add("%s: %s(), a token-pasting macro this script refused to expand" + % (relative, match.group(1))) + return sites + def state_bodies(): - """{function name: [body text]} over MG_State/GLState and MG_Impl/Pipe.""" - bodies = {} + """({function name: [body text]}, {function name: {decline reason}}, [analysed paths]) + over MG_State/GLState and MG_Impl/Pipe, macros expanded first.""" + paths = [] for root in STATE_ROOTS: - for directory, _, files in os.walk(root): - for name in sorted(files): - if not name.endswith((".h", ".cpp")): - continue - path = os.path.join(directory, name) - with open(path, "r", encoding="utf-8", errors="replace") as handle: - masked = mask_comments_and_strings(handle.read()) - for fn, start, end in function_bodies(masked): - bodies.setdefault(fn, []).append(masked[start:end]) - return bodies + paths += source_files(root) + expandable, refused = macro_table(paths) + # A macro this script MODELS is not an unread construct even though it is not expanded. + pasting = frozenset(name for name, body in refused.items() + if name not in NEVER_EXPAND and PASTE_RE.search(body)) + bodies = {} + taints = {} + for path in paths: + relative = os.path.relpath(path, REPO_ROOT).replace(os.sep, "/") + with open(path, "r", encoding="utf-8", errors="replace") as handle: + expanded = expand_macros(mask_comments_and_strings(handle.read()), expandable) + for fn, start, end in function_bodies(expanded): + body = expanded[start:end] + bodies.setdefault(fn, []).append(body) + sites = unmodelled_sites(body, relative, pasting) + if sites: + taints.setdefault(fn, set()) + taints[fn] |= sites + return bodies, taints, paths -def written_tokens(bodies): +def writers_outside(analysed): + """{MEM token: a file OUTSIDE the analysed roots that writes it}. + + The absence claim is only as good as the set of writers the analysis reads. Every .h/.cpp + under MobileGL/ that is not one of the analysed files is scanned with the same write + patterns, and a shutter member that turns up here is declined rather than answered. + + A file that DECLARES a member of that name is writing its own, not the frontend's - the + backend's PipeInputs mirrors half of GLState's member names - so its writes do not count. + That is the one judgement here, and it is the conservative way round only for names the + two sides share; a genuine outside writer of a frontend member does not declare it.""" + seen = set(analysed) + out = {} + for path in source_files(MOBILEGL_ROOT): + if path in seen: + continue + with open(path, "r", encoding="utf-8", errors="replace") as handle: + masked = mask_comments_and_strings(handle.read()) + relative = os.path.relpath(path, REPO_ROOT).replace(os.sep, "/") + own = set(MEMBER_DECL_RE.findall(masked)) + written = set(match.group(1) or match.group(2) for match in MEMBER_WRITE_RE.finditer(masked)) + written |= set(match.group(1) for match in MEMBER_ROOTED_WRITE_RE.finditer(masked)) + for member in written - own: + out.setdefault("MEM:" + member, relative) + return out + + +def written_tokens(bodies, taints=None): """{function name: set of tokens it transitively WRITES}, a fixed point over call names. - A token is MEM:, FIELD: or AGG:.""" + A token is MEM:, FIELD:, AGG: or + TAINT:. The last is not a write: it is "this body contains something the analysis + does not model", and it rides the same fixed point so that a mutator which REACHES an + unreadable body inherits it and is declined rather than answered.""" + taints = taints or {} reach = {} for name, bodylist in bodies.items(): - tokens = set() + tokens = set("TAINT:" + site for site in taints.get(name, ())) for body in bodylist: tokens |= set("AGG:" + m.group(1) for m in AGGREGATE_RE.finditer(body)) for match in MEMBER_WRITE_RE.finditer(body): tokens.add("MEM:" + (match.group(1) or match.group(2))) + # A write THROUGH a member records the member AND the field: the reader side + # resolves an accessor to the member, so recording only the field is what kept + # the two halves from ever meeting. + for match in MEMBER_ROOTED_WRITE_RE.finditer(body): + tokens.add("MEM:" + match.group(1)) tokens |= set("FIELD:" + m.group(1) for m in FIELD_WRITE_RE.finditer(body)) reach[name] = tokens changed = True @@ -407,11 +701,16 @@ def shutter_readers(): # builds the value being compared. for match in DIRTY_OR_RE.finditer(body): # Since the previous `dirty |=` of ANY form - the counter loop's included, or the - # window would start at the top of the walk and inherit every other bit's readers. + # window would start at the top of the walk and inherit every other bit's readers - + # AND since the last `}` before this one, whichever is later. The second boundary is + # what keeps the pack block's trailing `m_pack = pack;` out of the PATCH bit's + # window: `pack` is a local of the walk, so it expands to ctx.GetPixelStore + # Parameters and the patch shutter would read as though it compared the pixel store. previous = 0 for boundary in DIRTY_ANY_RE.finditer(body, 0, match.start()): previous = boundary.end() - window = body[previous:match.start()] + closing = body.rfind("}", 0, match.start()) + window = body[max(previous, closing + 1):match.start()] out.setdefault(match.group(1), set()) out[match.group(1)] |= readers_of(window) return out @@ -488,41 +787,59 @@ def answer_set(answer): return {part.strip() for part in answer.split("|") if part.strip()} -def object_class_problems(mapping, bits, movers, moved): +def object_class_problems(mapping, bits, movers, moved, outside=None): """The under-firing check for every answer the RenderState derivation cannot reach. - Returns (problems, verified, unverified) - `unverified` names the rows the derivation - had to decline, with the reason, so --check reports its own coverage instead of letting - a row it never looked at read as checked.""" + Returns (problems, verified, declined) - `declined` names the rows the derivation REFUSED + to answer, with the reason. Every branch below that does not end in a verdict ends here + instead, because the alternative is what this gate did to NEW_PIXEL_PACK: turn "this + script cannot read that construct" into "that mutator writes nothing".""" + outside = outside or {} problems = [] verified = 0 - unverified = [] + declined = [] render_bits = {RENDER_STATE_BIT, PIPELINE_STATE_BIT} for mutator in sorted(mapping): claimed = (answer_set(mapping[mutator]) & bits) - render_bits if not claimed: continue if mutator not in moved: - unverified.append("%s (no body found under MG_State/GLState or MG_Impl/Pipe to " - "derive from)" % mutator) + declined.append("%s (no body found under MG_State/GLState or MG_Impl/Pipe to " + "derive from)" % mutator) continue + blind = sorted(token[6:] for token in moved[mutator] if token.startswith("TAINT:")) for bit in sorted(claimed): shutter, resolved = movers.get(bit, (set(), False)) if not resolved: - unverified.append("%s <- %s (Tracker.h's shutter for that bit reads something " - "this script cannot resolve to a member)" % (mutator, bit)) + declined.append("%s <- %s (Tracker.h's shutter for that bit reads something " + "this script cannot resolve to a member)" % (mutator, bit)) continue if moved[mutator] & shutter: verified += 1 continue + # Everything past here would be an ABSENCE claim, so the gate first has to be + # able to say it read every writer of that shutter. Two things stop it, and each + # is a decline rather than a verdict. + unread = sorted(set(outside[token] for token in shutter if token in outside)) + if unread: + declined.append("%s <- %s (that shutter's members are written outside the " + "analysed roots too, e.g. %s, so 'it writes nothing that " + "shutter reads' is not a fact this script has)" + % (mutator, bit, unread[0])) + continue + if blind: + declined.append("%s <- %s (it reaches %s, so the write analysis is not " + "complete for this mutator)" % (mutator, bit, blind[0])) + continue problems.append( "UNDER-FIRING answer %s for %s - it writes nothing %s's shutter reads " "(shutter: %s), so a mutation through it publishes nothing" % (bit, mutator, bit, ", ".join(sorted(t.split(":", 1)[1] for t in shutter)))) - return problems, verified, unverified + return problems, verified, declined -def check_mapping(mapping, duplicates, scanned, bits, publishers=None, movers=None, moved=None): +def check_mapping(mapping, duplicates, scanned, bits, publishers=None, movers=None, moved=None, + outside=None): """Every problem the gate fails on, as a list of human-readable lines. BOTH directions: an unmapped mutator renders stale, and a row naming a mutator the scan no longer finds is a stale row that would keep a real hole looking covered. `publishers` is @@ -546,12 +863,17 @@ def check_mapping(mapping, duplicates, scanned, bits, publishers=None, movers=No continue problems.append("BAD answer %s for %s - not a MGPipeDirty bit name and not one of %s" % (answer, mutator, ", ".join(NON_BIT_ANSWERS))) - if len(answers) > 1 and answers & set(NON_BIT_ANSWERS): + partial = answers & set(PARTIAL_ANSWERS) + if len(answers) > 1 and (answers & set(NON_BIT_ANSWERS)) - partial: problems.append("BAD answer %s for %s - a non-bit answer stands alone" % (mapping[mutator], mutator)) + if partial and not answers & bits: + problems.append("BAD answer %s for %s - %s has to NAME the bits that move on some " + "of the paths that mutate; kPulledEveryVerb is the answer when " + "none does" % (mapping[mutator], mutator, "|".join(sorted(partial)))) if movers is not None and moved is not None: - object_problems, _, _ = object_class_problems(mapping, bits, movers, moved) + object_problems, _, _ = object_class_problems(mapping, bits, movers, moved, outside) problems += object_problems if publishers is None: @@ -615,7 +937,7 @@ def scan_all(): return sources, per_file, distinct_all -def self_test(scanned, bits, publishers, movers, moved): +def self_test(scanned, bits, publishers, movers, moved, outside=None): """Canned negative controls. Each MUST trip; trips == 0 is an error, which is the shape check_include_closure.py and gen_pipe.py --self-test already use.""" trips = 0 @@ -679,7 +1001,7 @@ def self_test(scanned, bits, publishers, movers, moved): with_dead_shutter = dict(real) with_dead_shutter["SetNamedTransformFeedbackBinding"] = "NEW_SO_TARGETS" problems = check_mapping(with_dead_shutter, real_duplicates, scanned, bits, publishers, - movers, moved) + movers, moved, outside) if any("UNDER-FIRING answer NEW_SO_TARGETS" in p for p in problems): trips += 1 else: @@ -691,13 +1013,69 @@ def self_test(scanned, bits, publishers, movers, moved): with_wrong_bit = dict(real) with_wrong_bit["SetCurrentVertexAttributeInt"] = "NEW_PIXEL_PACK" problems = check_mapping(with_wrong_bit, real_duplicates, scanned, bits, publishers, - movers, moved) + movers, moved, outside) if any("UNDER-FIRING answer NEW_PIXEL_PACK" in p for p in problems): trips += 1 else: failures.append("negative control 7 (a value-class answer whose shutter the mutator " "never moves) did NOT trip") + # 8. A row that says kPulledPartialShutter and names no bit. The whole point of that + # answer is to carry the bits that DO move, so an empty one is kPulledEveryVerb with + # a different spelling - and kPulledEveryVerb is the claim that got NEW_PIXEL_PACK + # wrong in the first place. + with_empty_partial = dict(real) + with_empty_partial["SetPixelStoreParam"] = "kPulledPartialShutter" + problems = check_mapping(with_empty_partial, real_duplicates, scanned, bits) + if any("has to NAME the bits" in p for p in problems): + trips += 1 + else: + failures.append("negative control 8 (kPulledPartialShutter naming no bit) did NOT trip") + + # 9. THE CONTROL FOR THE DECLINE PATH, and it is the shape of the defect this round fixed. + # The gate did not merely miss a check: it turned "this script cannot read that + # construct" into "that mutator writes nothing", and printed a verdict about + # RenderState.cpp that was false. So a mutator whose reachable text contains something + # the analysis does not model has to come out DECLINED and must NOT appear as a + # problem, whatever else is true of it. + blinded = dict(moved) + blinded["SetCurrentVertexAttributeInt"] = {"TAINT:a canned unexpandable token paste"} + problems, _, declined = object_class_problems(with_wrong_bit, bits, movers, blinded, outside) + if (not any("SetCurrentVertexAttributeInt" in p for p in problems) + and any(d.startswith("SetCurrentVertexAttributeInt <- NEW_PIXEL_PACK") for d in declined)): + trips += 1 + else: + failures.append("negative control 9 (a mutator whose write analysis is incomplete) did " + "NOT decline - the gate answered a question it cannot answer") + + # 10. The other decline reason, and the other half of the same principle: a shutter whose + # members have a writer OUTSIDE the roots this analysis reads. "Nobody writes it" is + # not a claim about code the script never opened. + hidden = dict(outside or {}) + for token in movers.get("NEW_PIXEL_PACK", (set(), False))[0]: + hidden[token] = "MobileGL/MG_Backend/a-file-this-analysis-never-reads.cpp" + problems, _, declined = object_class_problems(with_wrong_bit, bits, movers, moved, hidden) + if (not any("SetCurrentVertexAttributeInt" in p for p in problems) + and any("outside the analysed roots" in d for d in declined)): + trips += 1 + else: + failures.append("negative control 10 (a shutter member written outside the analysed " + "roots) did NOT decline - the gate claimed an absence over code it " + "never read") + + # THE POSITIVE CONTROL, and it is the row that was wrong: RenderState::SetPixelStoreParam + # writes NEW_PIXEL_PACK's shutter member sixteen times, through a token-pasting macro. If + # this ever reads as UNDER-FIRING again, the write analysis has lost the preprocessor and + # every absence claim in this gate is worthless. + with_the_bit = dict(real) + with_the_bit["SetPixelStoreParam"] = "NEW_PIXEL_PACK" + problems, _, declined = object_class_problems(with_the_bit, bits, movers, moved, outside) + if any("SetPixelStoreParam" in line for line in problems + declined): + failures.append("the positive control (SetPixelStoreParam DOES write " + "NEW_PIXEL_PACK's shutter) did not pass: %s" + % "; ".join(line for line in problems + declined + if "SetPixelStoreParam" in line)) + for failure in failures: print("dirty-surface self-test: %s" % failure) if trips == 0: @@ -706,7 +1084,8 @@ def self_test(scanned, bits, publishers, movers, moved): return 1 if failures: return 1 - print("dirty-surface self-test: %d negative controls, all tripped" % trips) + print("dirty-surface self-test: %d negative controls, all tripped; positive control OK " + "(SetPixelStoreParam's sixteen token-pasted writes are read)" % trips) return 0 @@ -734,8 +1113,9 @@ def main(): publishers = render_state_publishers() if not publishers: sys.exit("could not derive any RenderState setter out of %s" % RENDER_STATE_PATH) - bodies = state_bodies() - reach = written_tokens(bodies) + bodies, taints, analysed = state_bodies() + reach = written_tokens(bodies, taints) + outside = writers_outside(analysed) aggregates = aggregate_tokens(bodies, reach) moved = {name: expand_aggregates(tokens, aggregates) for name, tokens in reach.items()} readers = shutter_readers() @@ -749,13 +1129,13 @@ def main(): movers = shutter_movers(readers, bodies, aliases) if args.self_test: - return self_test(distinct_all, bits, publishers, movers, moved) + return self_test(distinct_all, bits, publishers, movers, moved, outside) mapping, duplicates = load_mapping() if args.check: problems = check_mapping(mapping, duplicates, distinct_all, bits, publishers, movers, - moved) + moved, outside) for problem in problems: print("dirty-surface: %s" % problem) if problems: @@ -764,7 +1144,7 @@ def main(): "RenderState.cpp actually publishes" % len(problems)) return 1 derived = sum(1 for m in mapping if m in publishers) - _, verified, unverified = object_class_problems(mapping, bits, movers, moved) + _, verified, declined = object_class_problems(mapping, bits, movers, moved, outside) prose = sorted(m for m in mapping if not (answer_set(mapping[m]) & bits)) print("dirty-surface: %d mutators, all mapped, no stale rows; %d render-state answers " "derived from RenderState.cpp and matching" % (len(mapping), derived)) @@ -773,10 +1153,22 @@ def main(): print("dirty-surface: %d other bit answers derived from their shutter in Tracker.h " "(under-firing only); %d declined; %d rows carry a prose answer (%s) that no " "derivation checks" - % (verified, len(unverified), len(prose), + % (verified, len(declined), len(prose), ", ".join(sorted(set(a for m in prose for a in answer_set(mapping[m])))))) - for row in unverified: - print("dirty-surface: not derived: %s" % row) + for row in declined: + print("dirty-surface: DECLINED, no verdict: %s" % row) + # The absence claim's own footprint, printed rather than assumed: what the write + # analysis read, and the one place it is still coarse. + print("dirty-surface: the under-firing half read %d function bodies across %d files " + "under %s, macros expanded first; %d of those bodies carry a construct it does " + "not model and taint whatever reaches them; %d members are written outside " + "those roots and any shutter that names one is declined; a FIELD token is " + "matched by name and not scoped to a type, so it only ever WIDENS what a " + "mutator is credited with writing" + % (sum(len(v) for v in bodies.values()), len(analysed), + " + ".join(os.path.relpath(root, REPO_ROOT).replace(os.sep, "/") + for root in STATE_ROOTS), + len(taints), len(outside))) return 0 total_functions = 0