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

Remove max_device_infos limit in riscv mux target #663

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dlopr
Copy link
Contributor

@dlopr dlopr commented Jan 29, 2025

There was a harcoded maximum of devices used as static size for a std::array.

This removes the limit and uses a dynamically sized std::vector instead.

There was a harcoded maximum of devices used as static size for a
std::array.

This removes the limit and uses a dynamically sized std::vector instead.
Copy link
Collaborator

@hvdijk hvdijk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The main idea looks good. Are you intending for it to still be possible to have invalid device info (in which case it will still need to be handled), or are you wanting to make that impossible?

// list ends with first non valid info entry
if (!info.is_valid()) {
break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we end up with invalid device info, shouldn't we still bail out? Alternatively, if we can ensure that info is always valid so that we no longer have to check, that would also be okay but in that case we should probably remove is_valid() entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's something that could happen with the std:array approach: only actual devices are initialized and the excess of entries in the array capacity are not.

With a vector, the size of the vector tells you the number of devices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This assumes that an uninitialized device info is the only way to end up with an invalid device info though. That is an assumption we currently try to avoid making: the code does:

    auto &dev_info = device_infos.emplace_back();
    dev_info.update_from_hal_info(hal_dev_info);
    dev_info.hal_device_index = i;
    // device info should be valid at this point
    assert(dev_info.is_valid());

Consider what the assert does here: it accounts for the possibility that there are bugs in update_from_hal_info that leaves dev_info in an invalid state, and ensures that in debug builds we get a crash due to an assertion failure. In release builds, the assert does nothing, we would carry on with the invalid device info already added to device_infos. And then, the removal of the if (!info.is_valid()) break; condition would cause that invalid device info to be used.

We could change the way we update dev_info to ensure that invalid device infos are never added to the vector (or possibly even that they can never exist in the first place), but unless we include such a change in this PR, I think we need to allow for the possibility that the vector contains invalid device infos and keep this if (!info.is_valid()) break; in.

modules/mux/targets/riscv/source/device_info_get.cpp Outdated Show resolved Hide resolved
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