Skip to content

🐛 Fix reading a latched AIGER file into a combinational network - #705

Merged
myskyko merged 2 commits into
lsils:masterfrom
marcelwa:upstream-aiger-reader-latches
Aug 29, 2026
Merged

🐛 Fix reading a latched AIGER file into a combinational network#705
myskyko merged 2 commits into
lsils:masterfrom
marcelwa:upstream-aiger-reader-latches

Conversation

@marcelwa

Copy link
Copy Markdown
Contributor

The bug

aiger_reader::on_header only materializes the latch outputs when the network type implements create_ro:

if constexpr ( has_create_ro_v<Ntk> )
{
  /* create latch outputs (ro) */
  for ( auto i = 0u; i < num_latches; ++i )
  {
    signals.push_back( _ntk.create_ro() );
  }
}

For a network type that does not — a plain aig_network, or the mig_network in this reader's own documented example — signals is left short by exactly the latch count, while every AIGER literal above the primary inputs still assumes those slots exist. on_and then does signals[left_lit >> 1] past the end of the vector and hands the result to create_and.

The only thing standing between that and undefined behaviour is one line in on_header:

assert( num_latches == 0 && "network type does not support the creation of latches" );

That condition is data-dependent — it comes from a user-supplied file, not from a programming invariant — so guarding it with assert means it does nothing at all under NDEBUG, i.e. in any release build.

Two failure modes follow:

  • Crash. On a design whose logic reaches far enough past the latch outputs, the out-of-bounds read faults. Reading a 4-bit LFSR (0 inputs, 4 latches, 3 ANDs) into an aig_network segfaults.
  • Silent corruption. On a design whose literals happen to stay inside the allocation it does not fault, and the file parses with return_code::success into a network quietly missing its registers. A one-latch file came back reporting 0 inputs and 1 output, having dropped the sequential half of the design without a word.

Confirmed with an ASan build against these headers. With assertions live it trips aiger_reader.hpp:139, the on_header guard. Under -DNDEBUG it reports std::vector<aig_network::signal>::operator[] with __n=4 on a size-1 vector, called from on_and( index=5, left_lit=8, right_lit=7 ) at aiger_reader.hpp:193.

The fix

Reading a latched file into a combinational network now flattens one timeframe of the design. Each latch output becomes a primary input, appended behind the file's own primary inputs, and each latch next-state function becomes a primary output, appended behind the file's own primary outputs — the transformation ABC calls comb.

I picked flattening over erroring out for three reasons. It is lossless, where refusing the file or dropping the latches is not. It keeps every literal resolving to the signal the file names, which is what actually fixes the memory error rather than papering over it. And it makes the mig_network example in this class's own documentation work on a sequential file instead of crashing on one — right now that example is only safe for combinational input, which the docs do not say. The sequential<Ntk> path is untouched.

Happy to switch it to a hard error instead if you would rather the reader refuse; I avoided that mainly because there is no throw anywhere in include/mockturtle/io/ today and lorina's callbacks have no way to abort a parse, so it would have been a departure in both style and mechanism.

Two supporting changes: recording and naming latches is no longer conditional on the network type, since the flattened path needs both. And on_header and the destructor disagreed about which trait decides whether a network can hold registers — create_ro in one, create_ri in the other — so both now consult a single has_registers predicate. That drift is what allowed the two halves to get out of step to begin with.

Tests

Three cases added to test/io/aiger_reader.cpp:

  • flattening a latched file into an aig_network, checked against the same file read into a sequential<aig_network> — same size, same gate count, same CI and CO counts, only distributed differently
  • the zero-primary-input shape that segfaulted, where every literal is reached through a latch output
  • the symbol table, which must still put a latch's name on the input standing in for it

The [aiger_reader] suite passes under -fsanitize=address on this branch, and the LFSR and an 8-bit accumulator both round-trip cleanly through the standalone reproducer that used to fault.

Note on behaviour

This does change what a caller sees: reading a sequential design into a combinational network now yields registers as free primary inputs rather than a crash. That is lossless and it is the documented meaning of the operation, but it is not the same circuit, so equivalence checking against the sequential original will not agree. A consumer that wants to refuse rather than flatten can check the latch count itself.

Found while tracking down a segfault in aigverse, which reads AIGER through this reader.

🤖 Generated with Claude Code

`aiger_reader::on_header` only materialized the latch outputs when the network
type implements `create_ro`. For one that does not, the reader's `signals`
vector was left short by exactly the latch count, while every AIGER literal
above the primary inputs still assumed those slots existed. `on_and` then
indexed past the end of the vector and handed the garbage it read to
`create_and`.

