Repository navigation
Conversation
Builds kc with AddressSanitizer and UndefinedBehaviorSanitizer, so the test suite reports memory errors, leaks (on Linux) and undefined behaviour as test failures. Only the kc target is instrumented; the solver library and generated simulators are built as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The loop over the species called GetSpecNo with 0..n-1, but species are numbered from 1. The first init-val was read from symtable[-1] and the last species was missing, so the initial values were shifted by one. UndefinedBehaviorSanitizer reports the out-of-bounds index. The in.dat files of the 21 models listed in skip.txt for this defect are now tested; the others change in their init-val lines only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TreeDerive had no case for ln, so the chain-rule factor stayed an empty node, which reads as the constant 0: d ln(u)/dx came out as 0 instead of u'/u. The new test model derivatives1 covers the derivatives of ln, log, exp, sin, cos and f^g. Its mode 1 Jacobian was checked by hand. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Most derivatives of functions built their result in the wrong nodes: - tan, tanh: cos(u)/cosh(u) was written over the Pow node, giving u'/(u*2) instead of u'/cos(u)^2 - asinh: the exponent 0.5 was set on the wrong node; the result was u' - acos: derived into a NULL pointer, so kc crashed - acosh: not implemented, the result was 0 - atan, atanh: correct only because a second derivation through the wrong union member got a NULL tree and did nothing These cases now use small node builders and state the rule they implement. The new test model derivatives2 covers all of them; its mode 1 Jacobian matches numerical differentiation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TreeEval returned the argument unchanged for these functions; the calls were commented out because C89 had no asinh, acosh and atanh. C99 does, so constants such as asinh(1.0) are now evaluated. derivatives2 declares one constant per function to test this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TreeReduce rewrote f+(-g) as f-(-g) and f-(-g) as f+(-g): it kept the negation node instead of its operand. Each reduction flipped the sign of g, and expressions are reduced again on every operation, so the result depended on how often a subtree was reduced. Affected are models where a negated term ends up on the right of a sum, e.g. through a reversible reaction. In gtf and shn90 single Jacobian entries had the wrong value (shn90 Jac[3][3] in mode 2: +0.0999 instead of -0.540), which changes their mode 1 eigenvalues and mode 3 end states. The changed entries were checked against finite differences of the generated right-hand sides. In the other models only the printed form changes, e.g. x+(-(y)) is now x-y. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GenerateHessian and GenerateKeldian stored the derivative by each dynamical variable at index i+NoOfSpec()-NumOfConstraint()-1, where i is the row of the matrix, instead of at the next free column. This overwrote the derivative by the last species, left the column of the dynamical variable empty and wrote out of bounds, so kc crashed on models with two parameters and a dynamical variable. EvalJacobian freed its name buffer twice and never freed rename. The new test model hessian covers this case; its second and third derivatives were checked by hand. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tree API copied its arguments, mutated the first one in place and left freeing to the caller. TreeCpy never freed what it overwrote and TreeReduce leaked on every call, and the table manager copied trees on both store and lookup without freeing them. Memory was leaked on almost every operation, and kc.y and codegen.c had a use-after-free and a double free. Trees are now immutable and share subtrees: - Every operation returns a new tree: $$=TreeAdd($1, $3) instead of create, add in place, copy and kill. - All nodes come from an arena, released at once by TreeFreeAll() at the end of main. TreeCreate, TreeCpy, TreeKill and TreeAssign* are gone; TreeConst and TreeVar build leaves. - The table manager stores and returns pointers. - Tree is a pointer to const, so the compiler rejects mutation. Reduction runs at the same points as before, so the generated code is unchanged: the outputs of all 79 models in all modes are identical to the previous commit, apart from timestamps. symbmath.c shrinks from about 1560 to 940 lines. InitCodeGenVar allocated the Tree arrays with sizeof(TreeNode) instead of sizeof(Tree). With the correct size, GenerateRateExpr no longer copies a slot past the end of v that was never filled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The header of the generated djacobian procedure assigned the last species again instead of each dynamical variable, at index i+NoOfSpec()/NumOfConstraint(): a typo for "-" that divides by zero in models without constraints. It now matches the other procedures, e.g. z:=xx_[3] instead of B:=xx_[1] in the hessian test model. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
v_store and r hold pointers only; the trees they point to live in the arena and stay referenced from v, rfw and rrv. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mixed never freed name and rename. ConIS and KnCont allocated them before a parameter check that returns early; they now allocate them after it. LeakSanitizer reported these on every model. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
One more matrix entry builds kc with KC_SANITIZE on Linux/clang, where LeakSanitizer is available, so memory errors, leaks and undefined behaviour fail the build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Addresses the TODO item "Better memory allocation (especially in
symbmath.{c,h})".Why
The expression-tree API copied its arguments, mutated the first one in place and left freeing to the caller.
TreeCpynever freed what it overwrote,TreeReduceleaked on every call, and the table manager copied trees on store and lookup without ever freeing them. Rewriting this code also brought several real bugs to light.How to review
Commits are ordered so each can be reviewed alone, and each one passes the test suite:
Add KC_SANITIZE build option: ASan + UBSan for kc only (generated simulators are unaffected).in.datinitial values were shifted by one species (out-of-bounds read, found by UBSan). The 21skip.txtentries for this defect are removed.asinh,acoshandatanhof constants returned their argument.f+(-g)andf-(-g)were reduced with the wrong sign. The changed Jacobian entries (gtf, shn90) were checked against finite differences and by hand. In the other models only the printed form changes (x+(-(y))becomesx-y).Immutable expression trees in an arena: the refactor itself. The generated output is byte-identical to the previous commit for all 79 models in all 5 modes (checked by diffing the outputs of both builds, timestamps excluded).$$=TreeAdd($1, $3).TreeFreeAll()at exit.Treeisconst TreeNode *, so the compiler rejects mutation.TreeCreate,TreeCpy,TreeKillandTreeAssign*are gone.symbmath.cshrinks from about 1560 to 940 lines.djacobianheader (wrong variable, and a division by zero without constraints), plus three small leaks.New test models:
derivatives1,derivatives2(all functions) andhessian(mode 5 with a dynamical variable).Testing
ctest: 395/395 pass on macOS (clang, Release with-Werror) and on Linux (GCC, Release with-Werror).KC_SANITIZE=ON: 395/395 pass on macOS, and on Linux/clang with LeakSanitizer, with no leaks or errors reported.Not in this PR
test/skip.txtare in the solvers (eigen.c,matrix.c), not in the tree code.kc.y, thepentryrules callNewDelta*twice, so the value$5is ignored.🤖 Generated with Claude Code