search

lsils / mockturtle

C++ logic network library
MIT License
191 stars 133 forks source link

Fix signal name assignment bug in ``names_view.hpp`` #638

Closed Drewniok closed 3 months ago

Drewniok commented 4 months ago

This PR addresses a bug related to signal name assignment within the names_view<Ntk>::operator= method. The code has been refactored to include a check for the emptiness of the current_pis vector before attempting to access its elements. This change ensures that the loop is only executed when the current_pis vector is not empty, preventing potential out-of-range accesses and improving code safety.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.47%. Comparing base (93a37e8) to head (056df91).

:exclamation: Current head 056df91 differs from pull request most recent head 2684518. Consider uploading reports for the commit 2684518 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #638 +/- ## ======================================= Coverage 83.47% 83.47% ======================================= Files 187 187 Lines 29692 29703 +11 ======================================= + Hits 24784 24795 +11 Misses 4908 4908 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lee30sonia commented 4 months ago

Thanks! As I try to understand the problem and the solution, I notice a few things:

  1. The current tests (in function test_copy_names_view) are not testing the assignment operator (operator= overload), but the copy constructor instead. This is the same for the test you added (names_view<Ntk> new_named_ntk_empty = ntk_empty;). To have the assignment operator called, we need to separate the declaration of the new named network and the assignment into two lines.
  2. To be honest, I don't really understand what the current implementation of the assignment operator is trying to achieve. The network assignment (Ntk::operator=( named_ntk );) happens in the end, so for the code before, this either has no PIs (if it is empty), or has the same number of PIs and with the same indices (if assignment happens between different versions of the same network), or has a completely unrelated set of PIs (if assignment happens between unrelated networks --- should be a rare situation). In none of the cases, it doesn't make sense to me to map the old PI signals to names in the network to be assigned.

Please allow me some more time to think about it more thoroughly. If you have any thoughts (e.g. from your usage scenario), please do let me know!

Drewniok commented 4 months ago

Thanks @lee30sonia for your helpful feedback! The situation is as follows: For a few months now, we have been getting an error in the PR (https://github.com/cda-tum/fiction/actions/runs/8452568703/job/23153203895?pr=317) in debug on Windows only. So I built everything on a Windows machine and investigated why the problem occurs in the CI. It turned out that windows in debug fails because of current_pis[i] in case of an empty current_pis vector.

lee30sonia commented 3 months ago

@Drewniok That's weird. It would be interesting to know how the test case that triggers the bug looks like. e.g., having a print-out in the assignment operator printing how many PIs *this and named_ntk has, respectively.

Drewniok commented 3 months ago

@Drewniok That's weird. It would be interesting to know how the test case that triggers the bug looks like. e.g., having a print-out in the assignment operator printing how many PIs *this and named_ntk has, respectively.

@lee30sonia The bug seems to manifest itself when updating a *this instance without primary inputs (pis) with another instance named_ntk that includes primary inputs. This mismatch causes incorrect behavior. I made adjustments to handle this specific scenario to ensure proper functionality. Additionally, I have implemented a test to validate the fix under these conditions.

Drewniok commented 3 months ago

Hi @lee30sonia, Have you found time to take a look at the changes yet? No problem if not! I only ask because after this is fixed, I will continue working on the mentioned PR in fiction. Thank you!

lee30sonia commented 3 months ago

Hi @Drewniok, sorry, I am on in-between-jobs vacation these weeks and don't have much time. This PR is on my TODO list and I promise to get back as soon as possible.

lee30sonia commented 3 months ago

Sorry for the delay and thanks again for the contribution! It looks good to me. I pushed just some minor changes and will merge once the tests are complete.