The only thing standing between that and undefined behaviour was

    assert( num_latches == 0 && "network type does not support the creation of latches" );

which is compiled out under `NDEBUG` -- that is, in every release build. The
result was a segmentation fault on any design whose logic reaches far enough
past the latch outputs, and a silently wrong network on any that does not: a
one-latch file whose literals all stay in bounds parsed "successfully" into a
network missing its registers entirely.

Reading a latched file into a combinational network now flattens one timeframe
of the design instead. Each latch output becomes a primary input, appended
behind the file's own primary inputs, and each latch next-state function becomes
a primary output, appended behind the file's own primary outputs. That is the
transformation ABC calls `comb`; it loses no logic, keeps every literal
resolving to the signal the file names, and makes the network type in this
reader's own documented example -- `mig_network`, which has no registers -- work
on a sequential file rather than crash on one.

Recording and naming latches is no longer conditional on the network type
either, since the flattened path needs both. The header and the destructor also
disagreed about which trait decides whether a network can hold registers,
`create_ro` in one and `create_ri` in the other; both now consult a single
`has_registers` predicate, which is what let them drift apart to begin with.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.07%. Comparing base (852605f) to head (4d2aa08).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
include/mockturtle/io/aiger_reader.hpp 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #705   +/-   ##
=======================================
  Coverage   84.07%   84.07%           
=======================================
  Files         190      190           
  Lines       29513    29515    +2     
=======================================
+ Hits        24813    24815    +2     
  Misses       4700     4700           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marcelwa added a commit to marcelwa/aigverse that referenced this pull request Aug 23, 2026
* 🐛 Refuse a latched AIGER file in the combinational readers

`read_aiger_into_aig` and `read_ascii_aiger_into_aig` segfaulted on any AIGER
file with latches.

The defect is in mockturtle's `aiger_reader`: `on_header` only materializes the
latch outputs when the network type implements `create_ro`, so for one that does
not, its `signals` vector is left short by exactly the latch count while every
literal above the primary inputs still assumes those slots exist. `on_and` then
reads past the end of the vector. The only guard was an `assert`, which is
compiled out under `NDEBUG` -- that is, in every wheel we ship.

Nothing in `aigverse` could produce a latched AIGER file until now, so the crash
was only reachable through an externally written one. That changes as soon as
`write_aiger` accepts a `SequentialAig`.

The reader used by the combinational bindings now refuses such a file in
`on_header`, before a single node is created, and the error names the sequential
reader to use instead. Refusing rather than reading is deliberate: mockturtle
will soon flatten a latched file into extra primary input and output pairs, one
per latch, which is lossless but hands back a network whose registers have become
free primary inputs -- a different circuit than the file describes, and one that
will not equivalence-check against it. Someone who reached for
`read_aiger_into_aig` on a sequential design wanted the sequential reader.

The upstream fix is marcelwa/mockturtle#12 and lsils/mockturtle#705. This guard
does not depend on either landing.

* 📝 Point the changelog entry at the actual PR number

* ⬆️🐢 Bump mockturtle to the revision that fixes the AIGER latch read

marcelwa/mockturtle#12 landed on `mnt`. The reader no longer walks off the end
of its own signal vector on a latched file -- it flattens one timeframe instead
-- so the guard in this branch is now the only thing standing between
`read_aiger_into_aig` and a silently reshaped network, rather than between it
and a segmentation fault.

* 💚 Fix the stale stubs and the clang-tidy warnings on the guard

Two CI failures, both mine.

**Stale stubs.** Adding to the readers' `Raises:` section changed the docstrings
the `.pyi` files are generated from, and I did not regenerate them.

Regenerating exposed a second problem: all four readers share one docstring
template, so the sequential ones were also claiming to refuse a latched file --
misleading for the two functions whose whole purpose is reading one. The clause
is now chosen per network type, so only the readers that actually refuse a
latched file document it, and they name the sequential readers to use instead.
nanobind copies docstrings, so building it at runtime is safe.

**Non-virtual destructor.** `refuse_latches` overrides a virtual function but
inherited a public non-virtual destructor from `lorina::aiger_reader`, which
`cppcoreguidelines-virtual-class-destructor` and `-Wnon-virtual-dtor` both flag,
once per instantiation. The reader is only ever a stack temporary handed to
lorina by const reference and is never deleted through a base pointer, but a
polymorphic type should not be left without a virtual destructor. Declare one,
and delete the copy and move operations that declaring it otherwise leaves
implicitly defined against the rule of five.

