Enforce CompilerOptionName serialization exhaustiveness (classifyCommandLineOption) - #12780
Draft
nv-slang-bot[bot] wants to merge 1 commit into
Draft
Enforce CompilerOptionName serialization exhaustiveness (classifyCommandLineOption)#12780nv-slang-bot[bot] wants to merge 1 commit into
nv-slang-bot[bot] wants to merge 1 commit into
Conversation
Reintroduce classifyCommandLineOption as the single source of truth for how writeCommandLineArgs treats each CompilerOptionName, classifying every option as Serialize, RepresentedElsewhere, or Omit. writeCommandLineArgs now gates on the Serialize classification instead of a blanket default that silently dropped any unhandled option (the project builds with -Wno-switch, so a missing case gives no compile signal -- the exact class that produced #12220). A new unit test (commandLineOptionClassificationIsExhaustive) iterates [0, CountOf) and fails on any Unclassified option, so a newly-added option cannot be silently dropped from the reproduction command line. The classification lives in its own translation unit (slang-command-line-option-class.cpp, depending only on the enum) so it can be recompiled into the unit-test module without exporting an internal free function from the DLL ABI. Behavior-preserving: the set of Serialize-classified options is exactly the set the existing formatting switch already handled, so the emitted command line is unchanged for every current option; the switch's default arm is now an unreachable SLANG_ASSERT_FAILURE.
Contributor
|
Automated notice (PR board sync) — do not reply to this comment. Auto-assigned @jkwak-work as shepherd for this Bot PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
CompilerOptionSet::writeCommandLineArgs(source/slang/slang-compiler-options.cpp) reconstructs adescriptive command line that is embedded in debug info — most visibly the SPIR-V
DebugEntryPointOpString(call siteslang-emit-spirv.cpp). It walks the stored option set and, for each option,runs a hand-maintained
switch (option.key)that formats the option's flag(s).Since #12243 merged, that switch ends in a blanket
Because the project builds with
-Wno-switch, adding a newCompilerOptionNameproduces nocompile-time signal if no arm handles it — the option is silently dropped from the reconstructed
command line. That is exactly the failure class that produced #12220 (
-glevel hardcoded, severaloptions dropped). #12243 fixed the immediate cases; per @pdeayton-nv's review the structural fix
(a single classification + an exhaustiveness guard) was pulled out into this follow-up (#12257).
Proposed solution
Reintroduce
classifyCommandLineOption(CompilerOptionName)as the single source of truth for howeach option is treated, and enforce its exhaustiveness with a unit test — the incremental path
@jkwak-work approved (issuecomment-5430974889). Every option is classified as one of:
SerializeRepresentedElsewhereOmitUnclassifiedwriteCommandLineArgsnow gates onSerializeat the top of its loop (continueotherwise). Theexisting per-option formatting
switchis kept unchanged — formatting is not pushed into theparser registry in this PR (that larger, ABI-adjacent refactor is explicitly deferred, per the
triage decision). The blanket
default:is replaced bySLANG_ASSERT_FAILURE, since after the gateit is only reachable by a
Serializeoption that lacks a formatting arm — a bug we want to failloudly rather than silently drop.
This is behavior-preserving: the 78
Serialize-classified options are exactly the 78 optionsthe existing formatting
switchalready had a case for (confirmed by set-diffing the classificationagainst the switch arms — the two sets are identical), so the reconstructed command line is unchanged
for every current option. The regression test
tests/spirv/cmd-arg-debug-info.slang(added with#12243) still passes, and the new
commandLineOptionClassificationIsExhaustiveunit test passes.Change summary
source/slang/slang-compiler-options.hCommandLineOptionClassenum +classifyCommandLineOptiondeclaration; update thewriteCommandLineArgsdoc comment.source/slang/slang-command-line-option-class.cpp(new)classifyCommandLineOptiondefinition — the single source of truth. Its own TU (depends only on the enum) so the unit test can recompile it without exporting a DLL symbol, mirroringslang-repro-validator.cpp.source/slang/slang-compiler-options.cppSerialize; replace the blanketdefault:withSLANG_ASSERT_FAILURE.tools/slang-unit-test/unit-test-command-line-option-classification.cpp(new)commandLineOptionClassificationIsExhaustive— iterates[0, CountOf)and fails on anyUnclassified.tools/CMakeLists.txt.cppintoslang-unit-test(unexported free function, same pattern asslang-repro-validator.cpp).Concepts and vocabulary
writeCommandLineArgsbuildsfrom a
CompilerOptionSetfor embedding in debug info; it describes the compile, it is notre-parsed by the compiler in normal flow.
RepresentedElsewherevsOmit— both are not emitted, but for different reasons.RepresentedElsewhereoptions carry a choice that is normalized into a serialized option, soemitting the source option too would double-count.
Omitoptions are excluded because theydescribe context/I/O/tooling/diagnostics, or (in a few API-only cases) affect emission but have no
CLI spelling to reconstruct.
-Wno-switch— the project disables the unhandled-enum-case warning, which is why acompile-checked exhaustive switch cannot enforce this; the unit test substitutes for it.
Process report
Full classification of all 159
CompilerOptionNamevalues (CountOf= 159; last real valueTraceCoverageBindlessIndex= 158). Re-derived deliberately against the current enum — the reviewer-approved reference in #12243's history (
8efc9c0c3f) predates one added option, so its counts werenot copied. Reconciliation (via
comm/uniq) confirms every value in[0, CountOf)isclassified exactly once, with no duplicates and no stale names.
Counts: 78
Serialize, 4RepresentedElsewhere, 77Omit(+ theCountOfsentinel,also
Omit).Serialize(78) — influences the generated artifact; emitted onto the reconstructed command lineMacroDefine,Include,Language,MatrixLayoutColumn,MatrixLayoutRow,ZeroInitialize,IgnoreCapabilities,RestrictiveCapabilityCheck,Profile,SkipSPIRVValidation,DisableShortCircuit,MinimumSlangOptimization,DisableNonEssentialValidations,DisableSourceMap,UnscopedEnum,PreserveParameters,Capability,DefaultImageFormatUnknown,DisableDynamicDispatch,DisableSpecialization,FloatingPointMode,DebugInformation,DebugInformationFormat,LineDirectiveMode,Optimization,Obfuscate,VulkanBindShift,VulkanBindShiftAll,VulkanBindGlobals,VulkanInvertY,VulkanUseDxPositionW,VulkanUseEntryPointName,VulkanUseGLLayout,VulkanEmitReflection,GLSLForceScalarLayout,EnableEffectAnnotations,IncompleteLibrary,DownstreamArgs,BindlessSpaceIndex,SPIRVResourceHeapStride,SPIRVSamplerHeapStride,LanguageVersion,TypeConformance,EnableExperimentalDynamicDispatch,GenerateWholeProgram,ForceDXLayout,DenormalModeFp16,DenormalModeFp32,DenormalModeFp64,UseMSVCStyleBitfieldPacking,ForceCLayout,ExperimentalFeature,EmitSeparateDebug,TraceCoverage,TraceCoverageBinding,TraceCoverageReservedSpace,TraceFunctionCoverage,TraceBranchCoverage,TraceCoverageCounterByteWidth,TraceCoverageBoolean,TraceCoverageBindlessIndex,SPIRVUnifiedDescriptorHeapStride,DebugInfoIncludeSource,DumpIntermediates,EmitSpirvMethod,EmitCPUMethod,EmbedDownstreamIR,NoMangle,NoHLSLBinding,NoHLSLPackConstantBufferElements,EnableExperimentalPasses,TrackLiveness,LoopInversion,LLVMTargetTriple,LLVMCPU,LLVMFeatures,AllowGLSL,PassThrough.RepresentedElsewhere(4) — normalized into a serialized option; emitting the source too would double-countEmitSpirvViaGLSL,EmitSpirvDirectly,EmitCPUViaCPP,EmitCPUViaLLVM.The command-line parser folds
-emit-spirv-directly/-emit-spirv-via-glslinto theEmitSpirvMethodenum, and-emit-cpu-via-cpp/-emit-cpu-via-llvmintoEmitCPUMethod(
slang-options.cpp). Those two method keys areSerialize, so the source flags must not also beemitted. (Note:
CompilerOptionSet::load— the C-API entry — additionally normalizes the SPIR-Vpair but not the CPU pair; see the follow-up note below.)
Omit(77 + theCountOfsentinel) — excluded from the reconstructionDepFile,EntryPointName,Specialize,Help,HelpStyle,ModuleName,Output,Stage,Target,Version,WarningsAsErrors,DisableWarnings,EnableWarning,DisableWarning,WarningLevel,DumpWarningDiagnostics,InputFilesRemain,EmitIr,ReportDownstreamTime,ReportPerfBenchmark,ReportCheckpointIntermediates,SourceEmbedStyle,SourceEmbedName,SourceEmbedLanguage,SPIRVCoreGrammarJSON,CompilerPath,DefaultDownstreamCompiler,DumpRepro,DumpReproOnError,ExtractRepro,LoadRepro,LoadReproDirectory,ReproFallbackDirectory,DumpAst,DumpIntermediatePrefix,DumpIr,DumpIrIds,PreprocessorOutput,OutputIncludes,ReproFileSystem,REMOVED_SerialIR,SkipCodeGen,ValidateIr,VerbosePaths,VerifyDebugSerialIr,NoCodeGen,FileSystem,Heterogeneous,ValidateUniformity,ArchiveType,CompileCoreModule,Doc,IrCompression,LoadCoreModule,ReferenceModule,SaveCoreModule,SaveCoreModuleBinSource,ParameterBlocksUseRegisterSpaces,EmitReflectionJSON,CountOfParsableOptions,UseUpToDateBinaryModule,SaveGLSLModuleBinSource,SkipDownstreamLinking,DumpModule,GetModuleInfo,GetSupportedModuleVersions,ReportDetailedPerfBenchmark,ValidateIRDetailed,DumpIRBefore,DumpIRAfter,EnableRichDiagnostics,ReportDynamicDispatchSites,EnableMachineReadableDiagnostics,DiagnosticColor,CompilerVersion,CoverageManifestOutput,SeparateDebugInfoOutput(+ theCountOfsentinel).These are target/stage/entry-point context (contributed by the emit site itself),
input/output/module-identity paths, output-policy sidecar paths, repro tooling, dump/introspection,
diagnostics routing & reporting, downstream toolchain selection/paths, deprecated/removed/sentinel
values, and a few API-only knobs (
UseUpToDateBinaryModule,SkipDownstreamLinking) that can affectemission but have no CLI spelling to reconstruct.
Judgment-call options — flagged for maintainer review
Please flag any you would categorize differently:
TraceCoverageBindlessIndex(158) →Serialize. The only option added since the reference.Its own enum doc states the index "is a compile-time constant and therefore part of the compiled
artifact"; the existing switch already serializes it alongside
TraceCoverageReservedSpace.CoverageManifestOutput(150) andSeparateDebugInfoOutput(156) →Omit. These areoutput-policy sidecar paths;
buildHashexplicitly excludes them from the compiler cache key(locked by
_testCoverageManifestOutputDoesNotAffectCompilerOptionHash/_testSeparateDebugInfoOutputDoesNotAffectCompilerOptionHash), so they do not describe theartifact bytes.
UseUpToDateBinaryModule(115),SkipDownstreamLinking(120) →Omit. API-only, no CLIspelling;
UseUpToDateBinaryModuleis likewise excluded frombuildHash.DebugInfoIncludeSource(157) →Serialize. Affects emitted SPIR-V debug info and has a CLIflag.
EmitSeparateDebug(124) →Serialize(affects emitted artifacts), while its output pathSeparateDebugInfoOutputisOmit(policy only) — the pair is deliberately split.Input-shape / layering check
The change adds no new input shape and reads no data by position:
classifyCommandLineOptionis apure key→class function over the existing
CompilerOptionNameenum, andwriteCommandLineArgsalready iterated the option set by key. The classification is the canonical single source of truth
(the switch's formatting arms and the assert-default consume it, rather than re-deriving the
decision), and the exhaustiveness invariant is asserted where a violation would otherwise be silent
(the unit test, standing in for the
-Wno-switch-suppressed compile check).Follow-ups (out of scope here)
m_commandOptions/initCommandOptions— remains deferred; this PR keeps the formatting switch.
CompilerOptionSet::loadnormalizes the legacy SPIR-V flags intoEmitSpirvMethodbut does notnormalize the CPU flags into
EmitCPUMethodfor API-provided entries. That asymmetry predates thisPR and does not affect the classification (both source flags are
RepresentedElsewhereand bothmethod keys are
Serialize); noted for a possible follow-up.Closes #12257.
🤖 Generated by an automated Slang coworker — may be inaccurate. A human maintainer should verify.