Skip to content

Throw if BLIF model doesn't exist in the arch xml #3214

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 3 commits into from
Jul 28, 2025

Conversation

gigeresk
Copy link
Contributor

@gigeresk gigeresk commented Jul 28, 2025

Adds: Throw an exception if a BLIF model doesn't exist in the architecture file. Previous behaviour was a segfault.
Also modifies a function that uses static_casting for invalid state to rather use std::optional.

@gigeresk gigeresk self-assigned this Jul 28, 2025
@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jul 28, 2025
@gigeresk
Copy link
Contributor Author

@AlexandreSinger Pending issue: the throw includes the lineno_ to remain consistent with other throws in the file, but the value of lineno_ seems to generally be the length of the file (not the specific line of the file). Shall I file this as an issue?

@gigeresk gigeresk requested a review from AlexandreSinger July 28, 2025 14:49
@AlexandreSinger
Copy link
Contributor

@gigeresk I believe that lineno_ being the length of the file is expected behavior in this case. The function that calls this verifier that you are updating is called after all the lines in the file has been parsed. At this time, the lineno should be set to the end of the file.

This does demonstrate that we are not verifying the models as they are read in; which may or may not be an issue. Since models are treated as blackboxes, its a design decision whether an error should be thrown during the parse or after (when we cross-reference with the architecture file).

In my opinion it is fine to keep the current behavior, but I agree that this is not very intuitive.

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.

These changes LGTM! Very welcome change, this segmentation fault has hit me a couple of times. I appreciate that you fixed it!

Please see my other comment on the lineno. Its up to you if you want to raise an issue to make this more clear, but I think its OK.

@AlexandreSinger
Copy link
Contributor

P.S. Regarding the failing SV testcase. It looks like its nothing. I think the test just needs to be rerun. Sometimes the GH servers have some issues.

@gigeresk
Copy link
Contributor Author

gigeresk commented Jul 28, 2025

@AlexandreSinger Thanks Alex-- I'll file an issue and re-trigger the tests

@gigeresk
Copy link
Contributor Author

Spin-off issue #3215

@gigeresk gigeresk marked this pull request as ready for review July 28, 2025 15:26
@gigeresk gigeresk merged commit 0b243a1 into master Jul 28, 2025
30 checks passed
@gigeresk gigeresk deleted the fredt_check_blif_model_is_in_arch branch July 28, 2025 17:49
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