`clang-tidy` is clean on the file, and 398 tests and `nox -s lint` pass.

* 📝 Drop the mockturtle bump from the changelog entry

The branch pinned the revision fixing the underlying out-of-bounds read,
but main reached b856d3e first via #459, so the entry was crediting this
PR with a bump it no longer carries. The guard never depended on that
fix landing anyway -- it sits in front of the buggy path.

* 📝 Shorten the AIGER latch guard changelog entry
@myskyko
myskyko merged commit 0886ebf into lsils:master Aug 29, 2026
18 checks passed
marcelwa added a commit to marcelwa/mockturtle that referenced this pull request Aug 31, 2026
* write_blif: name CIs after the node, not the topological index (lsils#704)

`topo_view` reimplements `node_to_index` as the position in the topological order,
while every other reference in this writer -- fanin lists and PO bridges -- names a
node by its raw id. The two agree only while the CI node ids happen to be
contiguous.

They are not contiguous in general. A `klut_network` produced by `lut_map` has gaps,
and so does any network where a primary input is created after a gate. There the
`.inputs` line declares names nothing reads, and the `.names` bodies reference names
that were never declared:

    .inputs pi2 pi3 pi4
    ...
    .names new_n4 pi5 new_n6

`pi4` is dead and `pi5` is undeclared. Lorina's own BLIF reader rejects that, but ABC
accepts it and ties the undeclared signal to constant 0 -- so the netlist reads back
as a well-formed circuit computing something else, with no error anywhere.

Found by combinational equivalence checking a mapped EPFL `mem_ctrl`, which came back
NOT_EQUIVALENT with 39 phantom inputs.

Fixed by deriving the name from the node, which is what the rest of the writer does.
The added test builds the smallest network with a CI gap and pins the exact output.

* 🐛 Fix reading a latched AIGER file into a combinational network (lsils#705)

* 🐛 Fix reading a latched AIGER file into a combinational network

`aiger_reader::on_header` only materialized the latch outputs when the network
type implements `create_ro`. For one that does not, the reader's `signals`
vector was left short by exactly the latch count, while every AIGER literal
above the primary inputs still assumed those slots existed. `on_and` then
indexed past the end of the vector and handed the garbage it read to
`create_and`.

The only thing standing between that and undefined behaviour was

    assert( num_latches == 0 && "network type does not support the creation of latches" );

which is compiled out under `NDEBUG` -- that is, in every release build. The
result was a segmentation fault on any design whose logic reaches far enough
past the latch outputs, and a silently wrong network on any that does not: a
one-latch file whose literals all stay in bounds parsed "successfully" into a
network missing its registers entirely.

Reading a latched file into a combinational network now flattens one timeframe
of the design instead. Each latch output becomes a primary input, appended
behind the file's own primary inputs, and each latch next-state function becomes
a primary output, appended behind the file's own primary outputs. That is the
transformation ABC calls `comb`; it loses no logic, keeps every literal
resolving to the signal the file names, and makes the network type in this
reader's own documented example -- `mig_network`, which has no registers -- work
on a sequential file rather than crash on one.

Recording and naming latches is no longer conditional on the network type
either, since the flattened path needs both. The header and the destructor also
disagreed about which trait decides whether a network can hold registers,
`create_ro` in one and `create_ri` in the other; both now consult a single
`has_registers` predicate, which is what let them drift apart to begin with.

* Warn when flattening latched AIGER

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: MyskYko <yoyuohlhjl@yahoo.co.jp>

* Fix uninitialised read in lut_map on networks with dangling nodes

`cut() = default` leaves `_length`, `_cend` and `_end` indeterminate.  A
default-constructed cut is reachable: `cut_set` and `lut_cut_set` hold an array
of them and `best()` returns `*_pcuts[0]` whether or not any cut has been
inserted.  `lut_map_impl::compute_share_mapping_init` iterates over every node
index and calls `best()`, and a node unreachable from the outputs is never
visited by the cut enumerator, so its cut set is empty and the following
`for ( auto leaf : cut )` in `compute_cut_data` walks a garbage end pointer and
indexes `cuts[leaf]` with whatever it finds.

Networks with unreachable nodes are not exotic: ABC's `&dch -f; &put` leaves the
choice-class members in as ordinary AND nodes, so every AIG written by the
standard `strash; &get; &dch -f; &put; write_aiger` front end has them (cavlc:
1271 ANDs written, 647 reachable).

Give the default constructor a defined empty state, and add a lut_mapper test
that maps a six-gate AIG whose last four gates drive no output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants