Add: windows port for coretrace-stack-analyzer#69
Conversation
![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: fcf5239845
ℹ️ 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".
SizzleUnrlsd
left a comment
SizzleUnrlsd
left a comment
There was a problem hiding this comment.
Thanks for pushing the first native Windows pass. I reviewed the final PR head and the overall direction makes sense, but I still see a few blocking issues before this is safe to merge. The main concern is that the current Windows CI path relaxes the regression harness enough that it can go green while real diagnostic regressions remain undetected. I also think the hard-coded diaguids patch and the silent dependency fallback to main make the build path brittle and non-reproducible.
| Suites can opt-out per-file via: // strict-diagnostic-count: false | ||
| """ | ||
| if os.name == "nt": | ||
| return False |
There was a problem hiding this comment.
Disabling strict diagnostic counts for every Windows fixture makes the suite much less effective as a regression guard. Combined with the relaxed location matching below, Windows can now pass even when the analyzer starts emitting extra diagnostics or when the matched diagnostic is only approximately the right one. I think this should stay opt-out per fixture rather than becoming the platform-wide default.
| return [ | ||
| path | ||
| for path in sorted(fixture_sources) | ||
| if is_fixture_source(path) and fixture_skip_reason(path) is None |
There was a problem hiding this comment.
This silently removes every windows-skip fixture from the main regression sweep. Right now that excludes a fairly large chunk of coverage, and later in this file Windows also skips some rule-coverage checks entirely. That means the job is validating a materially smaller support surface rather than a stable Windows tier. Could we surface these as explicit expected failures / unsupported cases instead of dropping them from the suite?
| endif() | ||
|
|
||
| set(coretrace_stale_diaguids_path | ||
| "C:/Program Files (x86)/Microsoft Visual Studio/2019/Professional/DIA SDK/lib/amd64/diaguids.lib") |
There was a problem hiding this comment.
This patch is tied to one specific Visual Studio 2019 Professional DIA path. If the imported LLVM target embeds a different absolute diaguids.lib location (different VS edition/version/layout), the replacement will stop matching and the Windows configure path breaks again. Please make this generic by rewriting any diaguids.lib entry discovered on the imported target rather than matching one hard-coded absolute path.
| fetch-depth: 1 | ||
|
|
||
| - name: Checkout coretrace-compiler main fallback | ||
| if: steps.checkout_compiler_ref.outcome == 'failure' |
There was a problem hiding this comment.
Falling back to main here can make the PR pass against a different dependency revision than the branch it is actually trying to validate. That hides inter-repo incompatibilities instead of surfacing them. I think this should fail fast when the matching ref is missing, or at least make the fallback explicit and non-default. The same pattern appears in the release workflow as well.
2 participants
This is the first iteration for a native Windows implementation of the coretrace-stack-analyzer tool.
It includes code refactoring from POSIX to LLVM/Win32 and many tests case.