Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trying to fix the failing unit test #2735

Merged
merged 6 commits into from
Oct 31, 2024
Merged

Conversation

soheilshahrouz
Copy link
Contributor

I couldn't reproduce the unit test failure on a local machine, so I created this PR to trigger a CI run and hopefully locate the segault.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Sep 21, 2024
@AlexandreSinger
Copy link
Contributor

AlexandreSinger commented Sep 21, 2024

@soheilshahrouz A suggestion which might make things easier is to increase the assert level for the unit test for your test and turning on the sanitizer so you can try and catch the bug in action.

Locally you can try building with:
CMAKE_PARAMS="-DVTR_ASSERT_LEVEL=3 -DVTR_ENABLE_SANITIZE=ON"
I set the assert level to 3, but you can increase it to 4 if you really want. I think the sanitizer will be the most help. It will slow things down tremendously, but since this is a unit test it should be pretty quick.

Edit:
The CI just does not seem to be consistent; so building and running locally may be better.

My guess is that this testcase has a memory leak; and specifically how the binary is built may change when it crashes and how (which explains why it crashes in different ways each time).

@soheilshahrouz soheilshahrouz changed the title [WIP] Trying to fix the failing unit test Trying to fix the failing unit test Oct 30, 2024
Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest adding an example ...

Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @soheilshahrouz , these changes LGTM.

My only comment is that I actually do not like the pairs() interface for a vector-like storage class. A vector is basically a contiguous array of values, where the index is not necessarily a part of the structure itself. The issue I see here is that you are generating the key of the pair when the iteration is happening; the key is not stored in the data structure directly. This is not going to be returned by reference (I think), or at least it really should not be since this is a reference to a temporary. I am not sure the performance impact of this. In my opinion the best way to iterate over a vector of values when you need the index as well as the value is to use a standard for loop.

Since this interface is optional to use though, I see no issue with adding it. But I do think it should be used sparingly (unless someone wants to dig into the assembly to see how this is being lowered lol).

@soheilshahrouz
Copy link
Contributor Author

@vaughnbetz
Ready to be merged into master.

@vaughnbetz vaughnbetz merged commit 660ecab into master Oct 31, 2024
56 of 103 checks passed
@vaughnbetz vaughnbetz deleted the temp_try_fix_odd_even_unit_test branch October 31, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code libvtrutil VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants