Skip to content

Fix segfault in atom_cluster_noc_group_check() #2618

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

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

soheilshahrouz
Copy link
Contributor

g_vpr_ctx.cl_helper().atom_noc_grp_id is not initialized when try_pack() is not called. This causes a segfault when re-clustering API is used as atom_cluster_noc_group_check() tries to access an uninitialized vector. This PR fixes this problem by check whether the vector is initilzided.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jun 17, 2024
@@ -1384,7 +1384,8 @@ bool atom_cluster_noc_group_check(AtomBlockId blk_id,
ClusterBlockId clb_index,
int verbosity,
NocGroupId& temp_cluster_noc_grp_id) {
const NocGroupId atom_noc_grp_id = g_vpr_ctx.cl_helper().atom_noc_grp_id[blk_id];
const auto& atom_noc_grp_ids = g_vpr_ctx.cl_helper().atom_noc_grp_id;
Copy link
Contributor

@vaughnbetz vaughnbetz Jun 17, 2024

Choose a reason for hiding this comment

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

I suggest a comment saying why this is necessary (some flows that just use the clustering legality check might not have this data structure alive). Can be added after CI completes / in another PR so we get this one in quickly if you like.

@vaughnbetz
Copy link
Contributor

Thanks for the fast action @soheilshahrouz .
Tagging @KA7E as she has a dependency on this one.
Do we have any NoC checks in the fast to run tests (vtr_strong etc.)? This has already passed the fast checks, which is good, but I'm not sure if we have a (tiny) NoC test in those ones. If not it would be good to add one @soheilshahrouz (in another PR) so we have a very fast NoC test.

@vaughnbetz vaughnbetz merged commit 029fb01 into master Jun 17, 2024
53 checks passed
@vaughnbetz vaughnbetz deleted the fix_segfault_atom_cluster_noc_group_check branch June 17, 2024 19:55
@vaughnbetz
Copy link
Contributor

Merging since CI passed, but I still think this should have a comment added @soheilshahrouz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants