b712b918c9fdf2aac65b5f6ceb076e4af584231c braney Tue Sep 1 12:29:36 2026 -0700 trackDbConditions: scan the whole library, and check the scanner against known cases, refs #37908 Four refinements, one of which fixes wrong output rather than noisy output. The scanned file list was hand-kept, and it had silently missed netCart.c, chainCart.c, pgSnp.c, hgMaf.c and a dozen more that read track settings on the drawing path. An unscanned read site is worse than an unclassified one: the every-path analysis was claiming a condition holds at every read of a setting while never having seen one of the reads. Three settings were carrying false claims because of it, barChartBars, barChartCategoryUrl and bigDataUrl. So scan hg/lib and hg/cgilib whole and let reachability decide which side of the browser each read belongs to. Coverage goes from 325 settings to 347. An early return is treated as a precondition only near the top of a function. The same shape four hundred lines down is sound but says nothing about the setting, and it was how the jsonp output check at the tail of doTrackForm came to look like a condition on filterBy. A negated disjunction is a conjunction, so NOT (A || B) now splits into NOT A and NOT B. The squishyPack guard was one unsplittable string that was neither a visibility condition nor a coverage one; it is now correctly both. --self-test checks the harvest against ten cases read out of the C by hand. Every one of them broke at least once while this was being built, usually silently, so they are checked rather than trusted, and --check runs them first and refuses to report anything if the scanner itself has moved. It earned its place immediately by catching two misclassifications in the same commit that added it. Also caches the harvest in a temp file keyed on the newest source mtime, since scanning takes forty seconds and reading the output takes several runs. Warm runs are now instant. diff --git src/hg/utils/trackDbConditions/harvestConditions.py src/hg/utils/trackDbConditions/harvestConditions.py index dcec4055008..02312851ca0 100755 --- src/hg/utils/trackDbConditions/harvestConditions.py +++ src/hg/utils/trackDbConditions/harvestConditions.py @@ -84,36 +84,39 @@ KEYWORDS = ("if", "while", "for", "switch") NOT_A_CALL = set(KEYWORDS) | {"else", "return", "sizeof", "case", "defined", "do"} # The two scopes. A file can be in both: wiggleCart.c is read from the drawing # code and from the config page, and the answer differs. SCOPES = { "render": ["hg/hgTracks/*.c"], "config": ["hg/hgTrackUi/*.c", "hg/hgTracks/config.c", "hg/hgTracks/searchTracks.c"], } # In hgTracks but not drawing: the configuration page and the track search page # both read track settings, and neither is the picture. NOT_RENDER = ("hg/hgTracks/config.c", "hg/hgTracks/searchTracks.c") -# Scanned for both scopes. hui.c and the *Ui.c files are not only the config -# page: they also hold the accessors the drawing code calls, so hicUiGetArcLimit -# has to be visible to the render graph as well as the config one. Which scope -# a read belongs to is then decided by who calls it, not by which file it is in. -SHARED = ["hg/lib/hui.c", "hg/lib/*Ui.c", "hg/lib/wiggleCart.c", - "hg/lib/trackDbCustom.c", "hg/lib/hdb.c"] +# Scanned for both scopes. Naming the libraries one at a time was wrong twice +# over: hui.c and the *Ui.c files are not only the config page, they hold the +# accessors the drawing code calls, and a hand-kept list silently missed +# netCart.c, chainCart.c, pgSnp.c and a dozen more that read track settings on +# the drawing path. A read site that is not scanned is worse than one that is +# unclassified: the every-path analysis would claim a condition holds at every +# read while never having seen one of them. So scan the libraries whole and let +# reachability decide which side of the browser each read belongs to. +SHARED = ["hg/lib/*.c", "hg/cgilib/*.c"] # A read reached with none of these is unguarded. Anything matching is noise # rather than a condition on the setting: it says the code got far enough to # run, not that the setting only applies sometimes. NOISE = re.compile( r"^NOT \((?:[\w>.\-]+(?:\s*->\s*\w+)* == NULL)" r"(?:\s*\|\|\s*[\w>.\-]+(?:\s*->\s*\w+)* == NULL)*\)$" r"|^errCatchStart\b" r"|^\w+ != NULL$" r"|^NOT \(isEmpty\(" r"|^NOT \(!\w+\)$" r"|^\w+ = \w+; \w+ != NULL" # for (x = y; x != NULL; ...) ) @@ -196,30 +199,33 @@ return len(s) def lineIndex(src): idx, line = [0] * (len(src) + 1), 1 for i, c in enumerate(src): idx[i] = line if c == "\n": line += 1 idx[len(src)] = line return idx LEAVES = re.compile(r"^\s*\{?\s*(return\b|errAbort\s*\(|continue\b|break\b)") +# How far into a function an early return still reads as a precondition. +PRECONDITION_LINES = 25 + def loadDefines(paths): """#define NAME "literal", so a macro-spelled setting is not missed. Also follows a macro defined as another macro. GRAY_LEVEL_SCORE_MIN is SCORE_MIN is "scoreMin", and stopping at the first hop loses the read. """ out, alias = {}, {} for path in paths: try: with open(path, errors="replace") as f: for line in f: m = re.match(r'\s*#define\s+([A-Z][A-Z0-9_]*)\s+"([^"]*)"', line) if m: out.setdefault(m.group(1), m.group(2)) @@ -342,34 +348,41 @@ m = re.match(r"\b(if|while|for|switch)\b", s[i:]) if m and (i == 0 or not (s[i-1].isalnum() or s[i-1] == "_")): word = m.group(1) j = skipSpace(s, i + len(word)) if j < n and s[j] == "(": close = matchParen(s, j) cond = re.sub(r"\s+", " ", src[j+1:close-1]).strip() end = stmtEnd(s, close) guards.append((close, end, word, cond, line[i])) condSpans.append((j, close)) p = skipSpace(s, end) if s[p:p+4] == "else" and not (p+4 < n and (s[p+4].isalnum() or s[p+4] == "_")): q = skipSpace(s, p + 4) guards.append((q, stmtEnd(s, q), "else", "NOT (%s)" % cond, line[p])) elif word == "if" and LEAVES.match(src[close:end]): - # the guarded statement leaves, so the rest of the function - # is only reached when the condition is false + # The guarded statement leaves, so the rest of the function + # is only reached when the condition is false. Only near the + # top of the function, though: that is a precondition, and + # people read it as one. The same shape four hundred lines + # down is just sequencing, and attributing it to every later + # read is sound but says nothing about the setting. It was + # how the jsonp output check in doTrackForm came to look like + # a condition on filterBy. for fs, fe, _ in funcs: if fs <= i < fe: + if line[i] - line[fs] <= PRECONDITION_LINES: guards.append((end, fe, "guard", "NOT (%s)" % cond, line[i])) break i = close continue i += 1 def condsAt(off): out = [] for g in guards: if g[0] <= off < g[1]: func = enclosing(g[0]) for part in splitConjuncts(g[3]): idents = re.findall(r"[A-Za-z_][A-Za-z0-9_]*", part) derived = sorted({fromSetting[(func, ident)] for ident in idents if (func, ident) in fromSetting} @@ -469,49 +482,81 @@ for one in sites[1:]: keys = {useKey(c) for c in one} common = {k: v for k, v in common.items() if k in keys} if not common: break # a test of the setting's own value says only that it is set useConds[setting] = [c for c in common.values() if c.get("derivedFrom") != [setting]] for read in reads: read["useConds"] = useConds.get(read["name"], []) if read["tdb"] else [] return {"file": rel, "funcs": [f[2] for f in funcs], "reads": reads, "calls": calls, "addrTaken": sorted(addrTaken)} -def splitConjuncts(text): - """A && B is two conditions. Splitting it is sound; splitting || is not.""" - parts, depth, cur = [], 0, "" - i = 0 +def splitOn(text, op): + """Split on a top-level operator, ignoring anything inside brackets.""" + parts, depth, cur, i = [], 0, "", 0 while i < len(text): c = text[i] if c in "([": depth += 1 elif c in ")]": depth -= 1 - if depth == 0 and text[i:i+2] == "&&": + if depth == 0 and text[i:i+2] == op: parts.append(cur) cur = "" i += 2 continue cur += c i += 1 parts.append(cur) - parts = [p.strip() for p in parts if p.strip()] + return [p.strip() for p in parts if p.strip()] + + +def unwrap(text): + """Drop one layer of parentheses when they wrap the whole expression.""" + text = text.strip() + while text.startswith("(") and text.endswith(")"): + depth = 0 + for i, ch in enumerate(text): + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0 and i != len(text) - 1: + return text # the parens are not a wrapper + text = text[1:-1].strip() + return text + + +def splitConjuncts(text): + """Break a condition into the separate things it requires. + + A && B is two conditions, and splitting it is sound. Splitting A || B is + not. But a negated disjunction is a conjunction, so NOT (A || B) splits + into NOT A and NOT B, which is where the useful ones hide: the squishyPack + guard reads NOT (visibility != tvPack || checkIfWiggling(...)), and as one + string it is neither a visibility condition nor a coverage one. + """ + inner = re.match(r"^NOT\s*\((.*)\)$", text.strip(), re.S) + if inner: + disjuncts = splitOn(inner.group(1), "||") + if len(disjuncts) > 1: + return ["NOT (%s)" % unwrap(part) for part in disjuncts] + parts = splitOn(text, "&&") return parts if len(parts) > 1 else [text] def condKey(cond): """Two conditions are the same when they say the same thing, spacing aside.""" return (re.sub(r"\s+", "", cond["text"]), cond["kind"]) def useKey(cond): """Looser key, for matching a test of a value against a test of the field holding it. The same test is written sameString(colorMode, ...) in the loader and sameString(btd->colorMode, ...) in the drawer, because the value was carried into a struct on the way. Dropping the field prefix makes those one condition. Only the use-site pass uses this; the every-path analysis keeps