Count the compiler warnings in a ParparVM native build, by owner - #5750
Count the compiler warnings in a ParparVM native build, by owner#5750shai-almog wants to merge 7 commits into
Conversation
A ParparVM build compiles four different kinds of C into one binary -- the translator's output, the ParparVM runtime, the hand-written port natives, and vendored third-party sources -- and their warnings arrive in one log with nothing to tell them apart. At that volume a real defect is invisible: an Apple API that is deprecated now and deleted in two releases, or a pointer/integer confusion in generated code, reads the same as the noise around it. Nothing in this tree has ever counted them, and the Android port has already shown what that costs (API 37 deleted FingerprintManager with every check green). Ownership cannot be recovered from the path. ByteCodeTranslator.execute() copies every non-class file into the same flat srcRoot as the generated code, so CN1Vision.m and com_codename1_ui_Form.m are indistinguishable siblings, and bytecode-translator-files.txt is a plain find over that directory. So the translator records provenance at each copy site into cn1-source-manifest.txt. It goes in the project root rather than srcRoot deliberately: getFileType() has no case for .txt, so anything left in srcRoot falls through to ***RESOURCES*** and is copied inside the shipped .app. check-native-warnings.py reads that manifest plus a build log and attributes every diagnostic to generated / runtime / port / vendored / sdk / toolchain, holding the result against a per-leg baseline in the manner of check-cast-semantics.sh. No baseline is committed yet -- one has to be frozen from a real CI leg, not from a local run. Three guards, because a gate that reads nothing reports success: - The build must have compiled everything the manifest lists. An incremental build recompiles nothing, reports no warnings, and is indistinguishable from a clean codebase; so is the documented ARCHS failure where xcodebuild "silently compiles NOTHING while still copying resources". - --probe injects one synthetic warning and asserts the whole chain reacts, on the real log with the real manifest, so a gate that has gone blind fails the day it breaks rather than the day someone notices it never fired. - --self-test checks the parser against a hand-authored fixture and the baselines against their own format. Both run in pr.yml and need no compiler. The census runs on scripts-ios.yml's build-ios leg because that is the one that can: it sets no CN1_IOS_DERIVED_DATA, so it wipes derived data and compiles cold, and its path filters are a superset of the other iOS legs'. It is gated on CN1_WARNING_CENSUS, which our workflows set and nothing a customer runs does, and it is report-only for now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0be6104ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Xcode names the source it is about to compile; ninja and make announce the | ||
| # object. Either way this is how we learn what the build ACTUALLY compiled, as | ||
| # opposed to what it could have compiled. | ||
| COMPILE_XCODE_RE = re.compile(r'^\s*CompileC\s+(?:"[^"]*"|\S+)\s+(?P<src>"[^"]+"|\S+)\s+normal\b') |
There was a problem hiding this comment.
On the checked build-ios workflow, CN1MetalShaders.metal is copied from Ports/iOSPort/nativeSources and included in the Xcode sources, but Xcode announces it with a CompileMetalFile operation rather than CompileC. Since this regex is the only Xcode compilation grammar while .metal is included in SOURCE_EXTS, check_completeness() always reports that shader as uncompiled and exits 2; run-ios-ui-tests.sh then swallows that status, so this leg produces no warning census or JSON artifact despite a successful cold build.
Useful? React with 👍 / 👎.
| // Only the one-file-per-class case has a file name worth recording. Under | ||
| // concatenation the classes are bucketed into concatenated_<n> and the | ||
| // provenance of an individual class is genuinely gone by the time the | ||
| // compiler sees it; the manifest says so by simply not naming these. |
There was a problem hiding this comment.
When concatenateFiles=true—used by iOS superfast builds and unconditionally by MacOSNativeBuilder—the compiler sees concatenated_<n>.m, but this branch deliberately records none of those generated files. Any warning emitted from a concatenated unit consequently has no manifest entry or external-path marker, so classify() treats it as unattributed and aborts the census instead of placing it in the generated group; record each nonempty concatenated output when realClose() creates it.
Useful? React with 👍 / 👎.
The completeness guard asked the wrong question. It required every source in the manifest to appear as compiled, but the manifest names every file in the generated project and a target legitimately builds a subset: a .metal goes through CompileMetalFile rather than CompileC, and a source can be excluded from a target outright. Demanding all of them would have failed the first real census for a reason that is not a defect. The thing actually worth catching is different: a build that compiled LESS than the one the baseline was frozen from. That is what an incremental build looks like, it reports no warnings, and it is indistinguishable from a clean codebase. So the ratchet is on coverage, recorded in coverage-<leg>.txt beside the baseline. It is exact, needs no threshold, and needs nobody to enumerate which files a given target happens to include. A build that compiles nothing at all is still fatal on its own. Sources the manifest lists that this target never builds are now reported rather than fatal, which is the honest reading of them. Also recognises the real task-line shape, checked against Xcode 26.3 output rather than assumed: the task name is followed by output, source, "normal", the arch, and a trailing "(in target ... from project ...)". Both that line and the CompileMetalFile form are in the fixture now, so neither can regress silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
METALView.h and METALView.m exist in both Ports/MacPort and Ports/iOSPort/nativeSources, so resolving a warning's file by name alone was ambiguous and would have refused the macOS leg outright the first time either one warned. The leg already answers it: a macOS build compiled the MacPort copy. Listing each leg's port trees most-specific-first states that rather than guessing at it, and a name claimed by a more specific tree is not reconsidered. A name that appears twice inside a SINGLE port tree still refuses to resolve. There the leg tells us nothing, and picking one would put a file nobody edited into the baseline. Also indexes each port tree once instead of walking it per diagnostic, which was O(diagnostics x files) against a census that carries thousands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"/Library/Developer/" also matches a developer's own ~/Library/Developer/Xcode/DerivedData, so a Swift package checkout under SourcePackages was being labelled an Apple SDK header. The vendored markers are the more specific ones and belong first. Neither group gates, so no verdict changes -- but the census exists to be read, and one that misattributes what it reports is not worth reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The summary table had a row per distinct warning kind, and the iOS leg carries far more of them than the clean target does. GitHub drops a step summary whole once it exceeds 1MB, so an unbounded table risks costing the entire report rather than its tail -- the failure mode being a census that ran, found everything, and showed nothing. Capped per group, with the hidden rows counted rather than silently dropped, and a line saying the tail is in the JSON dump. The census itself is unchanged; this only bounds what a human is asked to scroll. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 584caa96d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * somebody else's vendored code. Written out beside the generated project at the | ||
| * end of each output handler; see {@link SourceManifest}. | ||
| */ | ||
| static final SourceManifest sourceManifest = new SourceManifest(); |
There was a problem hiding this comment.
When ByteCodeTranslator.main() is invoked more than once in the same JVM—as the integration tests and embedded callers can do—this static manifest retains every entry from the previous application because neither main() nor Parser.cleanup() clears it. The next project therefore receives a manifest containing sources that do not exist in its output, and retained origins can also classify a same-named file using provenance from the earlier build; create or clear the manifest at the start of each translation.
Useful? React with 👍 / 👎.
| // globs srcRoot for sources, and the Apple path lists it into the Xcode project, | ||
| // where an unrecognised extension lands in the resources phase and ships inside | ||
| // the bundle. See SourceManifest. | ||
| sourceManifest.write(root); |
There was a problem hiding this comment.
For the windows and linux clean targets, embedWindowsResources()/embedLinuxResources() always generate cn1_resources_table.c and may also generate cn1_resources_data.S, but neither file is added to sourceManifest before it is written here. A compiler warning in either generated unit consequently has no manifest entry, so classify() treats it as unattributed and aborts the warning census instead of assigning it to generated code.
Useful? React with 👍 / 👎.
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
SpotBugs DM_CONVERT_CASE, and it is the tree's documented rule rather than a style nit: String.toLowerCase() is locale sensitive, and Codename One has no java.util.Locale to ask for the root one, so the fold a device performs depends on who is holding it. A protocol token another program parses back is exactly the case that must never be produced by folding. The four current constants happen to contain no dotted I, so nothing was broken today -- but the next one added could, and it would fail only for users whose device is set to Turkish or Azerbaijani, with nothing throwing. Writing the token out also means the manifest's wire format is stated in the enum rather than being an accident of the Java identifier, so renaming a constant can no longer silently change the file that check-native-warnings.py parses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74fe911480
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GNU_RE = re.compile( | ||
| r'^(?P<path>[^\s][^:]*(?::[^:\s][^:]*)*?):(?P<line>\d+):(?P<col>\d+):\s+' | ||
| r'(?P<sev>warning|error|note):\s+(?P<msg>.*)$') |
There was a problem hiding this comment.
When a compiler emits a diagnostic in the valid path:line: warning: ... form without a column, this pattern does not match because it requires :line:col:, while BARE_RE only accepts warnings beginning at column zero. Such warnings are silently omitted from the JSON and baseline comparison, allowing the census to report clean despite warnings in the build log; make the column portion optional and cover this form in the parser fixture.
Useful? React with 👍 / 👎.
| ByteCodeTranslator.sourceManifest.recordGenerated("cn1_class_method_index.h"); | ||
| fos = new FileOutputStream(new File(outputDirectory, "cn1_class_method_index.m")); | ||
| fos.write(bldM.toString().getBytes(StandardCharsets.UTF_8)); | ||
| fos.close(); | ||
| ByteCodeTranslator.sourceManifest.recordGenerated("cn1_class_method_index.m"); |
There was a problem hiding this comment.
When cn1.onDeviceDebug=true, writeSymbolSidecar() emits and the generated project compiles cn1_debug_symbols.c, but unlike the generated files recorded here, that sidecar is never passed to sourceManifest.recordGenerated(). A warning in it therefore has neither a manifest entry nor an external-path marker, so classify() marks it unattributed and aborts the census instead of assigning it to generated code; record the sidecar when it is emitted.
Useful? React with 👍 / 👎.
|
Compared 166 screenshots: 166 matched. |
|
Compared 166 screenshots: 166 matched. |
|
Compared 181 screenshots: 181 matched. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 144 screenshots: 144 matched. |
The first real census of an iOS build: 92,129 warning lines in a 1.17M-line log, none of which anything had ever counted. 60,952 of them are ours and gating, across 157 distinct kinds. Coverage confirms it came from a cold build -- 3,147 sources compiled, none of the manifest's 3,133 skipped. What the split by owner buys is visible immediately. Generated code is four emitters, two of which account for essentially all of it: 50,967 -Wunused-variable and 9,094 -Wincompatible-pointer-types-discards-qualifiers. The 814 warnings in the hand-written port were the ones worth finding, and they were unreadable underneath that: 470 deprecations (OpenGLES, and MPMoviePlayerController asking for AVPlayerViewController), 67 -Wshorten-64-to-32, 21 -Wint-conversion, 10 ARC bridge casts in non-ARC code, 6 -Wunsupported-availability-guard -- availability checks that do not guard -- and 4 -Wundeclared-selector, which is the class the macOS template already makes an error because it crashes on the device. Three log-transport defects had to be fixed first, all found in the real log rather than imagined. xcodebuild's output reaches the log through a pipe and a long diagnostic can arrive broken at an arbitrary byte: - split in the PATH, leaving a file name like "odename1_ui_Display.m" that belongs to nothing; - split in the MESSAGE, which is worse because the first half still parses and yields a message shape of "unuse" -- a baseline row that could never match again; - and occasionally bytes are LOST rather than split, so the path is gone outright. Those are counted and reported as lost, never attributed and never baselined: a row keyed on no file cannot recur, so baselining one would guarantee a stale entry later. Both joins are self-validating rather than guessed -- a path join must produce something that parses, a message join must complete a trailing flag that was absent. All three shapes are now in the parser fixture. A fileless diagnostic carrying a [-Wflag] is one of the lost ones, not a build-system warning; clang flags belong to file-scoped diagnostics, and that is what separates it from a genuine "Skipping duplicate build file". Two provenance rules the census asked for: an embedded watch or tv app is a second, independent ParparVM translation writing to a sibling -src directory, so its output is generated code by the same argument as the main app's; and an .xcframework is unzipped into the build's own products directory before its headers are compiled, so those arrive under a local-looking path and are not ours. The leg now gates rather than reporting, and runs --probe afterwards so a gate that has gone blind fails the same day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40e9fd9c40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if FLAG_RE.search(m.group("msg")): | ||
| lost += 1 | ||
| continue |
There was a problem hiding this comment.
When Clang emits a legitimate fileless driver diagnostic, such as clang: warning: argument unused during compilation: '-stdlib=libc++' [-Wunused-command-line-argument], this branch categorizes it as transport loss and drops it from both the JSON and baseline comparison. Such diagnostics are reproducible toolchain warnings rather than truncated file diagnostics, so a newly introduced command-line warning can leave the native-warning gate green; retain these as toolchain diagnostics instead of incrementing lost.
Useful? React with 👍 / 👎.
| ri_log "STAGE:WARNING_CENSUS_FAILED -> see the census output above" | ||
| exit 12 | ||
| fi | ||
| if ! "$REPO_ROOT/scripts/check-native-warnings.sh" "${CN1_WARNING_ARGS[@]}" --probe > /dev/null; then |
There was a problem hiding this comment.
In the GitHub Actions census path, this second invocation inherits GITHUB_STEP_SUMMARY; redirecting stdout does not stop check-native-warnings.py from appending directly to that file, and the script calls summarize() before handling --probe. Every successful run therefore publishes a second census containing the injected -Wcn1-gate-probe warning, making the human-facing report claim a warning that the actual build never emitted; unset the summary variable for the probe or handle the probe before rendering.
Useful? React with 👍 / 👎.
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
1 participant
Rationale is in the commit message and in code comments; this body is a pointer only.
What it does. Records where every file in a generated project came from (
cn1-source-manifest.txt), then attributes each compiler warning in a build log togenerated/runtime/port/vendored/sdk/toolchain. Ownership is not recoverable from the path -- the translator copies port natives into the same flat directory as its own output.Not gating yet. No baseline is committed. The census is report-only on
scripts-ios.yml'sbuild-iosleg (the only iOS leg that compiles cold), gated onCN1_WARNING_CENSUS, which nothing a customer runs sets.pr.ymlgains only a parser self-test that needs no compiler.Local measurement (clean target, JavaAPI-only app,
-Wall -Wextra): 3,802 warning lines from a cold build -- 3,035 in generated code across 8 kinds, 185 in the runtime. Top rows are-Wunused-parameter(1694) and-Wunused-variable(1002), both single-emitter fixes. The runtime carries several worth triaging as bugs rather than noise, including-Wpointer-to-int-cast(cast to a smaller integer type).Next step is freezing the baselines from this PR's CI run.
🤖 Generated with Claude Code