Fix segmentation fault when selecting instance in debug mode#9557
Fix segmentation fault when selecting instance in debug mode#9557krrish175-byte wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a segmentation fault that occurs when selecting an instance in debug mode. The changes involve moving a loop to be guarded by a null check on the instance pointer and adding boundary checks before accessing a vector of NesterovBase objects. The fixes appear correct and address the reported issue. I've noted an opportunity to refactor duplicated code into a helper function to improve maintainability.
src/gpl/src/graphicsImpl.cpp
Outdated
| size_t nb_index = 0; | ||
| if (nb_selected_index_ != kInvalidIndex) { | ||
| nb_index = nb_selected_index_; | ||
| } else { | ||
| } else if (!nbVec_.empty()) { | ||
| logger_->warn( | ||
| utl::GPL, 317, "Selected instance not found in any NesterovBase"); | ||
| } | ||
| if (nb_index >= nbVec_.size()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This logic for determining nb_index is duplicated in reportSelected() at lines 530-539. To improve maintainability and avoid future inconsistencies, consider extracting this logic into a helper function. A free function within an anonymous namespace would be a good approach, following the repository's general rules.
For example:
namespace {
std::optional<size_t> getSelectedNesterovBaseIndex(
size_t nb_selected_index,
const std::vector<std::shared_ptr<gpl::NesterovBase>>& nbVec,
utl::Logger* logger,
int msg_id)
{
size_t nb_index = 0;
if (nb_selected_index != gpl::GraphicsImpl::kInvalidIndex) {
nb_index = nb_selected_index;
} else if (!nbVec.empty()) {
logger->warn(
utl::GPL, msg_id, "Selected instance not found in any NesterovBase");
}
if (nb_index >= nbVec.size()) {
return std::nullopt;
}
return nb_index;
}
} // namespaceThis helper could then be called from both drawNesterov and reportSelected.
References
- Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.
Fix three issues that caused segmentation faults in GPL debug mode: 1. In debugForNesterovPlace(), the nbVec_ search for the selected instance was outside the 'if (inst)' guard, causing cell_handle->contains(nullptr) to be called when no debug instance was specified. 2. In drawNesterov(), accessing nbVec_[nb_index] for gradient drawing without checking that nb_index is within bounds of nbVec_. 3. In reportSelected(), same missing bounds check on nbVec_[nb_index]. Fixes The-OpenROAD-Project#9017 Signed-off-by: Krrish Biswas <krrish175byte@gmail.com> Signed-off-by: krrish175-byte <krrishbiswas175@gmail.com>
krrish175-byte
force-pushed
the
fix/gpl-debug-select-inst
branch
from
2eff4b5 to
f95968a
Compare
|
Hi @krrish175-byte, thanks for your contribution! Were you able to reproduce the issue? I don't remember what/when the issue is triggered exactly. I tried but couldn't reproduce the issue with master version on my end. |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Description
Fixes a segmentation fault that occurs when trying to select an instance in GPL debug mode. This regression was introduced after a previous fix that added region-aware instance selection logic.
Fixes #9017