Skip to content

Fix #11473: pragma warning state corruption across __include passes#11478

Open
nv-slang-bot[bot] wants to merge 1 commit into
masterfrom
dev/slang-fixer/fix-11473-pragma-warning-include
Open

Fix #11473: pragma warning state corruption across __include passes#11478
nv-slang-bot[bot] wants to merge 1 commit into
masterfrom
dev/slang-fixer/fix-11473-pragma-warning-include

Conversation

@nv-slang-bot
Copy link
Copy Markdown
Contributor

@nv-slang-bot nv-slang-bot Bot commented Jun 4, 2026

Fixes #11473.

Motivation

#pragma warning(disable: <id>) at the root translation-unit level can be silently clobbered by an unrelated #pragma warning(push)/(pop) inside an __included module file. Reduced repro from the issue:

// root.slang
module repro;
#pragma warning(disable: 30856)
__include "body.slang";    // emits 30856 (incorrectly NOT suppressed)
__include "footer.slang";
// footer.slang  -- unrelated push/pop touching a different id
implementing repro;
#pragma warning(push)
#pragma warning(disable: 31000)
#pragma warning(pop)

-Wno-30856 on the command line works (separate suppression path); only the in-source root pragma is broken.

Root cause (two combined facets)

  1. __include is a Slang declaration parsed in slang-parser.cpp. SemanticsDeclHeaderVisitor::visitIncludeDecl calls Linkage::findAndIncludeFile, which runs a fresh preprocessSource pass per included file. Each fresh pass constructs a new Preprocessor whose absoluteSourceLocCounter defaults to 0, and Preprocessor::pushInputFile calls sourceView->setAbsoluteLocationBase(absoluteSourceLocCounter). Net effect: every __included file's absolute-loc range starts at 0.

  2. Cross-file pragma state is shared via a sink-attached WarningStateTracker. Its WarningTimeline is keyed by absolute SourceLoc::RawValue, and addPragmaPop blanket-restores every known diagnostic id by looking up findEntry(absPushed) for each id, then writing the resulting specifier at the pop's absolute loc. With colliding 0-bases across passes, the timeline lookup misses the root-TU disable and the pop reverts 30856 to Default. Subsequent emissions of 30856 then fire.

A push/pop placed inline in a single preprocessing pass works correctly — the absolute axis is monotonic there — which isolates facet 1 as the trigger.

Fix shape

Carry the high-water mark of absoluteSourceLocCounter across preprocessSource passes via the already-shared WarningStateTracker (the only object that already lives per-TU across the N passes spawned by __include). At the start of each pass, seed the new preprocessor's counter from the tracker; at the end, write the new high-water mark back. Every later pass's locations now land strictly after the prior pass's max, so the per-id timeline binary search continues to find the correct push-time entry and the root-level disable survives across __include boundaries.

The internal timeline logic (WarningTimeline::findEntry is std::upper_bound on a sorted list) is already correct under monotonic locations — the only change needed is the cross-pass seeding.

Footprint: one new field on WarningStateTracker plus one seed and one write-back in preprocessSource.

Test

tests/diagnostics/pragma-warning-include-empty-pop-{main,body,footer}.slang reproduces the original three-file pattern (root disable + body produces 30856 + footer's unrelated push/pop). // CHECK-NOT: warning 30856 fires on master and passes after the fix.

Also verified locally with slangc -no-codegen that the existing tests/diagnostics/nested-pragma-*.slang family continues to suppress 30856 as before. Existing tests/diagnostics/pragma-warning-multifile-*.slang was already a known-failing regression test (its impl2 contains the comment "This should still NOT produce warning 30856 (but currently does due to the bug)"); behaviour there is unchanged by this PR — its expectation that a pop inside one included file should not extend the disable into a sibling included file is a separate, narrower question and out of scope here.

Out of scope (possible follow-up)

Triage suggested also scoping addPragmaPop to restore only the ids modified between the matching push/pop rather than blanket-restoring all known ids. That is an independent robustness improvement — useful, but it changes the data structure and the semantics of pop in subtle ways, so I left it out of this minimal fix. Filing as a follow-up makes sense if reviewers agree.

Notes

  • No public API change.
  • No ABI change (all in source/slang/slang-preprocessor.cpp; the new field is on a RefObject subclass that is never crossed over a public boundary).
  • Label: pr: non-breaking.

…include passes

Each `__include`d file is preprocessed in a fresh `preprocessSource`
pass that constructs a new Preprocessor with absoluteSourceLocCounter
defaulting to 0. The shared sink-attached WarningStateTracker keys
its per-id timelines by absolute SourceLoc, so colliding 0-bases
from different files break the timeline ordering: a `#pragma
warning(pop)` in a later __include'd file blanket-restores every
known id by looking up findEntry(pushedLocation), which under
collisions misses the root-TU disable and reverts the id to
Default. The user's root-level `#pragma warning(disable: 30856)`
then no longer applies on subsequent emissions.

Carry the high-water mark of absoluteSourceLocCounter across
passes via the already-shared WarningStateTracker. Every later
pass's locations land strictly after the prior pass's max, so the
timeline binary search finds the correct push-time entry and the
root disable survives. The internal timeline logic is unchanged.

Fixes #11473
@nv-slang-bot nv-slang-bot Bot requested a review from a team as a code owner June 4, 2026 19:13
@nv-slang-bot nv-slang-bot Bot requested review from bmillsNV and removed request for a team June 4, 2026 19:13
@nv-slang-bot nv-slang-bot Bot added the pr: non-breaking PRs without breaking changes label Jun 4, 2026
@nv-slang-bot nv-slang-bot Bot requested a review from tdavidovicNV June 4, 2026 19:14
@nv-slang-bot
Copy link
Copy Markdown
Contributor Author

nv-slang-bot Bot commented Jun 4, 2026

Suggested reviewers (no ping; based on git log and git log -S for source/slang/slang-preprocessor.cpp, especially the pragma-warning subsystem):

@nv-slang-bot
Copy link
Copy Markdown
Contributor Author

nv-slang-bot Bot commented Jun 4, 2026

FYI: filed #11479 to track the pragma-warning-multifile-* aspirational test that PR #7942 committed in a known-failing state. That test asserts cross-sibling pragma persistence semantics that this PR does not target — it's distinct from the absolute-source-loc collision fixed here, and may be either editorial (drop push/pop from impl1) or a real semantics decision. Cross-referenced and explicitly out-of-scope for this PR.

@nv-slang-bot
Copy link
Copy Markdown
Contributor Author

nv-slang-bot Bot commented Jun 4, 2026

CI ran and reported two non-aggregator failures: build-linux-release-gcc-wasm / build (undefined symbol on Slang::LanguageServerCore::* and Slang::WorkspaceVersion::getOrLoadModule at the slang-wasm.js link step) and test-macos-release-clang-aarch64 / test-slang-rhi (1 of 10,431,541 doctest assertions: cmd-query-resolve-host.cpu FAILED (0.00s)).

Both surfaces are disjoint from this PR's diff — adding a SourceLoc::RawValue absoluteLocCounter field to WarningStateTracker and seeding/writing-back Preprocessor::absoluteSourceLocCounter in preprocessSource. The failures don't reach those code paths.

Same checks pass on contemporary peer PRs (#11431, #11430, #11420). Re-running the failed jobs to disambiguate flake from a master regression that landed between those green peers and now.

@nv-slang-bot
Copy link
Copy Markdown
Contributor Author

nv-slang-bot Bot commented Jun 4, 2026

CI rerun classification

test-macos-release-clang-aarch64 / test-slang-rhi: ✅ confirmed flake. Passed cleanly on rerun. The failing assertion was cmd-query-resolve-host.cpu — 1 of 10,431,541 doctest assertions, single platform (other test-slang-rhi runs on Linux + Windows + macos-debug all passed first time).

build-linux-release-gcc-wasm / build: 🔴 reproducible on rerun, but surface-disjoint from this PR. Failure mode is undefined symbols at the slang-wasm.js link step:

error: undefined symbol: Slang::WorkspaceVersion::getOrLoadModule(Slang::String)
error: undefined symbol: Slang::LanguageServerCore::completion(...)
error: undefined symbol: Slang::LanguageServerCore::signatureHelp(...)
error: undefined symbol: Slang::LanguageServerCore::documentSymbol(...)
error: undefined symbol: Slang::LanguageServerCore::gotoDefinition(...)
error: undefined symbol: Slang::LanguageServerCore::semanticTokens(...)
error: undefined symbol: Slang::LanguageServerCore::completionResolve(...)
error: undefined symbol: Slang::LanguageServerCore::didOpenTextDocument(...)
error: undefined symbol: Slang::LanguageServerCore::didCloseTextDocument(...)
error: undefined symbol: Slang::LanguageServerCore::didChangeTextDocument(...)
error: undefined symbol: Slang::LanguageServerCore::init(...)
error: undefined symbol: Slang::LanguageServerCore::hover(...)
error: undefined symbol: Slang::Workspace::getCurrentVersion()

These are language-server / workspace symbols, all defined in source/slang/slang-language-server.cpp and source/slang/slang-workspace-version.cpp. This PR's diff is a SourceLoc::RawValue absoluteLocCounter field added to WarningStateTracker plus two assignments inside preprocessSource — categorically disjoint from these symbols.

Confirmed via build log: both slang-language-server.cpp.o and slang-workspace-version.cpp.o were compiled in the WASM build phase ([436/478] and [469/478]) yet libslang-compiler.so.0.2026.10.2 is linked against by slang-wasm without those symbols resolving.

Provenance check:

Hypothesis: latent breakage in master at 564ac9f0 that surfaces only at the slang-wasm.js Emscripten link, despite the LSP .cpp.o files compiling. Most likely candidates given the surface: a build-graph subtlety with how slang-common-objects propagates into the slang-compiler shared library under SLANG_EMBED_CORE_MODULE=ON for the Emscripten configuration. Possibly cache-related: this PR's WASM build is the first to rebuild master at 564ac9f0 from cold, so any cache poisoning at the boundary would appear here first.

Asks:

  • Maintainer review: is build-linux-release-gcc-wasm known to be flaky / known broken on master at 564ac9f0? If so, this PR is unblocked from CI's side.
  • If unknown: rebasing onto a newer master once one lands (or merging an unrelated trivial PR to advance master + force a re-run) should disambiguate cleanly.
  • Happy to dig further if requested, but I have no signal that the WASM failure is caused by this PR's diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: non-breaking PRs without breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

#pragma warning(push/pop) in included files can restore warning state incorrectly

1 participant