Fix compiler crashes and correctness bugs across lowering, MIR, and OSDI - #23
Merged
Conversation
Three lowering bugs in builtin analog operators: - ac_stim: only the no_equations path handled it, so a contributing use (V(a,b) <+ ac_stim(...)) fell through to unreachable!() and crashed the compiler. It now lowers to 0.0, which is the correct large-signal value (AC-analysis injection remains unimplemented). - idt(expr, ic): the IC-phase residual [val - ic, 0] pinned val = ic at DC but left the stored charge at 0, so when transient integration started the integral silently restarted from 0 instead of ic. The charge is now set to ic, making the state continuous across the DC->transient boundary. - idtmod: two bugs. The offset was read from args[2] (the modulus) instead of args[3], and the modulo wrap was applied inside the DAE residual, which makes the reactive residual jump by one modulus at each wrap and diverges the transient integrator. The state now integrates unbounded and the wrap (offset + floor_mod(val - offset, modulus)) is applied only to the returned value. Bugs identified by cross-auditing the Ngspice_OpenVAF_Enhancements fork (javaNoviceProgrammer), enhancements 26-28.
…< lexing
- hir_def: a string variable declared without an initializer hit
unreachable!("invalid var type") when its default body was lowered
(the match only covered Integer/Real/Array). Strings now default to "".
- parser: 'electrical ground gnd;' failed to parse - the discipline-first
branch of net_decl never accepted a net-type keyword after the
discipline name, so 'ground' choked the declaration list. The net-type
is now optionally eaten, mirroring the 'ground electrical gnd;' form.
- lexer: the three-character arithmetic-shift tokens <<< and >>> only
consumed two characters (single bump after the initial char), so
'<<<' lexed as ShlA followed by a stray '<'. (The parser does not yet
accept the operators; this just makes the token stream correct.)
Bugs identified by cross-auditing the Ngspice_OpenVAF_Enhancements fork
(javaNoviceProgrammer), enhancements 6 and 9.
Three defects that together made $simparam$str completely unusable: - hir_ty: the builtin signature returned Real instead of String, so any string-context use (s = $simparam$str(...)) was a type error. - osdi/stdlib.c: simparam_str iterated using the numeric names array as its loop bound while indexing names_str (an out-of-bounds read once the string list is shorter than the numeric one) and returned the matched *name* instead of the value. It now walks the NULL-terminated names_str list (with a NULL guard) and returns vals_str[i]. - syntax: the raw system-function name table spelled it "$simpara$str" (missing m) in raw::simparam_str and is_known(). Bugs identified by cross-auditing the Ngspice_OpenVAF_Enhancements fork (javaNoviceProgrammer), enhancement 25.
The parser recorded localparam declarations (is_local in the item tree) but nothing ever consumed the flag, so every localparam was silently overridable from the model card / instance line like a plain parameter. Thread is_local through ParamData and hir::Parameter, and force the parameter's "given" flag to constant false in the OSDI model and instance setup functions, so the declared default expression is always (re)computed and any simulator-written slot value is overwritten. Verified in the generated setup_model IR: the localparam's given-bit test disappears (constant-folded), while a plain parameter's remains. Bug identified by cross-auditing the Ngspice_OpenVAF_Enhancements fork (javaNoviceProgrammer), enhancement 9.
…ff literals
Two bugs in the laplace_nd state-space lowering:
- The output stage summed only num[0..n-1], silently dropping num[n] when
deg(num) == deg(den). Any exactly-proper transfer function (high-pass,
all-pass, notch numerators) lost its direct feedthrough: a first-order
high-pass H(s) = s*tau/(1 + s*tau) evaluated to a constant 0. The output
is now y = sum_k (num[k] - d*den[k]) x_k + d*input with d = num[n]/den[n],
which is exact for the existing state definition x_i = s^i w.
- array_coeffs re-widened coefficients using the pre-cast expression type.
lower_expr already applies the inference-inserted int->real cast, so a
mixed literal like '{0, 1.0e-6} got a second ifcast on an already-real
value, which the constant folder rejects with
'unreachable: invalid real operation ifcast' (compiler crash). Coefficient
widening now consults the resolved (post-cast) type.
First bug identified by cross-auditing the Ngspice_OpenVAF_Enhancements
fork (javaNoviceProgrammer), enhancement 31; second found while verifying
the fix.
…s constant indices Array accesses previously used the raw source index as the element position, so 'real g[2:5]; g[3]' silently read/wrote the wrong element (or fell off the end of the 4-element storage). Thread the declared lower bound (min(msb,lsb)) from the item tree through VarData to a new Variable::array_lo accessor and subtract it in both lower_index and assign_array_element; runtime index selects now compare against the declared indices (lo..lo+len). Constant indices outside the declared range are now a hard type-check error (rendered with the declared range) instead of silent misbehavior; non-constant out-of-range indices read 0.0 and skip the write.
'g = '{1.0, 2.0};' and 'g = h;' hit the todo!("arrays") in lower_array
and crashed the compiler. An array is not a single MIR value (it is one
place per element), so intercept array-typed destinations at the
assignment site and write the element places directly: array literals
lower each element expression, array-to-array copies read the source
element places. A cast recorded on the whole array expression (integer
literal array assigned to a real array) is applied per element.
- dominators: seed the post-dominator walk from every real exit block (as if they fed a virtual super-exit) instead of only layout.last_block(). With multiple exits, every block that could not reach that one specific block stayed unvisited and ipdom() returned None even for branches with a well-defined common post-dominator. The extra roots get the reverse entry as explicit idom; single-exit functions behave exactly as before. - simplify_cfg: do not remove a CFG-unreachable block while any of its instructions' results still has outstanding uses (e.g. a derivative instruction created by mir_autodiff that references values before being spliced into the layout). Zapping such a block left permanently dangling references. - simplify_cfg/layout: when merge_blocks folds away the layout's last block, move the surviving predecessor to the end (new Layout::move_block_to_end). Otherwise last_block() silently pointed at an unrelated block, breaking goto_exit-based passes in sim_back (e.g. dae/builder.rs ensure_optbarriers). Ported from javaNoviceProgrammer's Ngspice_OpenVAF_Enhancements fork; unit tests added here (the behavioral ones fail on the pre-fix code).
noise_table/noise_table_log crashed the compiler (index out of bounds
in topology linearization: the callback takes no MIR value args, but
instr_args[0] was read unconditionally) and, even without the crash,
the lowered table was a placeholder [(0,0)] and OSDI codegen hit
unimplemented!("noise tables").
- hir_lower: read the real (freq, power) pairs at compile time, either
from the inline constant array {f0, p0, f1, p1, ...} or from a
two-column data file resolved relative to the compilation root
(blank/comment lines skipped). New CompilationDB::root_file_dir.
- sim_back/topology: guard the empty arg list of noise callbacks.
- osdi/load: emit the run-time evaluator for load_noise: power =
piecewise-linear interpolation of the table over log10(frequency),
clamped to the endpoints, fully unrolled into constant-slope
fmul/fadd/fcmp/select chains. noise_table entries in the scalar
static-power ABI slots (unused by the per-frequency noise path)
report 0.
Ported from javaNoviceProgrammer's Ngspice_OpenVAF_Enhancements fork.
Verified: inline, _log and file tables compile; the embedded slopes/
intercepts/clamps match the table data exactly.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Kreijstal
marked this pull request as ready for review
July 29, 2026 20:15
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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
1 participant
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.
Summary
This sweep fixes independent compiler crashes and correctness defects across the frontend, lowering, MIR infrastructure, and the OSDI backend:
ac_stimlowering,idtinitial conditions, andidtmodwrap/offset handling<<<lexing$simparam$strtyping, runtime lookup, and its raw-name typolocalparamvalues out of the externally overridable OSDI parameter surface (closeslocalparamis incorrectly exposed as model parameter #18)laplace_ndand avoid double-casting mixed coefficient literalsnoise_table/noise_table_loglowering and remove related compiler crashesThe original nine commits remain separated by defect group. A tenth, isolated follow-up commit repairs pre-existing macOS CI failures exposed by the sweep:
QueryPastStatecallback reference after the absdelay redesignconstpointer qualifiersWhy
These defects either crashed compilation, silently produced incorrect equations or metadata, left MIR invariants inconsistent, or kept macOS CI from compiling and validating the current OSDI ABI.
Impact
Models using the affected operators, array constructs, local parameters, system functions, or noise tables now compile without the identified crashes and produce the intended runtime behavior. Existing integration models continue to compile through LLVM/OSDI.
Validation
cargo fmt --all -- --checkgit diff --checkcargo test -p lexer -p parser -p hir_def -p hir_ty -p hir_lower -p mir -p mir_optLLVM_SYS_211_PREFIX=/usr/lib/llvm21 RUN_SLOW_TESTS=1 cargo test -p osdi --no-default-features --features llvm21cargo test -p sourcegen osdi::gen_osdi_structs -- --nocaptureLLVM_SYS_211_PREFIX=/usr/lib/llvm21 cargo build --release --features llvm21LLVM_SYS_211_PREFIX=/usr/lib/llvm21 RUN_DEV_TESTS=1 cargo test --release --features llvm21