Fix Failure("nth") crash in remove_unused_parameters#678
Fix Failure("nth") crash in remove_unused_parameters#678
Conversation
In unused_i (lib/Simplify.ml line 236), List.nth ts i was called without
bounds checking. When update_table_current iterates over actual arguments
(es) of an EApp whose callee has fewer formal type parameters than actual
arguments (e.g. erased ghost params passed as () at call sites), i can
exceed List.length ts, causing a Failure("nth") crash.
The length guard at line 353 (if List.length es <= List.length ts) handles
this for the main transformation, but update_table_current is called before
that guard at line 344.
Add a bounds check so unused_i returns false for out-of-bounds indices,
which is the correct conservative behavior (don't eliminate unknown params).
Fixes #677
|
|
@tahina-pro is this something you can help with? I would love for this PR to have a corresponding regression in the test suite, which would allow me to understand the problem -- right now, without a supporting example, it's hard to determine whether this is the right fix or not |
|
@protz hey sorry for dropping the ball this week is kind of busy. I will add a repro Friday |
Add a JSON-based test (test/json/NthCrash.json) that reproduces the crash from #677: a DExternal2 with more hint names than type arrows triggers an out-of-bounds List.nth in unused_i when visit_DExternal iterates hints. This test crashes on master with Failure("nth") and succeeds with the fix. Introduces test/json/ as a new test subdirectory for JSON-encoded karamel AST inputs, with its own Makefile and a json-test target in test/Makefile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Sorry for the delay -- got caught up with stuff then conference travel. Ok, I was hoping you would give me a source .fst file rather than a .json. We do not have any JSON-based tests in the test suite right now, so this is a little strange. Looking at it, I believe that the input ast that is being fed to krml is invalid: you are providing more naming hints than there are arguments in the function type for the external. I would argue that this needs to be fixed upstream to trim the list of suggested names to at most the number of arrows in the type, but I would also take a patch in let t = mk_typ t in
let args, _ = flatten_arrow t in
let arg_names, _ = KList.split_at_nth (List.length args) in
DExternal2 ...at line 53 of Thanks! |
Fixes #677
Problem
remove_unused_parametersinlib/Simplify.mlcrashes withFailure("nth")when bundling modules where a function call site has more actual arguments than the callee's formal type parameters (as reported byflatten_arrow).This happens when erased/ghost parameters are passed as
()at call sites, but the callee's type signature doesn't include them (e.g., external functions likePulse.Lib.Array.PtsTo.op_Array_Accesswhose ghost args were erased from the type but not from call sites).Root Cause
In
unused_i(line 236),List.nth ts i = TUnitis called without bounds checking. Theupdate_table_currentcall at line 344 invisit_EAppiterates over actual argumentsesand callsunused arg_ifor each — butarg_ican exceedList.length tswhen there are more actual args than formal type params. The length guard at line 353 correctly handles this for the main transformation path, butupdate_table_currentis invoked before that guard.Fix
Add a bounds check so
unused_ireturnsfalsefor out-of-bounds indices (conservative: don't eliminate unknown params):Reproduction
Extract a Pulse module with
.fstiinterface (making implementations private), create a wrapper without.fsti, and bundle them:After the fix, extraction succeeds and produces correct C output with all functions.